Start reconciliation when a secret has changed by shuheiktgw · Pull Request #3459 · external-secrets/external-secrets

shuheiktgw · 2024-05-06T10:15:10Z

Problem Statement

We used to use Owns(&v1.Secret{}, builder.OnlyMetadata) in the SetupWithManager method, but depending on the creation policy, we don't set an owner reference. As a result, when users use a creation policy except for Owner, reconciliation does not start even if a secret has changed.

Related Issue

N/A

Proposed Changes

Use Watches with a custom field indexer to get the secret effectively.

Checklist

I have read the contribution guidelines
All commits are signed with git commit --signoff
My changes have reasonable test coverage
All tests pass with make test
I ensured my PR is ready for review with make reviewable

Signed-off-by: shuheiktgw <[email protected]>

pkg/controllers/externalsecret/suite_test.go

Signed-off-by: shuheiktgw <[email protected]>

pkg/controllers/externalsecret/externalsecret_controller_test.go

pkg/controllers/externalsecret/externalsecret_controller.go

gusfcarvalho · 2024-05-06T23:24:39Z

I am no expert on controller-runtime shenanigans, but did some comments there.

One question that I also have - are we watching potentially for all secrets in all namespaces? Or are we filtering that down to the target secrets only with the index bit?

Signed-off-by: shuheiktgw <[email protected]>

sonarqubecloud · 2024-05-09T00:28:46Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

shuheiktgw · 2024-05-09T00:31:11Z

One question that I also have - are we watching potentially for all secrets in all namespaces? Or are we filtering that down to the target secrets only with the index bit?

Please give me some time to test this locally 🙏 I believe it should not be a big problem since even if we use Owns, the informer caches all the secrets in all the namespaces...

shuheiktgw · 2024-05-09T23:48:53Z

@gusfcarvalho
I've tested it locally and confirmed it won't affect memory consumption much. I've created 1000 secrets (without any owner references) and checked how the operator's memory consumption changes 🙏

Operator created from this PR

NAME                                                CPU(cores)   MEMORY(bytes)
external-secrets-99b4fdcbd-zv5rw                    4m           477Mi
external-secrets-cert-controller-6bb9dcdfcc-6f6nt   1m           63Mi
external-secrets-webhook-5b584647c6-krkdr           1m           56Mi

v0.9.17

NAME                                                CPU(cores)   MEMORY(bytes)
external-secrets-64578fc69c-kqm8d                   1m           469Mi
external-secrets-cert-controller-6bb9dcdfcc-6f6nt   1m           56Mi
external-secrets-webhook-5b584647c6-krkdr           1m           56Mi

gusfcarvalho · 2024-05-10T08:16:30Z

my concern with watching the secrets is what happens if someone (a controller) updates a secret that we do not own any keys. Are we going to run a reconcile flow in there?

Not really because it misses our index defined by external-secrets, right?

😄

shuheiktgw · 2024-05-11T01:40:43Z

my concern with watching the secrets is what happens if someone (a controller) updates a secret that we do not own any keys. Are we going to run a reconcile flow in there? Not really because it misses our index defined by external-secrets, right?

I may not fully understand the question, but does that mean what happens when some entity updates a secret that ExternalSecret resources are not responsible for? In that case, this list call does not return any resources hence no ExternalSecret reconciliation requests are enqueued. I hope that answers your question!

gusfcarvalho · 2024-05-11T11:30:18Z

Yes that was my question! Thanks!!

* Start reconciliation when a secret has changed Signed-off-by: shuheiktgw <[email protected]> * Prolong the test timeout Signed-off-by: shuheiktgw <[email protected]> * Use predicate.ResourceVersionChangedPredicate instead Signed-off-by: shuheiktgw <[email protected]> --------- Signed-off-by: shuheiktgw <[email protected]>

* Start reconciliation when a secret has changed Signed-off-by: shuheiktgw <[email protected]> * Prolong the test timeout Signed-off-by: shuheiktgw <[email protected]> * Use predicate.ResourceVersionChangedPredicate instead Signed-off-by: shuheiktgw <[email protected]> --------- Signed-off-by: shuheiktgw <[email protected]> Signed-off-by: Bude8 <[email protected]>

* Start reconciliation when a secret has changed Signed-off-by: shuheiktgw <[email protected]> * Prolong the test timeout Signed-off-by: shuheiktgw <[email protected]> * Use predicate.ResourceVersionChangedPredicate instead Signed-off-by: shuheiktgw <[email protected]> --------- Signed-off-by: shuheiktgw <[email protected]> Signed-off-by: Bill Hamilton <[email protected]>

Start reconciliation when a secret has changed

c73eee2

Signed-off-by: shuheiktgw <[email protected]>

shuheiktgw requested a review from a team as a code owner May 6, 2024 10:15

shuheiktgw requested a review from knelasevero May 6, 2024 10:15

shuheiktgw commented May 6, 2024

View reviewed changes

pkg/controllers/externalsecret/suite_test.go Show resolved Hide resolved

Prolong the test timeout

c86ee1f

Signed-off-by: shuheiktgw <[email protected]>

shuheiktgw commented May 6, 2024

View reviewed changes

pkg/controllers/externalsecret/externalsecret_controller_test.go Show resolved Hide resolved

gusfcarvalho reviewed May 6, 2024

View reviewed changes

pkg/controllers/externalsecret/externalsecret_controller.go Show resolved Hide resolved

gusfcarvalho reviewed May 6, 2024

View reviewed changes

pkg/controllers/externalsecret/externalsecret_controller.go Outdated Show resolved Hide resolved

Use predicate.ResourceVersionChangedPredicate instead

c840d54

Signed-off-by: shuheiktgw <[email protected]>

shuheiktgw force-pushed the secret_reconciliation branch from a0266d8 to c840d54 Compare May 9, 2024 00:28

gusfcarvalho approved these changes May 10, 2024

View reviewed changes

gusfcarvalho merged commit 30f2f90 into main May 11, 2024

gusfcarvalho deleted the secret_reconciliation branch May 11, 2024 11:30

shuheiktgw mentioned this pull request May 14, 2024

Fix flaky ES controller test #3493

Merged

5 tasks

jhoblitt mentioned this pull request May 15, 2024

Update Helm release external-secrets to v0.9.18 lsst-it/k8s-cookbook#394

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

Start reconciliation when a secret has changed#3459

Start reconciliation when a secret has changed#3459
gusfcarvalho merged 3 commits intomainfrom
secret_reconciliation

shuheiktgw commented May 6, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gusfcarvalho commented May 6, 2024

Uh oh!

sonarqubecloud bot commented May 9, 2024

Uh oh!

shuheiktgw commented May 9, 2024 •

edited

Loading

Uh oh!

shuheiktgw commented May 9, 2024

Uh oh!

gusfcarvalho commented May 10, 2024

Uh oh!

shuheiktgw commented May 11, 2024

Uh oh!

gusfcarvalho commented May 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Comments

Conversation

shuheiktgw commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem Statement

Related Issue

Proposed Changes

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gusfcarvalho commented May 6, 2024

Uh oh!

sonarqubecloud bot commented May 9, 2024

Quality Gate passed

Uh oh!

shuheiktgw commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shuheiktgw commented May 9, 2024

Operator created from this PR

v0.9.17

Uh oh!

gusfcarvalho commented May 10, 2024

Uh oh!

shuheiktgw commented May 11, 2024

Uh oh!

gusfcarvalho commented May 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shuheiktgw commented May 6, 2024 •

edited

Loading

shuheiktgw commented May 9, 2024 •

edited

Loading