-
Notifications
You must be signed in to change notification settings - Fork 13
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
OAuth integration #95
Conversation
ba958f5
to
a3a5fcb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments ...
src/main/java/io/strimzi/test/container/AuthenticationType.java
Outdated
Show resolved
Hide resolved
@@ -88,6 +92,21 @@ public class StrimziKafkaContainer extends GenericContainer<StrimziKafkaContaine | |||
private ToxiproxyClient toxiproxyClient; | |||
private Proxy proxy; | |||
|
|||
// OAuth fields | |||
private boolean oauthEnabled; | |||
private String keycloakRealm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OAuth can work with different authorization servers, apparently you assume that Keycloak is used. Having realm separately makes sense if you also configure Keycloak with this info. But this is just on the Kafka side and is really independent of Keycloak. But then you make it dependent on Keycloak by assuming in the code how to construct the JWKS Endpoint URL, Issuer URL, and Token Endpoint URL by using keycloakRealm
and KeycloakOAuthUri
. I depends on how modular / pluggable / extensible / generic you want your code to be here. Currently you are coupling it tightly to Keycloak. Also consider that these URLs have in fact recently changed and older versions of Keycloak have these endpoints on slightly different URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I depends on how modular / pluggable / extensible / generic you want your code to be here
The goal for now is to stick with just Keycloak. And possibly in the future we can add more support for other AS (f.e., hydra). But I agree that I can update those parts (i.e., keycloakRealm, KeycloakOAuthUri) to have a more generic name.
* GSSAPI (Kerberos) authentication. | ||
*/ | ||
GSSAPI; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could also add PLAIN
, since you support SCRAM_*
. On the client side PLAIN
is the same as OAUTH_OVER_PLAIN
, on the server side it is more like SCRAM_*
.
src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java
Outdated
Show resolved
Hide resolved
properties.setProperty("advertised.listeners", "PLAINTEXT://localhost:9092"); | ||
properties.setProperty("listener.security.protocol.map", "PLAINTEXT:PLAINTEXT,SSL:SSL,SASL_PLAINTEXT:SASL_PLAINTEXT,SASL_SSL:SASL_SSL"); | ||
properties.setProperty("listeners", listeners); | ||
properties.setProperty("inter.broker.listener.name", "BROKER1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On one hand you have a free list of listeners
that can be configured. On the other hand you expect that BROKER1
is part of that list. Maybe also pass the info as a function parameter so you have to specify it in the same place where you create the list of listeners. During any kind of refactoring you may miss this when it's in a different place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I have updated the configs. Now it looks a little bit better and it's dynamic so a lot of code is simpler.
src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java
Outdated
Show resolved
Hide resolved
984c4c1
to
021ba87
Compare
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
ceffe4f
to
467d26f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the review I needed to see logging from Strimzi OAuth library, and the effective server.properties
file. I was able to achieve that by editing the StrimziKafkaContainer.java
code:
diff --git a/src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java b/src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java
index 010643a..704dd3a 100644
--- a/src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java
+++ b/src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java
@@ -269,7 +269,11 @@ public class StrimziKafkaContainer extends GenericContainer<StrimziKafkaContaine
}
command += "bin/kafka-storage.sh format -t=\"" + this.clusterId + "\" -c /opt/kafka/config/kraft/server.properties \n";
- command += "bin/kafka-server-start.sh /opt/kafka/config/kraft/server.properties \n";
+ command += "cp /opt/kafka/config/log4j.properties /tmp/log4j.properties\n";
+ command += "echo \"log4j.logger.io.strimzi=DEBUG\" >> /tmp/log4j.properties\n";
+ command += "cat /opt/kafka/config/kraft/server.properties\n";
+ command += "export KAFKA_LOG4J_OPTS=\"-Dlog4j.configuration=file:/tmp/log4j.properties\"\n";
+ command += "bin/kafka-server-start.sh /opt/kafka/config/kraft/server.properties\n";
}
Utils.asTransferableBytes(serverPropertiesFile).ifPresent(properties -> copyFileToContainer(
It would be nice to have a way to configure logging and turning on the dumping to the console of server.properties
, but that may be addressed in another PR.
Also a few words in the README.md
about how to troubleshoot the problems with Podman timeouts would be very welcome. For me it helped to run:
podman server ssh --username root
echo '[engine]
service_timeout=0' >> /etc/containers/containers.conf
Although, I still see some timeouts and tests stalling, which seems to happen due to resource exhaustion as a consequence of containers spinned up for tests not being cleaned up as they should.
src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/io/strimzi/test/container/StrimziKafkaContainer.java
Outdated
Show resolved
Hide resolved
…java Co-authored-by: Marko Strukelj <marko.strukelj@gmail.com> Signed-off-by: Maros Orsak <maros.orsak159@gmail.com>
I think this method already exists but needs a few modifications to also change public StrimziKafkaContainer withKafkaLog(Level level) {
String log4jConfig = "log4j.rootLogger=" + level.name() + ", stdout\n" +
"log4j.appender.stdout=org.apache.log4j.ConsoleAppender\n" +
"log4j.appender.stdout.layout=org.apache.log4j.PatternLayout\n" +
"log4j.appender.stdout.layout.ConversionPattern=[%d] %p %m (%c)%n\n";
// Copy the custom log4j.properties into the container
this.withCopyToContainer(
Transferable.of(log4jConfig.getBytes(StandardCharsets.UTF_8)),
"/opt/kafka/config/log4j.properties"
);
return self();
}
Yeah I agree I have opened two issues to cover those problems. Thanks for the review it helped a lot! @mstruk |
This PR adds support for OAuth inside the test container. Moreover, now one can use two authentication types:
Those types are automatically configured on the server side (e.g., Kafka broker). So, if someone wants to test it, they only need to configure clients.
Also, I have added a dependency
testcontainers-keycloak
, which greatly simplifies the testing process. This should solve [1] together with [2].Update: It also adds a lot of UTs and ITs.
[1] - #63
[2] - strimzi/test-container-images#29