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

set up individual secrets for what used to be HCPConfig secret #1608

Merged
merged 11 commits into from
Oct 11, 2022

Conversation

jmurret
Copy link
Member

@jmurret jmurret commented Oct 10, 2022

NOTE: At first glance this looks huge, but its really because the same sets of bats tests and helm failure logic are in multiple files to check that for each secret, if one of the secretName or the secretKey is set, then both are set. Besides this, this actually a very small change.

Motivation:

  • the question of whether or not each of these environment variables needs to be individually configured in the helm chart was first posed by Iryna in my original PR
  • there are 6 six environment variables that canbe set for the hcpsdk. 3 are required and 3 are optional. in order to make the setting of these environment variables optional in server-statefulset, they need to be individual helm values so that the system can check whether they are nil or not.
  • this change replaces what consul-k8s CLI - hcp self-managed clusters - fix the required environment variables that should just default to production settings #1593 tried to do bu did notwork (described below).
  • the current method of trying to allow the environment variable to read a null key from a secret without any conditional logic such as below errors on the pod during startup and therefor still causes the user to set all 6 environment variables when using the CLI.
              valueFrom:
                secretKeyRef:
                  name: {{ .Values.global.cloud.secretName }}
                  key: scada-address 

Changes proposed in this PR:

  • make all 6 of the environment variable their own configurable helm chart value
  • conditionally set the 3 optional ones in server-statefulset based on the presence of a value
  • conditionally set them in the CLI preset values for cloud preset based on the presence of a value when the user runs the CLI
  • add helm check for the 3 required values when global.cloud.enabled=true
  • add helm check for all 6 six secrets so that when either the secretName or secretValue is provided, both are provided

How I've tested this PR:

  • unit / acceptance tests
  • manually

How I expect reviewers to test this PR:
👀

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@jmurret jmurret requested review from a team, ndhanushkodi and t-eckert and removed request for a team October 11, 2022 12:45
@jmurret jmurret marked this pull request as ready for review October 11, 2022 12:45

Usage: {{ template "consul.validateCloudConfiguration" . }}
Usage: {{ template "consul.validaterequiredCloudSecretsExist" . }}
Copy link
Member Author

Choose a reason for hiding this comment

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

changed name. this used to be one secret with 6 six keys (3 required and 3 optional). Now there are 6 secrets for more flexibility and the ability to allow optional values.


{{/*
Fails global.cloud.enabled is true and one of the following secrets has either an empty secretName or secretKey.
- global.cloud.resourceId.secretName / secretKey
Copy link
Member Author

Choose a reason for hiding this comment

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

This simply checks all 6 secrets so that if either secretName or secretKey are provided, then both are provided.


if err := i.saveBootstrapTokenSecret(config); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

got rid of the small helper functions like saveBootstrapTokenSecret() because they really did not do much and I moved everything in line so its quicker to read and know that they are all behaving the same way.

@jmurret jmurret requested a review from t-eckert October 11, 2022 15:17
Copy link
Contributor

@t-eckert t-eckert left a comment

Choose a reason for hiding this comment

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

Great work!

charts/consul/test/unit/server-acl-init-job.bats Outdated Show resolved Hide resolved
Copy link
Contributor

@curtbushko curtbushko left a comment

Choose a reason for hiding this comment

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

Looks pretty good! I have a few questions/suggestions but nothing in particular that I would block on.

cli/preset/cloud_preset.go Show resolved Hide resolved
charts/consul/values.yaml Show resolved Hide resolved
cli/preset/cloud_preset.go Outdated Show resolved Hide resolved
cli/preset/cloud_preset.go Outdated Show resolved Hide resolved
charts/consul/templates/_helpers.tpl Show resolved Hide resolved
@jmurret jmurret merged commit 3c104c0 into main Oct 11, 2022
@jmurret jmurret deleted the jm/hcp-secrets branch October 11, 2022 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants