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

Fix kubernetes rule name hashing #854

Merged
merged 2 commits into from
Nov 22, 2018

Conversation

greenboxal
Copy link
Contributor

@greenboxal greenboxal commented Nov 18, 2018

This fixes a ID generation issue when a single ingress contains two hostnames that would collide given the previous routeID function.

The problem was caused because of the sanitization rule, that replaced any non-word with an underscore. Given my-api.prd.company.com and my-api-prd.company.com, they would be sanitized to the same name, my_api_prd_company_com.

Somewhere down the code, one would effectively replace the other rule, allowing only one of the domains to be actually registered. (probably by putting the routes on a map using eskip.Route.Id as key)

My solution was to include the index of the ingress host in the name. Anything else that could make the ID unique would work as well (hashing, etc)

@szuecs
Copy link
Member

szuecs commented Nov 19, 2018

I think this will fix the routeID and in the kubernetes dataclient we also mask . with [.] so it should be fine in general. So my mentioned comment here #853 (comment) does not apply.

@szuecs
Copy link
Member

szuecs commented Nov 19, 2018

👍

@szuecs
Copy link
Member

szuecs commented Nov 19, 2018

@aryszka please review, thanks

@aryszka
Copy link
Contributor

aryszka commented Nov 22, 2018

👍

@aryszka
Copy link
Contributor

aryszka commented Nov 22, 2018

👍

1 similar comment
@szuecs
Copy link
Member

szuecs commented Nov 22, 2018

👍

@szuecs szuecs merged commit 6c4657f into zalando:master Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants