search

devfile / api

Kube-native API for cloud development workspaces specification
Apache License 2.0
237 stars 58 forks source link

Finalize endpoints #84

Closed davidfestal closed 3 years ago

davidfestal commented 3 years ago

What does this PR do?

This PR implements the agreement about how container endpoints are managed in Devfile 2.0, as expressed in this comment: https://github.com/devfile/kubernetes-api/issues/33#issuecomment-629345779

What issues does this PR fix or reference?

https://github.com/devfile/kubernetes-api/issues/33#issuecomment-629345779

Is your PR tested? Consider putting some instruction how to test your changes

Yes, in che.openshift.io

davidfestal commented 3 years ago

I have a doubt about the path field. In the comment I followed (https://github.com/devfile/kubernetes-api/issues/33#issuecomment-629345779), it appears as a real specified field, but I seem to remember that at some point we had somehow discussed about putting it into the implementation-dependent attributes @kadel @l0rd do you remember ?

I assume it would make sense since the scheme (also used to build the final URL), is already in the implementation-dependent attributes.

davidfestal commented 3 years ago

Another question: in devfile 1.0, it seems endpoints are also available for other compoents, mainly kubernetes and openshift. Is it something we also want in devfile 2.0 ? @l0rd wdyt ?

l0rd commented 3 years ago

In the comment I followed (#33 (comment)), it appears as a real specified field, but I seem to remember that at some point we had somehow discussed about putting it into the implementation-dependent attributes @kadel @l0rd do you remember ?

@davidfestal sorry but I don't remember. And I would rather use the format specified in the comment you have linked: path is in the ingress spec already, scheme is not. Another consideration is that we may merge scheme and protocol as http or ws will always translate to tpc in the corresponding k8s ingress no?

Another question: in devfile 1.0, it seems endpoints are also available for other compoents, mainly kubernetes and openshift. Is it something we also want in devfile 2.0 ? @l0rd wdyt ?

That would be useful yes.

EDIT: Some idea about how to solve the scheme/protocol debate can be found on https://github.com/kubernetes-sigs/service-apis and in particular the API Sketch document

davidfestal commented 3 years ago

In the comment I followed (#33 (comment)), it appears as a real specified field, but I seem to remember that at some point we had somehow discussed about putting it into the implementation-dependent attributes @kadel @l0rd do you remember ?

@davidfestal sorry but I don't remember. And I would rather use the format specified in the comment you have linked: path is in the ingress spec already, scheme is not.

OK, so maybe I mis-remembered. Let's keep it in the Spec part.

Another consideration is that we may merge scheme and protocol as http or ws will always translate to tpc in the corresponding k8s ingress no?

I like this idea

Another question: in devfile 1.0, it seems endpoints are also available for other compoents, mainly kubernetes and openshift. Is it something we also want in devfile 2.0 ? @l0rd wdyt ?

That would be useful yes.

OK, I will update it.

EDIT: Some idea about how to solve the scheme/protocol debate can be found on https://github.com/kubernetes-sigs/service-apis and in particular the API Sketch document

So looking into this document, and taking in account your previous remark, I assume we could come back to having a single protocol field, included in the Specification as well, with the following available values:

@l0rd Does it make sense to you ?

l0rd commented 3 years ago

Do we need the secure field? I mean if we add https and wss to the list of protocols we do not need the secure field at all right?

davidfestal commented 3 years ago

Do we need the secure field? I mean if we add https and wss to the list of protocols we do not need the secure field at all right?

In fact secure is more than simply adding TLS support on http and ws application protocols. It is mainly expected to drive how endpoints are protected by some sort of authentication through workspace routing controllers.

So I assume it still makes sense to auto-promote http endpoints to https when Secure is set (as done in Devfile 1.0), since authenticating an endpoint without enabling TLS makes no sense afaict.

OTOH it seems we might also want have https endpoints without requiring authentication.

So I'd rather consider the protocol and the secure fields 2 distinct things, and add https and wss in the list of available protocols.

@l0rd do you agree ?

davidfestal commented 3 years ago

Result is this documentation: image

gorkem commented 3 years ago

What does the tool (che/odo) supposed to do when it sees a secure field?

davidfestal commented 3 years ago

What does the tool (che/odo) supposed to do when it sees a secure field?

In Che, it triggers the setup of some authentication on secured endpoints:

Finally, when using the workspace CRD controller, the precise implementation of secured endpoint authentication is implemented by the Workspace Routing controller selected based on the routing class.

l0rd commented 3 years ago

So I'd rather consider the protocol and the secure fields 2 distinct things, and add https and wss in the list of available protocols.

@l0rd do you agree ?

@davidfestal ok I think we can live with that right now but we may need to review that because it's mainly used for Che plugins hence not sure it should be part of the spec of the devfile. It should be a Che configuration.

davidfestal commented 3 years ago

So I'd rather consider the protocol and the secure fields 2 distinct things, and add https and wss in the list of available protocols. @l0rd do you agree ?

@davidfestal ok I think we can live with that right now but we may need to review that because it's mainly used for Che plugins hence not sure it should be part of the spec of the devfile. It should be a Che configuration.

So do you mean I'll merge the current PR as is for now, and we'll open a distinct issue to discuss the specific case of the secure field ?

l0rd commented 3 years ago

So do you mean I'll merge the current PR as is for now, and we'll open a distinct issue to discuss the specific case of the secure field ?

yes

davidfestal commented 3 years ago

@l0rd Could you please approve this PR then, it seems I cannot merge without at least one approval :-)