Try and associate a pull policy when we know its a floating tag. #423

SamBarker · 2024-11-19T04:11:18Z

Type of change

Bugfix

Description

Fixes #421

Additional Context

We should default to pull always when we know we are specifying a floating tag.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

...c/test/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaClusterTest.java

impl/src/main/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaCluster.java

k-wall · 2024-11-20T10:11:26Z

impl/src/main/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaCluster.java

@@ -837,4 +847,6 @@ private String buildScramUsersEnvVar() {
                .collect(Collectors.joining(";"));
    }

+    record PerImagePullPolicy(DockerImageName dockerImageName, ImagePullPolicy pullPolicy) {


PerImagePullPolicy is not a good name. ImageWithPullPolicy might be better.

impl/src/main/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaCluster.java

SamBarker

I'm seeing test failures locally because kafka-native:latest has moved to Kafka 3.9.0 😢

Not sure how to handle that.

2024-11-21 12:19:49 ERROR tc.quay.io/ogunalp/kafka-native:latest:549 - Log output from the failed container:
__  ____  __  _____   ___  __ ____  ______ 
 --/ __ \/ / / / _ | / _ \/ //_/ / / / __/ 
 -/ /_/ / /_/ / __ |/ , _/ ,< / /_/ /\ \   
--\___\_\____/_/ |_/_/|_/_/|_|\____/___/   
2024-11-20 23:19:06,290 ERROR [io.qua.run.Application] (main) Failed to start application: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
	at io.quarkus.runtime.Application.start(Application.java:101)
	at io.quarkus.runtime.ApplicationLifecycleManager.run(ApplicationLifecycleManager.java:121)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:71)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:44)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:124)
	at io.quarkus.runner.GeneratedMain.main(Unknown Source)
Caused by: java.lang.IllegalArgumentException: requirement failed: advertised.listeners cannot use the nonroutable meta-address 0.0.0.0. Use a routable IP address.
	at scala.Predef$.require(Predef.scala:337)
	at kafka.server.KafkaConfig.validateValues(KafkaConfig.scala:1022)
	at kafka.server.KafkaConfig.<init>(KafkaConfig.scala:852)
	at kafka.server.KafkaConfig.<init>(KafkaConfig.scala:185)
	at kafka.server.KafkaConfig.fromProps(KafkaConfig.scala:101)
	at com.ozangunalp.kafka.server.EmbeddedKafkaBroker.start(EmbeddedKafkaBroker.java:216)
	at com.ozangunalp.kafka.server.Startup.startup(Startup.java:38)
	at com.ozangunalp.kafka.server.Startup_Observer_startup_lDxI_XYb49w9c3LH146P8PmORkk.notify(Unknown Source)
	at io.quarkus.arc.impl.EventImpl$Notifier.notifyObservers(EventImpl.java:351)
	at io.quarkus.arc.impl.EventImpl$Notifier.notify(EventImpl.java:333)
	at io.quarkus.arc.impl.EventImpl.fire(EventImpl.java:80)
	at io.quarkus.arc.runtime.ArcRecorder.fireLifecycleEvent(ArcRecorder.java:163)
	at io.quarkus.arc.runtime.ArcRecorder.handleLifecycleEvents(ArcRecorder.java:114)
	at io.quarkus.deployment.steps.LifecycleEventsBuildStep$startupEvent1144526294.deploy_0(Unknown Source)
	at io.quarkus.deployment.steps.LifecycleEventsBuildStep$startupEvent1144526294.deploy(Unknown Source)
	... 7 more

SamBarker · 2024-11-20T23:38:04Z

...c/test/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaClusterTest.java

