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

Implement conf.toml Helm generation on-the-fly based on the domain (#954) #1041

Merged
merged 18 commits into from
Jun 24, 2024

Conversation

SleepySquash
Copy link
Contributor

@SleepySquash SleepySquash commented Jun 17, 2024

Resolves #954

Synopsis

Helm should allow specifying different configuration for different domains.

Solution

This PR adds:

  • nginx returning conf.toml based on the $host;
  • Helm generating such .yaml files based on the provided values.
  1. The $host based retrieval of conf.toml is the easy part
  location = /conf {
    root       /var/www;
    expires    1m;
    try_files  /conf/$host.yaml /conf/$host.toml $uri.toml $uri.yaml $uri  =404;
  }

-> nginx will try to get the $host.toml file first, and if fails to do so, then fallbacks to conf.toml. E.g. if we are localhost, then nginx will try to return localhost.toml instead of conf.toml by default.

  1. The Helm part is a bit harder. We need to cycle through ingress.hosts specified in the values and form a .toml file for each host being a merge of the common .Values.conf and a patch configuration for the provided host.
    I've managed to accomplish that by using initContainers for every host. The containers use the mergeOverwrite Helm function to merge values. And then using the simple toToml I echo the merged values into the emptyDir mount I then mount into the app container.
    See Implement conf.toml Helm generation on-the-fly based on the domain (#954) #1041 (comment) for details.

So, given the following values.yaml file:

ingress:
  hosts:
    - localhost
    - example.com
    - empty

conf:
  server:
    http:
      url: https://domain.com
      port: 443
    ws:
      url: wss://domain.com
      port: 443
  localhost:
    legal:
      copyright: sup
  example.com:
    link:
      prefix: https://example.com/prefix
    server:
      http:
        port: 49000
      ws:
        port: 49001

(notice the empty domain having no patch, localhost domain adding the legal part and the example.com overwriting the ports)

We get the following files:
localhost.yaml

server:
  http:
    port: 443
    url: https://domain.com
  ws:
    port: 443
    url: wss://domain.com
legal:
  copyright: sup

example.com.yaml

server:
  http:
    port: 49000
    url: https://domain.com
  ws:
    port: 49001
    url: wss://domain.com
link:
  prefix: https://example.com/prefix

empty.yaml

server:
  http:
    port: 443
    url: https://domain.com
  ws:
    port: 443
    url: wss://domain.com

Checklist

  • Created PR:
    • In draft mode
    • Name contains issue reference
    • Has type and k:: labels applied
  • Before review:
    • Documentation is updated (if required)
    • Tests are updated (if required)
    • Changes conform code style
    • CHANGELOG entry is added (if required)
    • FCM (final commit message) is posted or updated
    • Draft mode is removed
  • Review is completed and changes are approved
    • FCM (final commit message) is approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • All temporary labels are removed

@SleepySquash SleepySquash added enhancement Improvement of existing features or bugfix k::deploy Changes that involve application deployment labels Jun 17, 2024
@SleepySquash SleepySquash added this to the 0.1.0-alpha.14 milestone Jun 17, 2024
@SleepySquash SleepySquash self-assigned this Jun 17, 2024
@SleepySquash
Copy link
Contributor Author

SleepySquash commented Jun 18, 2024

FCM

Allow tuning remote configuration per individual host in Helm chart (#1041, #954)

- add `conf-renderer` container and `template.sh` script generating YAML configuration per Ingress host
- support YAML format in `Config.init()` for remote configuration parsing
- use `try_files` for `/conf` Nginx route to return `$host` based configuration

@SleepySquash SleepySquash marked this pull request as ready for review June 18, 2024 09:17
@SleepySquash
Copy link
Contributor Author

SleepySquash commented Jun 18, 2024

Discussed: current solution creates separate init container for each domain, which is NOT the desired behaviour, as this may eventually break the Helm chart deployment with more and more domains being added (imagine 1000 init containers). Ideally we should have a single init container accepting the Values context and then separating the different configurations for each domain, so that Helm doesn't embed a lot of stuff in the deployment resource.

@SleepySquash SleepySquash removed the request for review from tyranron June 18, 2024 11:34
Copy link
Contributor

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

Discussed

Copy link
Contributor

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@SleepySquash please, recheck my changes before merging.

helm/messenger/templates/deployment.yaml Outdated Show resolved Hide resolved
@tyranron tyranron added the feature New feature or request label Jun 23, 2024
@SleepySquash SleepySquash merged commit e23283b into main Jun 24, 2024
24 checks passed
@SleepySquash SleepySquash deleted the 954-impl-dynamic-conf branch June 24, 2024 07:23
github-actions bot added a commit that referenced this pull request Jun 24, 2024
…1041, #954)

- add `conf-renderer` container and `template.sh` script generating YAML configuration per Ingress host
- support YAML format in `Config.init()` for remote configuration parsing
- use `try_files` for `/conf` Nginx route to return `$host` based configuration

Co-authored-by: tyranron <tyranron@gmail.com> e23283b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix feature New feature or request k::deploy Changes that involve application deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement conf.toml nginx based generation on-the-fly based on the domain
2 participants