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

Adding a config.toml value to the helm chart #1420

Closed
wants to merge 4 commits into from
Closed

Adding a config.toml value to the helm chart #1420

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
183866d
Adding a config.toml value to the helm chart
arschles Oct 16, 2019
8ac51c8
Adding some docs
arschles Oct 16, 2019
08c88dd
Merge branch 'master' into config-helm
arschles Nov 26, 2019
55ada71
Merge branch 'master' into config-helm
arschles Mar 20, 2020
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
14 changes: 14 additions & 0 deletions charts/athens-proxy/templates/config-configtoml.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{{- if .Values.configToml.enabled -}}
apiVersion: v1
kind: ConfigMap
metadata:
name: {{ template "fullname" . }}-configtoml
labels:
app: {{ template "fullname" . }}
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
release: "{{ .Release.Name }}"
heritage: "{{ .Release.Service }}"
data:
config.toml: |-
{{ .Values.configToml.data }}
{{- end -}}
10 changes: 10 additions & 0 deletions charts/athens-proxy/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,11 @@ spec:
mountPath: "/usr/local/lib"
readOnly: true
{{- end }}
{{- if .Values.configToml}}
- name: configtoml
mountPath: "/config"
readOnly: true
{{- end }}
{{- if .Values.netrc.enabled }}
- name: netrc
mountPath: "/etc/netrc"
Expand Down Expand Up @@ -192,6 +197,11 @@ spec:
configMap:
name: {{ template "fullname" . }}-upstream
{{- end }}
{{- if .Values.configToml.enabled }}
- name: configtoml-config
configMap:
name: {{ template "fullname" .}}-configtoml
{{- end }}
{{- if .Values.netrc.enabled }}
- name: netrc
secret:
Expand Down
10 changes: 10 additions & 0 deletions charts/athens-proxy/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,16 @@ gitconfig:
# Key in the kubernetes secret that contains git config data.
secretKey: gitconfig

configToml:
enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you prefer enabled-switch approach?

Why not to just always use this config, without any conditions? I guess the default values should be fine, so we can leave it empty as you did. If not, we can provide some useful settings, maybe from https://github.com/gomods/athens/blob/master/config.dev.toml but still always mouning the config to the container.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hypnoglow I set enabled: false to keep the Kubernetes deployment the same, by default. So, unless someone wants to customize the config file, there won't be any new config map the next time they deploy.

Regarding useful settings, the Athens image already has that config.dev.toml file baked into it. Is that what you were thinking?

Copy link
Contributor

@hypnoglow hypnoglow Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I mean we may omit enabled switch completely.

The reason is that I suppose config.dev.toml serves only for documentation purposes, and it contains only values that are Athens defaults. Bundling this file into the image actually is the same as passing empty config.

If I'm wrong and config.dev.toml contains some values that are not Athens default, we can simply put them here in the chart default values. This way we also don't need enabled switch.

For example:

configToml: |
  SomeNonDefaultValue = "foo"

So if users want to customize config, they will rewrite configToml with their value, otherwise they have the same defaults. enabled switch is not needed in both cases.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

@hypnoglow ah, I think I partially see now what you mean. Regarding the defaults, I would say that there are several "layers" of defaults.

There are hard-coded ones like here the embedded config.dev.toml one, and then the ones in the helm values.yaml file. To be honest, I doubt the defaults in code match, the toml file and the helm values.yaml file all match up. However, there are reasons why we do have all three layers. I'm happy to explain them if you'd like or that would help 😄

Regarding the overrides, are you saying that the configToml: key could have just part of a config toml file, and that we would use it to override the one that is embedded in the Athens docker image?

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the defaults, I would say that there are several "layers" of defaults.
However, there are reasons why we do have all three layers.

Ah, it's a bit clearer now, thanks for explanations. However, don't those multiple layers add unnecessary complexity to the overall Athens configuration? BTW I guess it is going a bit offtopic. If you believe we should stick to those "layers" for now and keep Helm chart in accordance, we should go with it then.

Regarding the overrides, are you saying that the configToml: key could have just part of a config toml file, and that we would use it to override the one that is embedded in the Athens docker image?

Yes, exactly. My point was that we can use default config options directly in helm values that will always override embedded config.

Copy link
Member Author

Choose a reason for hiding this comment

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

However, don't those multiple layers add unnecessary complexity to the overall Athens configuration?

@hypnoglow I personally think they do, but there is a need for folks to customize configuration at each of the levels, and we have the defaults in place to show where to customize configuration.

For example, the Helm values.yaml is there show some examples of how folks can customize their Kubernetes deployment. Those fields get translated to environment values passed to Athens, however, so the configuration mechanism doesn't change from other deployments of Athens, for what that's worth 😄 .

If you believe we should stick to those "layers" for now and keep Helm chart in accordance, we should go with it then.

I do think we need to stick with the layers, and I'd like to see them documented (I just created an issue for that, and I'm planning to do it today).

