Keep secret checksum annotation keys unique and valid

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/cloud-native/gitlab-operator:jmrp-secret-checksum-key-collision into master

What does this MR do?

annotateSecretsChecksum cut each checksum/secret-<name> annotation key to 63 characters, which keeps the first 47 characters of the Secret name. Two Secrets used by one workload whose names share those 47 characters got the same key, so only one checksum was kept and which one depended on map order. A name whose 47th character is - or . gave a key that ends with it, and the API server refuses that workload, so every reconcile stopped there.

This MR adds internal.TruncateWithHash beside internal.Truncate, and annotateSecretsChecksum uses it. A key that fits in 63 characters is returned unchanged. A longer key keeps a prefix and ends with - and the first 10 hexadecimal characters of the SHA-256 hash of the full key, so it is unique per Secret name, the same on every reconcile, at most 63 characters long, and ends with an alphanumeric character. internal.Truncate has no caller left after this change; I kept it to keep the fix small, and I can remove it here if you prefer.

Behavior change: a workload that uses a Secret whose key is longer than 63 characters gets a new key once after the upgrade, which rolls it once. Keys of up to 63 characters keep their names, so a workload whose Secret names are short is not rolled.

How I tested it

New specs in the controller suite, in controllers/annotate_secrets_checksum_test.go, call annotateSecretsChecksum on a Deployment that mounts the Secrets, served by the controller-runtime fake client. For two short names, for two names that differ only after the cut, and for a name whose cut lands right after a dash, they check one valid key of at most 63 characters per Secret, the same over 21 calls, and for the short names that each key keeps its name.

go test ./controllers -count=1 -run TestAPIs -v -ginkgo.focus annotateSecretsChecksum

On master the two long name specs fail and the short name spec passes (output below); with this change all three pass. The new specs for TruncateWithHash in controllers/internal/helpers_test.go pass with go test ./controllers/internal/ (13 of 13 specs).

In the CI images (gitlab-operator-build-base-golang-1.27 and golangci-lint:v2.14-alpine), on b9225313 with this commit:

  • task slow-unit-tests with each chart version in CHART_VERSIONS (10.5.0, 10.4.2 and 10.3.6): all six suites passed each time, the Controller Suite with 28 of 28 specs
  • task unit-tests with chart 10.5.0: 26 suites passed
  • task lint: 0 issues. in all three of its runs
  • go test -race on ./controllers/internal/ and on the new controller specs

I also asked envtest's API server, in throwaway specs that are not part of this MR. On master, a server-side dry run of a Deployment with the key checksum/secret-gitlab-production-europe-west-gitlab-workhorse- is refused, and a GitLab whose Postgres password Secret is named gitlab.gitlab-postgres-production-europe-west1.credentials.postgresql.acid.zalan.do never gets past its GitLab Exporter Deployment (details in the issue). With this change the dry run is accepted, and the same GitLab installs, with checksum/secret-gitlab.gitlab-postgres-production-eu-2682d16dd5 on its exporter, Sidekiq and webservice Deployments.

Output of the new specs on master 
• [FAILED] [0.002 seconds]
annotateSecretsChecksum when The cut lands right after a dash [It] Should add one valid key per Secret, the same on every run
/src/controllers/annotate_secrets_checksum_test.go:75

  [FAILED] checksum/secret-gitlab-production-europe-west-gitlab-workhorse-
  Expected
      <[]string | len:1, cap:1>: [
          "name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')",
      ]
  to be empty
  In [It] at: /src/controllers/annotate_secrets_checksum_test.go:83

• [FAILED] [0.001 seconds]
annotateSecretsChecksum when Two Secret names differ only after the cut [It] Should add one valid key per Secret, the same on every run
/src/controllers/annotate_secrets_checksum_test.go:75

  [FAILED] Expected
      <map[string]string | len:1>: {
          "checksum/secret-gitlab-production-europe-west1-gitlab-workhorse": "95264e72f3bc4d9eeaa76358ab1efe68b8abfde9dd2b45958f4cbe1119952eea",
      }
  to have length 2
  In [It] at: /src/controllers/annotate_secrets_checksum_test.go:79

Ran 3 of 28 Specs in 5.191 seconds
FAIL! -- 1 Passed | 2 Failed | 0 Pending | 25 Skipped

Checklist

See Definition of done.

For anything in this list which will not be completed, please provide a reason in the MR discussion.

Required

  • Merge Request Title and Description are up to date, accurate, and descriptive
  • MR targeting the appropriate branch
  • MR has a green pipeline on GitLab.com
  • When ready for review, MR is marked as ready (not a draft) and assigned to a reviewer.

Expected (please provide an explanation if not completing)

  • Test plan indicating conditions for success has been posted and passes
  • Documentation created/updated: not needed, nothing documented changes
  • Tests added
  • Integration tests added to GitLab QA: not needed, the controller suite covers it
  • Equivalent MR/issue for omnibus-gitlab opened: not applicable
  • Validate potential values for new configuration settings. Formats such as integer 10, duration 10s, URI scheme://user:passwd@host:port may require quotation or other special handling when rendered in a template and written to a configuration file. Not applicable, no new setting.

Reviewers checklist

  • As soon as you begin reviewing, select Start a review on your first comment so the MR shows Reviewer started review.

Related issues

Closes #2336 (closed)

Merge request reports