Update how too-long endpoint hostnames are handled #1801
Conversation
Update the hostname format used for endpoints whose hostnames are too long from <workspace-id>-<order>.<base-domain> to <workspace-id>-<endpoint-name>.<base-domain> This is necessary as the iteration order through endpoints is random (iterating through Go maps is random), resulting in inconsistent numbers used for <order>. Using a combination of workspace ID and endpoint name should always be valid: * Workspace IDs are 25 characters long * Endpoint names are restricted to max 15 characters by the Devfile API * Endpoint names and workspace IDs are required to be alphanumeric with dashes, starting and ending with an alphanumeric character * Endpoint names are unique across all endpoints in the workspace Signed-off-by: Angel Misevski <[email protected]>
amisevsk
requested review from
nickboldt,
tolusha,
ibuziuk and
l0rd
as code owners
| // If subdomain is not valid (e.g. too long), use alternate format | ||
| // The below should always be under 63 characters, as endpoint names are limited to 15 characters and workspace IDs are | ||
| // 25 characters. | ||
| return fmt.Sprintf("%s-%s.%s", u.workspaceID, endpointInfo.endpointName, baseDomain) |
There was a problem hiding this comment.
What about sorting endpoints by theirs names here [1] instead of changing host names?
I just have some concerns, maybe someone relies on hostnames.
[1]
che-operator/controllers/devworkspace/solver/che_routing.go
Lines 485 to 486 in 7c7bd4f
There was a problem hiding this comment.
I considered sorting the endpoints instead of this approach, but honestly iterating through a map in sorted order in Go is annoying (since there's no way to easily get a keyset, etc.) :)
These hostnames are not going to be reliably static in any case since this functionality is triggered only when the default endpoint name becomes too long. For example, some users might see the normal format while others see the legacy format (depending on how long usernames are). You could even end up in a situation where half your endpoints are normal, and half are legacy. In the past, we've pointed people at the endpoint-url attribute to determine the actual hostname used for each devfile endpoint.
I'm fine with sorting the endpoints, though I had a small concern that I was missing something in how they're handled, as the original issue has order numbers going up to 13 (which may just be how many endpoints the devfile has, I don't know).
An additional bonus of doing it this way is that we don't run into situations where endpoints could be updated unexpectedly -- e.g. with order, even sorted, if you update the devfile to have an endpoint earlier in the order, all your legacy-hostname endpoints might change.
There was a problem hiding this comment.
Thank you for explanation, make sense to me.
|
/test v14-operator-test |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amisevsk, dkwon17, tolusha 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 |
|
I took some extra time to test edge cases here:
|
|
/test v14-che-behind-proxy |
|
@amisevsk: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@amisevsk |
|
Build 3.12 :: operator_3.x/341: Console, Changes, Git Data |
|
Build 3.12 :: sync-to-downstream_3.x/6043: Console, Changes, Git Data |
|
Build 3.12 :: push-latest-container-to-quay_3.x/4221: Console, Changes, Git Data |
|
Build 3.12 :: get-sources-rhpkg-container-build_3.x/5918: devspaces-operator : 3.x :: Build 58323439 : quay.io/devspaces/devspaces-rhel8-operator:3.12-12 |
|
Build 3.12 :: update-digests_3.x/5644: Console, Changes, Git Data |
|
Build 3.12 :: operator_3.x/341: Upstream sync done; /DS_CI/sync-to-downstream_3.x/6043 triggered |
|
Build 3.12 :: operator-bundle_3.x/2570: Console, Changes, Git Data |
|
Build 3.12 :: sync-to-downstream_3.x/6045: Console, Changes, Git Data |
|
Build 3.12 :: push-latest-container-to-quay_3.x/4222: Console, Changes, Git Data |
|
Build 3.12 :: copyIIBsToQuay/2437: Console, Changes, Git Data |
|
Build 3.12 :: sync-to-downstream_3.x/6045: Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/5920 triggered; /job/DS_CI/job/dsc_3.x triggered; |
|
Build 3.12 :: operator-bundle_3.x/2570: Upstream sync done; /DS_CI/sync-to-downstream_3.x/6045 triggered |
|
Build 3.12 :: dsc_3.x/1767: Console, Changes, Git Data |
|
Build 3.12 :: update-digests_3.x/5644: Detected new images: rebuild operator-bundle |
|
Build 3.12 :: dsc_3.x/1767: 3.12.0-CI |
|
Build 3.12 :: operator-bundle_3.x/2571: Console, Changes, Git Data |
|
Build 3.12 :: sync-to-downstream_3.x/6047: Console, Changes, Git Data |
|
Build 3.12 :: push-latest-container-to-quay_3.x/4224: Console, Changes, Git Data |
|
Build 3.12 :: copyIIBsToQuay/2438: Console, Changes, Git Data |
|
Build 3.12 :: sync-to-downstream_3.x/6047: Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/5922 triggered; /job/DS_CI/job/dsc_3.x triggered; |
|
Build 3.12 :: operator-bundle_3.x/2571: Upstream sync done; /DS_CI/sync-to-downstream_3.x/6047 triggered |
|
Build 3.12 :: dsc_3.x/1768: Console, Changes, Git Data |
|
Build 3.12 :: dsc_3.x/1768: 3.12.0-CI |
What does this PR do?
Update the hostname format used for endpoints whose hostnames are too long from
to
This is necessary as the iteration order through endpoints is random (iterating through Go maps is random), resulting in inconsistent numbers used for
<order>.Using a combination of workspace ID and endpoint name should always be valid:
Screenshot/screencast of this PR
N/A
What issues does this PR fix or reference?
Closes eclipse-che/che#22774
How to test this PR?
See reproducer in linked issue; after change, long endpoints should get the hostname
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or referenceandHow to test this PRcompletedReviewers
Reviewers, please comment how you tested the PR when approving it.