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.

Sign up for GitHub

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

Update how too-long endpoint hostnames are handled #1801

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Update how too-long endpoint hostnames are handled #1801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions controllers/devworkspace/solver/che_routing_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1569,24 +1569,24 @@ func TestReportSubdomainExposedEndpointsLongUsername(t *testing.T) {
if e1.Name != "e1" {
t.Errorf("The first endpoint should have been e1 but is %s", e1.Name)
}
if e1.Url != "https://wsid-1.down.on.earth/1/" {
t.Errorf("The e1 endpoint should have the following URL: '%s' but has '%s'.", "https://wsid-1.down.on.earth/1/", e1.Url)
if e1.Url != "https://wsid-e1.down.on.earth/1/" {
t.Errorf("The e1 endpoint should have the following URL: '%s' but has '%s'.", "https://wsid-e1.down.on.earth/1/", e1.Url)
}

e2 := m1[1]
if e2.Name != "e2" {
t.Errorf("The second endpoint should have been e2 but is %s", e1.Name)
}
if e2.Url != "https://wsid-2.down.on.earth/2.js" {
t.Errorf("The e2 endpoint should have the following URL: '%s' but has '%s'.", "https://wsid-2.down.on.earth/2.js", e2.Url)
if e2.Url != "https://wsid-e2.down.on.earth/2.js" {
t.Errorf("The e2 endpoint should have the following URL: '%s' but has '%s'.", "https://wsid-e2.down.on.earth/2.js", e2.Url)
}

e3 := m1[2]
if e3.Name != "e3" {
t.Errorf("The third endpoint should have been e3 but is %s", e1.Name)
}
if e3.Url != "http://wsid-3.down.on.earth/" {
t.Errorf("The e3 endpoint should have the following URL: '%s' but has '%s'.", "https://wsid-3.down.on.earth/", e3.Url)
if e3.Url != "http://wsid-e3.down.on.earth/" {
t.Errorf("The e3 endpoint should have the following URL: '%s' but has '%s'.", "https://wsid-e3.down.on.earth/", e3.Url)
}
}

Expand Down
6 changes: 4 additions & 2 deletions controllers/devworkspace/solver/endpoint_strategy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,10 @@ func (u UsernameWkspName) getEndpointPathPrefix(endpointPath string) string {
func (u UsernameWkspName) getHostname(endpointInfo *EndpointInfo, baseDomain string) string {
subDomain := fmt.Sprintf("%s-%s-%s", u.username, u.workspaceName, endpointInfo.endpointName)
if errs := validation.IsValidLabelValue(subDomain); len(errs) > 0 {
// if subdomain is not valid, use legacy paths
return fmt.Sprintf("%s-%d.%s", u.workspaceID, endpointInfo.order, baseDomain)
// 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

order := 1
for componentName, endpoints := range routing.Spec.Endpoints {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for explanation, make sense to me.

}

return fmt.Sprintf("%s.%s", subDomain, baseDomain)
Expand Down
Loading