-
Notifications
You must be signed in to change notification settings - Fork 184
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
Remove route agent "submariner.io/cniIfaceIp" node annotation #3010
Conversation
...as it's used by components outside of the route agent Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
🤖 Created branch: z_pr3010/tpantelis/cni_iface_ip |
Re: submariner-io/submariner#3010 Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Re: submariner-io/submariner#3010 Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
@@ -52,21 +52,34 @@ func NewNodeController(config *syncer.ResourceSyncerConfig, pool *ipam.IPPool, n | |||
nodeName: nodeName, | |||
} | |||
|
|||
clusterCIDR := "" | |||
if len(clusterCIDRs) > 0 { |
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.
Don't we want to report an error if len(clusterCIDRs) isn't bigger than 0?
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.
yeah we could although the operator would never pass empty cluster CIDRs anyway. I have a follow-up PR to check all clusterCIDRs
so this check will go away anyway.
@@ -61,7 +63,7 @@ const ( | |||
globalIngressIPName = "nginx-ingress-ip" | |||
kubeProxyIPTableChainName = "KUBE-SVC-Y7DIXXI5PNAUV7FB" | |||
serviceName = "nginx" | |||
cniInterfaceIP = "10.20.30.40" | |||
cniInterfaceIP = "169.254.1.50" |
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.
Why do we need to change 'cniInterfaceIP' value ?
cniInterfaceIP shouldn't be from globalIP cidr range
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 changed it to be in range of localCIDR
b/c originally I was checking for that in the fake cni.DiscoverFunc
like the real one does but I later changed cni.DiscoverFunc
to just verify that the configured local CIDR is passed in. localCIDR
is also used as the global CIDR which actually doesn't really matter for the tests but logically should be defined separately.
@@ -86,56 +81,3 @@ func discover(clusterCIDR string) (*Interface, error) { | |||
|
|||
return nil, errors.Errorf("unable to find CNI Interface on the host which has IP from %q", clusterCIDR) | |||
} | |||
|
|||
func AnnotateNodeWithCNIInterfaceIP(nodeName string, clientSet kubernetes.Interface, clusterCidr []string) error { |
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 assume that after upgrade we'll have orphaned/useless cni IP address annotation that won't be deleted ,
do we want to handle it ?
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.
We can't remove it b/c then we'd need update permission for nodes which we're removing. I don't see any way we can handle it other than to add a release note. It's just an annotation so doesn't hurt anything.
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.
Yep, adding release note sounds reasonable.
...rather than retrieving the CNI IP from the route agent Node annotation. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
...as globalnet no longer uses it. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
It checks if the health check IP is set on Endpoint and, if not, it doesn't verify the latency info but that could also be due to an issue in the gateway and/or globalnet with determining the IP. Instead parse the env vars in the gateway pod to determine if the health check var is present and enabled. If so then expect the health check IP to be set. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
🤖 Closed branches: [z_pr3010/tpantelis/cni_iface_ip] |
Re: submariner-io/submariner#3010 Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Re: submariner-io/submariner#3010 Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Re: submariner-io/submariner#3010 Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Release notes for submariner-io/submariner#3010 Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Release notes for submariner-io/submariner#3010 Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Release notes for submariner-io/submariner#3010 Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Release notes for submariner-io/submariner#3010 Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
The route agent discovers the CNI IP and adds the annotation to the node for use by Globalnet. However granting RBAC permission to update nodes poses a potential vulnerability exploitation by attackers. We can eliminate this annotation and the RBAC permission by having Globalnet discover the CNI IP itself.
See commits for details.