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

allow seperator in autogenerated routes to be configureable #2666

Closed
twardnw opened this issue May 20, 2021 · 5 comments · Fixed by #2953
Closed

allow seperator in autogenerated routes to be configureable #2666

twardnw opened this issue May 20, 2021 · 5 comments · Fixed by #2953
Assignees
Labels

Comments

@twardnw
Copy link
Contributor

twardnw commented May 20, 2021

In our charts it is currently assumed that a service name will be prepended to the router pattern with a .. This should be configurable to allow administrators the option to choose a different DNS-safe seperator, such as -. The build-deploy script should also check that the length of FQDNs generated using a customized seperator are also DNS-safe.

@smlx
Copy link
Member

smlx commented May 21, 2021

I think this is a duplicate of #2573

@twardnw twardnw self-assigned this May 21, 2021
twardnw added a commit that referenced this issue May 21, 2021
@twardnw
Copy link
Contributor Author

twardnw commented Oct 5, 2021

@tobybellwood , would it be possible to have this assigned in the near future? I have a customer where we've hacked a fix for this and they are operating from an out-of-date fork, but are now seeing issues around the length of FQDNs being generated with the customized seperator. I'd like to not have to fix that with another hack of their build-deploy script :D

@Schnitzel
Copy link
Contributor

so in my understanding this is what we would need to do:

Currently the creation of the autogenerated ingress host happens on two places:
in the lagoon api you can define on the openshift or project object the router pattern which looks something like: ${environment}.${project}.test1.amazee.io

  1. to generate the environment and project host happens inside lagoon kbd:
    https://github.com/amazeeio/lagoon-kbd/blob/e23cf2a8df46a221677196d3b873bd188332e688/controllers/lagoonbuild_helpers.go#L515-L530

  2. then inside the helmcharts for each service we are adding the prefix for the service:

    {{- define "nginx.autogeneratedHost" -}}
    {{ if not .prefix }}
    {{- printf "%s.%s" .root.Release.Name .root.Values.routesAutogenerateSuffix | trimSuffix "-" -}}
    {{ else }}
    {{- printf "%s.%s.%s" .prefix .root.Release.Name .root.Values.routesAutogenerateSuffix | trimSuffix "-" -}}
    {{ end }}
    {{- end -}}

this the end we would like to be able to add the servicename into the routerpatttern, so we could define something like:
${service}.${environment}.${project}.test1.amazee.io or for customers that don't like to generate subdomains per service and environment: ${service}-${environment}-${project}.test1.amazee.io

so my suggestion would be:

  • remove the replacement of environment and projectvariables from lagoon-kbd
  • do the replacement of service environment and project in helmchart
  • make the replacement clever enough to automatically add the servicename as a prefix if it's not defined in the routerPattern (to keep backwards compatilibity). <-- could also happen on lagoon-kbd level so that helmcharts always can expect a router pattern with ${service} and to keep the complexity inside the _helpers.tpl low.

@twardnw
Copy link
Contributor Author

twardnw commented Oct 18, 2021

One part of this that caught a customer recently is to ensure that when the separator is not a . to shorten the string to a DNS safe length

@Schnitzel
Copy link
Contributor

discussed in lagoon tech meeting:

  • remove logic from kbd
  • add replacement logic (service, environment, project) to helmcharts
  • add lagoon core upgrade script that adds ${service}. to all routerPatterns
  • add validator to routerPattern in lagoon core api that checks that the routerPattern has ${service} ${environment} ${project} defined in it
  • update documentation

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 a pull request may close this issue.

3 participants