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

Fix issue where sealed secrets status is not updated if sealed secret… #1295

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

mowirth
Copy link
Contributor

@mowirth mowirth commented Aug 20, 2023

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

Additional information

@alvneiayu
Copy link
Collaborator

thanks for your contribution @mowirth. Let me take some time to test it and review it. Again, thanks a lot.

@alvneiayu
Copy link
Collaborator

hi @mowirth,
First of all, thanks a lot for your PR.

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

@mowirth
Copy link
Contributor Author

mowirth commented Sep 15, 2023

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,
Moritz

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2023

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.

@github-actions github-actions bot added the Stale label Oct 1, 2023
@alvneiayu alvneiayu removed the Stale label Oct 1, 2023
@alvneiayu
Copy link
Collaborator

hi @mowirth

thanks a lot for the new modifications. I am testing it, I will come back ASAP with some feedback.

Álvaro

@alvneiayu
Copy link
Collaborator

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>
@mowirth
Copy link
Contributor Author

mowirth commented Oct 15, 2023

@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.

Copy link
Collaborator

@alvneiayu alvneiayu left a 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

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.

Status shows no key could decrypt secret for successful created secret
2 participants