Upgrade to kafka-admin-api 0.0.10 and configure OAuth variables #377
Conversation
operator/src/main/java/org/bf2/operator/operands/AdminServer.java
Outdated
Show resolved
Hide resolved
| @@ -81,43 +81,55 @@ public static ManagedKafka getDefault(String name, String namespace, String boot | |||
| String endpointTlsCert, String endpointTlsKey, String oauthClientId, String oauthTlsCert, | |||
| String oauthClientSecret, String oauthUserClaim, String oauthJwksEndpoint, String oauthTokenEndpoint, | |||
| String oauthIssuerEndpoint) { | |||
| ManagedKafka mk = new ManagedKafkaBuilder() | |||
|
|
|||
| ManagedKafkaAuthenticationOAuth oauth = null; | |||
There was a problem hiding this comment.
I know that it's not related to this PR but I suggested in the past to move this getDefault out of the API; imho it should be in a Utils class used from tests and not in the API module. Do you think you can move this out somehow?
There was a problem hiding this comment.
I had offered up a couple of alternatives in an older pr, but there was never any commitment to an alternative, so it was left alone.
There was a problem hiding this comment.
@ppatierno, @shawkins , would it make more sense to be in systemtest/src/main/java/org/bf2/systemtest/framework/resource/ManagedKafkaResourceType.java ?
There was a problem hiding this comment.
Yes it makes sense and it was the original place where we have default mk in past
There was a problem hiding this comment.
So it turns out that both sync and operator have a dependency on that method from tests and mock classes. @shawkins, I assume you encountered that in the past when you looked at this?
There was a problem hiding this comment.
well from my POV it should be independent, so we should have have method getDefaultMK in systemtests and also in operator.
There was a problem hiding this comment.
@MikeEdgar yes, see earlier attempts and discussion here #209 It made sense to have a single default method because the same logic was spread over three places. Now that less of the cr is required it makes a little less sense - you are only required to provide the endpoint and version information and then none of the existing logic will fail (of course test methods based upon the current getDefault may be expecting other fields to be set). There are several ways to address this - separate implementations, moving to common, or even more quakus like solutions of a overriding a factory bean. My issue on the pr before is that we were spending an inordinate amount of time on ancillary discussions rather than committing to solution.
There was a problem hiding this comment.
I am for separate implementations which is what @kornys has suggested as well. Maybe we need different default MK across system tests and sync/operator. Let's avoid the so called "ancillary discussions" and commit to a solution this time ;-)
|
@MikeEdgar I think we can also enable oauth for kafka in dev mk profile and enable keycloak installation by default see #379 then we can use keycloak on minikube also |
+1 from me on that, but I'd like to keep it in a separate PR. |
|
@MikeEdgar yup I agree let's do it in different PR |
MikeEdgar
force-pushed
the
configure_admin_api_oauth
branch
from
626139e to
69f5700
Compare
|
@MikeEdgar for some reason I see that admin pod is in pending state during systemtests smoke test run in gh action |
|
@k-wall , @kornys - this needs to be updated to 0.0.10 first. Admin API pod is pending due to the missing trusted certificate that 0.0.10 is capable of accepting. I have the changes locally. The PR for the Admin API release is now open: bf2fc6cc711aee1a0c2a/kafka-admin-api#86 |
MikeEdgar
changed the title
Signed-off-by: Michael Edgar <medgar@redhat.com>
Signed-off-by: Michael Edgar <medgar@redhat.com>
Signed-off-by: Michael Edgar <medgar@redhat.com>
Signed-off-by: Michael Edgar <medgar@redhat.com>
Signed-off-by: Michael Edgar <medgar@redhat.com>
MikeEdgar
force-pushed
the
configure_admin_api_oauth
branch
from
69f5700 to
2cd72da
Compare
@kornys - please take a look at the test changes and let me know if you have any concerns. The change for Oauth in the CR is necessary because Admin API 0.0.9 requires a valid JWKS endpoint now when Oauth is enabled.
Once your PR with Keycloak is merged, I'll incorporate what you've done here. Having Keycloak available will be nice to be able to test Admin API in a system test.