Different protocols in the same Service definition with type=LoadBalancer #1438
Conversation
|
Welcome @janosi! |
|
Hi @janosi. Thanks for your PR. I'm waiting for a kubernetes 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/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
k8s-ci-robot
added
kind/kep
|
/assign @caseydavenport |
|
|
||
| Once that limit is removed those Service definitions will reach the Cloud Provider LB controller implementations. The logic of the particular Cloud Provider LB controller and of course the actual capabilities and architecture of the backing Cloud Load Balancer services determines how the actual exposure of the application really manifests. For this reason it is important to understand the capabilities of those backing services and to design this feature accordingly. | ||
|
|
||
| ### User Stories [optional] |
|
|
||
| As a Kubernetes cluster user I want to expose an application that provides its service on different protocols with a single cloud provider load balancer IP. In order to achieve this I want to define different `ports` with mixed protocols in my Service definition of `type: LoadBalancer` | ||
|
|
||
| ### Implementation Details/Notes/Constraints [optional] |
There was a problem hiding this comment.
| ### Implementation Details/Notes/Constraints [optional] | |
| ### Implementation Details/Notes/Constraints |
|
|
||
| ### Option Control Alternatives | ||
|
|
||
| #### Annotiation in the Service definition |
There was a problem hiding this comment.
| #### Annotiation in the Service definition | |
| #### Annotation in the Service definition |
| #### OpenStack | ||
|
|
||
| The OpenStack CPI supports TCP, UDP, HTTP(S) in Service definitions and can configure the Octavia listeners with the protocols defined in the Service. | ||
| OpenStack Octavia supports TCP, UDP and HTTP(S) on listeners, an own listener must be configured for each protocol, and different listeners can be used on the same LB instance. | ||
|
|
||
| Summary: the OpenStack based clouds that use Octavia v2 as their LBaaS seems to support this feature once implemented. Pricing is up to their model. |
There was a problem hiding this comment.
Octavia version should >= 5.0.0 which first release in OpenStack Train
There was a problem hiding this comment.
@thinalai Thank you for the comment! Indeed, I can see this bugfix in the 5.0.0 release notes
Fixed bug which prevented the creation of listeners for different protocols on the same port (i.e: tcp port 53, and udp port 53).
I add this note to the KEP.
|
For reference, the initial issue/PRs that added validation against mixed protocols: |
|
|
||
| [GCP forwarding_rules_charges](https://cloud.google.com/compute/network-pricing#forwarding_rules_charges) suggest that the same K8s Service definition would result in the creation of 2 forwarding rules in GCP. This has the same fixed price up to 5 forwarding rule instances, and each additional rule results in extra cost. | ||
|
|
||
| A user can ask for an internal TCP/UDP Load Balancer via a K8s Service definition that also has the annotation `cloud.google.com/load-balancer-type: "Internal"`. Forwarding rules are also part of the GCE Internal TCP/UDP Load Balancer architecture, but in case of Internal TCP/UDP Load Balancer it is not supported to define different forwarding rules with different protocols for the same IP address. That is, for Services with type=LoadBalancer and with annotation `cloud.google.com/load-balancer-type: "Internal"` this feature would not be supported. |
There was a problem hiding this comment.
How would a user find out this feature isn't supported for this particular case? It sounds like either searching logs or realizing the LB is not serving the forwarding rules as expected?
There was a problem hiding this comment.
This is an interesting point. The current GCP CPI implementation takes the first protocol value it finds in the Service definition and uses that for all ports in the Service when creating an internal LB. There is no check for violations of the "single protocol" rule, i.e. it assumes that the filter in the K8s API already blocked a mixed protocol setup.
If this filter is removed by this feature then - according to my understanding - the GCP CPI will setup an internal load balancer with the same protocol on all forwarding rules, even if the trigger was a Service with mixed protocol setup. There is no error feedback to the user.
There was a problem hiding this comment.
We do not have arbitrary status, but we could add it. status.loadBalancer could grow a field or two. Or we could use events.
There was a problem hiding this comment.
Indeed. Thank you!
Which solution would you prefer: the new field(s) in the status.loadBalanceror the events?
I do not see them as contradictory, though. Events are moved to external troubleshooting systems pretty fast, so maybe it is worth to keep the actual state of the Service in-cluster too.
There was a problem hiding this comment.
I would love to see improvements to the LB status fields if possible.
At the very least, there should be some form of conditions to allow for better extensibility.
There was a problem hiding this comment.
Agree - status for this would be good, and events are orthogonal (both are good). That said this limitation has been removed :)
There was a problem hiding this comment.
Do you mean users can use different protocols on internal GCE LBs now?
I assume, documentation update is also coming? Here it still states that multiple forwarding rules for the same IP can have either UDP or TCP, but not both: https://cloud.google.com/load-balancing/docs/internal#multiple_forwarding_rule
And backend services also has similar limitation: https://cloud.google.com/load-balancing/docs/internal#backend-service
Or it is only my bad English parser :)
There was a problem hiding this comment.
Do you mean users can use different protocols on internal GCE LBs now?
Yes :)
I'll ping docs people. See https://cloud.google.com/kubernetes-engine/docs/how-to/internal-load-balancing#enabling_shared_ip
|
|
||
| ## Proposal | ||
|
|
||
| The first thing proposed here is to lift the hardcoded limitation in Kubernetes that currently rejects Service definitions with different protocols if their type is LoadBalancer. Kubernetes would not reject Service definitions like this from that point: |
There was a problem hiding this comment.
hardcoded limitation -> API validation
janosi
force-pushed
the
mixedprotocollb
branch
from
b2993d8 to
860cb18
Compare
Fixed. Squashed my internal (i.e. before PR creation) commits into one. |
|
|
||
| [GCP forwarding_rules_charges](https://cloud.google.com/compute/network-pricing#forwarding_rules_charges) suggest that the same K8s Service definition would result in the creation of 2 forwarding rules in GCP. This has the same fixed price up to 5 forwarding rule instances, and each additional rule results in extra cost. | ||
|
|
||
| A user can ask for an internal TCP/UDP Load Balancer via a K8s Service definition that also has the annotation `cloud.google.com/load-balancer-type: "Internal"`. Forwarding rules are also part of the GCE Internal TCP/UDP Load Balancer architecture, but in case of Internal TCP/UDP Load Balancer it is not supported to define different forwarding rules with different protocols for the same IP address. That is, for Services with type=LoadBalancer and with annotation `cloud.google.com/load-balancer-type: "Internal"` this feature would not be supported. |
There was a problem hiding this comment.
We do not have arbitrary status, but we could add it. status.loadBalancer could grow a field or two. Or we could use events.
| - if a user is happy with 2 NLB (Service) instances for TCP and UDP still the user has two more forwarding rules to be billed - i.e. it has the same effect on pricing as if those TCP and UDP endpoints were behind the same NLB instance | ||
| - already now the bills of users is affected if they have more than 5 forwarding rules as the result of their current Service definitions (e.g. 6 or more port definitions in a single Serice, or if the number of all port definitions in different Services is 6 or more, etc.) | ||
|
|
||
| That is, if we consider the "single Service with 6 ports" case the user has to pay more for that single Service ("single LB instance") than for another Service (another LB instance) with 5 or less ports already now. It is not the number of LBs that matters. This phenomenon is already there with the current practice, and the enabling of mixed protocols will not change it to the worse. |
There was a problem hiding this comment.
This is not right? We can (and do) set up a single Forwarding Rule for multiple ports.
There was a problem hiding this comment.
You are right, I was wrong here. Excuse me for this, fixed.
|
|
||
| The other risk is in the introduction of this feature without an option control mechanism, i.e. as a general change in Service handling. In that case there is the question whether this feature should be a part of the conformance test set, because it can affect the conformance of cloud providers. | ||
|
|
||
| A possible mitigation is to put the feature behind option control. |
There was a problem hiding this comment.
If we allow existing services to have this condition in 1.18 (e.g. implementing it and not rejecting updates to objects that already are in this state) but not allow new services (e.g. rejecting a create or an update that enters this state), then we can allow create in 1.19, knowing that rollback to 1.18 is safe.
I think? Am I missing anything?
There was a problem hiding this comment.
Seems right to me -- we followed the same approach when we added finalizers for Service LBs. The only thing to consider is the code path won't be vetted as much if we don't allow new services to use it at all, at the very least there should be an opt-in for mixed protocols so we can verify it works on v1.18 but we need to make sure users know the consequence of enabling (i.e. no rollback). Maybe we do need a feature gate?
There was a problem hiding this comment.
yeah, these changes are always nuanced. I think the right thing is to allow existing uses and allow new uses IFF gate is enabled. That puts anyone who uses it under the gate at rollback risk.
I have asked @liggitt for a consult, since he is involved in all of these :)
There was a problem hiding this comment.
I think the right thing is to allow existing uses and allow new uses IFF gate is enabled.
That's correct. The essential thing that default configurations (e.g. no opt into alpha feature gate) of version N does not allow new uses (via creation or update of an object that doesn't currently use the relaxed validation) that would be considered invalid by version N-1.
That puts anyone who uses it under the gate at rollback risk.
That is why we put relaxed validation and new fields in GA objects under an alpha gate for one release, and limit API server rollback/skew to a single version.
There was a problem hiding this comment.
Strictly speaking - it doesn't HAVE to be "alpha", it could just be off-by-default beta (not much difference practically).
There was a problem hiding this comment.
To resolve this comment, change this to reference the definitive plan, as opposed to options.
There was a problem hiding this comment.
Fixed. Let's see if it is better now. Please check.
| The implementation of the basic logic is ready in this PR: | ||
| https://github.com/kubernetes/kubernetes/pull/75831 | ||
|
|
||
| Currently a feature gate is used to control its activation status. Though if we want to keep this feature behind option control even after it reaches its GA state we should come up with a different solution, as feature gates are used to control the status of a feature as that graduates from alpha to GA, and they are not meant for option control for features with GA status. |
There was a problem hiding this comment.
I'm not sure an alpha is warranted here, but maybe. It's not a huge change, except in the cloud providers it could be. Thoughts?
There was a problem hiding this comment.
Yeah I think we skip alpha as long as rollbacks don't break LBs as you mentioned here #1438 (comment)
There was a problem hiding this comment.
Do I understand right then that the agreed change is the following:
- in 1.18 creating a mixed Service is allowed if the feature flag is ON. We add notification to the documentation that in this case rollback is not possible, or at least can break things depending on the cloud provider implementation
- in 1.18 we allow update of such Services even if the feature flag in OFF
- in 1.19 we change the create path so, that even if the feature flag is OFF mixed Services can be created
?
Thank you!
There was a problem hiding this comment.
in 1.18 creating a mixed Service is allowed if the feature flag is ON. We add notification to the documentation that in this case rollback is not possible, or at least can break things depending on the cloud provider implementation
That's correct, and that means the feature gate should be alpha and disabled by default (we should not enable something by default that breaks skew/rollback).
in 1.18 we allow update of such Services even if the feature flag in OFF
Correct. That either means someone enabled-then-disabled the feature flag, or someone created the service via a future release API server. In both cases, we want to preserve/allow update of that object.
in 1.19 we change the create path so, that even if the feature flag is OFF mixed Services can be created
?
Typically, we would leave the create/update logic the same as in 1.18, and simply promote the feature flag to beta and enable it by default in 1.19. That gives a release to get broader feedback and bug reports, but allows someone to turn off the feature and stop allowing new uses if problems are found, without cutting a new Kubernetes 1.19.x patch release.
Once the feature graduates to GA and is locked on (e.g. in 1.20), then all feature-gate-conditional logic can be dropped.
There was a problem hiding this comment.
@liggitt @thockin @andrewsykim I think I updated the document according to this logic. Please check and suggest how to continue.
|
That makes sense.
…On Thu, Jan 16, 2020, 3:24 PM Andrew Sy Kim ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In keps/sig-network/20200103-mixed-protocol-lb.md
<#1438 (comment)>
:
> +Our feature does not introduce new values or new fields. It enables the usage of an existing value in existing fields, but with a different logic. I.e. if someone creates a Service with mixed protocol setup and then rollbacks the API server to a version that does not implement this feature the clients will still get the Service with mixed protocols when they read that via the rollback'd API. If the client (CPI implementation) has been rollbacked, too, then the client may receive such a Service setup that it does not support.
+
+- Alibaba: no risk. The current CPI and LB already supports the mixed protocols in the same Service definition. If this feature is enabled in an API server and then the API server rollback is executed the CPI can still handle the Services with mixed protocol sets.
+- AWS: no risk. The current CPI accepts Services with TCP protocol only, i.e. after a K8s upgrade a user still cannot use this feature. Consequently, a rollback in the K8s version does not introduce any issues.
+- Azure: no risk. The current CPI and LB already supports the mixed protocols in the same Service definition. The situation is the same as with the Alibaba CPI.
+- GCE: currently the GCE CPI assumes that a Service definition contains a single protocol value, as it assumes that the Service Controller already rejected Services with mixed protocols. While the Service Controller really did so a while ago, it does not do this anymore. It means a risk.
+- IBM Cloud VPC: no risk. The same situation like in the case of AWS.
+- IBM Cloud Classic: no risk. The CPI and NLB already supports TCP and UDP in the same Service definition. The same situation like in the case of Alibaba.
+- OpenStack: no risk. The CPI and NLB already supports TCP and UDP in the same Service definition. The same situation like in the case of Alibaba.
+- Oracle: no risk. The CPI and LB already supports mixed protocols. The same situation like in the case of Alibaba.
+- Tencent Cloud: no risk. The CPI and LB already supports mixed protocols. The same situation like in the case of Alibaba.
+
+
+The other risk is in the introduction of this feature without an option control mechanism, i.e. as a general change in Service handling. In that case there is the question whether this feature should be a part of the conformance test set, because it can affect the conformance of cloud providers.
+
+A possible mitigation is to put the feature behind option control.
Seems right to me -- we followed the same approach when we added
finalizers for Service LBs. The only thing to consider is the code path
won't be vetted as much if we don't allow new services to use it at all, at
the very least there should be an opt-in for mixed protocols so we can
verify it works on v1.18 but we need to make sure users know the
consequence of enabling (i.e. no rollback). Maybe we do need a feature gate?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1438?email_source=notifications&email_token=ABKWAVF37SUFCU2P4BL4LV3Q6DUEBA5CNFSM4KEOJSSKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCSCVH5Q#discussion_r367701090>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVCZMVNVBP6FVG6PTXLQ6DUEBANCNFSM4KEOJSSA>
.
|
|
@caseydavenport @thockin @andrewsykim Do you think there is a chance to get it into 1.18 enhancements? Or do you see some gaps here? Thank you! |
| - a feature flag shall control whether loadbalancer Services with mixed protcol configuration are accepted or not at validation time | ||
| - we must add a note to the documentation that if such Service is created then it may break things after a rollback - it depends on the cloud provider implementation | ||
| - the update of such services shall be possible even if the feature flag is OFF. This is to prepare for the next release when the feature flag is removed from the create path, too, and after a rollback to the first release the update of existing Service objects must be possible | ||
| - the CPI implementations shall be prepared to deal with Services with mixed protocol configurations. It does not mean that the CPI implementations have to accept such Services. It means that the CPI implementations shall be able to process such Services. The CPI implementation can still reject such a Service if it cannot manage the corresponding cloud load balancers for those Services, or for any other reasons. The point is, that the system (Service - CPI implementation - cloud LB) must be in a consitent state in the end, and the user must have meaningful information about that state. For exempla, if the Service is not accepted by the CPI implementation this fact shall be available for the user. |
There was a problem hiding this comment.
What does it mean to "reject" a service?
There was a problem hiding this comment.
Well, just like currently some CPIs do not accept a service if for example not all ports are TCP. My assumption was that that is a kind of accepted way how a CPI can "reject" a service. Similarly, the CPI can still "reject" the service on the same way if it does not want to support this feature.
| #### Proposed solution | ||
|
|
||
| In the first release: | ||
| - a feature flag shall control whether loadbalancer Services with mixed protcol configuration are accepted or not at validation time |
|
@caseydavenport @thockin @andrewsykim Would you like to see some more updates on this KEP? |
|
@thockin event though we missed the 1.18 deadline with this one, I would like to progress. Please advise what is next. If the KEP is good to go, I can update the necessary meta fields, and it can be merged then. If there are still things to update in it, I would like to know those. |
|
Hope this helps with triage: In 2019, this feature request was apparently added to milestones: v1.17, v1.18 in kubernetes/kubernetes#80394 Other PRs and Issues seem related:
|
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
the
sig/architecture
|
|
||
| In the first release: | ||
| - a feature flag shall control whether new loadbalancer Services with mixed protcol configuration can be created or not | ||
| - we must add a note to the documentation that if such Service is created then it may break things after a rollback - it depends on the cloud provider implementation | ||
| - the update of such Services shall be possible even if the feature flag is OFF. This is to prepare for the next release when the feature flag is removed from the create path, too, and after a rollback to the first release the update of existing Service objects must be possible | ||
| - the CPI implementations shall be prepared to deal with Services with mixed protocol configurations. It does not mean that the CPI implementations have to accept such Services. It means that the CPI implementations shall be able to process such Services. The CPI implementation can still validate the Service objects to check whether any of those has mixed protocol configuration and if the CPI cannot support such Service for any reasons the CPI shall indicate that clearly to the user, and the CPI should not create Cloud LB resources for the "accepted part" of the Service. The point is, that the system (Service - CPI implementation - cloud LB) must be in a consitent state in the end, and the user must have meaningful information about that state. For example, if the Service is not accepted by the CPI implementation this fact shall be available for the user, either as a new field in `Service.status.loadBalancer` or as a new Event (or both). | ||
| - the CPI implementations shall be prepared to deal with Services with mixed protocol configurations. Either via supporting such Service definitions, or clearly indicating to the users that the Service could not be processed as specified. As we can see from our analysis some CPIs support other protocols than TCP and UPD in the Service definitions, while others support only TCP and UDP. That is the term "mixed protocol support" does not always mean that all possible protocol values are supported by a CPI. For this reason a nicely behaving CPI shall |
|
|
||
| [GCP forwarding_rules_charges](https://cloud.google.com/compute/network-pricing#forwarding_rules_charges) suggest that the same K8s Service definition would result in the creation of 2 forwarding rules in GCP. This has the same fixed price up to 5 forwarding rule instances, and each additional rule results in extra cost. | ||
|
|
||
| A user can ask for an internal TCP/UDP Load Balancer via a K8s Service definition that also has the annotation `cloud.google.com/load-balancer-type: "Internal"`. Forwarding rules are also part of the GCE Internal TCP/UDP Load Balancer architecture, but in case of Internal TCP/UDP Load Balancer it is not supported to define different forwarding rules with different protocols for the same IP address. That is, for Services with type=LoadBalancer and with annotation `cloud.google.com/load-balancer-type: "Internal"` this feature would not be supported. |
There was a problem hiding this comment.
Do you mean users can use different protocols on internal GCE LBs now?
Yes :)
I'll ping docs people. See https://cloud.google.com/kubernetes-engine/docs/how-to/internal-load-balancing#enabling_shared_ip
| "description": "The protocol for this port", | ||
| "type": "string" | ||
| }, | ||
| "name": { |
There was a problem hiding this comment.
I don't think we need name in status
| "description": "The name of this port within the service.", | ||
| "type": "string" | ||
| }, | ||
| "ready": { |
There was a problem hiding this comment.
Are there more states that a single port or set of ports might be in? I'm wondering if bool is right or enum or even Conditions. Bool is simple, maybe good enough...
There was a problem hiding this comment.
I have been thinking about it, but could not find more states for the time being.
| "description": "Specifies whether the port was configured for the load-balancer.", | ||
| "type": "boolean" | ||
| }, | ||
| "message": { |
There was a problem hiding this comment.
We don't have a lot of precedent on this sort of thing, other than Conditions. Perhaps simply mapping ports here, and adding Conditions to status would be more expressive. E.g.:
conditions:
- type: LoadBalancerMixedProtocolNotSupported
There was a problem hiding this comment.
I created a "conditions" field right into "Service.status.loadBalancer", and defined the first condition with this name.
There was a problem hiding this comment.
While we're at it, should there be a condition for just this protocol is not supported? i.e. for CPI that can do LB mixed protocols, but the protocol for this port cannot work, either because it's flat-out not supported, e.g., UDP on AWS ELB, or because for some reason the Load Balancer cannot be built with multiple protocols when this one is present.
At least in the AWS UDP-on-ELB case, this would be better than just an event saying 'We couldn't do that' as we get now. The event still carries details, but being able to set a condition would be nice too.
There was a problem hiding this comment.
Do you think the new "portStatuses" could be used to indicate problems with the particular ports? I.e. the "Service.status.conditions" would show problem with the mixed protocol setup, and then the "ingress.portStatuses" could be used to provide more information.
| }, | ||
| "type": "array" | ||
| }, | ||
| "portStatuses": { |
There was a problem hiding this comment.
I think we want ports under ingress - ingress is a plural on purpose, so ports should be tracked based on each ingress point.
There was a problem hiding this comment.
Fixed, moved the portStatuses under ingress.
| "description": "LoadBalancerStatus represents the status of a load-balancer.", | ||
| "properties": { | ||
| "ingress": { | ||
| "description": "Ingress is a list containing ingress points for the load-balancer. Traffic intended for the service ld be sent to these ingress points.", |
There was a problem hiding this comment.
"ld"? Checking the docs for LoadBalancerStatus, that should be "should". Edit: I also recognise (now) that it's not intended to be changed here, as this is a pre-existing structure.
There was a problem hiding this comment.
Hmmm, bad copy-paste. Fixed.
Move portStatus inside the ingress definition of an LB. Add a conditions field to the Service.status.loadBalancer and add the first official condition "LoadBalancerMixedProtocolNotSupported to it.
k8s-ci-robot
added
size/XL
|
|
||
| ```json | ||
| "io.k8s.api.core.v1.LoadBalancerCondition": { |
There was a problem hiding this comment.
I have to say, it's considerably harder to review this as openAPI than as Go code :)
There was a problem hiding this comment.
Yep, it sounded like a good choice for API specification. 😄 +1 that Go would be easier 😄
| ], | ||
| "type": "object" | ||
| }, | ||
| "io.k8s.api.core.v1.PortStatus": { |
There was a problem hiding this comment.
Is kube-proxy expected to consume this in any way into iptables/IPVS rules or is this just for reporting? Might be worth clarifying this
There was a problem hiding this comment.
Good question. If IPVS and iptables set up captures for the LBVIP + proto + port, and the LB itself doesn't support mixed protocols, should kube-proxy also not capture them?
There was a problem hiding this comment.
I wonder about the exact handling. I mean, no rules at all, or rather drop those packets that are anyway not handled by the LB on the external interface?
There was a problem hiding this comment.
So, I added a kube-proxy chapter. Please check and if you are OK with that please remove the hold flag from this one.
thockin
added
the
tide/merge-method-squash
|
I'm going to Approve this now, but I like Andrew's question. We can either answer it as a followup or as one more rev on this. Remove the hold as you see fit. Also, I set the "squash" merge option for this, so you don't need to manually squash. /approve |
k8s-ci-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: janosi, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
k8s-ci-robot
removed
the
lgtm
|
...ahh, and a new lgtm if you are still OK with this one. |
thockin
removed
the
do-not-merge/hold
|
/lgtm hold removed |
k8s-ci-robot
added
the
lgtm
This is the KEP for
kubernetes/kubernetes issue kubernetes/kubernetes#23880
kubernetes/kubernetes PR kubernetes/kubernetes#75831
kubernetes/enhancements issue: #1435