expose Helm chart values for custom certs #2367

rahil-p · 2021-11-11T04:32:09Z

What type of PR is this?

/kind feature

What this PR does:

This PR provides a convenient alternative for installing Agones with custom certificates. Instead of having to pull and modify the contents of the chart, custom certificates may be passed in as values:

helm install my-release agones/agones \
  --set agones.allocator.generateTLS=false \
  --set-file agones.allocator.tlsCert=mydir/tls.crt \ # or --set agones.allocator.tlsCert="$(cat mydir/tls.crt)"
  --set-file agones.allocator.tlsKey=mydir/tls.key \
  --set-file "agones.allocator.clientCAs.my-ca\.crt"=mydir/ca.crt

Which issue(s) this PR fixes:

Closes #2364

Special notes for your reviewer:

agones-bot · 2021-11-11T05:04:37Z

Build Failed 😱

Build Id: a2526e4b-474c-4dff-8e33-aba4a46ca17d

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

site/content/en/docs/Installation/Install Agones/helm.md

roberthbailey · 2021-11-11T08:58:21Z

install/helm/agones/templates/extensions.yaml

@@ -38,7 +38,7 @@ spec:
        {{- if .Values.agones.controller.generateTLS }}
  caBundle: {{ b64enc $ca.Cert }}
        {{- else }}
-  caBundle: {{ .Files.Get "certs/server.crt" | b64enc }}
+  caBundle: {{ default (.Files.Get "certs/server.crt") .Values.agones.controller.tlsCert | b64enc }}


Would it work to put "certs/server.crt" into values.yaml and just use the new variable here?

From https://helm.sh/docs/chart_template_guide/functions_and_pipelines/:

In an actual chart, all static default values should live in the values.yaml, and should not be repeated using the default command (otherwise they would be redundant).

I'm not particularly familiar with the --set-file option, but it looks like if the new parameter is a file name and we keep the .Files.Get here referencing the variable, then it might work (but using the --set flag instead of --set-file because you wouldn't pass the file contents into the variable).

The new parameter expects the file's contents. The --set-file option will set a value to the contents of the file at the provided path - it's equivalent to --set value="$(cat value.txt)".

The idea behind the new parameter is to allow the user to pass the certificate directly as a value. When the value isn't provided, .Files.Get will be used as a fallback to read the file from the certs directory. I included this so that the old way of passing in certificates (i.e. modifying the files in certs) continues to be supported.

That said, I think it could simplify things to remove the fallback and simply expect the value to be passed.

Let me know if that makes sense and if you'd like to see any changes.

agones-bot · 2021-11-13T14:36:57Z

Build Succeeded 👏

Build Id: 2b9e695b-57ec-48ad-8385-2385f6cd0ee4

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.19.0-adbfa06
image: gcr.io/agones-images/agones-ping:1.19.0-adbfa06
Linux C++ SDK (build): agonessdk-1.19.0-adbfa06-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.19.0-adbfa06.zip

A preview of the website (the last 30 builds are retained):

https://adbfa06-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2367/head:pr_2367 && git checkout pr_2367
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.19.0-adbfa06

agones-bot · 2021-11-13T15:31:27Z

Build Succeeded 👏

Build Id: b02fc29b-2039-4f29-9177-a364703fa5af

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.19.0-28f5dc6
image: gcr.io/agones-images/agones-ping:1.19.0-28f5dc6
Linux C++ SDK (build): agonessdk-1.19.0-28f5dc6-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.19.0-28f5dc6.zip

A preview of the website (the last 30 builds are retained):

https://28f5dc6-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2367/head:pr_2367 && git checkout pr_2367
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.19.0-28f5dc6

agones-bot · 2021-11-13T18:09:17Z

Build Failed 😱

Build Id: 5db5082a-0652-435b-bdf3-e6611cdb0db4

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2021-11-13T18:41:08Z

Build Failed 😱

Build Id: d3c14421-450a-49dc-96b1-1d5b5a871887

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

rahil-p · 2021-11-13T19:39:21Z

I pushed a commit that removes reading in defaults from the certs directory. This simplifies the template. It also should provide some security in case of a user failing to pass in their own certificate - instead of silently falling back on the repository certificate, the server will log an error and exit after failing to load empty credentials.

This is breaking for releases that disable allocator.generateTLS or controller.generateTLS. If this isn't the right call, let me know and I'll revert it.

Edit: I updated the Makefile to pass in the chart certificates when generating install/yaml/install.yaml (the resulting file matches upstream).

roberthbailey · 2021-11-15T21:26:34Z

I like these changes, but I'm on the fence about whether this will break too many folks.

@markmandel - WDYT?

markmandel · 2021-11-15T23:19:07Z

🤔 I'm on the fence with the breaking change as well. I feel like not many people use a custom certs directory (I'm actually not sure how you would do so to be honest, I assume you would have to grab the chart source, and run it locally???)

But it's hard to judge impact. Maybe we should have this PR merge with the non-breaking behaviour for now, and file an issue for the breaking change, and see if anyone objects?

