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

Provide an easier way to bring your own certificates via helm chart installation #2175

Closed
sudermanjr opened this issue Jul 8, 2021 · 4 comments · Fixed by #2177
Closed
Labels
kind/feature New features for Agones
Milestone

Comments

@sudermanjr
Copy link
Contributor

Is your feature request related to a problem? Please describe.
There doesn't seem to be a great way to bring your own certificates when using the helm installation method. As it stands now, it's possible to either allow automatic certificate generation, or disable generation of certs, which then puts in place static certificates that exist in the helm chart.

The only way to bring your own certificates is to clone the chart, edit the files, and then install from the clone.

Describe the solution you'd like
I have a working branch that allows disabling the creation of all k8s secrets (specifically for the allocator). This enables me to provision the secrets up-front in kubernetes, and then start up agones using those secrets (and thus certificates). I think this is the ideal situation.

Describe alternatives you've considered
The other possibility that could be done in addition to the proposed solution (or instead of), is to allow specifying the certs in your values file, which could then be passed in using different methods with helm (and other helm-wrapper tools). I went with the first solution in my test because it allows the handling of secrets outside of helm, which opens up more sophisticated secrets management methods.

Additional context
The diff on my proposed solution. If there are no objections, I'll gladly open a PR.

@sudermanjr sudermanjr added the kind/feature New features for Agones label Jul 8, 2021
@roberthbailey
Copy link
Member

We do have a few parameters related to certificates. From https://agones.dev/site/docs/installation/install-agones/helm/#configuration

Parameter Description Default
agones.controller.generateTLS Set to true to generate TLS certificates or false to provide your own certificates in certs/* true
agones.allocator.generateClientTLS Set to true to generate client TLS certificates or false to provide certificates in certs/allocator/allocator-client.default/* true
agones.allocator.generateTLS Set to true to generate TLS certificates or false to provide certificates in certs/allocator/* true
agones.allocator.disableMTLS Turns off client cert authentication for incoming connections to the allocator. false
agones.allocator.disableTLS Turns off TLS security for incoming connections to the allocator. Only applicable to the REST API. It currently does not work for the gRPC API. (issue) false

I think it would be useful if you could enumerate which certificates you want to be able to disable / bring yourself. Also, does it suffice to have a single flag to control on/off or would we want to introduce individual configuration parameters for each certificate? The latter seems more flexible, but it means you would have to specify a bunch of different arguments when installing your helm chart.

@sudermanjr
Copy link
Contributor Author

sudermanjr commented Jul 9, 2021

The specific certificates I'm referring to are related to the allocator - the server cert and key for the allocator grpc service.

In the chart, disabling generateTLS just switches to using certificates that are baked into the chart:

From my original description

As it stands now, it's possible to either allow automatic certificate generation, or disable generation of certs, which then puts in place static certificates that exist in the helm chart.

To elaborate on that, since the chart only allows either auto-generated certs, or switches to using the .Files directive, I am forced to use auto-generated, or the certs that are already in the chart folder. Naturally using cert and key that are already posted in a public repo is unacceptable. In addition, using auto-generated certs for the allocator service doesn't work either, because they change every time I update the chart, effectively breaking my client.

Reference to current chart:

{{- if .Values.agones.allocator.generateTLS }}
tls.crt: {{ b64enc $cert.Cert }}
tls.key: {{ b64enc $cert.Key }}
{{- else }}
tls.crt: {{ .Files.Get "certs/allocator/server.crt" | b64enc }}
tls.key: {{ .Files.Get "certs/allocator/server.key" | b64enc }}
{{- end }}

Furthermore, I don't want to completely disable tls or mTLS for the allocator service. This means that none of the currently provided flags provides any route forward for me.

As it stands in my branch, I would very much prefer to simply disable the creation of all allocator certificates and create them myself. This means the allocator-tls, allocator-tls-ca and allocator-client-ca will all be created before installing the helm chart, and maintained outside of the helm chart.

Note that this only applies to the allocator. This seems sufficient to me because the allocator is the only service that is being used from outside of the kubernetes cluster. The controller and the admission webhook are internal to the system, and can be rotated/changed/etc. as much as needed since both client and server exist in a controlled system.

Link to my branch for the example - https://github.com/fairwindsops/agones/blob/sudermanjr/allow-disabling-allocator-certs/install/helm/agones/templates/service/allocation.yaml

@roberthbailey
Copy link
Member

To elaborate on that, since the chart only allows either auto-generated certs, or switches to using the .Files directive, I am forced to use auto-generated, or the certs that are already in the chart folder.

The way I read "Set to true to generate client TLS certificates or false to provide certificates in certs/allocator/allocator-client.default/*" I think the intent was to have this be a way to provide your own certs my grabbing the chart and then adding your files locally. It sounds like this isn't working for your use case though.

So what you are looking for isn't to "bring your own certificates via helm chart installation" (by making helm more flexible to install them a different way) but instead to "bring your own certificates instead of installing them with the helm chart".

This sounds good and I think we should document this since pattern in https://agones.dev/site/docs/advanced/allocator-service/ in addition to adding the new flag to the helm chart configuration page so that it gets more visibility.

@sudermanjr
Copy link
Contributor Author

So what you are looking for isn't to "bring your own certificates via helm chart installation" (by making helm more flexible to install them a different way) but instead to "bring your own certificates instead of installing them with the helm chart".

Sure, that's probably a good way to put it.

I think the intent was to have this be a way to provide your own certs my grabbing the chart and then adding your files locally

That definitely appears to be the intent, but with Helm, it is much nicer to just install the chart from the repository rather than having to keep a copy of it around somewhere.

@roberthbailey roberthbailey added this to the 1.16.0 milestone Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New features for Agones
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants