Skip to content
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

fix reload network endpoints #42825

Merged
merged 3 commits into from
Jan 17, 2023

Conversation

ramaraochavali
Copy link
Contributor

@ramaraochavali ramaraochavali commented Jan 16, 2023

Fixes #42818

  • Ambient
  • Configuration Infrastructure
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Policies and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali ramaraochavali requested a review from a team as a code owner January 16, 2023 08:30
@istio-policy-bot
Copy link

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 16, 2023
@ramaraochavali
Copy link
Contributor Author

This regressed as part of this #42818. Sorry about that

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali ramaraochavali requested review from a team as code owners January 16, 2023 08:55
@istio-testing istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 16, 2023
@@ -124,7 +124,7 @@ func (mgr *NetworkManager) reloadNetworkEndpoints() {
for _, networkconf := range oldNetworks.Networks {
for _, ne := range networkconf.Endpoints {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to rename ne as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
for _, ne := range networkconf.Endpoints {
if len(ne.GetFromCidr()) > 0 {
newEndpoints = append(newEndpoints, ne)
for _, oe := range networkconf.Endpoints {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of my new year resolutions is to start asking for more comments in the code :-). Few lines of comments explaining why we are doing this would help.

Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from correctness part

@istio-testing istio-testing merged commit f921038 into istio:master Jan 17, 2023
@ramaraochavali ramaraochavali deleted the fix/network_handler branch January 18, 2023 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
6 participants