-
Notifications
You must be signed in to change notification settings - Fork 298
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: set correct IP address type for machine #3379
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @CharlieR-o-o-t! |
Hi @CharlieR-o-o-t. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/retest |
@CharlieR-o-o-t: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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.
This does not work like this.
There are places where explicitly MachineExternalIP get's filtered for.
Example:
if machineAddr.Type != clusterv1.MachineExternalIP { |
Also we should make sure to not break other potential consumers and this would be a breaking change for them.
Do you know if there is a good definition in upstream documentation of the separation between ExternalIP and InternalIP? This could would rely on the definition of the private subnets.
In general I agree that this should be InternalIP, but we need to be careful.
cc @neolit123 , @rikatz
I think a way forward would be to set both for some time and maybe deprecate setting the ExternalIP, maybe via a feature gate so we could go through normal deprecation lifecycle. /ok-to-test |
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 know if there is a good definition in upstream documentation of the separation between ExternalIP and InternalIP? This could would rely on the definition of the private subnets.
node internal ip = ip of the node as accessed from the cluster
node external ip = 'real' node ip for external access
/cc @lubronzhan
since i'm not a network expert
privateBlocks := []*net.IPNet{ | ||
{IP: net.IPv4(10, 0, 0, 0), Mask: net.CIDRMask(8, 32)}, // 10.0.0.0/8 | ||
{IP: net.IPv4(172, 16, 0, 0), Mask: net.CIDRMask(12, 32)}, // 172.16.0.0/12 | ||
{IP: net.IPv4(192, 168, 0, 0), Mask: net.CIDRMask(16, 32)}, // 192.168.0.0/16 | ||
{IP: net.IPv4(127, 0, 0, 1), Mask: net.CIDRMask(8, 32)}, // 127.0.0.0/8 (loopback) | ||
{IP: net.IPv4(169, 254, 0, 0), Mask: net.CIDRMask(16, 32)}, // 169.254.0.0/16 (link-local) | ||
} |
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.
note that cluster local subnets are not may not be the same as local network subnets.
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 don't think that would be the correct way to determine if the ip is internal. it would be an internal network ip, but not necessary the internal cluster ip allocated to a node. to check if the ip is internal as per k8s' definition, i think one would have to check how the controller manager cidr is configured.
e.g. kcm is started with --cluster-cidr=10.244.0.0/16
Hello, thank you for quick reply, agree that we need to be carefull. About breaking change. This method is only used to check that we have at least one IP on control-plane bootstrap:
IP output from the function is never used (only in _test.go) We are using cluster-api-provider-vsphere for long time and only now noticed this issue, when we decided to monitor machines with pubic network. |
Signed-off-by: Siarhei Rasiukevich <[email protected]>
Definition from k8s https://github.com/kubernetes/kubernetes/blob/cd060979bfae61630c829d4a88f1fba2c60b3ad1/pkg/apis/core/types.go#L5378-L5389
The The InternalIP is could be from RFC 1918 private IP address ranges:
Still, the the customer could decide what IP range to be used for their internalIP on the node when they configure vsphere-cpi. For example, some lazy customer just choose to use a public IP to pass to CPI. As for the impact of this change: For this case, maybe the ideal solution would be, like what vsphere-cpi does, pass a range of IPs as internalMachineRange through cluster object, so this status could use that range to filter what IP to show for |
@lubronzhan @neolit123 I can rework this as it's done in vsphere-cpi. So user will define CIDRs for internal/external. |
Usually, keep the behavior the same to not break anyone |
What this PR does / why we need it:
It's incorrect to always set ExternalIP type for all machine IP addresses. Need to fix that.
Which issue(s) this PR fixes
Fixes issue #947