helm: add job validating configuration on deploy by hugoShaka · Pull Request #19333 · gravitational/teleport

hugoShaka · 2022-12-13T15:40:15Z

This PR adds helm hooks deploying a test configuration job and running teleport configure --test to validate the teleport.yaml configuration is sane.

Note: merge only once the #18857 has been merged

webvictim · 2022-12-14T15:04:54Z

I wish Github supported stacked diffs...

Maybe change the base of your PR to your custom branch temporarily so it only shows the relevant changes?

hugoShaka · 2022-12-14T15:24:05Z

I wish Github supported stacked diffs...

Maybe change the base of your PR to your custom branch temporarily so it only shows the relevant changes?

My bad, I forgot to change the base

webvictim

Looks good, I skimmed most of the stuff that I'd already reviewed because Github isn't smart enough to show me a merged view I guess?!

webvictim · 2022-12-14T15:03:56Z

@@ -0,0 +1,7 @@
+# This setup is not safe for production because the proxy will self-sign its certificate.
+# Use those values for testing only


Suggested change

# Use those values for testing only

# Use these values for testing only

webvictim · 2022-12-15T20:08:24Z

+{{- end -}}
+
+{{- define "teleport-cluster.auth.config.custom" -}}
+{{ fail "'custom' mode has been depreacted with chart v12 because of the proxy/auth split, see http://link" }}


Reminder to update this one when we know what the link will be :D

webvictim · 2022-12-15T20:08:50Z

+{{- end }}
+  teleport.yaml: |2
+    {{- mustMergeOverwrite (include $configTemplate . | fromYaml) $auth.teleportConfig | toYaml | nindent 4 -}}
+{{- end }}


nit: No trailing line break

webvictim · 2022-12-15T20:11:00Z

+
+{{- define "teleport-cluster.proxy.config.custom" -}}
+{{ fail "'custom' mode has been depreacted with chart v12 because of the proxy/auth split, see http://link" }}
+{{- end -}}


nit: No trailing line break

Also missing link ofc

webvictim · 2022-12-15T20:11:44Z

+data:
+  teleport.yaml: |2
+    {{- mustMergeOverwrite (include $configTemplate . | fromYaml) $proxy.teleportConfig | toYaml | nindent 4 -}}
+{{- end }}


nit: No trailing line break

webvictim · 2022-12-15T20:13:33Z

+      validateConfigOnDeploy: false
+    asserts:
+      - hasDocuments:
+          count: 0


nit: No trailing line break

webvictim · 2022-12-15T20:15:52Z

@@ -0,0 +1 @@
+{}


I'm not sure why this is blank? I guess there's no snapshotting needed, so rather than have no file it just has an empty object?

webvictim · 2022-12-15T20:18:52Z

+{{- end -}}
+
+{{- define "teleport-cluster.auth.config.custom" -}}
+{{ fail "'custom' mode has been depreacted with chart v12 because of the proxy/auth split, see http://link" }}


Suggested change

{{ fail "'custom' mode has been depreacted with chart v12 because of the proxy/auth split, see http://link" }}

{{ fail "'custom' mode has been deprecated with chart v12 because of the proxy/auth split, see http://link" }}

webvictim · 2022-12-15T20:19:12Z

+{{- end -}}
+
+{{- define "teleport-cluster.proxy.config.custom" -}}
+{{ fail "'custom' mode has been depreacted with chart v12 because of the proxy/auth split, see http://link" }}


Suggested change

{{ fail "'custom' mode has been depreacted with chart v12 because of the proxy/auth split, see http://link" }}

{{ fail "'custom' mode has been deprecated with chart v12 because of the proxy/auth split, see http://link" }}

hugoShaka · 2022-12-15T21:02:05Z

Looks good, I skimmed most of the stuff that I'd already reviewed because Github isn't smart enough to show me a merged view I guess?!

Ugh, I rebased the base branch and now it's lost.I'll rebase again all the satellite branches.

marcoandredinis

LGTM 👍

Just one question:

