feat: disable oauth2 client deletion

[David-Wobrock]

This new feature allows to disable hydra-maester OAuth2 client deletion. It can be a pitfall that, when a Kube CRD gets deleted, the OAuth2 client gets deleted too.

OAuth2 clients can be critical for production environments to work properly, and allowing to delete one when a Kubernetes CRD gets deleted has shown to be brittle for us. The CRDs are can be (temporarily) deleted for various reasons: human error, misconfiguration, etc.)
This feature should make hydra-maester safer.

Related Issue or Design Document

No issue or design document - it's an implementation attempt of a feature that we would find useful on hydra-maester :)

Checklist

• I have read the contributing guidelines and signed the CLA.
• I have referenced an issue containing the design document if my change introduces a new feature.
• I have read the security policy.
• I confirm that this pull request does not address a security vulnerability.
  If this pull request addresses a security vulnerability,
  I confirm that I got approval (please contact security@ory.sh) from the maintainers to push the changes.
• I have added tests that prove my fix is effective or that my feature works.
• I have added the necessary documentation within the code base (if appropriate).

Further comments

[Demonsthere]

I agree with your goal, but not the method of implementation. Imho a better solution would be to add a field to the CR deletionPolicy: Delete | Orphan with delete being the default option. Orhpan would allow for deleting the CR without client deletion.
wdyt? :)

[David-Wobrock]

I agree with your goal, but not the method of implementation. Imho a better solution would be to add a field to the CR deletionPolicy: Delete | Orphan with delete being the default option. Orhpan would allow for deleting the CR without client deletion. wdyt? :)

That would work too 👍 And more fine-grained, since it can be defined on a client-per-client basis.

I'll push some edits.

[Demonsthere]

The code looks simple and good :) I am just wondering about an integration test case for this, supplying 2 clients with different policies, deleting the CRs and asserting the controller state afterwards, wdyt?

[David-Wobrock]

The code looks simple and good :) I am just wondering about an integration test case for this, supplying 2 clients with different policies, deleting the CRs and asserting the controller state afterwards, wdyt?

Would be neat!
But I'm having a hard time getting them up and running in a stable manner 😕

Also, is there already a manner to check that an oauth2 client has been properly deleted?

I'd be grateful for a hand here 😅

[Demonsthere]

If we would like to do it in-code only, then https://github.com/ory/dockertest would be useful. However a full e2e test would require starting up k8s in k3d or similar, deploying the controller with hydra using helm and a build image, applying manifests and then calling the hydra api to list clients. Delete both clients (assuming they have different policies) and listing clients again.
I don't know if there is a nice framework which would make it easy for us, or coding it as a script 😅.
We can also provide the tests as a followup, but in that case i'd like to perform the scenario manually to check it beforehand ;)

[David-Wobrock]

If we would like to do it in-code only, then https://github.com/ory/dockertest would be useful. However a full e2e test would require starting up k8s in k3d or similar, deploying the controller with hydra using helm and a build image, applying manifests and then calling the hydra api to list clients. Delete both clients (assuming they have different policies) and listing clients again. I don't know if there is a nice framework which would make it easy for us, or coding it as a script 😅. We can also provide the tests as a followup, but in that case i'd like to perform the scenario manually to check it beforehand ;)

Hey @Demonsthere !

Finally came around to have some time and dive into the integration tests.

I've added two tests:

• one that verifies that when Orphan, the Hydra DeleteOAuth2Client is not called (because skipped)
• one that verifies that when Delete, the Hydra DeleteOAuth2Client is called

I hope that'll do the trick :) Let me know what you think.

[Demonsthere]

In general I have no issues with the changes and tests :) The only thing I would modify is to change the type to use iota and specify the valid values.
Also, please run the generation makefile commands to generate the api schemas :)

[David-Wobrock]

Hey @Demonsthere 👋

Sure, I pushed some changes.
I hope everything is up to date now :)

[Demonsthere]

LGTM! :)

[haslersn]

This seems to be wrong - 1 is delete and 2 is orphan. With this CRD, we cannot set deletionPolicy: 2, so it's impossible to use this new feature.

[haslersn]

Anyway, why were integers chosen here? This is extremely non-descriptive. Instead strings Delete/Retain should be supported.

[David-Wobrock]

I reckon there is indeed a bug, feel free to open a pull request to address the issue :)