I agree that ideally the helm values.yaml file should be up to date with the config.dev.toml and the hard-coded defaults, and we can try to do that, but inevitably they will become out of sync. I am going to try to write documentation clear enough that folks can both understand where the layers are, how to read each one (config.toml, values.yaml, etc...), and that they may diverge.

My point was that we can use default config options directly in helm values that will always override embedded config.

The merging option is interesting, but wouldn't someone set environment variables or provide a values.yaml file if they want to override just part of the config.toml file? This PR is intended to allow people to override everything at once.

Let me know what you think, and thank you for the discussion!

Copy link
Contributor

Choose a reason for hiding this comment

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

@arschles Thank you for your efforts on improving documentation.
I believe that we can try to reduce layers complexity, somewhat like this:

  1. Standalone app can run with defaults (should be documented) with the possible custom config.toml or environment variables provided by user.
  2. Docker should run with defaults or probably with minimal default config.toml with only docker-specific options tuned, if it is even required. Custom configuration should be provided as config.toml-volume or environment variables by user.
  3. Helm Chart and other Kubernetes deployment builds on top of the Docker image, but with default configuration tuned with k8s-specific values (only if required), present in values.yaml or similar and allowing to be customized by user.

This way users will always know that they get the same configuration, whether they running Athens standalone, in Docker or in Kubernetes.

The merging option is interesting, but wouldn't someone set environment variables or provide a values.yaml file if they want to override just part of the config.toml file? This PR is intended to allow people to override everything at once.

I didn't mean merging. To recap what I suggested:

  1. If we use enabled-switch approach, then with enabled: false helm chart runs with Docker-builtin config.toml and custom configuration can be passed only with environment variables. With enabled: true we completely override Docker-builtin config.toml file with contents in data key. In this approach, configToml.data contains no default configuration, and users setting enabled: true will not know that this will override default Docker-builtin config completely and consequences.

  2. If we don't use enabled switch, then configToml contents ALWAYS override Docker-builtin config.toml, so users don't even have to know about that config file built in the docker image. This way we can embed default configuration for Helm deployment directly into values.yaml, and it always get injected. If user needs to change it, they can add/remove configuration options in their custom values.yaml-file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hypnoglow you're welcome

Standalone app can run with defaults (should be documented) with the possible custom config.toml or environment variables provided by user.

This is working now. You can use the -config_file flag (we will need to document that)

Docker should run with defaults or probably with minimal default config.toml with only docker-specific options tuned, if it is even required. Custom configuration should be provided as config.toml-volume or environment variables by user.

We would need to them remove the current config.toml file from the docker image, which will cause backward compatibility issues. And that is the root problem we will need to solve.

Would it be helpful for Athens to have an environment variable like CONFIG_FILE:

  • Set to /path/to/file.toml to tell Athens to read from a specific config file. you can already pass it the -config_file flag, but that is more difficult when running in a docker container
  • Set to off to tell Athens to completely ignore the config file and go with hard-coded defaults and environment variables

Helm Chart and other Kubernetes deployment builds on top of the Docker image, but with default configuration tuned with k8s-specific values (only if required), present in values.yaml or similar and allowing to be customized by user.

We still will need to keep the default config.toml in the docker image, but we can also make it easier here to mount your own config.toml file or turn it off

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

(P.S. I'll respond much more quickly this time. I have far less travel this year 😄)

data: |-
# See https://docs.gomods.io/configuration/ for an introduction on what
# values you can put in here.
#
# Otherwise, for an exhaustive list of the configuration values you
# can put here, see config.dev.toml at the root of the
# github.com/gomods/athens repository
upstreamProxy:
# This is where you can set the URL for the upstream module repository.
# If 'enabled' is set to true, Athens will try to download modules from the upstream when it doesn't find them in its own storage.
Expand Down
19 changes: 19 additions & 0 deletions docs/content/install/install-on-kubernetes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,3 +288,22 @@ helm install gomods/athens-proxy --name athens --namespace athens \
--set gitconfig.secretName=athens-proxy-gitconfig \
--set gitconfig.secretKey=gitconfig
```

### Overriding the default `config.toml`

The [Docker images for Athens](https://hub.docker.com/r/gomods/athens) come with a default `config.toml` file (which is the same as the [`config.dev.toml`](https://github.com/gomods/athens/blob/master/config.dev.toml)) in the repository). We've added lots of common configuration values to the Helm chart that you might like to override, but if you're interested in setting a huge set of configuration values, or values that the chart doesn't currently let you set, you might want to set up your own `config.toml` and inject that into your Athens pods in your Kubernetes deployment.

The Athens chart supports doing that via a `configToml.enabled` and `configToml.data` value. Since your `config.toml` file will have newlines in it and will likely be large, we recommend that you add your complete `config.toml` to an `override-values.yaml` file above. Here is what the relevant section of your `override-values.yaml` file should look like:

```yaml
configToml:
enabled: true
data: |-
<your config.toml here>
```

After you have your complete `override-values.yaml` set up, including your new `configToml` section, you can run your `helm install` command as above:

```console
$ helm install gomods/athens-proxy -n athens --namespace athens -f override-values.yaml
```