Do we have to recreate everything in order to run the teleport configure test?
Things can get out of sync between
examples/chart/teleport-cluster/templates/auth/predeploy_job.yaml and
examples/chart/teleport-cluster/templates/auth/statefulset.yaml

It seems we even create another token for the proxy to join, even though is not used (or is it?)

hugoShaka · 2022-12-16T20:30:59Z

Do we have to recreate everything to run the teleport configure test? Things can get out of sync between examples/chart/teleport-cluster/templates/auth/predeploy_job.yaml and examples/chart/teleport-cluster/templates/auth/statefulset.yaml

In its current state, we need the config and whatever is mounted (because it can contain parts of the config). So we have to duplicate at least the podSpec and the configMap. Ideally, the teleport configure --test would also test backend connectivity in auth, so it would require cloud credential mounts as well. This is why I left all those mounts and env vars.

I don't know if putting some parts of the podSpec in a template would improve maintainability. This becomes a challenge, especially as we don't want to deploy additional containers like the operator.

webvictim · 2022-12-16T21:44:05Z

Ideally, the teleport configure --test would also test backend connectivity in auth,

JFYI it doesn't (but it should 😁)

Co-authored-by: Gus Luxton <gus@goteleport.com>

Part of [RFD-0096](#18274) This PR adds helm hooks deploying a test configuration job and running `teleport configure --test` to validate the `teleport.yaml` configuration is sane.

hugoShaka requested review from marcoandredinis and webvictim December 13, 2022 15:40

github-actions Bot requested review from alistanis and smallinsky December 13, 2022 15:40

github-actions Bot added helm size/md labels Dec 13, 2022

hugoShaka mentioned this pull request Dec 13, 2022

Deploy split proxy/auth with helm chart #18857

Merged

7 tasks

hugoShaka changed the base branch from master to hugo/chart-split-proxy-auth December 14, 2022 15:24

hugoShaka force-pushed the hugo/chart-split-proxy-auth branch from f844519 to 620b06f Compare December 15, 2022 14:08

hugoShaka changed the title ~~Hugo/chart validate conf job~~ helm: add job validating configuration on deploy Dec 15, 2022

hugoShaka requested a review from tigrato December 15, 2022 19:57

webvictim approved these changes Dec 15, 2022

View reviewed changes

webvictim reviewed Dec 15, 2022

View reviewed changes

marcoandredinis approved these changes Dec 16, 2022

View reviewed changes

hugoShaka added 2 commits December 19, 2022 12:20

helm: add config validation hook

ac6f7eb

fixup! helm: add config validation hook

8be205d

hugoShaka force-pushed the hugo/chart-validate-conf-job branch from 402b5d1 to 8be205d Compare December 19, 2022 17:22

hugoShaka and others added 2 commits December 19, 2022 12:25

Update examples/chart/teleport-cluster/values.yaml

eb0efbf

Co-authored-by: Gus Luxton <gus@goteleport.com>

Fix linebreaks + rename test predeploy file

838527d

hugoShaka merged commit c83b7d1 into hugo/chart-split-proxy-auth Dec 19, 2022

hugoShaka deleted the hugo/chart-validate-conf-job branch December 19, 2022 17:30

		@@ -0,0 +1,7 @@
		# This setup is not safe for production because the proxy will self-sign its certificate.
		# Use those values for testing only

	# Use those values for testing only
	# Use these values for testing only

	{{ fail "'custom' mode has been depreacted with chart v12 because of the proxy/auth split, see http://link" }}
	{{ fail "'custom' mode has been deprecated with chart v12 because of the proxy/auth split, see http://link" }}

Conversation

hugoShaka commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webvictim commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hugoShaka commented Dec 14, 2022

Uh oh!

webvictim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webvictim Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugoShaka commented Dec 15, 2022

Uh oh!

marcoandredinis left a comment

Choose a reason for hiding this comment

Uh oh!

hugoShaka commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webvictim commented Dec 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hugoShaka commented Dec 13, 2022 •

edited

Loading

webvictim commented Dec 14, 2022 •

edited

Loading

webvictim Dec 15, 2022 •

edited

Loading

hugoShaka commented Dec 16, 2022 •

edited

Loading