-
Notifications
You must be signed in to change notification settings - Fork 91
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
adds optional cronJob to copySecretJob to avoid stale secrets. #792
Conversation
6354db0
to
323068b
Compare
323068b
to
7aabce4
Compare
Hi @ianhundere, this feature can be helpful to automate things. Thank you. Do we need to add some logic around restarting the |
@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. |
7e29bb8
to
9dbde6b
Compare
8a46ab4
to
2f2e929
Compare
a75971c
to
dc34b48
Compare
@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? |
@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. |
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. |
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:
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. |
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. |
yep yep / agreed. thanks for the quick response. i'm outta pocket today, but will rebase/push tomorrow morn. |
317c3c5
to
24dc1dc
Compare
Signed-off-by: ianhundere <138915+ianhundere@users.noreply.github.com>
Signed-off-by: ianhundere <138915+ianhundere@users.noreply.github.com>
24dc1dc
to
f48f138
Compare
@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!" |
There was a problem hiding this 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!
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.yaml
according to semver. Where applicable, update and bump the versions in any associated umbrella chartvalues.yaml
and added to the README.md. The helm-docs utility can be used to generate the necessary content. Usehelm-docs --dry-run
to preview the content.ct lint
command.