search

kubernetes / enhancements

Enhancements tracking repo for Kubernetes
Apache License 2.0
3.44k stars 1.48k forks source link

Support of mixed protocols in Services with type=LoadBalancer #1435

Closed janosi closed 1 year ago

janosi commented 4 years ago

Enhancement Description

KEP document: https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20200103-mixed-protocol-lb.md

GA k/k PR in v1.26: https://github.com/kubernetes/kubernetes/pull/112895

Please to keep this description up to date. This will help the Enhancement Team track efficiently the evolution of the enhancement

janosi commented 3 years ago

@feiskyer I do not think it is a bug. It is a desired new function of kube-proxy, described in the KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1435-mixed-protocol-lb#kube-proxy

andrewsykim commented 3 years ago

@janosi are you able to work on this for v1.23? Otherwise, I think other folks might be interested in taking this up

aojea commented 3 years ago

I've tried to move it forward this cycle, but I couldn't find the cloud providers information

All of the major clouds support this or indicate non-support properly

it has to meet the alpha to beta criteria https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1435-mixed-protocol-lb#alpha---beta-graduation

@andrewsykim we should check the beta criteria too, you are more familiar than me with cloud providers, do we have at least 2 major cloud providers implementing this?

janosi commented 3 years ago

@andrewsykim Unfortunately, I cannot promise I could deal with this one :( @aojea Thank you for moving it forward!

andrewsykim commented 3 years ago

@cheftako has a proposal open right now that introduces a webhook frame in the CCM component https://github.com/kubernetes/enhancements/pull/2928, which would also allow for cloud providers to easily run validation webhooks for mixed protocols, this would make it safer for us to remove the existing validation logic in apiserver today.

andrewsykim commented 3 years ago

do we have at least 2 major cloud providers implementing this?

From discussing with @feiskyer I know that Azure has this need, will discuss with other folks as well to gather more input

kishorj commented 3 years ago

AWS customers have also requested for mixed protocol support and this would be a welcome change. The external AWS Load Balancer Controller already support mixed protocols for NLB (*with limitations), the in-tree controller doesn't work yet since it assumes the protocol to be the same for all ports. The in-tree controller needs to be fixed before this feature graduates to beta, please add it to the graduation criteria.

Limitation AWS NLB currently doesn't support adding separate TCP and UDP listeners for the same port. We might be able to use TCP_UDP type listeners, but will require that the target ports be the same for both TCP and UDP. We plan to add the TCP_UDP support to the AWS Load Balancer controller, not the in-tree controller.

Tasks

The AWS LB controller changes will be independent of the k8s release.

andrewsykim commented 3 years ago

@bowei @freehan @thockin can you comment on mixed protocol support for GCP? I think that in addition to input from AWS (@kishorj) and Azure (@feiskyer) would be enough for us to do justify the promotion to Beta in v1.23

thockin commented 3 years ago

It's not implemented. I don't think it's very complicated to implement, but the testing might be. I don't think it's on the near-term plan, though?

andrewsykim commented 3 years ago

I think not implemented is ok, but if we removed the validation logic and users tried to set mixed protocols on Services, would that result in weird failure states?

thockin commented 3 years ago

They would get half of what they asked for without very clear signals as to why.

On Fri, Sep 3, 2021 at 11:34 AM Andrew Sy Kim @.***> wrote:

I think not implemented is ok, but if we removed the validation logic and users tried to set mixed protocols on Services, would that result in weird failure states?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/kubernetes/enhancements/issues/1435#issuecomment-912732722, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKWAVGO2SFA4DOGHGCY2PDUAEIMPANCNFSM4KDYR43Q . 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.

andrewsykim commented 3 years ago

If the GCP cloud provider had validation logic to error early (without starting the LB creation) and emit an event would that be sufficient enough to graduate this to beta in v1.23? Trying to gauge if validation at the controller level is sufficient enough to unblock this feature or if we should explicitly call out that every cloud provider needs to have this fully implemented to move this forward.

The right fix is a cloud-provider specific validation webhook that can validate the port/protocol combination, but that depends on https://github.com/kubernetes/enhancements/pull/2928 which would not be widely available in v1.23.

thockin commented 3 years ago

I don't like standing in the way of progress. If you're signing up to finish the work in kube-proxy, Andrew I think it's probably OK to unblock this.

@bowei @freehan What do you think?

adampl commented 3 years ago

Why care if some cloud providers implement it or not? It's their responsibility to catch up with the API changes. If they don't care, they're free to offer an older K8s version which doesn't have this "problem".

aojea commented 3 years ago

Why care if some cloud providers implement it or not? It's their responsibility to catch up with the API changes. If they don't care, they're free to offer an older K8s version which doesn't have this "problem".

it's not about cloud providers, it is about users, so you don't have an API that may work or not. The user expects to set that field and work ...

bbkgh commented 3 years ago

Can we have this feature as Beta in 1.23?

rtheis commented 3 years ago

From an IBM Cloud perspective, we don't have immediate plans to leverage this feature. However, we can and will add code (and user feedback) to prevent mixed protocol LBs from being created until we support this feature. Thank you.

pillamk commented 3 years ago

Hello

I've tried to move it forward this cycle, but I couldn't find the cloud providers information

All of the major clouds support this or indicate non-support properly

it has to meet the alpha to beta criteria https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1435-mixed-protocol-lb#alpha---beta-graduation

@andrewsykim we should check the beta criteria too, you are more familiar than me with cloud providers, do we have at least 2 major cloud providers implementing this?

Hello @aojea @andrewsykim , thanks for driving this forward. May I know if there is any indicative Kubernetes Release (v 1.23 ?) this feature will move to atleast beta state? Thankyou.

bridgetkromhout commented 2 years ago

We discussed this briefly in sig-network today. We need to test across the major clouds to ensure we meet the alpha-to-beta graduation criteria (of support, or indicating non-support properly - we should avoid having the undefined state Tim mentions above). We also will need to update the production readiness review for the beta state.

We will aim for 1.24, as 1.23 code freeze is very soon.

Possibly to-do via @kishorj - "the in-tree controller doesn't work yet since it assumes the protocol to be the same for all ports. The in-tree controller needs to be fixed before this feature graduates to beta, please add it to the graduation criteria."

bridgetkromhout commented 2 years ago

The MixedProtocolLBService feature was added as alpha for 1.20. We'd like to move MixedProtocolLBService from alpha to beta after completing these items on the to-do list for Alpha -> Beta Graduation - targeting for K8s 1.24 release.

feiskyer commented 2 years ago

Update for Azure: Azure has already supported the feature and e2e has been added recently by https://github.com/kubernetes-sigs/cloud-provider-azure/pull/897.

bridgetkromhout commented 2 years ago

Update for Azure: Azure has already supported the feature and e2e has been added recently by kubernetes-sigs/cloud-provider-azure#897.

@feiskyer Thanks! is there any additional CPI work needed? It was mentioned in https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1435-mixed-protocol-lb#azure (edit: confirmed with @feiskyer that Azure is ready - no additional work needed)

rtheis commented 2 years ago

Update for IBM: We will be preventing mixed protocol LBs from being created until we support the feature. I'll provide an update when https://github.com/IBM-Cloud/cloud-provider-ibm contains the change.

rtheis commented 2 years ago

Update for IBM: PR merged and synced to open source to prevent mixed protocol LBs from being created until we support the feature: https://github.com/IBM-Cloud/cloud-provider-ibm/commit/5f8779cffb2d349f9dbe5d5a22e73da7eb6bcd14.

kishorj commented 2 years ago

Update for AWS: PR https://github.com/kubernetes/cloud-provider-aws/pull/287 to perform further validation. Feature gtg from AWS side.

pillamk commented 2 years ago

The MixedProtocolLBService feature was added as alpha for 1.20. We'd like to move MixedProtocolLBService from alpha to beta after completing these items on the to-do list for Alpha -> Beta Graduation - targeting for K8s 1.24 release.

Thanks a lot @bridgetkromhout . Looking forward for this to be in 1.24 version.

bridgetkromhout commented 2 years ago

Discussed in sig-network; will follow up with @aojea for kube-proxy and @robscott to ensure this going to beta won't break Google customers.

robscott commented 2 years ago

Chatted with @bowei about this. We'll work on this, no need to block on our account.

bridgetkromhout commented 2 years ago

Kube-proxy had some changes in https://github.com/kubernetes/kubernetes/pull/106030 - @aojea, are there additional updates needed to ensure that kube-proxy uses the port status information from Service.status.loadBalancer.ingress in order not to allow traffic to those ports that could not be opened by the load balancer either?

aojea commented 2 years ago

Kube-proxy had some changes in kubernetes/kubernetes#106030 - @aojea, are there additional updates needed to ensure that kube-proxy uses the port status information from Service.status.loadBalancer.ingress in order not to allow traffic to those ports that could not be opened by the load balancer either?

I was checking the KEP and I have doubts about the expected behavior for kube-proxy, the KEP says that it should drop the traffic to the port of the Service based on the Status field, @thockin @janosi please bear with me:

  1. I create a LB service with UDP and TCP ports
apiVersion: v1
kind: Service
metadata:
  name: mixed-protocol
spec:
  type: LoadBalancer
  ports:
    - name: dns-udp
      port: 53
      protocol: UDP
    - name: dns-tcp
      port: 53
      protocol: TCP
  selector:
    app: my-dns-server
  1. Unfortunately, my cloud provider doesn't allow it and I have an error on the Service.Status "I can't open port 53 UDP, sorry"

The KEP says that kube-proxy should drop that traffic, but LoadBalancer services are effectively a composition of a ClusterIP server (NodePort too but there is a KEP to decouple them and is not related to this problem)

I don't think that kube-proxy should drop the traffic that the LB can not forward, the Service is still valid on both ports internally, dropping traffic to one of the ports based on the Service Status set by the loadbalancer will make it inconsistent and racy.

rhockenbury commented 2 years ago

Hi @janosi! 1.24 Enhancements team here. Just checking in as we approach enhancements freeze at 18:00pm PT on Thursday Feb 3rd. This enhancement is targeting beta for 1.24.

Here’s where this enhancement currently stands:

The status of this enhancement is currently marked at risk. Please plan to merge #3196 before enhancements freeze for this enhancement to be tracked for the 1.24 cycle. Thanks!

bridgetkromhout commented 2 years ago

Hi, @rhockenbury - we're waiting for the Production Readiness Review to be approved by the PRR team.

thockin commented 2 years ago

I think the question is not about clusterIP traffic - that clearly should work (since multi-protocol is a feature). NodePort traffic is the same for the same reason. The question then becomes about VIP_like LB traffic. A packet to LBIP:LBPORT/wrong-protocol. Given that some LBs use NodePort and some use VIP, it would, at best, be inconsistent.

So I think we should not do extra work to drop it.

bridgetkromhout commented 2 years ago

Thanks, @aojea & @thockin - updated per feedback to clarify based on Tim's comment.

bridgetkromhout commented 2 years ago

The status of this enhancement is currently marked at risk. Please plan to merge #3196 before enhancements freeze for this enhancement to be tracked for the 1.24 cycle. Thanks!

Hi, @rhockenbury - with the merge of #3196 we believe we are ready for this enhancement to move from alpha to beta in the 1.24 cycle. Thanks!

rhockenbury commented 2 years ago

Looks good. I've moved this KEP to tracked on the enhancements tracking sheet. All set for enhancement freeze! 🚀

nate-double-u commented 2 years ago

Hi @bridgetkromhout :wave: 1.24 Docs lead here.

This enhancement is marked as Needs Docs for the 1.24 release.

Please follow the steps detailed in the documentation to open a PR against the dev-1.24 branch in the k/website repo. This PR can be just a placeholder at this time and must be created before Thursday, March 31st, 2022 @ 18:00 PDT.

Also, if needed take a look at Documenting for a release to familiarize yourself with the docs requirement for the release.

Thanks!

valaparthvi commented 2 years ago

Hi @bridgetkromhout & @thockin :wave: 1.24 Release Comms team here.

We have an opt-in process for the feature blog delivery. If you would like to publish a feature blog for this issue in this cycle, then please opt in on this tracking sheet.

The deadline for submissions and the feature blog freeze is scheduled for 01:00 UTC Wednesday 23rd March 2022 / 18:00 PDT Tuesday 22nd March 2022. Other important dates for delivery and review are listed here: https://github.com/kubernetes/sig-release/tree/master/releases/release-1.24#timeline.

For reference, here is the blog for 1.23.

Please feel free to reach out any time to me or on the #release-comms channel with questions or comments.

Thanks!

rhockenbury commented 2 years ago

Hi @janosi

I'm checking in as we approach 1.24 code freeze at 01:00 UTC Wednesday 30th March 2022.

Please ensure the following items are completed:

For this KEP, it looks like there aren't any K/K PRs. Let me know if there are any PRs that we should be tracking for 1.24 code freeze.

Let me know if you have any questions.

rhockenbury commented 2 years ago

Hi @janosi @bridgetkromhout

It looks like you are targeting beta for the v1.24 release. I was not able to find a k/k PR for promoting the MixedProtocolLBService feature flag to beta. Let me know if I missed something or if you will be opting out of beta for this release cycle.

gracenng commented 2 years ago

Hi, 1.24 Enhancements Lead here 👋. With code freeze now in effect, this enhancement has not met the criteria for the freeze and has been removed from the milestone.

As a reminder, the criteria for code freeze is:

All PRs to the kubernetes/kubernetes repo have merged by the code freeze deadline Feel free to file an exception to add this back to the release. If you plan to do so, please file this as early as possible.

Thanks! /milestone clear

bridgetkromhout commented 2 years ago

I was not able to find a k/k PR for promoting the MixedProtocolLBService feature flag to beta.

I missed this and will be filing - thanks.

bridgetkromhout commented 2 years ago

Hi @bridgetkromhout 👋 1.24 Docs lead here.

This enhancement is marked as Needs Docs for the 1.24 release.

Please follow the steps detailed in the documentation to open a PR against the dev-1.24 branch in the k/website repo. This PR can be just a placeholder at this time and must be created before Thursday, March 31st, 2022 @ 18:00 PDT.

Also, if needed take a look at Documenting for a release to familiarize yourself with the docs requirement for the release.

Thanks!

Hi @nate-double-u I've opened https://github.com/kubernetes/website/pull/32696 - thanks.

gracenng commented 2 years ago

Added this back to the sheet, all set here!

thockin commented 2 years ago

@bridgetkromhout is this to go GA in 25?

bridgetkromhout commented 2 years ago

@bridgetkromhout is this to go GA in 25?

@thockin I don't see a good reason it can't move forward, although I'm not sure the docs as they stand meet the currently-required standard. (Note that I did not pursue moving this forward in advance of the 1.25 enhancements freeze, so perhaps 1.26?)

thockin commented 2 years ago

Candidate for GA in 1.26

rhockenbury commented 2 years ago

/label tracked/yes /remove-label tracked/no /stage stable

janosi commented 2 years ago

@thockin @rhockenbury @bridgetkromhout I am sorry for being away from this this long. I can help in moving this forward, if you think so.

rhockenbury commented 2 years ago

Hello @janosi 👋, 1.26 Enhancements team here.

Just checking in as we approach enhancements freeze on 18:00 PDT on Thursday 6th October 2022.

This enhancement is targeting for stage stable for 1.26 (correct me, if otherwise)

Here's where this enhancement currently stands:

For this KEP, we would just need to update the following:

The status of this enhancement is marked as at risk. Please keep the issue description up-to-date with appropriate stages as well. Thank you!