+                    .isNotNull().satisfies(perImagePullPolicy -> {
+                        assertThat(perImagePullPolicy.dockerImageName().getVersionPart()).isEqualTo("latest-kafka-" + version.value());
+                        final ImagePullPolicy actualPullPolicy = perImagePullPolicy.pullPolicy();
+                        assertThat(actualPullPolicy).isInstanceOf(ImagePullPolicy.class); // We can't see `AlwaysPullPolicy` as it's not public


Hmm not convinced this should actually be this policy. Its not strictly part of this test either.

It serves to document that you wish to assert that an always pull policy is in place, but you cannot because of the protection policy. I'd have made the test assert the policy's class name (ugly/fragile though that is).

Did you see my suggestion to implement our own ImagePullPolicy. We'd implement the class to give always behaviour for tags that start latest and default otherwise. It would be cinch to unit test too.

I had but I hadn't realised the implications that is probably a good move.

I think this PR then just becomes about sorting the image selection so it all happens in one testable place.

SamBarker · 2024-11-20T23:38:40Z

pom.xml

@@ -378,6 +382,10 @@
                        <systemPropertyVariables>
                            <container.logs.dir>${project.build.directory}/container-logs/</container.logs.dir>
                        </systemPropertyVariables>
+                        <argLine>
+                            --add-opens java.base/java.util=ALL-UNNAMED


Required so we can use junit-pioneer to mess with env vars.

k-wall · 2024-11-21T09:01:35Z

impl/src/main/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaCluster.java

@@ -115,8 +129,8 @@ public class TestcontainersKafkaCluster implements Startable, KafkaCluster, Kafk
    // This uid needs to match the uid used by the kafka container to execute the kafka process
    private static final String KAFKA_CONTAINER_UID = "1001";
    private static final int READY_TIMEOUT_SECONDS = 120;
-    private final DockerImageName kafkaImage;
-    private final DockerImageName zookeeperImage;
+    public static final String MAJOR_MINOR_PATCH = "\\d+(\\.\\d+(\\.\\d+)?)?";


Precompile the RE

Doh good point.

k-wall · 2024-11-21T09:07:45Z

pom.xml

+            <dependency>
+                <groupId>org.junit-pioneer</groupId>
+                <artifactId>junit-pioneer</artifactId>
+                <version>2.3.0</version>


extract a variable for this.

k-wall · 2024-11-21T10:36:02Z

I verified that with your refactoring on this PR, KAFKA_IMAGE_REPO/KAFKA_IMAGE_TAG env vars can still be used to direct TestContainersKafkaCluster to an alternative image registry/organisation.

k-wall · 2024-11-21T13:43:38Z

...c/test/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaClusterTest.java

+
+    private static Stream<Version> fixedVersions() {
+        return Stream.of(
+                version("3.6.0"),


Is there a reason to stop at 3.6.0?

It's the same set as the TemplateTest which tries to actually fire things up (this test was written as that was failing and hard to debug).

I'm not sure the set of versions actually matters but while we are talking about it I'll add the missing versions.

Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED

k-wall · 2024-11-21T23:14:48Z

Did you rebase already? If you haven't container base tests will fail against the new images because the Kafka configuration will be wrong.

Enforce we always pull Release and Snapshot tags. It then works of the assumption that older versioned tags are stable so the cached copy is likely to be good. It will fall back to the default policy if the versioned image is less than 7 days old. Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED

Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED

sonarcloud · 2024-11-22T04:12:40Z

Quality Gate failed

Failed conditions
1 Security Hotspot
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

SamBarker · 2024-11-22T05:45:00Z

Quality Gate failed

Failed conditions
1 Security Hotspot

It doesn't like the regex. I need to have another go at simplifying it or its usage.

0.0% Coverage on New Code (required ≥ 80%)

Errr I wrote tests. What's happening to coverage??? A question for Monday

k-wall · 2024-11-22T09:17:55Z

impl/src/main/java/io/kroxylicious/testing/kafka/testcontainers/FloatingTagPullPolicy.java

+
+import io.kroxylicious.testing.kafka.common.Version;
+
+public class FloatingTagPullPolicy extends AbstractImagePullPolicy implements ImagePullPolicy {


Why not make this a KroxyliciousPullPolicy that knows about the special rules for latest etc and otherwise just falls back to PullPolicy.defaultPolicy() for all other tags. That way TestcontainerKafkaCluster can just use the policy unconditionally.

Not sure I follow. I think this could be applied unconditionally, it's not currently however.

Extending the abstract means we are only invoked if there is already an image in the cache.

SamBarker requested a review from k-wall November 19, 2024 08:34

SamBarker commented Nov 19, 2024

View reviewed changes

...c/test/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaClusterTest.java Outdated Show resolved Hide resolved

k-wall reviewed Nov 19, 2024

View reviewed changes

impl/src/main/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaCluster.java Outdated Show resolved Hide resolved

k-wall reviewed Nov 20, 2024

View reviewed changes

impl/src/main/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaCluster.java Outdated Show resolved Hide resolved

k-wall reviewed Nov 20, 2024

View reviewed changes

k-wall mentioned this pull request Nov 20, 2024

Alternative Suggestion for #423 #424

Closed

5 tasks

k-wall reviewed Nov 20, 2024

View reviewed changes

impl/src/main/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaCluster.java Show resolved Hide resolved

SamBarker commented Nov 20, 2024

View reviewed changes

SamBarker force-pushed the issue-421 branch from ae4e9d7 to ccd3427 Compare November 20, 2024 23:41

k-wall reviewed Nov 21, 2024

View reviewed changes

SamBarker marked this pull request as ready for review November 21, 2024 09:23

k-wall reviewed Nov 21, 2024

View reviewed changes

SamBarker added 5 commits November 22, 2024 10:40

Try and associate a pull policy when we know it's a floating tag.

a1ed983

Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED

Resolve containers in one place inside TestcontainersKafkaCluster

b989d5b

Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED

PR Feedback. Expand version range to cover more recent releases.

51e1052

Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED

PR Feedback. Pre compile regex

ec8817b

Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED

PR Feedback. variable for junit-pioneer version

d95a9ca

Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED

SamBarker added 2 commits November 22, 2024 16:35

fixup! PR Feedback. Custom ImagePullPolicy

75a9e66

Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED

SamBarker force-pushed the issue-421 branch from ccd3427 to 75a9e66 Compare November 22, 2024 03:52

k-wall reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try and associate a pull policy when we know its a floating tag. #423

Try and associate a pull policy when we know its a floating tag. #423

SamBarker commented Nov 19, 2024

k-wall Nov 20, 2024

SamBarker left a comment

SamBarker Nov 20, 2024

k-wall Nov 21, 2024

k-wall Nov 21, 2024

SamBarker Nov 21, 2024

SamBarker Nov 20, 2024

k-wall Nov 21, 2024

SamBarker Nov 21, 2024

k-wall Nov 21, 2024

k-wall commented Nov 21, 2024

k-wall Nov 21, 2024

SamBarker Nov 21, 2024

k-wall commented Nov 21, 2024

sonarcloud bot commented Nov 22, 2024

SamBarker commented Nov 22, 2024 •

edited

Loading

Quality Gate failed

k-wall Nov 22, 2024

SamBarker Nov 22, 2024


		import io.kroxylicious.testing.kafka.common.Version;

		public class FloatingTagPullPolicy extends AbstractImagePullPolicy implements ImagePullPolicy {

Try and associate a pull policy when we know its a floating tag. #423

Are you sure you want to change the base?

Try and associate a pull policy when we know its a floating tag. #423

Conversation

SamBarker commented Nov 19, 2024

Type of change

Description

Additional Context

Checklist

Choose a reason for hiding this comment

SamBarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-wall commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-wall commented Nov 21, 2024

sonarcloud bot commented Nov 22, 2024

Quality Gate failed

SamBarker commented Nov 22, 2024 • edited Loading

Quality Gate failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamBarker commented Nov 22, 2024 •

edited

Loading