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

adds optional cronJob to copySecretJob to avoid stale secrets. #792

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

ianhundere
Copy link
Contributor

@ianhundere ianhundere commented Jul 26, 2024

closes #791

Description of the change

This PR adds a cronJob in conjunction w/ the copy secrets job.

Existing or Associated Issue(s)

Additional Information

I included the secret delete permission due to the following error:
Error from server (Conflict): Operation cannot be fulfilled on secrets "<secret_name>": the object has been modified; please apply your changes to the latest version and try again

^ When trying to apply changes to secrets, I would receive the patch permission error about the resource version. This happens when the resource version of the secret being patched does not match the current resource version stored (e.g. used to manage optimistic concurrency). So if the secret has been modified since retrieved, the resource version will no longer match, thus preventing any further patch operation(s) to avoid overwriting changes made by other operations.

We could also retrieve the latest version of the secret, and then apply our changes to the latest version, and then patch, but I thought simply deleting / applying was better.

Checklist

  • Chart version bumped in Chart.yaml according to semver. Where applicable, update and bump the versions in any associated umbrella chart
  • Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
  • JSON Schema generated.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.
Linting charts...

------------------------------------------------------------------------------------------------------------------------
 Charts to be processed:
------------------------------------------------------------------------------------------------------------------------
 scaffold => (version: "0.6.62", path: "charts/scaffold")
------------------------------------------------------------------------------------------------------------------------

"sigstore" already exists with the same configuration, skipping
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "sigstore" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 6 charts
Downloading fulcio from repo https://sigstore.github.io/helm-charts
Downloading rekor from repo https://sigstore.github.io/helm-charts
Downloading trillian from repo https://sigstore.github.io/helm-charts
Downloading ctlog from repo https://sigstore.github.io/helm-charts
Downloading tuf from repo https://sigstore.github.io/helm-charts
Downloading tsa from repo https://sigstore.github.io/helm-charts
Deleting outdated charts
Linting chart "scaffold => (version: \"0.6.62\", path: \"charts/scaffold\")"
Checking chart "scaffold => (version: \"0.6.62\", path: \"charts/scaffold\")" for a version bump...
Old chart version: 0.6.61
New chart version: 0.6.62
Chart version ok.
Validating /Users/ianhundere/Projects/helm-charts/charts/scaffold/Chart.yaml...
Validation success! 👍

Linting chart with values file "charts/scaffold/ci/ci-values.yaml"...

==> Linting charts/scaffold
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

------------------------------------------------------------------------------------------------------------------------
 ✔︎ scaffold => (version: "0.6.62", path: "charts/scaffold")
------------------------------------------------------------------------------------------------------------------------
All charts linted successfully

@ianhundere ianhundere force-pushed the adds_copysecrets_cronjob branch from 6354db0 to 323068b Compare July 26, 2024 02:00
@ianhundere ianhundere changed the title adds cronJob. add optional cronJob to copySecretJob to avoid stale secrets. Jul 26, 2024
@ianhundere ianhundere force-pushed the adds_copysecrets_cronjob branch from 323068b to 7aabce4 Compare July 26, 2024 12:52
@ianhundere ianhundere changed the title add optional cronJob to copySecretJob to avoid stale secrets. adds optional cronJob to copySecretJob to avoid stale secrets. Jul 27, 2024
@vipulagarwal
Copy link
Contributor

vipulagarwal commented Jul 29, 2024

Hi @ianhundere, this feature can be helpful to automate things. Thank you.

Do we need to add some logic around restarting the tuf-server to update the TUF root? On the initial setup, tuf-server pod keeps on failing unless these secrets are populated by the copySecretsJob. I am unsure if it will automatically pickup the new keys from the updated secrets by the cron job, once it is up and running.

@ianhundere
Copy link
Contributor Author

ianhundere commented Jul 29, 2024

Do we need to add some logic around restarting the tuf-server to update the TUF root?

@vipulagarwal that's a good callout, thank you.

i'll include an additional container to restart the tuf deployment to create a new TUF root.