This reverts commit 2a9872c.

rahil-p · 2021-11-16T01:24:01Z

I'd guess that workflow is the most common - it would probably look something like this during deployment:

helm pull agones/agones --untar
cp -TR my-certs/ agones/certs/
helm install my-release agones --values overrides.yaml

I think a new issue is a good call - it could benefit from some more discussion.

agones-bot · 2021-11-16T01:50:37Z

Build Failed 😱

Build Id: ec6d99bc-16ae-410c-ba83-c3e0fd66431a

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

markmandel · 2021-11-16T16:32:54Z

Flake in TestAutoscalerBasicFunctions

markmandel · 2021-11-16T17:42:56Z

@roberthbailey you've been tracking this more closely than I have. If you are happy with this PR, I'm happy to squeeze this in before RC today.

agones-bot · 2021-11-16T18:19:46Z

Build Succeeded 👏

Build Id: bedddd62-418d-4a35-84af-0908b485f07b

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.19.0-3ace450
image: gcr.io/agones-images/agones-ping:1.19.0-3ace450
Linux C++ SDK (build): agonessdk-1.19.0-3ace450-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.19.0-3ace450.zip

A preview of the website (the last 30 builds are retained):

https://3ace450-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2367/head:pr_2367 && git checkout pr_2367
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.19.0-3ace450

google-oss-prow · 2021-11-16T18:26:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rahil-p, roberthbailey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [roberthbailey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rahil-p added 4 commits November 10, 2021 20:36

expose Helm chart values for custom certs

2156621

add custom cert defaults to Helm values

aa7e304

document Helm values for custom certs

ac7fb22

rename Helm tlsCrt value to tlsCert

af8ad3c

google-cla bot added the cla: yes label Nov 11, 2021

google-oss-prow bot added the size/S label Nov 11, 2021

google-oss-prow bot requested review from EricFortin and roberthbailey November 11, 2021 04:32

roberthbailey reviewed Nov 11, 2021

View reviewed changes

move doc entries to New Configuration Features

adbfa06

expose Helm chart value for allocator's allowed client CAs

28f5dc6

google-oss-prow bot added size/M and removed size/S labels Nov 13, 2021

remove Helm file reading from certs

2a9872c

rahil-p force-pushed the main branch from 01039ad to 2a9872c Compare November 13, 2021 18:15

revert "remove Helm file reading from certs"

25f8881

This reverts commit 2a9872c.

Merge branch 'main' into main

3ace450

roberthbailey approved these changes Nov 16, 2021

View reviewed changes

google-oss-prow bot assigned roberthbailey Nov 16, 2021

google-oss-prow bot added the lgtm label Nov 16, 2021

google-oss-prow bot added the approved label Nov 16, 2021

roberthbailey merged commit dd6d188 into googleforgames:main Nov 16, 2021

SaitejaTamma added this to the 1.19.0 milestone Nov 16, 2021

SaitejaTamma added the kind/feature New features for Agones label Nov 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expose Helm chart values for custom certs #2367

expose Helm chart values for custom certs #2367

rahil-p commented Nov 11, 2021 •

edited

Loading

agones-bot commented Nov 11, 2021

roberthbailey Nov 11, 2021

rahil-p Nov 13, 2021 •

edited

Loading

agones-bot commented Nov 13, 2021

agones-bot commented Nov 13, 2021

agones-bot commented Nov 13, 2021

agones-bot commented Nov 13, 2021

rahil-p commented Nov 13, 2021 •

edited

Loading

roberthbailey commented Nov 15, 2021

markmandel commented Nov 15, 2021

rahil-p commented Nov 16, 2021 •

edited

Loading

agones-bot commented Nov 16, 2021

markmandel commented Nov 16, 2021

markmandel commented Nov 16, 2021

agones-bot commented Nov 16, 2021

google-oss-prow bot commented Nov 16, 2021

expose Helm chart values for custom certs #2367

expose Helm chart values for custom certs #2367

Conversation

rahil-p commented Nov 11, 2021 • edited Loading

agones-bot commented Nov 11, 2021

roberthbailey Nov 11, 2021

Choose a reason for hiding this comment

rahil-p Nov 13, 2021 • edited Loading

Choose a reason for hiding this comment

agones-bot commented Nov 13, 2021

agones-bot commented Nov 13, 2021

agones-bot commented Nov 13, 2021

agones-bot commented Nov 13, 2021

rahil-p commented Nov 13, 2021 • edited Loading

roberthbailey commented Nov 15, 2021

markmandel commented Nov 15, 2021

rahil-p commented Nov 16, 2021 • edited Loading

agones-bot commented Nov 16, 2021

markmandel commented Nov 16, 2021

markmandel commented Nov 16, 2021

agones-bot commented Nov 16, 2021

google-oss-prow bot commented Nov 16, 2021

rahil-p commented Nov 11, 2021 •

edited

Loading

rahil-p Nov 13, 2021 •

edited

Loading

rahil-p commented Nov 13, 2021 •

edited

Loading

rahil-p commented Nov 16, 2021 •

edited

Loading