-
Notifications
You must be signed in to change notification settings - Fork 359
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
Support LoadBalancerIPMode in AntreaProxy #6102
Conversation
38d6775
to
c804cac
Compare
} | ||
|
||
func IsVIPMode(ing v1.LoadBalancerIngress) bool { | ||
if ing.IPMode == nil { |
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.
In the K8s proxy code, they also check the FeatureGate: https://github.com/kubernetes/kubernetes/blob/4b8e819355d791d96b7e9d9efe4cbafae2311c88/pkg/proxy/util/utils.go#L335-L337
I guess in our case we believe there is no need to introduce support for that FeatureGate and it would just add complexity?
To be honest, I am not entirely sure why they have that check in the K8s code. It feels like extra redundancy in that case, since the apiserver already has that FeatureGate and will drop the field if it is not enabled. Maybe @tnqn knows.
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 guess in our case we believe there is no need to introduce support for that FeatureGate and it would just add complexity?
You are right. There are some reasons why I removed the FeatureGate:
- If a user wants to set an ingress IP to be used in
LoadBalancerIPModeProxy
(the related Service traffic is sent to external loadBalancer), personally, I think there is no need to set another global switch like the FeatureGate in kube-proxy. The users should know what they are doing. - The field associated with
LoadBalancerIPModeProxy
andLoadBalancerIPModeVIP
has been in GA stage. IMO, I think the FeatureGate is redundant. - A not very strong reason, the change of this PR only affects the ingress IPs consumed by AntreaProxy. It doesn't change any code in AntreaProxy directly at all.
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.
@antoninbas I think it may be related to version skew cases. For example:
A new alpha feature is introduced in 1.28. The code is experimental and shouldn't run by default. In 1.29 the feature gets enhanced and gets promoted to beta. In some production clusters, there may be a version skew between kube-apiserver and kube-proxy. If kube-proxy doesn't have its own feature gate and just relies on kube-apiserver to gate it, the experimental code in kube-proxy 1.28 will run unexpectedly once kube-apiserver is upgrade to 1.29.
For this specific case, it seems to us there is no risk to support the field without a feature gate, but I guess Kubernetes is more strict on this, especially when there is already a feature gate, it would be harder to justify the new code is safe enough to skip using the existing feature gate to isolate 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.
In some production clusters, there may be a version skew between kube-apiserver and kube-proxy
That makes sense. I believe kube-proxy is usually updated last too.
The field associated with LoadBalancerIPModeProxy and LoadBalancerIPModeVIP has been in GA stage. IMO, I think the FeatureGate is redundant.
What do you mean by that?
In any case, I am fine with not having a FeatureGate in the Agent for this, as it does seem very low risk.
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.
@antoninbas I meant that the LoadBalancerIPMode
has been supported in "k8s.io/api/core/v1" 0.29 which is in GA stage, but the latest kube-proxy code consuming LoadBalancerIPMode
is still isolated by a FeatureGate, so I thought of that the FeatureGate is redundant. Thanks for @tnqn 's explanation, I understood the reason why the FeatureGate is added.
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.
@hongliangl I think what confused us is why you say it's in GA stage, regardless of K8s 1.29 or "k8s.io/api/core/v1" 0.29 (actually feature gate should have nothing to do with the package). It's alpha from what I can see: https://github.com/kubernetes/kubernetes/blob/20d0ab7ae808aaddb1556c3c38ca0607663c50ac/pkg/features/kube_features.go#L941-L944
c804cac
to
2255a17
Compare
} | ||
|
||
func IsVIPMode(ing v1.LoadBalancerIngress) bool { | ||
if ing.IPMode == nil { |
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.
@antoninbas I think it may be related to version skew cases. For example:
A new alpha feature is introduced in 1.28. The code is experimental and shouldn't run by default. In 1.29 the feature gets enhanced and gets promoted to beta. In some production clusters, there may be a version skew between kube-apiserver and kube-proxy. If kube-proxy doesn't have its own feature gate and just relies on kube-apiserver to gate it, the experimental code in kube-proxy 1.28 will run unexpectedly once kube-apiserver is upgrade to 1.29.
For this specific case, it seems to us there is no risk to support the field without a feature gate, but I guess Kubernetes is more strict on this, especially when there is already a feature gate, it would be harder to justify the new code is safe enough to skip using the existing feature gate to isolate 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.
LGTM
Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
2255a17
to
d87aa98
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
} | ||
|
||
func IsVIPMode(ing v1.LoadBalancerIngress) bool { | ||
if ing.IPMode == nil { |
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.
@hongliangl I think what confused us is why you say it's in GA stage, regardless of K8s 1.29 or "k8s.io/api/core/v1" 0.29 (actually feature gate should have nothing to do with the package). It's alpha from what I can see: https://github.com/kubernetes/kubernetes/blob/20d0ab7ae808aaddb1556c3c38ca0607663c50ac/pkg/features/kube_features.go#L941-L944
/test-all |
For #5342