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

change endpoint name limit to 63 characters #289

Merged

Conversation

yangcao77
Copy link
Contributor

Signed-off-by: Stephanie [email protected]

What does this PR do?

This PR changes the length limit of Endpoint Name from 15 to 63 to match DNS1123 and be consistent with the other K8s resources defined in devfile.

the length limit was defined to be 15 was because the container port name defined in pod has to be less than 15 characters
since library is not going to use the endpoint name as the port name(#287), the length limit should match the other K8s resources

What issues does this PR fix or reference?

#288

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

Docs PR

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion if we should keep 15 or increase it to 63 but another reason why endpoint name mayb have limitation in 15 chars, an implementation may want to use it for the host as part of a subdomain, like $instanceId-endpointName.$ingressdomain -> http://workspacec89506a320d9495d-theia-3100.192.168.39.170.nip.io <- it's from devworkspace operator, but we should be able to use a dedicated subdomain there, like http://workspacec89506a320d9495d.theia-3100.192.168.39.170.nip.io

It's even worse when an implementation does not have permissions to use customHosts and would like to have a meaningful host, then when route is named with endpoint-name, it will get $endpoint_name-$namespace.$router_host assigned host.

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I prefer the 15 character limit, since that's the limit on the analogous k8s object (a named container endpoint should map to a named container port if possible). From reading redhat-developer/odo#4060, I understand the motivation, but also don't see the direct link to the changes in this PR, especially in light of #287.

From the DevWorkspace operator perspective, increasing this limit has a minor impact on readability; with the 15 char limit, I could have a port named database-conn which could be translated to a similarly named container port. With the 63-char limit, all container ports will have to have less intelligible names (e.g. 9000-tcp) as there's no surefire way to truncate.

Either way it's not hard to work around, so I'm approving the PR. I totally see how having a single name field that's shorter than the rest could be confusing (as it is for container ports on k8s!)

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, yangcao77
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yangcao77
Copy link
Contributor Author

since odo is going to increase the URL name limit to 63 chars, odo writes the URL info into devfile Endpoints entry, which has conflict with the schema rule. @kadel

#287 is just to truncate the container port name inside pod, is it going to affect DevWorkspace or console? Is DevWorkspace/console going to read generated pod spec and get the container ports? Note thatdevfile.Endpoint still contains values defined in devfile. Also note that for each devfile endpoint, it does not have to has an exact match of container port, since there can have multiple endpoints sharing the same port number, but the container port & service should only have one defined.

@sleshchenko I can see your point of having 15 chars limit is "safer" in your case, since you are going to include $instanceId and $ingressdomain within the host, but host also has limitation of 63 characters. limiting the endpoint name to be less than 15 characters does not completely resolve that issue, it is still possible that the host length will be >63 chars; which means Devworkspace will still need to verify the length before actually use it.

@amisevsk

This comment has been minimized.

@yangcao77
Copy link
Contributor Author

confirmed with @maysunfaisal that Console is not consuming container port name. #288 and #287 won't affect Console's behavior, merging this PR

@yangcao77 yangcao77 merged commit 2734643 into devfile:master Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants