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

Add volumes and env vars to helm hook test pod #673

Merged

Conversation

Iristyle
Copy link
Contributor

@Iristyle Iristyle commented Jan 7, 2022

  • Fix test typo

  • Add basic server-test Pod tests

    • This covers all existing functionality that matches what's
      present in server-statefulset.bats
  • Fix server-test helm hook Pod rendering

    • Properly adhere to the global.enabled flag and the presence of
      the injector.externalVaultAddr setting, the same way that
      the servers StatefulSet behaves
  • Uses the same extraEnvironmentVars, volumes and volumeMounts set on
    the server statefulset to configure the Vault server test pod used by
    the helm test hook

  • This is necessary in situations where TLS is configured, but the
    certificates are not affiliated with the k8s CA / part of k8s PKI

  • Fixes Test Helm Hook Pod should be more configurable #665

@hashicorp-cla
Copy link

hashicorp-cla commented Jan 7, 2022

CLA assistant check
All committers have signed the CLA.

@Iristyle Iristyle force-pushed the gh-665-make-vault-test-pod-configurable branch from 7a7af2f to ab6813f Compare January 7, 2022 22:39
@Iristyle Iristyle force-pushed the gh-665-make-vault-test-pod-configurable branch 9 times, most recently from 6512fc5 to bde931c Compare January 11, 2022 00:38
@Iristyle Iristyle marked this pull request as ready for review January 11, 2022 00:51
@Iristyle Iristyle marked this pull request as draft January 11, 2022 00:51
@@ -35,6 +37,9 @@ spec:
fi

exit 0

volumeMounts:
{{ template "vault.mounts" . }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This drags in the audit and data volumes which doesn't make sense... shooting for something simpler

@Iristyle Iristyle force-pushed the gh-665-make-vault-test-pod-configurable branch from bde931c to e45c3db Compare January 11, 2022 18:22
@Iristyle Iristyle marked this pull request as ready for review January 11, 2022 20:43
@Iristyle
Copy link
Contributor Author

I've verified this fixes the TLS issues in the test chart. Logs now look like:

Checking for sealed info in 'vault status' output                                                                                 
Attempt 0...                                                                                                                      
Error checking seal status: Get "https://cd4pe-vault.default.svc:8200/v1/sys/seal-status": dial tcp 10.106.187.188:8200: connect: 
Attempt 1...                                                                                                                      
Error checking seal status: Get "https://cd4pe-vault.default.svc:8200/v1/sys/seal-status": dial tcp 10.106.187.188:8200: connect: 
Attempt 2...                                                                                                                      
Error checking seal status: Get "https://cd4pe-vault.default.svc:8200/v1/sys/seal-status": dial tcp 10.106.187.188:8200: connect: 
Attempt 3...                                                                                                                      
Error checking seal status: Get "https://cd4pe-vault.default.svc:8200/v1/sys/seal-status": dial tcp 10.106.187.188:8200: connect: 
Attempt 4...                                                                                                                      
sealed: true

@tvoran tvoran added enhancement New feature or request vault-server Area: operation and usage of vault server in k8s labels Jan 11, 2022
Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

Would you mind adding unit tests for these options? I think it's probably time we added a test/unit/server-test.bats. Should be able to use server-statefulset.bats for examples of testing extraEnvironmentVars, volumeMounts, and volumes.

@@ -537,7 +537,7 @@ load _helpers
cd `chart_dir`
local object=$(helm template \
--show-only templates/server-statefulset.yaml \
--set 'server.stanadlone.enabled=true' \
--set 'server.standalone.enabled=true' \
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch!

Comment on lines 39 to 48
volumeMounts:
{{- if .Values.server.volumeMounts }}
{{- toYaml .Values.server.volumeMounts | nindent 12}}
{{- end }}
volumes:
{{- if .Values.server.volumes }}
{{- toYaml .Values.server.volumes | nindent 8}}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

I think the indents can be four less (8 and 4) since it's in a Pod instead of a Deployment template.

Suggested change
volumeMounts:
{{- if .Values.server.volumeMounts }}
{{- toYaml .Values.server.volumeMounts | nindent 12}}
{{- end }}
volumes:
{{- if .Values.server.volumes }}
{{- toYaml .Values.server.volumes | nindent 8}}
{{- end }}
volumeMounts:
{{- if .Values.server.volumeMounts }}
{{- toYaml .Values.server.volumeMounts | nindent 8}}
{{- end }}
volumes:
{{- if .Values.server.volumes }}
{{- toYaml .Values.server.volumes | nindent 4}}
{{- end }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can re-render and check... but I also did verify that the current indentation works in a live deployment to map the volumes in question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right on the indentation rendering wrong -- verified with

helm template --show-only templates/tests/server-test.yaml --set 'server.volumes[0].name=plugins' --set 'server.volumes[0].emptyDir={}' --set 'server.volumeMounts[0].name=plugins' --set 'server.volumeMounts[0].mountPath=/usr/local/libexec/vault' --set 'server.volumeMounts[0].readOnly=true' .

@Iristyle Iristyle force-pushed the gh-665-make-vault-test-pod-configurable branch 2 times, most recently from fa5f4a4 to d1c9508 Compare January 13, 2022 23:30
@Iristyle
Copy link
Contributor Author

Follow-on PR at #678 brings some more feature parity to the test pod

Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

Thanks for adding all the tests. Just one more suggestion and I think we'll be good to go.

test/unit/server-test.bats Show resolved Hide resolved
 - This covers all existing functionality that matches what's
   present in server-statefulset.bats
@Iristyle Iristyle force-pushed the gh-665-make-vault-test-pod-configurable branch from d1c9508 to 694f2da Compare January 19, 2022 17:14
 - Properly adhere to the global.enabled flag and the presence of
   the injector.externalVaultAddr setting, the same way that
   the servers StatefulSet behaves
 - Uses the same extraEnvironmentVars, volumes and volumeMounts set on
   the server statefulset to configure the Vault server test pod used by
   the helm test hook
 - This is necessary in situations where TLS is configured, but the
   certificates are not affiliated with the k8s CA / part of k8s PKI

 - Fixes hashicorpGH-665
@Iristyle Iristyle deleted the gh-665-make-vault-test-pod-configurable branch January 20, 2022 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request vault-server Area: operation and usage of vault server in k8s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test Helm Hook Pod should be more configurable
3 participants