-
Notifications
You must be signed in to change notification settings - Fork 566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify relationship btw MeshNetworks and ENABLE_HCM_INTERNAL_NET #3433
Conversation
Add comments explaining the ability to use MeshNetworks to configure Envoy's internal_address_config via ENABLE_HCM_INTERNAL_NETWORK Signed-off-by: Jackie Elliott <[email protected]>
😊 Welcome @jaellio! This is either your first contribution to the Istio api repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
Signed-off-by: Jackie Elliott <[email protected]>
@@ -114,6 +114,9 @@ message Network { | |||
// locality: us-east-1a | |||
// ``` | |||
// | |||
// If `ENABLE_HCM_INTERNAL_NETWORKS` is set to true, MeshNetworks can be used to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we need an example of how to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Networks
defaults to all mesh IPs then maybe not, but I need to clarify the default value of Networks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my quick investigation, I don't think there is a default Network
configured. I had previously thought it defaulted to all mesh IPs. @ramaraochavali Since you are more familiar with this configuration could confirm this and review an additional example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I the documentation is correct. But I would hesitate to add the temporary feature flag reference here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update this documentation when the feature flag is removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But nothing is changed in network API. So not sure why we have to update here. Why is n't the main istio release notes not sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked having the information here so it would be clearly displayed in our api docs on istio.io. Otherwise, the only documentation on how to utilize MeshNetworks to preserve envoy internal headers. I could modify/simplify the description and link to the upgrade note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think specifying MeshNetworks can also be used to define "internal" addresses of mesh and provide an example is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @jaellio that the feature flag + example is useful//necessary here. Users who run into the Envoy behavior change are going to come to Istio documentation to tell them what to do. We should have a clear answer for them
and configuring MeshNetworks. Signed-off-by: Jackie Elliott <[email protected]>
In response to a cherrypick label: new pull request created: #3444 |
Add comments explaining the ability to use MeshNetworks to configure Envoy's internal_address_config via
ENABLE_HCM_INTERNAL_NETWORK
istio/istio#53402
Related: istio/istio#52037