@ianhundere ianhundere force-pushed the adds_copysecrets_cronjob branch 7 times, most recently from 7e29bb8 to 9dbde6b Compare July 30, 2024 15:23
@ianhundere ianhundere force-pushed the adds_copysecrets_cronjob branch 2 times, most recently from 8a46ab4 to 2f2e929 Compare August 5, 2024 20:03
@ianhundere ianhundere force-pushed the adds_copysecrets_cronjob branch 2 times, most recently from a75971c to dc34b48 Compare August 16, 2024 14:31
@ianhundere
Copy link
Contributor Author

@bobcallaway do you think there'd be any interest in something like this?

@bobcallaway
Copy link
Member

@bobcallaway do you think there'd be any interest in something like this?

Could you position why using something like https://external-secrets.io/latest/ wouldn't work?

@ianhundere
Copy link
Contributor Author

ianhundere commented Sep 10, 2024

@bobcallaway that's a good point, and while we are using external secrets, those external secrets point to the secrets within' each respective service namespace (e.g. trillian-system, rekor-system, fulcio-system, ctlog-system, tsa-system etc), but what happens is if one of those secrets is updated, TUF won't know about it since it runs only once and never again checks to see if its respective secrets are up-to-date unless TUF's copied secrets are removed "and" TUF is deployed again (e.g. rerun the copySecretJob).

tl;dr provide a quick/native way to ensure TUF's copied secrets are updated/pointing to their respective source of truth regularly.

edit/further context: things like intermediate/leaf certs (e.g. fulcio/tsa), trillian auth, etc are stored in secrets manager to use w/ external secrets, but for some of the dynamically created secrets (e.g. rekor public key etc) we rely on the created k8s secret and not an external secret.

@bobcallaway
Copy link
Member

ah. in sigstore public-good-instance, we are using https://github.com/stakater/Reloader as a "watcher" to bounce deployments when k8s secrets are changed (and they are auto-updated by external-secrets).

i'm not against the concept here, just trying to reduce the number of moving pieces w/i the helm-charts.

@ianhundere
Copy link
Contributor Author

ianhundere commented Sep 10, 2024

i'm not against the concept here, just trying to reduce the number of moving pieces w/i the helm-charts.

i totally understand, less is often more. 😅

while we do store some things from svc namespaces (e.g. tsa cert-chain, fulcio intermediate/ca certs, etc) we don't currently have a mechanism to save the following in external secrets:

  • rekor public key (found in rekor ns)
  • fulcio-cert (found in ctlog ns)
  • ctfe/ctlog public key (found in ctlog ns)

we have found this helpful especially during the development phase; either way, let me know if this is something you think the community might find value in, and if so, i'll rebase to resolve conflicts / implement any suggested changes.

@bobcallaway
Copy link
Member

ah, i think i see the issue here now (which is the use of jobs & init containers to dynamically create secrets in the helm charts) vs creating those assets a priori and putting them into a secret manager/vault and using external-secrets.

I think adding this PR is fine given the existing pattern, but outside of ephemeral testing clusters I'd think a more intentional IaC approach would be a better approach for users.

@ianhundere
Copy link
Contributor Author

ianhundere commented Sep 12, 2024

yep yep / agreed. thanks for the quick response. i'm outta pocket today, but will rebase/push tomorrow morn.

@ianhundere ianhundere force-pushed the adds_copysecrets_cronjob branch 3 times, most recently from 317c3c5 to 24dc1dc Compare September 12, 2024 14:53
@ianhundere ianhundere force-pushed the adds_copysecrets_cronjob branch from 24dc1dc to f48f138 Compare September 12, 2024 14:55
@ianhundere
Copy link
Contributor Author

ianhundere commented Oct 25, 2024

@bobcallaway just checking in to see if there’s anything else you’d like me to adjust or clarify in this PR. hopefully this'll help others keep secrets up-to-date w/o extra customizations etc.

thanks again for your consideration / all the work on the Sigstore project!"

Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for the delay!

@bobcallaway bobcallaway merged commit e88db0e into sigstore:main Oct 25, 2024
3 checks passed
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.

add optional cronJob to copySecretJob to avoid stale secrets.
3 participants