Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Istio-based integration test subproject ( Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Runner
participant IstioExt as IstioTestExtension
participant ClusterExt as IstioClusterExtension
participant K8s as Kubernetes API
participant Pod as Test Pod
participant Entry as IstioPodEntryPoint
TestRunner->>IstioExt: interceptTestMethod()
IstioExt->>IstioExt: check RUNNING_IN_K8S_POD
alt host (not in pod)
IstioExt->>ClusterExt: get KubernetesClient
ClusterExt-->>IstioExt: KubernetesClient
IstioExt->>K8s: create Pod (image + args --class/--method)
K8s-->>IstioExt: Pod created
IstioExt->>Pod: wait for readiness (podCustomizer.isPodHealthy)
Pod-->>IstioExt: Pod ready
Pod->>Entry: run IstioPodEntryPoint (container)
Entry->>Entry: run specified test via JUnit Launcher
Entry-->>Pod: exit code & logs
Pod-->>IstioExt: container exit
IstioExt->>TestRunner: pass/fail (collect logs on failure)
IstioExt->>K8s: delete Pod
else running in pod
IstioExt->>TestRunner: invoke test locally
end
sequenceDiagram
participant ClusterSetup as IstioClusterExtension.setUp
participant State as IstioState
participant Helper as K8sClusterHelper
participant K3s as K3sContainer
participant Installer as IstioInstaller
participant istioctl as istioctl CLI
ClusterSetup->>State: connectOrCreate()
alt kubeconfig exists
State->>State: connect using existing kubeconfig
else kubeconfig missing
State->>Helper: startK3sAndWaitReady()
Helper->>K3s: start rancher/k3s container
Helper-->>State: return K3sContainer + kubeconfig
State->>State: write kubeconfig to disk
end
State->>Installer: runIstioctlUninstall(kubeconfig)
Installer->>istioctl: istioctl uninstall --purge -y
istioctl-->>Installer: exit
State->>Installer: waitForIstioRemoval(client)
State->>Installer: installIfNeeded(kubeconfig, profile)
Installer->>istioctl: istioctl install --kubeconfig ...
istioctl->>K3s: deploy istiod components
Installer->>Installer: waitForIstiodReady()
Installer-->>State: istio ready
State-->>ClusterSetup: return IstioState with client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/EnabledIfDockerAvailable.java (1)
25-28: Consider adding a brief Javadoc.While the annotation name is self-documenting, adding a one-line Javadoc would align with the project's convention for public API documentation. Example:
/** * Enables the annotated test class or method only if Docker daemon is available. */As per coding guidelines, "Javadoc: all public classes and public/protected methods must have Javadoc".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/EnabledIfDockerAvailable.java` around lines 25 - 28, Add a one-line Javadoc to the public annotation EnabledIfDockerAvailable explaining its purpose; e.g., document that the annotation enables the annotated test class or method only when the Docker daemon is available and mention it uses DockerAvailableCondition via `@ExtendWith` to perform the check. Place the Javadoc immediately above the annotation declaration for EnabledIfDockerAvailable so it satisfies the project's public API documentation guideline.it/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioTestingBlockHoundIntegration.java (1)
15-17: Minor: Remove extra blank line between license header and package statement.There's an extra blank line after the license header (line 16) which is inconsistent with the typical Java file format where the package declaration immediately follows the closing comment.
Suggested fix
* under the License. */ - package com.linecorp.armeria.it.istio.testing;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioTestingBlockHoundIntegration.java` around lines 15 - 17, Remove the extra blank line between the file header comment and the package declaration so the package statement immediately follows the closing comment; locate the top of the file around the package declaration for the IstioTestingBlockHoundIntegration class (package com.linecorp.armeria.it.istio.testing) and delete the stray blank line after the license block.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/DockerAvailableCondition.java (1)
26-29: Consider extracting the double-negative for readability.The condition
!HostOnlyExtension.notRunningInPod()uses a double negative which can be confusing. Consider extracting to a local variable with a positive name:final boolean runningInPod = !HostOnlyExtension.notRunningInPod(); if (runningInPod) { return ConditionEvaluationResult.enabled("Running inside K8s pod"); }Alternatively, if
HostOnlyExtensionis under your control, consider adding a positiveisRunningInPod()method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/DockerAvailableCondition.java` around lines 26 - 29, The double-negative check (!HostOnlyExtension.notRunningInPod()) in DockerAvailableCondition should be made clearer: introduce a positive-named local boolean (e.g., runningInPod) assigned from the negative call and use it in the if, or (if you can change HostOnlyExtension) replace the call with a new positive helper method isRunningInPod() and use that; update the conditional in the method inside DockerAvailableCondition to use the new positive variable/method (referencing HostOnlyExtension.notRunningInPod() or HostOnlyExtension.isRunningInPod() and the DockerAvailableCondition class).it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.java (1)
83-85: Thelauncher.discover()call is redundant.The
Launcher.execute()method performs discovery internally when given aLauncherDiscoveryRequest. The separatediscover()call on line 84 is unnecessary.♻️ Suggested simplification
final SummaryGeneratingListener listener = new SummaryGeneratingListener(); final Launcher launcher = LauncherFactory.create(); - launcher.discover(request); launcher.execute(request, listener);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.java` around lines 83 - 85, The call to Launcher.discover(request) is redundant because Launcher.execute(request, listener) already performs discovery for a LauncherDiscoveryRequest; remove the explicit launcher.discover(request) invocation in IstioPodEntryPoint (the block that creates Launcher via LauncherFactory.create(), calls launcher.discover(request), then launcher.execute(request, listener)) and keep only launcher.execute(request, listener) to simplify and avoid duplicate discovery.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.java (1)
61-66: Capture stderr fordocker savefailure diagnostics.When
docker savefails, the error message doesn't include the actual error output, making debugging difficult. Consider capturing stderr similar to how it's done forctr images import.♻️ Proposed improvement
- final Process saveProcess = new ProcessBuilder("docker", "save", IMAGE_NAME) - .redirectOutput(tempFile.toFile()) - .start(); - if (saveProcess.waitFor() != 0) { - throw new IllegalStateException("docker save exited with non-zero status"); - } + final ProcessBuilder pb = new ProcessBuilder("docker", "save", IMAGE_NAME) + .redirectOutput(tempFile.toFile()); + final Process saveProcess = pb.start(); + final int exitCode = saveProcess.waitFor(); + if (exitCode != 0) { + final String stderr = new String(saveProcess.getErrorStream().readAllBytes()); + throw new IllegalStateException( + "docker save exited with status " + exitCode + ": " + stderr); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.java` around lines 61 - 66, The docker save invocation using new ProcessBuilder("docker", "save", IMAGE_NAME") does not capture stderr, so when saveProcess.waitFor() != 0 the thrown IllegalStateException lacks diagnostic output; modify the ProcessBuilder for docker save (the saveProcess) to capture stderr like the ctr images import path does (e.g., redirectError to a temporary file or capture the error stream), and include the captured stderr contents in the IllegalStateException message so failures around IMAGE_NAME and tempFile produce actionable diagnostics.dependencies.toml (1)
94-96: Istio version 1.29.0 is valid and released (February 16, 2026).Consider updating to Istio 1.29.1 (released March 10, 2026), the latest patch in the 1.29.x series, to include any available bug fixes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependencies.toml` around lines 94 - 96, Update the istio dependency version from "1.29.0" to the current 1.29.x patch release "1.29.1" in dependencies.toml by changing the istio = "1.29.0" entry to istio = "1.29.1" so the project benefits from the latest 1.29.x bug fixes; verify no other references override this value and run dependency resolution to confirm compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioEnv.java`:
- Around line 62-64: The podJvmArgs() method currently calls
require(ISTIO_POD_JVM_ARGS_ENV) which throws if the ISTIO_POD_JVM_ARGS
environment variable is absent; change podJvmArgs() to read the env var
defensively (e.g., System.getenv or the existing helper that returns
null/optional) and return an empty string when the variable is not set instead
of throwing; update references to podJvmArgs() if they expect non-null but can
handle empty string.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java`:
- Around line 112-128: The runCommand method can hang if the subprocess (e.g.,
istioctl) never exits; modify runCommand(List<String> command, Path workDir,
String logPrefix) to enforce a configurable timeout: start the process as
before, consume stdout/stderr in a background thread or continue using the
current reader, then call process.waitFor(timeout, TimeUnit.SECONDS) (or
similar) and if it returns false call process.destroyForcibly(), wait a short
grace period, capture/flush remaining output, and throw an IllegalStateException
indicating the timeout and the command; ensure the thrown exception and log
include the command, exit/timeout status, and logPrefix so callers of runCommand
can fail fast when istioctl hangs.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodCustomizer.java`:
- Around line 52-55: The isPodHealthy(Pod pod) method in IstioPodCustomizer
assumes pod.getStatus() is non-null and can throw an NPE; update
isPodHealthy(Pod pod) to defensively check pod.getStatus() for null before
calling getPhase() (mirror the base interface default behavior), returning false
if status is null or if the phase is not "Running", so use pod.getStatus() !=
null and then check pod.getStatus().getPhase().
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.java`:
- Around line 53-55: When parsing the "--port" CLI arg in IstioPodEntryPoint
(inside main where serverPort is set), wrap the Integer.parseInt(args[i + 1])
call in a try/catch for NumberFormatException; on catch log a clear error via
the same logger or System.err including the invalid value (args[i + 1]) and exit
with a non-zero status so the process fails fast instead of throwing a stack
trace.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java`:
- Around line 205-216: The current waitForReady method returns as soon as any
Pod is in phase "Running", which misses whether the application container(s) and
the istio-proxy sidecar are actually started and ready; update the predicate
inside K8sClusterHelper.poll (in waitForReady) to inspect each
Pod.getStatus().getContainerStatuses() and only return true when (a) the
server/application container(s) have ready==true and state running/terminated as
appropriate, and (b) there exists a container named "istio-proxy" with
ready==true (or otherwise validate sidecar injection), so the method only
completes when both app containers and the istio sidecar are present and ready
for serviceName.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioState.java`:
- Around line 67-68: connectOrCreate() currently always calls reinstallIstio(),
which runs istioctl uninstall --purge and can destroy Istio on shared clusters;
change the flow so reinstallIstio() is only invoked when a new cluster is
created (or when an explicit force/reinstall flag is set), not when
attachToExisting/attachToKubeconfig succeeds; update connectOrCreate() to detect
whether the kubeconfig was created vs attached (the return value or a
created/attached boolean from the cluster creation logic) and only call
reinstallIstio(kubeconfigPath, client) in the created-new case; ensure the same
fix is applied to the other branch referenced around the code block covering
lines 96-105 that also calls reinstallIstio().
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java`:
- Around line 142-156: The waitForPodHealthy method currently throws on timeout
without surfacing pod logs; update waitForPodHealthy(KubernetesClient client,
String podName, String namespace, PodCustomizer podCustomizer) to fetch the Pod
object and the logs from the test container (via
client.pods().inNamespace(namespace).withName(podName).inContainer(...).getLog(...)
or equivalent) when healthy==false and include that output in the thrown
IllegalStateException or log it before deletion; ensure you handle null
pod/status and IOException from log retrieval and include
pod.getMetadata().getName()/pod.getStatus() and the retrieved logs in the error
message to aid debugging.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/K8sClusterHelper.java`:
- Around line 59-68: startK3sAndWaitReady currently leaves the K3sContainer
(k3s) running if readiness waiting or IstioTestImage.loadIntoK3s throws; wrap
the post-start logic so any exception stops the container before propagating:
after creating k3s via startK3s(), use a try/catch (or try/finally with rethrow)
around the client wait (waitForReadyNode) and the
IstioTestImage.loadIntoK3s(k3s) call; on any failure call k3s.stop() (or
close/shutdown method on K3sContainer) and rethrow the exception so a successful
path still returns a running k3s but all error paths clean up the container.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/PodCustomizer.java`:
- Around line 42-44: The isPodHealthy default implementation can NPE if
pod.getStatus() is null; update isPodHealthy(Pod pod) to defensively check
pod.getStatus() (and optionally pod.getStatus().getPhase()) before calling
getPhase(), and return false (or an appropriate non-healthy value) when status
or phase is null so it safely handles pods in early lifecycle stages.
In `@it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/EnvoyDebugTest.java`:
- Around line 85-100: The test envoyBootstrapFile currently only logs files and
can pass when no bootstrap JSON exists; change it to assert that at least one
filename in filenames endsWith(".json") (or that
filenames.stream().anyMatch(name -> name.endsWith(".json")) is true) and fail
the test if none are found, then continue to log/read the matching file(s) as
before; update references in the method (dir, filenames, the for-loop) so the
assertion occurs before iterating the files and reading their contents.
---
Nitpick comments:
In `@dependencies.toml`:
- Around line 94-96: Update the istio dependency version from "1.29.0" to the
current 1.29.x patch release "1.29.1" in dependencies.toml by changing the istio
= "1.29.0" entry to istio = "1.29.1" so the project benefits from the latest
1.29.x bug fixes; verify no other references override this value and run
dependency resolution to confirm compatibility.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/DockerAvailableCondition.java`:
- Around line 26-29: The double-negative check
(!HostOnlyExtension.notRunningInPod()) in DockerAvailableCondition should be
made clearer: introduce a positive-named local boolean (e.g., runningInPod)
assigned from the negative call and use it in the if, or (if you can change
HostOnlyExtension) replace the call with a new positive helper method
isRunningInPod() and use that; update the conditional in the method inside
DockerAvailableCondition to use the new positive variable/method (referencing
HostOnlyExtension.notRunningInPod() or HostOnlyExtension.isRunningInPod() and
the DockerAvailableCondition class).
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/EnabledIfDockerAvailable.java`:
- Around line 25-28: Add a one-line Javadoc to the public annotation
EnabledIfDockerAvailable explaining its purpose; e.g., document that the
annotation enables the annotated test class or method only when the Docker
daemon is available and mention it uses DockerAvailableCondition via `@ExtendWith`
to perform the check. Place the Javadoc immediately above the annotation
declaration for EnabledIfDockerAvailable so it satisfies the project's public
API documentation guideline.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.java`:
- Around line 83-85: The call to Launcher.discover(request) is redundant because
Launcher.execute(request, listener) already performs discovery for a
LauncherDiscoveryRequest; remove the explicit launcher.discover(request)
invocation in IstioPodEntryPoint (the block that creates Launcher via
LauncherFactory.create(), calls launcher.discover(request), then
launcher.execute(request, listener)) and keep only launcher.execute(request,
listener) to simplify and avoid duplicate discovery.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.java`:
- Around line 61-66: The docker save invocation using new
ProcessBuilder("docker", "save", IMAGE_NAME") does not capture stderr, so when
saveProcess.waitFor() != 0 the thrown IllegalStateException lacks diagnostic
output; modify the ProcessBuilder for docker save (the saveProcess) to capture
stderr like the ctr images import path does (e.g., redirectError to a temporary
file or capture the error stream), and include the captured stderr contents in
the IllegalStateException message so failures around IMAGE_NAME and tempFile
produce actionable diagnostics.
In
`@it/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioTestingBlockHoundIntegration.java`:
- Around line 15-17: Remove the extra blank line between the file header comment
and the package declaration so the package statement immediately follows the
closing comment; locate the top of the file around the package declaration for
the IstioTestingBlockHoundIntegration class (package
com.linecorp.armeria.it.istio.testing) and delete the stray blank line after the
license block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 723aaaf8-995a-4498-aa5e-9413e03dfc1f
📒 Files selected for processing (24)
dependencies.tomlit/xds-istio/build.gradleit/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/DockerAvailableCondition.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/EnabledIfDockerAvailable.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/HostOnlyExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioClusterExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioEnv.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodCustomizer.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodTest.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioState.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/K8sClusterHelper.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/PodCustomizer.javait/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioImageBuildTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioStartupTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioTestingBlockHoundIntegration.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/EchoConfigurator.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/EnvoyDebugTest.javait/xds-istio/src/test/resources/META-INF/services/reactor.blockhound.integration.BlockHoundIntegrationsettings.gradle
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioEnv.java
Show resolved
Hide resolved
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java
Show resolved
Hide resolved
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodCustomizer.java
Show resolved
Hide resolved
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.java
Show resolved
Hide resolved
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java
Show resolved
Hide resolved
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioState.java
Show resolved
Hide resolved
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java
Show resolved
Hide resolved
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/K8sClusterHelper.java
Show resolved
Hide resolved
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/PodCustomizer.java
Show resolved
Hide resolved
it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/EnvoyDebugTest.java
Show resolved
Hide resolved
Contributor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java (1)
166-176: Consider addingcontainerPortto the container spec for clarity.While Kubernetes Services can target any port on a pod, explicitly declaring
containerPortin the container spec improves documentation and enables port-related tooling. This is optional but aids maintainability.💡 Suggested addition
.withArgs("--server-factory", configuratorClass.getName(), "--port", String.valueOf(port)) + .addNewPort() + .withContainerPort(port) + .endPort() .addNewEnv() .withName("JAVA_TOOL_OPTIONS")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java` around lines 166 - 176, Add an explicit container port to the server container spec to improve clarity and tooling support: in the container builder chain that starts with .addNewContainer().withName("server")... (the block using .withArgs(..., String.valueOf(port)) and .withImagePullPolicy("Never")), insert a port declaration such as .addNewPort().withContainerPort(port).endPort() before .endContainer() so the container explicitly exposes the same port number passed via String.valueOf(port).it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java (1)
161-185: Consider throwing on timeout instead of silently returning failure exit code.When the test container times out without terminating, the method logs a warning and returns exit code 1. This causes the test to fail with a generic "pod failed" message, obscuring the actual timeout. Throwing an explicit timeout exception would make the failure reason clearer.
💡 Suggested change
if (!terminated) { - logger.warn("Timed out waiting for test container in pod '{}' to terminate", podName); + throw new IllegalStateException( + "Timed out waiting for test container in pod '" + podName + "' to terminate"); } return exitCode[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java` around lines 161 - 185, The method waitForTestContainerTerminated currently logs a warning and returns exit code 1 on timeout; change it to throw an explicit exception instead so callers see a clear timeout failure. In waitForTestContainerTerminated, after K8sClusterHelper.poll returns false (i.e., when terminated is false) replace the logger.warn + return exitCode[0] path with throwing a descriptive runtime exception (e.g., IllegalStateException or a custom TimeoutException) that includes podName and namespace; keep the successful return of the discovered exit code unchanged. Ensure any callers of waitForTestContainerTerminated can handle or propagate this exception as needed.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.java (1)
61-69: Process error stream should be consumed during execution to avoid potential deadlock.If
docker savewrites significant output to stderr before exiting, the process can block waiting for the buffer to be drained. Reading stderr only afterwaitFor()may miss content or cause hangs. Consider redirecting stderr to stdout or consuming it concurrently.💡 Suggested fix using redirectErrorStream
final Process saveProcess = new ProcessBuilder("docker", "save", IMAGE_NAME) + .redirectErrorStream(true) .redirectOutput(tempFile.toFile()) .start(); final int exitCode = saveProcess.waitFor(); if (exitCode != 0) { - final String stderr = new String(saveProcess.getErrorStream().readAllBytes()); throw new IllegalStateException( - "docker save exited with status " + exitCode + ": " + stderr); + "docker save exited with status " + exitCode + + ". Check Docker daemon availability and image existence."); }Alternatively, redirect stderr separately to a temp file or use
ProcessBuilder.Redirect.DISCARDif the error details aren't critical.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.java` around lines 61 - 69, The docker save invocation may deadlock because stderr is only read after waitFor(); modify the ProcessBuilder setup that constructs saveProcess (the ProcessBuilder call with "docker", "save", IMAGE_NAME and the saveProcess variable) to consume stderr during execution—either call redirectErrorStream(true) on the ProcessBuilder so stderr is merged into the redirected output, or redirectError(ProcessBuilder.Redirect.to(...))/redirectError(ProcessBuilder.Redirect.DISCARD) to a temp file before start(), or spawn a background thread to read saveProcess.getErrorStream() while the process runs; ensure the stream is consumed before calling saveProcess.waitFor().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java`:
- Around line 81-87: enableNamespaceInjection may NPE when
ns.getMetadata().getLabels() is null; update the edit lambda to defensively
initialize the labels map if null before putting "istio-injection":"enabled". In
the Kubernetes edit block (enableNamespaceInjection /
client.namespaces().withName(...).edit), check ns.getMetadata().getLabels() and
if it is null create a new HashMap and set it via
ns.getMetadata().setLabels(...), then put the label and return ns; keep the
logger call unchanged.
- Around line 126-131: The code calls process.waitFor(DEFAULT_READY_TIMEOUT) but
Process has no waitFor(Duration) overload; change the call to use the
waitFor(long timeout, TimeUnit unit) overload (e.g.,
process.waitFor(DEFAULT_READY_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS)) and
add the necessary import for java.util.concurrent.TimeUnit; keep the existing
timeout handling and exception message (you may use
DEFAULT_READY_TIMEOUT.toMinutes() or DEFAULT_READY_TIMEOUT.toMillis() in the
message to preserve precision) and update references around the wait to use the
new boolean result variable as before.
---
Nitpick comments:
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java`:
- Around line 166-176: Add an explicit container port to the server container
spec to improve clarity and tooling support: in the container builder chain that
starts with .addNewContainer().withName("server")... (the block using
.withArgs(..., String.valueOf(port)) and .withImagePullPolicy("Never")), insert
a port declaration such as .addNewPort().withContainerPort(port).endPort()
before .endContainer() so the container explicitly exposes the same port number
passed via String.valueOf(port).
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java`:
- Around line 161-185: The method waitForTestContainerTerminated currently logs
a warning and returns exit code 1 on timeout; change it to throw an explicit
exception instead so callers see a clear timeout failure. In
waitForTestContainerTerminated, after K8sClusterHelper.poll returns false (i.e.,
when terminated is false) replace the logger.warn + return exitCode[0] path with
throwing a descriptive runtime exception (e.g., IllegalStateException or a
custom TimeoutException) that includes podName and namespace; keep the
successful return of the discovered exit code unchanged. Ensure any callers of
waitForTestContainerTerminated can handle or propagate this exception as needed.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.java`:
- Around line 61-69: The docker save invocation may deadlock because stderr is
only read after waitFor(); modify the ProcessBuilder setup that constructs
saveProcess (the ProcessBuilder call with "docker", "save", IMAGE_NAME and the
saveProcess variable) to consume stderr during execution—either call
redirectErrorStream(true) on the ProcessBuilder so stderr is merged into the
redirected output, or
redirectError(ProcessBuilder.Redirect.to(...))/redirectError(ProcessBuilder.Redirect.DISCARD)
to a temp file before start(), or spawn a background thread to read
saveProcess.getErrorStream() while the process runs; ensure the stream is
consumed before calling saveProcess.waitFor().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68d725ad-74d1-416f-9102-114359cdc151
📒 Files selected for processing (14)
dependencies.tomlit/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/DockerAvailableCondition.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/EnabledIfDockerAvailable.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/HostOnlyExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodCustomizer.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/K8sClusterHelper.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/PodCustomizer.javait/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioTestingBlockHoundIntegration.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/EnvoyDebugTest.java
✅ Files skipped from review due to trivial changes (2)
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/EnabledIfDockerAvailable.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/PodCustomizer.java
🚧 Files skipped from review as they are similar to previous changes (8)
- it/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioTestingBlockHoundIntegration.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/DockerAvailableCondition.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodCustomizer.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/HostOnlyExtension.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.java
- dependencies.toml
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/K8sClusterHelper.java
- it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/EnvoyDebugTest.java
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java
Show resolved
Hide resolved
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6706 +/- ##
============================================
- Coverage 74.46% 73.96% -0.50%
- Complexity 22234 24234 +2000
============================================
Files 1963 2188 +225
Lines 82437 91088 +8651
Branches 10764 11917 +1153
============================================
+ Hits 61385 67376 +5991
- Misses 15918 18046 +2128
- Partials 5134 5666 +532 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ikhoon
approved these changes
Apr 9, 2026
it/xds-istio/build.gradle
Outdated
| implementation libs.junit5.platform.launcher | ||
| implementation libs.kubernetes.client.api | ||
| implementation libs.kubernetes.client.impl | ||
| implementation libs.kubernetes.httpclient.okhttp |
Contributor
There was a problem hiding this comment.
Question) Could we use Armeria Kubernetes client implementation? If not, is there any reason we can't use it?
Contributor
Author
There was a problem hiding this comment.
No reason in particular, done
| } | ||
|
|
||
| def kubeconfigEnvValue = | ||
| layout.buildDirectory.file('kubeconfig/kubeconfig.yaml').get().asFile.absolutePath |
Contributor
There was a problem hiding this comment.
Question) Where does this file come from?
Contributor
Author
There was a problem hiding this comment.
This is written here: https://github.com/jrhee17/armeria/blob/7c2dc4282cf46a1207757a707334a2ee6d83892c/it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioState.java#L91
The config is written so that each test has the option of reusing the existing k8s cluster since starting a new container takes > 1min.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a subset of #6700
Motivation
Armeria's xDS support targets Istio as a primary deployment environment, but all existing tests use a fake/stub control plane. There is no way to catch regressions in actual Istio compatibility — Istiod's ADS bootstrap quirks, mTLS certificate distribution via SDS, Envoy sidecar injection, and so on — without running against a real cluster.
Modifications
it/xds-istio: a JUnit 5 test harness module that spins up a K3s cluster via Testcontainers and installs Istio into it.IstioClusterExtension: a@RegisterExtensionthat manages K3s cluster lifecycle. Reuses an existing cluster if a kubeconfig is present and connectable; otherwise starts a fresh one.IstioServerExtension: a@RegisterExtensionthat deploys an Armeria server workload (configured via aServerConfigurator) into the cluster as a Kubernetes Deployment + Service with Istio sidecar injection enabled.@IstioPodTestandIstioTestExtension: intercept annotated test methods and run them inside a temporary Kubernetes Pod instead of locally, propagating pass/fail and full pod logs back to JUnit.IstioPodEntryPoint: the entry point for the test image. It can either execute a single JUnit test method (--class/--method) or start an Armeria server (--server-factory/--port).IstioTestImage: builds a Docker image from the test runtime classpath and loads it into K3s's internal containerd store so pods can useimagePullPolicy: Never.HostOnlyExtension: a base class that suppresses extensionsetUp/tearDownwhen running inside a pod, preventing recursive cluster creation.EnabledIfDockerAvailable: an@ExecutionConditionthat skips all tests when the Docker daemon is not reachable.PodCustomizer/IstioPodCustomizer: an extension point for tests that need to customise the generatedPodSpec.IstioStartupTest(Istiod readiness) andEnvoyDebugTest(in-cluster HTTP reachability, Envoy admin stats, config dump, bootstrap file inspection).build.gradlethat downloadsistioctlfor the host OS/arch at build time and passes cluster-related paths as environment variables to test tasks. Unsupported OS/arch combinations disable all tasks with a warning.:it:xds-istiotosettings.gradleand the required dependencies (testcontainers-k3s,kubernetes-httpclient-okhttp,istioversion) todependencies.toml.Result