-
Notifications
You must be signed in to change notification settings - Fork 62
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] handle cases where the internal-ip is already set (e.g. kubelet) #236
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #236 +/- ##
==========================================
+ Coverage 56.43% 56.50% +0.06%
==========================================
Files 12 12
Lines 1887 1890 +3
==========================================
+ Hits 1065 1068 +3
Misses 672 672
Partials 150 150 ☔ View full report in Codecov by Sentry. |
20dee87
to
a777899
Compare
7a52a5a
to
ff4c10f
Compare
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.
LGTM
if addr.Type == v1.NodeInternalIP { | ||
addresses = append(addresses, v1.NodeAddress{Type: v1.NodeInternalIP, Address: addr.Address}) | ||
} | ||
} |
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.
Should we make sure same ip is not already present in the slice? For example, if this InstanceMetadata() gets called again after 5-10 mins, then we'll end up adding all the existing ip's again into the slice. Not sure if k8s automatically handles such case.
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.
It doesn't matter if it is, it just needs to be present: https://github.com/kubernetes/cloud-provider/blob/master/node/helpers/address.go#L114-L135
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.
lgtm
Related to #111
General:
Pull Request Guidelines:
In the case that a Node's internal IP is already set (e.g. to a VLAN IP), the CCM will fail to set the external IP for the Node: