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

Uri shortener for long host label endpoints #2083

Merged
merged 2 commits into from Jul 30, 2020

Conversation

guicassolato
Copy link
Contributor

It will automatically shorten host labels of system-generated proxy endpoints, that exceed the 63-char limit imposed by RFC-1035.

Closes THREESCALE-5089

@guicassolato guicassolato self-assigned this Jul 29, 2020
@guicassolato guicassolato changed the title Uri shortner for long host label endpoints Uri shortener for long host label endpoints Jul 29, 2020
It will automatically shorten host labels of system-generated proxy endpoints, that exceed the 63-char limit imposed by RFC-1035.

Closes [THREESCALE-5089](https://issues.redhat.com/browse/THREESCALE-5089)
Martouta
Martouta previously approved these changes Jul 29, 2020
Copy link
Contributor

@Martouta Martouta left a comment

Choose a reason for hiding this comment

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

👏 💪 🥇 🎉 🕺 💃

@hallelujah
Copy link
Contributor

hallelujah commented Jul 29, 2020

I wonder, instead of using system_name and tenant_name can't we just generate a UUID?
OK UUID is not user friendly but typing by hand 63+ chars in a URL without error is unlikely

The route created in OpenShift could have annotations to query by system_name and tenant_name.
WDYT?

@guicassolato
Copy link
Contributor Author

I wonder, instead of using system_name and tenant_name can't we just generate a UUID?

I think the problem is that this will become the public URL of the service. Customers will probably prefer keep getting something easier to remember and yet still be more or less in control of the host name (up to a point).

@hallelujah
Copy link
Contributor

I really do not know, it is a copy paste anyway, used in scripts not something you type in your browser url bar.

@guicassolato
Copy link
Contributor Author

guicassolato commented Jul 29, 2020

@hallelujah, the 63-char limit per hostname label is currently being easily reached in Openshift when service discovery is used because we generate the system name. It's not (directly) chosen by the customer. It's "#{openshift_namespace}-#{service_name}". And then system name of the service is used in the template to generate the proxy endpoints, where we append yet "-#{tenant_org_name}"-apicast-staging or "-#{tenant_org_name}"-apicast-production.

About how the endpoints are used afterwards, I really don't know. You may be right. I just didn't wanna break behaviour too much.

@guicassolato guicassolato requested a review from a team July 29, 2020 14:06
@hallelujah
Copy link
Contributor

hallelujah commented Jul 29, 2020

I was thinking of using a UUID or something instead of the template when it is generated by ServiceDiscovery
Because shortening is prone to name collision.

2 services with long enough name can generate a similar system name that will be shortened, leading to 2 generated same URLs pointing to 2 services (one accepted by OpenShift, the other not)

@guicassolato
Copy link
Contributor Author

I was thinking of using a UUID or something instead of the template when it is generated by ServiceDiscovery
Because shortening is prone to name collision.

2 services with long enough name can generate a similar system name that will be shortened, leading to 2 generated same URLs pointing to 2 services (one accepted by OpenShift, the other not_
We can then add annotations to the route so it fixes the issue once and for all

@hallelujah, that's why I used a SHA-1 based on the original hostname label. The chance of name collision here is virtually zero (or as low as SHA-1 algorithm has to generate two hashes with the same 7-char prefix for two different sources). In other words, given that "#{openshift_namespace}-#{service_name}" has to be unique, we have pretty much the same risk of generating duplicate hostnames as, e.g., git has of generating two short commit hashes 😉.

Copy link
Contributor

@hallelujah hallelujah left a comment

Choose a reason for hiding this comment

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

Right! I thought it was truncate then SHA1. Sorry I am tired :(

Good job @guicassolato 🎖️ 🚀 :shipit:

@guicassolato guicassolato merged commit 1888094 into master Jul 30, 2020
@guicassolato guicassolato deleted the truncate-long-url-labels branch July 30, 2020 09:36
hallelujah pushed a commit that referenced this pull request Sep 10, 2020
Uri shortener for long host label endpoints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants