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

Ensure entry is removed from CM on secret error. #1605

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented Mar 14, 2022

Signed-off-by: Ville Aikas vaikas@chainguard.dev

Summary

If there's an entry in the Configmap, say a secret existed and a valid entry was made, but then secret was deleted or modified, remove an existing entry.

Fix #1601

Ticket Link

Fixes

Release Note


Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
@cpanato cpanato requested a review from hectorj2f March 14, 2022 11:13
@hectorj2f
Copy link
Contributor

hectorj2f commented Mar 14, 2022

@vaikas question: why shouldn’t we get an error of a secret that is been used? Removing something from a configmap sounds like a smart hidden way to me. Am i am missing something?

@vaikas
Copy link
Contributor Author

vaikas commented Mar 14, 2022

@vaikas question: why shouldn’t we get an error of a secret that is been used? Removing something from a configmap sounds like a smart hidden way to me. Am i am missing something?

Sorry, not parsing. Iff there's an error with the CIP, say referring to a bad secret (doesn't exist, it's malformed, etc.). You will get an error and an event for it as well. For example, here we receive an event when a secret is missing:
https://github.com/sigstore/cosign/pull/1605/files#diff-f95503af2405ad769f56a5e3ec662f2d57380441e636ba81ffe33b93e00ea4ebR252

What this PR is addressing is the fact that we have a sequence like this:

  1. CIP created, Secret is valid => entry in the CM is made
  2. Secret updated (or deleted). At this point the CM is stale, it's holding invalid data.
  3. Removal of stale entry in the CM
  4. Return an error from the Reconcile loop => Causes an event to be emitted (like the link above).

Does that make sense @hectorj2f , or are you asking something else?

@hectorj2f
Copy link
Contributor

@vaikas thanks. My concern was between steps 1 and 2. I am used to set an ownership reference to the secret, so whenever someone attempts to delete it should get an error that is used by our other resource.

@vaikas
Copy link
Contributor Author

vaikas commented Mar 14, 2022

@hectorj2f yeah, that is usually how we'd do it if we'd be the one controlling the resource. We could add an ownerref but that feels a bit wonky since we don't really own that in this case. But, even if we did that, we would still have to deal with the case where the secret contains invalid data.

@vaikas vaikas merged commit 8906eff into sigstore:main Mar 14, 2022
@vaikas vaikas deleted the issue-1601 branch March 14, 2022 11:59
@github-actions github-actions bot added this to the v1.7.0 milestone Mar 14, 2022
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
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.

Remove entries from CM if secret goes missing
3 participants