feat: Include remove orphans logic

[dacamposol]

Relates to: #1272

Signed-off-by: Daniel Campos Olivares dacamposol@gmail.com

[paul-the-alien]

Greetings!
Thank you for contributing to this project!
If this is your first time contributing, please make
sure to read the Developer and Contributing Process guides.
Please also mind and follow our Code of Conduct.

Useful commands:

• make fmt: Formats the code
• make check-diff: Ensures the branch is clean
• make reviewable: Ensures a PR is ready for review

[ghost]

👇 Click on the image for a new way to code review

• Make big changes easier — review code in small groups of related files

• Know where to start — see the whole change at a glance

• Take a code tour — explore the change with an interactive tour

• Make comments and review — all fully sync’ed with github

  Try it now!

Legend

[dacamposol]

@gusfcarvalho implemented deletion and orphans logic.

Also, I included an unit test and generated all the required manifests.

[gusfcarvalho]

Thanks @dacamposol !

[gusfcarvalho]

/ok-to-test sha=37d59ae7c7d25056e62318e5a50763b4ebef78c5

[gusfcarvalho]

hi @dacamposol ! It seems there are some issues with the unit tests still.

Also, the check-diff is complaining because you have commited the files under charts/templates/crds. This folder should be git ignored IIRC. I believe if you delete it the check-diff will pass.

you can also run make check-diff locally to see if it would pass or not!

[dacamposol]

hi @dacamposol ! It seems there are some issues with the unit tests still.

Also, the check-diff is complaining because you have commited the files under charts/templates/crds. This folder should be git ignored IIRC. I believe if you delete it the check-diff will pass.

you can also run make check-diff locally to see if it would pass or not!

I'm trying to fix the unit test, but to be honest this is my first time using Gomega and Gigko, and it works a little bit weird compared with Testify (through debugging I can see that the deletion is successfully executed, but afterwards when I ask for the secret to the fake client, the old one is still there and the new one isn't).

Do you know if I have to configure something to actually see the Delete operations reflected in the fake?

[gusfcarvalho]

Do you know if I have to configure something to actually see the Delete operations reflected in the fake?

IIRC this issue might be due to envtest not simulating delete properly... maybe we can move this unit test to an e2e instead. This way the scenario would be more realistic. You can do it by adding a common e2e under e2e/suites/provider/common/common.go, and then adding the entry in specific provider cases (we can start with vault at e2e/suites/provider/vault/vault.go).

[gusfcarvalho]

vault e2e allows us to test this e2e behavior locally more easily. To run e2e tests, run:
(you need to have kind installed)

make -C ./e2e start-kind; make test.e2e GINKGO_LABELS=vault

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[dacamposol]

@gusfcarvalho done!

PS: I had a lot of issues to run the e2e-tests on my arm64 machine, I think the Makefile for the e2e tests should be adapted.

[dacamposol]

@moolen could you allow the testing workflows to start?

[dacamposol]

@moolen or @gusfcarvalho, can I merge this?

[gusfcarvalho]

/ok-to-test sha=a28fab92bdc5423115f48e425ba9a45e37baff1c

[dacamposol]

@gusfcarvalho the e2e-tests didn't work.

I tried to check the logs but there are none, it seems it has been skipped or something.

[moolen]

I think the value must be json, as vault doesn't allow you to store a simple string.

[moolen]

due to the nature how the e2e tests are ran you can't easily click on the Details link.

The run is here: https://github.com/external-secrets/external-secrets/runs/7771612959?check_suite_focus=true

from line 3351:

[vault] sync secrets
  [common] should delete orphaned secrets with Owner creation policy with token auth [vault]
  /home/runner/work/external-secrets/external-secrets/e2e/framework/framework.go:115
------------------------------
• [FAILED] [35.880 seconds]
[vault] [vault]
/home/runner/work/external-secrets/external-secrets/e2e/suites/provider/cases/vault/vault.go:38
  sync secrets
  /home/runner/work/external-secrets/external-secrets/e2e/suites/provider/cases/vault/vault.go:42
    [It] [common] should delete orphaned secrets with Owner creation policy with token auth
    /home/runner/work/external-secrets/external-secrets/e2e/framework/framework.go:115

[gusfcarvalho]

@moolen @knelasevero @sebagomez just found a way to implement the test coverage as unit tests. The e2e tests were still failing with that approach (something weird happening that I could not take too much time investigating).

With the added changes, we should be fine with merging this PR, but since I've added some code, can any of you please take a final look? 😄

[gusfcarvalho]

/ok-to-test sha=5e1ff3b

[moolen]

I've had a brief look at the code changes. The approach is reasonable, two small nits around naming/readability, see below.

(possibly dumb) question: why are creationPolicy=Owner secrets not affected? They get cleaned up only once the ExternalSecret gets deleted. So if the spec.target.name changes, it'll create a new secret and leave the old one as-is or am i mistaken?

Theoretically there is still an edge case where secret resources can orphan:
If the controller dies between the secret creation and the es.status update. Then you get into an inconsistent/irreconcilable state where the old secret does not get removed.
For me this is negligible if only creationPolicy!=Owner secrets are affected.

One way around this, for instance, is to implement a label-based garbage collection mechanism.

[gusfcarvalho]

(possibly dumb) question: why are creationPolicy=Owner secrets not affected? They get cleaned up only once the ExternalSecret gets deleted. So if the spec.target.name changes, it'll create a new secret and leave the old one as-is or am i mistaken?

Actually, only creationPolicy:Owner is affected, as this logic is applied on the default case of the switch as you can see in here

One way around this, for instance, is to implement a label-based garbage collection mechanism.

As in the controller should create a label on the external-secret with the target secret name? Or rather a label to the generated secret?

[gusfcarvalho]

@moolen I took a look at using labels to filter out secrets - the main issue with that after making some code here is that for each create operation we would need to add a List on secrets. This does not seem to be a problem since this is done only when a new secret is created, but it is the price to check labels.

On the bright side is that it is really a more robust solution.

[gusfcarvalho]

@moolen @knelasevero I think this PR can be reviewed again. The implementation now follows label matching

[moolen]

LGTM! Thanks for bringing this over the finish line 🏁

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.8% Duplication

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.6% Duplication

[agaudreault]

I believe that the owner label behavior introduced by this PR breaks the merge secret. It makes it impossible for multiple ExternalSecret to manage the same underlying secret as documented in docs/guides/ownership-deletion-policy.md.

Note: the controller takes ownership of a field even if it is owned by a different entity. Multiple ExternalSecrets can use creationPolicy=Merge with a single secret as long as the fields don't collide - otherwise you end up in an oscillating state.

[gusfcarvalho]

The owner label should only trigger on creationPolicy=Owner. How would that generate conflict?

[agaudreault]

The label is added on all Secret. If 2 external secrets target the same secret with creationPolicy=Merge, the label will still be added and each update of the secret will cause the other ES to update the label value, causing an oscillating state.

[gusfcarvalho]

oh, I see. I'll provide a fix for it! Thanks for the call!