Skip to content
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

KEP: Adding AppProtocol to Services and Endpoints #1422

Merged
merged 1 commit into from Jan 25, 2020

Conversation

@robscott
Copy link
Member

robscott commented Dec 27, 2019

The lack of standardized application protocol support for Services and Endpoints has frustrated countless Kubernetes end users. With EndpointSlices, we added a new AppProtocol field as a first step, adding that same field to Services and Endpoints will ensure full API support for application protocols.

This issue was initially raised here where @thockin suggested filing a KEP. I'm happy to also file a PR with implementation if this is something we want to proceed with. I'd love to get this into 1.18 if possible.

/sig network
/cc @thockin @dcbw

@robscott

This comment has been minimized.

Copy link
Member Author

robscott commented Dec 27, 2019

/assign @thockin

Copy link
Member

thockin left a comment

I'm fine with this, but I want you to do 2 things:

  1. Let this cook for a week or two past the holidays, and socialize it at the next sig-net
  2. Reach out to folks who own the existing annotations and see if we can get them to support this

/approve
/lgtm
/hold

You can remove the hold when the above is done ;)

@lachie83

This comment has been minimized.

Copy link
Member

lachie83 commented Jan 3, 2020

cc @feiskyer @andyzhangx @khenidak @aramase - can you please review?

@M00nF1sh

This comment has been minimized.

Copy link
Contributor

M00nF1sh commented Jan 4, 2020

I'm good with this since it's ready included in EndpointSlice object.(And can help add support for this from AWS side)

Only concern is that the root problem(in my understanding) is we are lacking support to configure application-specific hints on per port basis. e.g.HealthCheckProtocol/HealthCheckInterval/SSLCertificate/etc, so this will only solves a subnet of the problem.

/lgtm

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Jan 4, 2020

Copy link
Member

feiskyer left a comment

Looks good in general, but I'm agreeing with @M00nF1sh that even with this new AppProtocol, other annotations may still be required to fully support the feature.

Take https for example, SSLCertificate is also required to make it work. So another annotation is still required to use the https protocol.

Do we consider to support more protocol details (e.g. application options) together with this new property?

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Jan 6, 2020

@khenidak

This comment has been minimized.

Copy link

khenidak commented Jan 6, 2020

Thanks @robscott -

Let us talk about this in sig-network. I fail to see the value that justifies the complexities that this will bring. In a lot of ways, this moves services from objects that describes L4 to objects that describe L4 and L7

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Jan 6, 2020

@robscott robscott force-pushed the robscott:app-protocol-kep branch from 27e165c to 4e9cce5 Jan 6, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm label Jan 6, 2020
@mattmoor

This comment has been minimized.

Copy link

mattmoor commented Jan 7, 2020

@thockin it would be useful to be able to support such a hint in the PodSpec itself as well.

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Jan 9, 2020

@mattmoor Can you talk about the use case distinct from Service?

@mattmoor

This comment has been minimized.

Copy link

mattmoor commented Jan 9, 2020

It's effectively the same as for a Service, but there are use cases (e.g. Knative) where we have a need to let the user specify the application protocol where they can provide a PodSpec (incl. ports), but we synthesize the K8s Service and related Ingress abstractions around that.

Currently we have the pleasure of following in the Istio footsteps you are so fond of, but would love something better. 🤣

The simplest example of where a PodSpec level hint would help is: How would this hint get populated on Services created via kubectl expose? 🤷‍♂

@hbagdi

This comment has been minimized.

Copy link

hbagdi commented Jan 10, 2020

This would help the Ingress use-case quite a bit!

Questions:

  • How would this work with #1438?
    There are usually multiple protocols above L4 that are possible on the same port via ALPN, upgrade headers or what not. We often expose gRPC, HTTP/2, HTTP/1.1 (all on top of TLS) on the same port (443). And in future, HTTP/3 will be added into that list.
  • Will Kubernetes itself make use of this or is it only a hint for other applications and integrations? Seems like later, but then is it possible to add guidelines on who decides the protocol to be set? Imagine that my app runs gRPC, which uses HTTP/2.0 for transport. Now, a health-check service will want to treat the AppProtocol as HTTP, but a proxy will want gRPC.
@robscott

This comment has been minimized.

Copy link
Member Author

robscott commented Jan 14, 2020

Hey @hbagdi, thanks for the great questions! It seems like there is a fairly common desire to support multiple application protocols per port number. Under the current restraints, the most straightforward way to do that would be to transform the proposed AppProtocol field into an AppProtocols list.

Alternatively, if #1438 is approved and removes the requirement for port numbers in a list to be unique, that could provide a way to specify multiple app protocols simply by having multiple port entries.

Either way, this is not a field Kubernetes itself would use, but would be a hint for consumers of the API. Not all consumers will support all app protocols, but the field can serve as a hint for those that do.

@thockin How would you feel about supporting multiple app protocols per port?

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Jan 25, 2020

@mattmoor I can perhaps buy that, but it probably warrants a small KEP of its own.

@hbagdi I think I'd like to start small and expand if we can see really concrete use cases. I admit I don't know enough about them. I'm going to look at and hopefully approve this KEP as is, but if we can flesh out why multiple app-protocols is really important, that seems minor enough not to invalidate this. Note that we would want to carry that through to EndpointSlice, which already exists in the singular form.

Copy link
Member

thockin left a comment

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 25, 2020
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 25, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: M00nF1sh, robscott, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@robscott

This comment has been minimized.

Copy link
Member Author

robscott commented Jan 25, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 15adb4c into kubernetes:master Jan 25, 2020
3 checks passed
3 checks passed
cla/linuxfoundation robscott authorized
Details
pull-enhancements-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jan 25, 2020
@hbagdi hbagdi mentioned this pull request Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.