Remove cluster.hosts #4959
Remove cluster.hosts #4959
Conversation
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
|
|
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
There was a problem hiding this comment.
Thanks for taking this on. I have a couple of questions to get started with this review. This is going to be a pretty big breaking change if we go ahead with it; we have already deprecated the hosts field, but I can't remember why we didn't decide to keep it around for DNS addresses..
| protocol: TCP | ||
| address: disccovery.yourcompany.net | ||
| port_value: 80 | ||
| load_assignment: |
There was a problem hiding this comment.
Have you validated all of these modified YAML?
There was a problem hiding this comment.
I randomly tested them manually. Will have another test session today.
There was a problem hiding this comment.
Hum, I think @srikailash or @junr03 probably could give some tips on testing these templates here. 🙂
There was a problem hiding this comment.
@dio reached out over slack. For the v2 templates I suggest doing what I asked @srikailash to do when we converted them from v1 to v2: render the templates into config files and run them over the validation server, and the config_test
| - endpoint: | ||
| address: | ||
| socket_address: | ||
| address: google.com |
There was a problem hiding this comment.
I'm not sure how much it makes sense to have locality-based DNS addresses. It seems somewhat inelegant burying inside a singleton locality the DNS resolved address, which might map to multiple global localities.. Thoughts?
There was a problem hiding this comment.
I'm also in favor of keeping both but that was not specifically only for DNS addresses #3261 (comment)
| // **This field is deprecated**. Set the | ||
| // :ref:`load_assignment<envoy_api_field_Cluster.load_assignment>` field instead. | ||
| // | ||
| repeated core.Address hosts = 7 [deprecated = true]; |
There was a problem hiding this comment.
??? Can you set DNS addresses in load assignment ?
And how long has this been implemented? i.e. was it available in Envoy as of June 2018? The reason I am asking is because we can update this in Istio as well as long as we can support Envoys from around istio 1.0 (which is around june)
There was a problem hiding this comment.
For istio-1.0 releases, we already switched to forked Envoy with necessary patches
https://github.com/istio/proxy/blob/release-1.0/WORKSPACE#L38
So I don't think this will affect that?
There was a problem hiding this comment.
Can you set DNS addresses in load assignment
I think yes. It is implemented since this: #3864
The relevant discussion probably here: #3261 (comment)
|
@htuch I think the relevant conversation is here: #3261 (comment) I'm more than happy to support both hosts and load_assignment and set to have hosts for DNS addresses. |
|
@dio yeah, IDK, I think now that we have DNS supported inside EDS load assignments, we've basically unlocked the Pandoras box, so what you have in this PR is fine. It would be nice to have some variant of |
Cool. Yeah, I agree with this. |
|
@dio that said, I'm basically OK with what you have here. It would be ideal to validate as many of the examples as is reasonable. FWIW, in an ideal world it would be awesome to be able to consume all our examples from buildable/testable configs. But, that's probably more work than it's worth. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
dio
removed
the
stale
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Description:
Since Cluster's load_assignment field is implemented (#3864), remove Cluster's hosts.
Risk Level: Medium
Testing: Existing tests
Release Notes: TBA
Fixes #4618
Signed-off-by: Dhi Aurrahman dio@tetrate.io