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

Chart verification failing due to vault being uninitialized/sealed #954

Closed
komish opened this issue Sep 7, 2023 · 1 comment · Fixed by #966
Closed

Chart verification failing due to vault being uninitialized/sealed #954

komish opened this issue Sep 7, 2023 · 1 comment · Fixed by #966
Labels
bug Something isn't working openshift

Comments

@komish
Copy link

komish commented Sep 7, 2023

Describe the bug

This is related to the Red Hat chart certification process (https://github.com/openshift-helm-charts/charts), I help maintain the certification workflows.

A recent feature went into chart-verifier allowing it to monitor the readiness of statefulsets and daemonsets. Historically, it has only checked for Deployment readiness.

With this change, the vault chart fails to complete execution because the vault server stateful set is uninitialized and sealed by default.

Looking at the server test you provide in-chart, I see that you just check to see that you can poll the API and get the seal status, but unfortunately that never runs because we always wait to execute the test until the workloads are ready.

By default the server.readinessProbe.path value from values.yaml is commented, meaning a shell exec is executed against the server pod to determine readiness. That's what's failing.

https://github.com/hashicorp/vault-helm/blob/main/templates/server-statefulset.yaml#L155-L169

Your documentation indicates that the values.yaml can be updated to contain a path that returns HTTP 204 if sealed/uninitialized:

https://developer.hashicorp.com/vault/docs/platform/k8s/helm/run#probes

If this was set in the values.yaml file for your chart, it seems like this should work for certification. Would this be something you would consider setting as a part of the default values.yaml?

CC-ing the owners listed in the certified chart entry:

@tvoran
@jasonodonnell
@tomhjp
@calvn
@swenson

To Reproduce
Steps to reproduce the behavior:

  1. Install chart-verifier's development version https://github.com/redhat-certification/chart-verifier/releases/tag/0.1.0 (we have not cut a release that contains this. Our nightly workflows caught this bug
  2. Run it against the current chart you have
  3. Observe the "permanent hang", (or more specifically, a timeout at 30 minutes. If you want, set the timeout value to a shorter value to see the failure)

Expected behavior
The statefulset to become ready so that we can continue with executing chart-testing.

Environment
OpenShift (any recent version should work)

Chart values:
The default included in your chart repo.

Additional context

I do see a values.openshift.yaml in your repo. The problem with using that overlayed over your default values.yaml is that our certification workflows don't take that into account, unless Helm automatically reads in all values.*.yaml files (I don't think it does). When you folks submit your chart to us, you're submit just the chart, and our automation runs chart-verifier in a fixed way on your behalf to generate a result and check for a passing status. Adding the path values that is tolerant to uninitialized states by default in just the values.yaml simplifies this greatly.

@komish komish added the bug Something isn't working label Sep 7, 2023
@tvoran
Copy link
Member

tvoran commented Oct 3, 2023

Hi @komish, thanks for the heads up. We're still discussing this internally, but we may be able to set that default for the openshift version of the chart. (The values.openshift.yaml file contains the defaults we set before submitting the chart to the openshift charts repo.)

tvoran added a commit that referenced this issue Oct 24, 2023
Changes the default server readiness probe to pass when the server is
uninitialized, in order to pass the latest version of the
chart-verifier test (see #954) for details.

Also updates the chart-verifier used in our tests to 1.13.0 (latest).
tvoran added a commit that referenced this issue Oct 26, 2023
)

Changes the default server readiness probe to pass when the server is
uninitialized, in order to pass the latest version of the
chart-verifier test (see #954) for details.

Also updates the chart-verifier used in our tests to 1.13.0 (latest).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working openshift
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants