fix(controller): Ensure that OAuth2Client reconciliation creates hydra client for specs #83
Conversation
|
|
alexandrem
changed the title
|
Hey @alexandrem, thank you for such a fast contribution! I take your PR and review it (already started) :) |
| @@ -352,17 +364,27 @@ func parseSecret(secret apiv1.Secret, authMethod hydrav1alpha1.TokenEndpointAuth | |||
| }, nil | |||
| } | |||
|
|
|||
| func (r *OAuth2ClientReconciler) getHydraClientForClient(oauth2client hydrav1alpha1.OAuth2Client) (HydraClientInterface, error) { | |||
| func (r *OAuth2ClientReconciler) getHydraClientForClient( | |||
There was a problem hiding this comment.
This IMO should have a test, unit at least
There was a problem hiding this comment.
I'll work on this today.
There was a problem hiding this comment.
Usually I find it a bit anti-idiomatic to create a unit test for a private function.
I'm wondering if we should we create an integration test for this instead?
|
@alexandrem Thank you for replying, I will address your changes and comments today but later in the day, I have to finish first my stuff :) |
|
@piotrmsc I just pushed some fixes in 9633930 I was able to setup my environment to run the integration tests. Everything passes now. I had to introduce a oauth2 client factory function for the new reconciler logic introduced in this PR to be able to work with the mock object used during the integration tests. This is abstracted with functional options so that it doesn't complicate normal usage outside of tests. Let me know if you think there's a better way though. The tests made me realize there was an issue with the oauth2 client map concurrent usage during reconciliation loops, so it's now secured with a mutex. |
|
so what issues did you have with integration tests to set it up? |
|
I have a Mac M1. Kubebuilder tooling and K8s ecosystem is not up to par yet, unfortunately. It would help to have all of that containerized or rework the tooling in this project to simplify the tests prerequisites. I might propose something in another PR :) |
|
yeah, some time ago I see there was an upgrade to kubebuilder v2 done but make targets were not updated and for ex Regarding your changes, I will take a look tomorrow in the morning (CEST) :) |
The reconciliation logic for OAuth2Client resources doesn't create custom hydra client for the requested CR spec and always falls back to the default hydra client.
This PR fixes this issue and ensures that different hydra clients with their own admin URLs, ports and endpoints can be used concurrently.
Note that some code refactoring was required to get rid of some import cycles introduced by the change.
I'm looking for some guidance to add a unit test if necessary.
Related issue(s)
#82
Checklist
and signed the CLA.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
appropriate).