-
Notifications
You must be signed in to change notification settings - Fork 691
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
Fix issue where sealed secrets status is not updated if sealed secret… #1295
Conversation
thanks for your contribution @mowirth. Let me take some time to test it and review it. Again, thanks a lot. |
hi @mowirth, I have some concerns about this solution because these lines that you are deleting will generate other issue already fixed in the past and also we are going to increase the CPU a lot (#223). In my opinion, we need to find an alternative way to solve this problem. Thanks a lot again Álvaro |
Hi @alvneiayu , I added a check to only send an update if the status was changed - this way, a status change is still propagated, but only when necessary. Best regards and thanks, |
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
hi @mowirth thanks a lot for the new modifications. I am testing it, I will come back ASAP with some feedback. Álvaro |
hi @mowirth I tested it and it seems that the performance is OK. We have several tests included inside the controller_test.go file to check the Status. Could you take a look if it is possible to include a new test to be sure that future changes (new PRs from other issues) will not generate the same issue resolved? Thanks a lot again for your PR Álvaro |
… was not modified inbetween generations Signed-off-by: Moritz Wirth <mw@flanga.io>
Signed-off-by: Moritz Wirth <mw@flanga.io>
Signed-off-by: Moritz Wirth <mw@flanga.io>
@alvneiayu I added some tests to check whether the expected behaviour regarding the update is applied and that an errorneous update with a previously successfully unsealed sealed secret properly updates the condition. |
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, and thanks a lot @mowirth for your awesome contribution
Description of the change
Removes a check used to prevent unnecessary updates of the sealed secrets status.
Benefits
The check is used to prevent unnecessary updates on the sealed secrets status.
However, in some cases, the sealed secret might not be updated to successfully generate the secret.
For example, if a secret is managed by a different controller, the generation from a sealedSecret would fail; if the controller is removed later, the secret would be unsealed successfully.
In this case, the check prevents a successful update leading to issues when the secret is successfully unsealed (and the successful unseal is reported), but the secret status is not updated since the generation/observedGeneration steps were not updated.
This is also an issue when using management tools such as ArgoCD, since the status would always report an error, while the secret is successfully unsealed.
Possible drawbacks
May increase updates of sealed secrets status if not necessary.
Applicable issues
no key could decrypt secret
for successful created secret #853Additional information