-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
Add IP mode field to loadbalancer status ingress #97681
Conversation
@Sh4d1: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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/test-infra repository. |
@@ -43,3 +45,15 @@ func SetDefaults_NetworkPolicy(obj *networkingv1.NetworkPolicy) { | |||
} | |||
} | |||
} | |||
|
|||
func SetDefaults_Ingress(obj *networkingv1.Ingress) { |
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.
cc @liggitt this should fix #92312 (comment)
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 did not realize this struct was reused for Ingress! I don't think this is right. It happens to work for the fields that exist today, but I don't know of any Ingress implementation that is VIP-like.
I think we should probably clone the definition of type LoadBalancerStatus
into the networking apigroup.
@bowei @rramkumar1 @aledbf - agree or disagree?
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.
And if we do that, it should be a standalone prefactoring commit. :)
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 think we should probably clone the definition of type LoadBalancerStatus into the networking apigroup.
+1
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.
Wouldn't that be a breaking change in client-go? 🤔
/test pull-kubernetes-e2e-kind-ipv6 |
Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>
9fc0514
to
7d62217
Compare
/retest |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
/retest |
/test pull-kubernetes-e2e-ubuntu-gce-network-policies |
/retest |
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.
Thanks for bringing this back.
// delivering traffic with the destination IP and port set to the node's IP and nodePort or to the pod's IP and targetPort. | ||
// This field can only be set when the ip field is also set, and defaults to "VIP" if not specified. | ||
// +optional | ||
IPMode *LoadBalancerIPMode `json:"ipMode,omitempty" protobuf:"bytes,3,opt,name=ipMode"` |
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.
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.
Interesting, I remember that both PR were close in time, and this had to be reverted #96454 , is it possible that is the reason? that this PR predated the PortStatus one?
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.
So should I bump to 5 ? or keep 3? 🤔
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.
Ahh, I am sorry :( I am pretty sure I missed to fallback to 3 when the other PR was reverted :(
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's fine, as long as we understand what happened :)
@@ -1160,8 +1160,8 @@ func (proxier *Proxier) syncProxyRules() { | |||
|
|||
// Capture load-balancer ingress. | |||
fwChain := svcInfo.serviceFirewallChainName | |||
for _, ingress := range svcInfo.LoadBalancerIPStrings() { | |||
if ingress != "" { | |||
for _, ingress := range svcInfo.LoadBalancerIngress() { |
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.
For future - it might have made these reviews a touch easier if there was a single "prefactoring" commit that made this (effectively no-op) structural change, and then the "real" commit would have been that much easier to see.
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.
Noted! (I can still split it if needed 😄 )
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.
@Sh4d1 ,
One of my colleagues has a vested interest in moving this forward. Is it possible to go ahead and split things up as @thockin suggested? I imagine when he is able to conduct a Prod Readiness review that he'll need to start from scratch anyway due to the nearly two year gap in between the above comment and now. Since it would make it easier, maybe split them? I can help too if you like.
) | ||
// jump to service firewall chain | ||
writeLine(proxier.natRules, append(args, "-j", string(fwChain))...) | ||
if !utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) || *ingress.IPMode == v1.LoadBalancerIPModeVIP { |
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.
How about capturing this as a helper function like:
func isVIP(ing *LoadBalancerIngress) bool {
if !utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) {
return true // backwards compat
}
return ing.IPMode == v1.LoadBalancerIPModeVIP
}
This makes the eventual cleanup easier. We should probably be doing more of this.
for _, ingress := range svcInfo.LoadBalancerIPStrings() { | ||
if ingress != "" { | ||
for _, ingress := range svcInfo.LoadBalancerIngress() { | ||
if ingress.IP != "" && (!utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) || *ingress.IPMode == v1.LoadBalancerIPModeVIP) { |
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.
same comment re: helper function
|
||
if len(ips) > 0 { | ||
ipFamilyMap = utilproxy.MapIPsByIPFamily(ips) | ||
correctIngresses, incorrectIngresses := utilproxy.FilterIncorrectLoadBalancerIngress(service.Status.LoadBalancer.Ingress, sct.ipFamily) |
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 switched from a "filter incorrect" model to a "map by family" model because it is easier to think about and more copyable as a pattern. Can we convert this to that model, please?
var invalidIngresses []v1.LoadBalancerIngress | ||
|
||
for _, ing := range ingresses { | ||
correctIP := MapIPsByIPFamily([]string{ing.IP})[ipFamily] |
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 feels like going out of the way to use the existing API, but it doesn't really fit. You're going to waste time mapping by IP when you KNOW there's just one IP. I think it would be cleaner to iterate the input list, map by family, then return that map.
@@ -43,3 +45,15 @@ func SetDefaults_NetworkPolicy(obj *networkingv1.NetworkPolicy) { | |||
} | |||
} | |||
} | |||
|
|||
func SetDefaults_Ingress(obj *networkingv1.Ingress) { |
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 did not realize this struct was reused for Ingress! I don't think this is right. It happens to work for the fields that exist today, but I don't know of any Ingress implementation that is VIP-like.
I think we should probably clone the definition of type LoadBalancerStatus
into the networking apigroup.
@bowei @rramkumar1 @aledbf - agree or disagree?
@@ -43,3 +45,15 @@ func SetDefaults_NetworkPolicy(obj *networkingv1.NetworkPolicy) { | |||
} | |||
} | |||
} | |||
|
|||
func SetDefaults_Ingress(obj *networkingv1.Ingress) { |
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.
And if we do that, it should be a standalone prefactoring commit. :)
@k8s-triage-robot: Closed this PR. 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/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Sh4d1 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 |
@Sh4d1: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
Is this PR still needed, please rebase if so (or we can close it?) |
I think they are waiting for #106242 to be accepted/merged. |
It's still in my queue
…On Mon, Jan 10, 2022 at 9:14 AM Niclas Schad ***@***.***> wrote:
Is this PR still needed, please rebase if so (or we can close it?)
I think they are waiting for #106242
<#106242> to be accepted.
—
Reply to this email directly, view it on GitHub
<#97681 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVAYCTWWFEW3DFP2L33UVMHW3ANCNFSM4VSYUJKA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
It seems unlikely thatr I will get to this as a take-over for the 1.24 cycle |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. 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/test-infra repository. |
@@ -1230,7 +1232,7 @@ func (proxier *Proxier) syncProxyRules() { | |||
"-A", string(kubeExternalServicesChain), |
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 not wirte reject rule when IPMode == v1.LoadBalancerIPModeProxy?
Hey, now that #106242 is merged should we proceed with this PR or did we move the functionality to a different PR? |
If someone wants to pick it up, great! Otherwise it is in my queue
for "eventually"
…On Thu, Nov 3, 2022 at 1:09 AM Niclas Schad ***@***.***> wrote:
Hey,
now that #106242 is merged should we proceed with this PR or did we move the functionality to a different PR?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you modified the open/close state.Message ID: ***@***.***>
|
I am taking a look. Tim, I may reach out on Slack. Thanks! |
This one has been implemented now. |
Signed-off-by: Patrik Cyvoct patrik@ptrk.io
What type of PR is this?
/kind feature
What this PR does / why we need it:
Implements kubernetes/enhancements#1392
Which issue(s) this PR fixes:
Fixes #79783
Fixes #66607
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/assign @thockin