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: add ready condition to OAuth2ClientStatus #122

Merged
merged 3 commits into from
Mar 27, 2023

Conversation

andremarianiello
Copy link
Contributor

Adds Ready condition to OAuth2ClientStatus. This allows k8s clients with support for status conditions to determine if the client has been synchronized with hydra and the corresponding secret for an OAuth2Client is ready to be read.

Related Issue

This change fixes an issue I was having when trying to use hydra-maester with OAuth2Clients in terraform. Since this is a custom resource I was using the kubernetes_manifest resource, and I was also using terraform to extract the client credentials from the k8s secret. However, this was not working because terraform attempts to read the secret immediately after it finishes creating the OAuth2Client, and when the secret doesn't exist yet (which happens like 75%) of the time, the terraform apply fails. The kubernetes_manifest has built in support for "conditions" which are a k8s concept that help controllers and clients communicate. I can tell terraform to wait for the Ready condition to be True and this will stop terraform from trying to read the secret too early. Controllers like cert-manager use a similar technique.

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 [email protected]) 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

I haven't worked on a kubebuilder k8s controller before, so if I did anything wrong in that regard, please let me know!

@CLAassistant
Copy link

CLAassistant commented Mar 25, 2023

CLA assistant check
All committers have signed the CLA.

@andremarianiello andremarianiello changed the title Add ready condition to OAuth2ClientStatus Draft: Add ready condition to OAuth2ClientStatus Mar 25, 2023
@andremarianiello andremarianiello marked this pull request as draft March 25, 2023 23:49
@andremarianiello andremarianiello changed the title Draft: Add ready condition to OAuth2ClientStatus Add ready condition to OAuth2ClientStatus Mar 25, 2023
@andremarianiello andremarianiello changed the title Add ready condition to OAuth2ClientStatus Fix: Add ready condition to OAuth2ClientStatus Mar 25, 2023
@andremarianiello andremarianiello changed the title Fix: Add ready condition to OAuth2ClientStatus fix: Add ready condition to OAuth2ClientStatus Mar 25, 2023
@andremarianiello andremarianiello force-pushed the add-ready-condition branch 3 times, most recently from 0e1c7d6 to 24d82f4 Compare March 26, 2023 00:17
@andremarianiello
Copy link
Contributor Author

I'm not sure why the docker scan is failing, as the error message looks like

  1 error occurred:
  	* discovered vulnerabilities at or above the severity threshold

so I'm not sure what's wrong.

@andremarianiello andremarianiello force-pushed the add-ready-condition branch 3 times, most recently from cceddf6 to b6d08a7 Compare March 27, 2023 14:39
@Demonsthere
Copy link
Collaborator

Hello there!
There is a bug in the scanner version we have, that it does not show the actual report on the CVEs. I will try to fix the CI as soon as I can to unblock this

@andremarianiello
Copy link
Contributor Author

Thanks for helping with this @Demonsthere ! I rebased onto the commit to added to master this morning and that gave me enough info to know what versions to bump. CI seems happy now.

@andremarianiello andremarianiello marked this pull request as ready for review March 27, 2023 15:13
@andremarianiello andremarianiello changed the title fix: Add ready condition to OAuth2ClientStatus fix: add ready condition to OAuth2ClientStatus Mar 27, 2023
Copy link
Collaborator

@Demonsthere Demonsthere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates :) The addition of status condition makes perfect sense, and is a welcome addition.

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.

3 participants