Allow endpoints with the same targetPort if they are on the same container component #822
Conversation
amisevsk
changed the title
pkg/validation/validation-rule.md
Outdated
| @@ -14,7 +14,8 @@ The validation will be done as part of schema validation, the rule will be intro | |||
|
|
|||
| ### Endpoints: | |||
| - all the endpoint names are unique across components | |||
| - all the endpoint ports are unique across components. Only exception: container component with `dedicatedpod=true` | |||
| - endpoint ports must be unique across components -- to components cannot have the same target port, but one component may have two endpoints with the same target port. This restriction does not apply to container components with `dedicatedPod` set to `true`. | |||
There was a problem hiding this comment.
typo: "to components..." should be "two components..."
There was a problem hiding this comment.
Embarrassing! Fixed, thank you 😄
Allow multiple endpoints to have the same target point if and only if they are attached to the same container. Otherwise, target ports must be unique unless dedicatedPod is set to true. Allowing multiple endpoints to use the same port is required for some applications which require two routes to the same endpoint, in order to create isolation between components within the browser. Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
amisevsk
force-pushed
the
duplicated-endpoints
branch
from
f7cea9f to
478f636
Compare
|
/lgtm |
| @@ -14,7 +14,8 @@ The validation will be done as part of schema validation, the rule will be intro | |||
|
|
|||
| ### Endpoints: | |||
| - all the endpoint names are unique across components | |||
| - all the endpoint ports are unique across components. Only exception: container component with `dedicatedpod=true` | |||
| - endpoint ports must be unique across components -- two components cannot have the same target port, but one component may have two endpoints with the same target port. This restriction does not apply to container components with `dedicatedPod` set to `true`. | |||
There was a problem hiding this comment.
If we allow the use of the same port on one container, why not allow the same ports on two different containers? Isn't basically the same situation as in most cases the containers will be in on Pod?
We don't need to block this PR on this.
We can revisit it in a separate issue.
There was a problem hiding this comment.
Kubernetes won't allow multiple containerPorts within the same pod, so allowing the same port on different containers will result in the pod being rejected as invalid. For endpoints on the same container, only one containerPort is created (and only one service within the pod can listen on that port).
The endpoint defines more information than just the port. In particular, we use endpoints to define URL paths, protocols, and whether a route should be created. This results in multiple ingresses/routes that map to 1 or more services, that end up all pointing at one port in the container. An example would be something like
https://url1.com/path1 ───────────┐
▼
service port 8080 ──► container port 8080
▲
https://url2.com/path2 ───────────┘
Perhaps the ultimately better solution (requiring a new APIVersion) would be to extend endpoints to allow configuring something like this in one endpoint entry.
There was a problem hiding this comment.
Kubernetes won't allow multiple containerPorts within the same pod, so allowing the same port on different containers will result in the pod being rejected as invalid. For endpoints on the same container, only one containerPort is created (and only one service within the pod can listen on that port).
oh, right.
In this case, we have quite a broken implementation of endpoints in devfile/library :-/
Currently, it maps every endpoint into the container port.
By re-introducing this change devfile/library will have to start doing smarter things with endpoints than just blindly mapping 1:1 to container ports.
/cc @elsony @yangcao77
I think that this is another proof of how badly we need a common layer that controls the basic principles of how Devfile is translated to k8s resources.
If we can't have it on code level in for of some shared library then at least we need to have documentation describing the ground rules of how devfile is translated to k8s resources. Otherwise, problems like this will keep coming.
There was a problem hiding this comment.
Yes. I have aware that library need to make change accordingly. But library can still use the endpoint name, if multiple endpoints uses same port, library can use the first endpoint uses that port. Or Odo want to change back to the old behavior? all use port-<portNumber> instead?
|
/approve |
|
/approve |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amisevsk, elsony, ibuziuk, kadel 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 |
Update devfile/api dependency to include changes from PR devfile/api#822 Signed-off-by: Angel Misevski <[email protected]>
Update devfile/api dependency to include changes from PR devfile/api#822 Signed-off-by: Angel Misevski <[email protected]>
Update devfile/api dependency to include changes from PR devfile/api#822 Signed-off-by: Angel Misevski <[email protected]>
* Update devfile/api dependency to fix validation of devworkspaces Update devfile/api dependency to include changes from PR devfile/api#822 Signed-off-by: Angel Misevski <[email protected]> * Regenerate files for new devfile/api version Signed-off-by: Angel Misevski <[email protected]>
What does this PR do?:
Implements the suggested solution in #821 (comment), in particular:
Documentation on endpoints and in validation-rule.md is updated to reflect this fact.
Note: this PR requires additional handling by tools that use endpoint names for containerPort names, as it's possible to generate invalid pod specs by not de-duplicating containerPorts -- cc: @kadel
Which issue(s) this PR fixes:
Closes #821
PR acceptance criteria:
Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.
Unit/Functional tests
QE Integration test
Documentation
Client Impact
How to test changes / Special notes to the reviewer: