Kubernetes integration test suite #619
Conversation
|
Warning Rate limit exceeded@Tharsanan1 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughIntroduces a Cucumber-based Kubernetes operator integration test framework: CI workflow now invokes Go-based BDD tests; adds test CLI, TestState, step libraries (K8s/Helm/HTTP/assert), feature files, Makefile automation, and local registry manifests for running integration tests locally or in CI. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Job
participant Suite as Test Suite (godog)
participant State as TestState
participant Helm as Helm / Chart Ops
participant K8s as Kubernetes Cluster
participant HTTP as Gateway / App
CI->>Suite: invoke kubernetes/it tests (env vars)
Suite->>State: Initialize (kubeconfig -> clients)
Suite->>Helm: install/operator chart (with image overrides)
Helm->>K8s: helm install/upgrade -> create operator resources
K8s-->>Helm: operator pods running
Suite->>K8s: apply Gateway / RestApi CRs
K8s-->>Suite: resource accepted
Suite->>K8s: WaitForCondition(Resource Programmed)
K8s-->>Suite: condition met
Suite->>State: StartPortForward(gateway-router)
State->>K8s: kubectl port-forward (spawn)
Suite->>HTTP: send HTTP requests to localhost
HTTP-->>Suite: responses (captured)
Suite->>Suite: assert responses (AssertSteps)
Suite->>State: StopAllPortForwards / Cleanup
Suite->>CI: write test-output.log -> upload artifact
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Tharsanan1
force-pushed
the
operator-integ-test
branch
from
4a8ee7d to
1151fa5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (18)
kubernetes/it/features/multi_namespace.feature (1)
1-74: LGTM! Well-structured multi-namespace test feature.The feature file correctly tests cross-namespace API exposure through a cluster-scoped gateway. The Background setup is appropriate, and the inclusion of a cleanup scenario is good practice.
Consider whether the 5-second wait (line 68) is sufficient for all environments. In heavily loaded CI systems, reconciliation might take longer. You may want to verify this timeout is adequate or consider using a condition check instead of a fixed wait.
kubernetes/it/README.md (1)
148-170: Add language identifier to fenced code block.The project structure code block at line 148 is missing a language identifier, which affects markdown rendering and syntax highlighting.
🔎 Proposed fix
-``` +```text kubernetes/it/ ├── Makefile # Build and test commandskubernetes/it/manifests/registry.yaml (2)
1-32: Add persistent storage to prevent data loss on pod restarts.The registry Deployment lacks a PersistentVolumeClaim, meaning all pushed images will be lost if the pod restarts. This could cause test flakiness and require re-pushing images.
🔎 Add PersistentVolume for registry storage
spec: containers: - name: registry image: registry:2 ports: - containerPort: 5000 + volumeMounts: + - name: registry-storage + mountPath: /var/lib/registry + volumes: + - name: registry-storage + persistentVolumeClaim: + claimName: registry-pvcYou'll also need to create a PersistentVolumeClaim:
apiVersion: v1 kind: PersistentVolumeClaim metadata: name: registry-pvc namespace: registry spec: accessModes: - ReadWriteOnce resources: requests: storage: 10Gi
16-20: Consider adding resource limits and pinning image version.For better test stability and resource management:
- Pin the registry image to a specific version (e.g.,
registry:2.8.3)- Add resource limits to prevent resource exhaustion
🔎 Proposed improvements
containers: - name: registry - image: registry:2 + image: registry:2.8.3 ports: - containerPort: 5000 + resources: + requests: + memory: "128Mi" + cpu: "100m" + limits: + memory: "512Mi" + cpu: "500m"kubernetes/it/cmd/main.go (1)
1-142: LGTM! Well-structured test runner with proper lifecycle management.The test runner correctly implements the godog test suite with:
- Proper initialization and cleanup hooks
- State reset between scenarios to prevent leakage
- Comprehensive logging for debugging
- Environment-driven tag filtering
- Defensive nil checks before operations
One minor enhancement: Consider adding a defer/recover in InitializeTestSuite's BeforeSuite to ensure cleanup happens even if initialization panics. However, the current Fatal approach is acceptable for test code.
kubernetes/it/features/jwt_authentication.feature (1)
25-52: Background contains duplicate steps that should be consolidated.Lines 27-35 have redundant duplicate steps:
httpbin is deployed in namespace "default"appears at lines 28 and 34mock-jwks is deployed in namespace "default"appears at lines 29 and 35the gateway configuration ConfigMap "test-gateway-config" existsappears at lines 30 and 33These duplicates add unnecessary execution time without benefit.
🔎 Proposed fix to remove duplicates
Background: Given namespace "default" exists Given cert-manager is installed And httpbin is deployed in namespace "default" And mock-jwks is deployed in namespace "default" And the gateway configuration ConfigMap "test-gateway-config" exists in namespace "default" Given I install the operator in namespace "operator" And the operator is installed in namespace "operator" - And the gateway configuration ConfigMap "test-gateway-config" exists in namespace "default" - And httpbin is deployed in namespace "default" - And mock-jwks is deployed in namespace "default" And I apply the following CR:kubernetes/it/steps/k8s_steps.go (1)
514-518: Default pluralization may fail for irregular resource names.The default case simply appends "s" to the kind, which won't work for resources with irregular plurals (e.g., "Policy" → "policies", "Ingress" → "ingresses"). Consider using the REST mapper's
ResourceFormethod for proper pluralization, or document this limitation.kubernetes/it/steps/assert_steps.go (1)
301-322:getJSONFielddoesn't support array indexing.The dot-notation traversal only handles
map[string]interface{}. JSON paths likeitems.0.nameordata[0].valuewon't work since arrays return[]interface{}type and fall through to the error case.If array access is needed in tests, consider extending this:
🔎 Possible enhancement for array support
for _, part := range parts { switch v := current.(type) { case map[string]interface{}: current = v[part] case []interface{}: idx, err := strconv.Atoi(part) if err != nil || idx < 0 || idx >= len(v) { return nil, fmt.Errorf("invalid array index %q", part) } current = v[idx] default: return nil, fmt.Errorf("cannot navigate to %q in %T", part, current) } }kubernetes/it/suite_test.go (1)
101-114: Consider addinghelmSteps.Reset()for consistency.The
Beforehook resetstestState,httpSteps, andk8sSteps, but nothelmSteps. IfHelmStepshas any mutable state that should be cleared between scenarios, it should also be reset here.🔎 Proposed fix
ctx.Before(func(ctx context.Context, sc *godog.Scenario) (context.Context, error) { log.Printf("Starting scenario: %s", sc.Name) if testState != nil { testState.Reset() } if httpSteps != nil { httpSteps.Reset() } if k8sSteps != nil { k8sSteps.Reset() } + // helmSteps has no mutable state to reset currently return ctx, nil })kubernetes/it/state.go (2)
88-89: RESTMapper created with nil schemes has no type knowledge.
meta.NewDefaultRESTMapper(nil)creates an empty mapper with no registered schemes. This won't be able to map GVKs (GroupVersionKind) to GVRs (GroupVersionResource). If any code relies on the mapper for type resolution, it will fail.Consider using
restmapper.NewDeferredDiscoveryRESTMapperfrom the API server's discovery client instead:🔎 Proposed fix using discovery-based mapper
+import ( + "k8s.io/client-go/restmapper" +) + // Create REST mapper -restMapper := meta.NewDefaultRESTMapper(nil) +discoveryClient := clientset.Discovery() +restMapper := restmapper.NewDeferredDiscoveryRESTMapper( + memory.NewMemCacheClient(discoveryClient), +)
140-145: Fixed 3-second delay for port-forward readiness could be flaky.The hardcoded
time.Sleep(3 * time.Second)may be insufficient under load or on slower systems. Consider adding a TCP dial check to confirm the port is actually listening:🔎 Proposed enhancement with TCP check
// Wait for port forward to be ready with TCP check deadline := time.Now().Add(10 * time.Second) for time.Now().Before(deadline) { conn, err := net.DialTimeout("tcp", fmt.Sprintf("localhost:%d", localPort), time.Second) if err == nil { conn.Close() return nil } time.Sleep(500 * time.Millisecond) } return fmt.Errorf("port forward not ready after 10s")kubernetes/it/Makefile (1)
14-16: Hardcoded personal Docker Hub username should be a placeholder.
DOCKER_HUB_USER ?= tharsananuses a personal username as the default. This could cause confusion or accidental pushes to the wrong registry. Consider using a placeholder that clearly indicates it must be set:🔎 Proposed fix
# Docker Hub username - change this to your Docker Hub username -DOCKER_HUB_USER ?= tharsanan +DOCKER_HUB_USER ?= $(error DOCKER_HUB_USER is not set. Please set it to your Docker Hub username)Or use a safer default:
-DOCKER_HUB_USER ?= tharsanan +DOCKER_HUB_USER ?= your-dockerhub-usernamekubernetes/it/cmd/state.go (1)
140-145: Fixed sleep is fragile for port-forward readiness.The 3-second sleep doesn't guarantee the port-forward is ready and may be insufficient on slower systems. Consider polling the local port for connectivity instead.
🔎 Suggested improvement
s.portForwards[key] = cmd - // Wait for port forward to be ready - time.Sleep(3 * time.Second) + // Wait for port forward to be ready by polling the local port + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + conn, err := net.DialTimeout("tcp", fmt.Sprintf("localhost:%d", localPort), 500*time.Millisecond) + if err == nil { + conn.Close() + return nil + } + time.Sleep(200 * time.Millisecond) + } + return fmt.Errorf("port-forward to %s not ready after 10s", key) - return nil }kubernetes/it/steps/helm_steps.go (5)
114-121: Redundant and confusing namespace creation logic.Lines 117-120 are problematic:
- Line 117 runs a dry-run command but discards the output.
- Lines 118-120 apply a namespace manifest but ignore errors.
- Helm's
--create-namespaceflag (line 126) already handles namespace creation.Consider removing this redundant code.
🔎 Suggested cleanup
func (h *HelmSteps) installOperatorWithOptions(namespace, watchNamespaces string) error { - // Ensure namespace exists - exec.Command("kubectl", "create", "namespace", namespace, "--dry-run=client", "-o", "yaml").Run() - execCmd := exec.Command("kubectl", "apply", "-f", "-") - execCmd.Stdin = strings.NewReader(fmt.Sprintf("apiVersion: v1\nkind: Namespace\nmetadata:\n name: %s", namespace)) - execCmd.Run() - args := []string{ "upgrade", "--install", "gateway-operator",
197-202: Silent error suppression may hide issues.Ignoring all errors from
helm uninstallmakes debugging difficult. Consider logging the error for visibility, even if you don't fail the step.🔎 Suggested improvement
func (h *HelmSteps) uninstallHelmRelease(releaseName, namespace string) error { cmd := exec.Command("helm", "uninstall", releaseName, "-n", namespace) - cmd.Run() // Ignore errors + if output, err := cmd.CombinedOutput(); err != nil { + // Log but don't fail - release may not exist + log.Printf("helm uninstall %s: %v (output: %s)", releaseName, err, output) + } return nil }
269-269: Using:latesttag reduces test reproducibility.The
kennethreitz/httpbin:latestimage tag may change over time, potentially causing flaky tests. Consider pinning to a specific version.🔎 Suggested fix
- image: kennethreitz/httpbin:latest + image: kennethreitz/httpbin:0.9.2
292-295: Unnecessary sleep beforekubectl wait.The 5-second sleeps before
kubectl waitare redundant sincekubectl waitalready polls until the condition is met or timeout. These add unnecessary delays to test execution.🔎 Suggested fix for httpbinDeployed
} // Wait for ready - time.Sleep(5 * time.Second) cmd = exec.Command("kubectl", "wait", "--for=condition=ready", "pod",Also applies to: 353-356
409-413: Hardcoded namespace assumption for mock-jwks service.The issuer and JWKS URIs assume mock-jwks is deployed in the
defaultnamespace. Consider parameterizing this to match where mock-jwks is actually deployed.This could cause JWT validation failures if mock-jwks is deployed in a different namespace than
default.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
kubernetes/it/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
.github/workflows/operator-integration-test.ymlkubernetes/it/Makefilekubernetes/it/README.mdkubernetes/it/cmd/cmdkubernetes/it/cmd/main.gokubernetes/it/cmd/state.gokubernetes/it/features/full_lifecycle.featurekubernetes/it/features/gateway_lifecycle.featurekubernetes/it/features/jwt_authentication.featurekubernetes/it/features/multi_namespace.featurekubernetes/it/features/restapi_lifecycle.featurekubernetes/it/features/scoped_mode.featurekubernetes/it/go.modkubernetes/it/manifests/registry.yamlkubernetes/it/state.gokubernetes/it/steps/assert_steps.gokubernetes/it/steps/helm_steps.gokubernetes/it/steps/http_steps.gokubernetes/it/steps/k8s_steps.gokubernetes/it/suite_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
kubernetes/it/suite_test.go (4)
kubernetes/it/state.go (2)
TestState(38-56)NewTestState(59-106)kubernetes/it/steps/k8s_steps.go (2)
K8sSteps(43-53)NewK8sSteps(56-64)kubernetes/it/steps/http_steps.go (2)
HTTPSteps(49-58)NewHTTPSteps(61-69)kubernetes/it/steps/helm_steps.go (2)
HelmSteps(32-42)NewHelmSteps(45-57)
kubernetes/it/steps/k8s_steps.go (1)
cli/src/internal/gateway/resources.go (1)
Resource(34-38)
kubernetes/it/cmd/state.go (1)
kubernetes/it/state.go (2)
NewTestState(59-106)TestState(38-56)
🪛 checkmake (0.2.2)
kubernetes/it/Makefile
[warning] 21-21: Target body for "help" exceeds allowed length of 5 (27).
(maxbodylength)
[warning] 3-3: Missing required phony target "all"
(minphony)
🪛 Checkov (3.2.334)
kubernetes/it/manifests/registry.yaml
[medium] 1-21: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-21: Minimize the admission of root containers
(CKV_K8S_23)
🪛 markdownlint-cli2 (0.18.1)
kubernetes/it/README.md
148-148: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OSV Scanner (2.3.1)
kubernetes/it/go.mod
[HIGH] 1-1: golang.org/x/oauth2 0.23.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 1-1: golang.org/x/oauth2 0.23.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
🔇 Additional comments (23)
kubernetes/it/README.md (1)
1-325: Excellent comprehensive documentation!The README provides thorough coverage of prerequisites, quick start, running tests with tags, configuration, project structure, debugging, and troubleshooting. This will significantly improve developer experience.
kubernetes/it/manifests/registry.yaml (1)
1-32: Security context warnings are acceptable for test registry.Static analysis flagged missing security context hardening (allowPrivilegeEscalation, runAsNonRoot). These are generally acceptable for a local test registry that runs in an isolated test environment and is not exposed to production traffic.
Note: If this registry will be used in shared/production-like environments, consider adding security context constraints.
kubernetes/it/features/gateway_lifecycle.feature (1)
1-55: LGTM! Well-designed Gateway lifecycle test.The feature file provides comprehensive coverage of Gateway CR lifecycle operations, including creation, programming verification, pod existence checks, and cleanup validation. The 180-second timeout for Gateway programming and the 10-second wait for cleanup are reasonable.
kubernetes/it/features/full_lifecycle.feature (1)
1-192: LGTM! Comprehensive end-to-end operator lifecycle test.This feature file provides excellent coverage of the complete operator lifecycle:
- Prerequisites installation (cert-manager, httpbin, mock-jwks)
- Operator installation and health checks
- Gateway creation and verification
- RestApi CRUD operations with HTTP invocation
- Cleanup and resource validation
- Operator uninstall/reinstall
- Full cycle after reinstall
The use of different local ports (8080 vs 8090) effectively prevents port conflicts between scenarios. The granular tagging strategy enables flexible test execution.
kubernetes/it/features/restapi_lifecycle.feature (1)
1-108: LGTM! Complete RestApi lifecycle coverage.The feature file effectively tests RestApi CR lifecycle operations:
- Create: API creation, programming verification, and successful invocation
- Update: Adding new operation (PUT) and verifying it works
- Delete: API deletion and verification that routes are removed (404 response)
The Background setup efficiently provides a ready Gateway for all scenarios, and the verification of route removal with a 404 response is a good practice.
kubernetes/it/features/scoped_mode.feature (2)
40-45: Scenario has implicit dependency on prior scenario execution.The
@scoped-rbacscenario at line 42 usesGiven the operator is installed in namespace "scoped-test"as a precondition, but it doesn't install the operator itself. It relies on the@scoped-installscenario having run first.If these scenarios are meant to run in a specific order, consider using Cucumber's
@ordertags or documenting this dependency. Alternatively, make this scenario self-contained by including the installation step.
1-128: Well-structured scoped mode test feature.The feature comprehensively tests scoped operator mode including RBAC verification, positive and negative namespace isolation tests, and proper cleanup. The test design is solid.
.github/workflows/operator-integration-test.yml (2)
28-28: Go version correctly set to 1.23.This is appropriate as Go 1.23 is the current stable version.
223-242: Clean refactoring to cucumber-based tests with proper artifact handling.The workflow now properly:
- Runs cucumber tests via
go testwith appropriate timeout- Configures necessary environment variables for registry, charts, and image settings
- Uploads test output as artifact regardless of outcome (
if: always())kubernetes/it/steps/k8s_steps.go (1)
66-118: Comprehensive step registration with good coverage.The
Registermethod provides extensive step definitions covering namespaces, CR lifecycle, status conditions, pod/service verification, RBAC checks, port forwarding, and utilities. The organization into logical groups is clean and maintainable.kubernetes/it/steps/assert_steps.go (1)
50-77: Comprehensive assertion step definitions with good coverage.The assertion library covers status codes, headers, body content, and JSON validation. The registration is well-organized and provides flexible assertion capabilities for BDD tests.
kubernetes/it/suite_test.go (1)
48-65: Well-structured Godog test suite entry point.The
TestFeaturesfunction properly initializes and runs the godog test suite with appropriate options for formatting, paths, and output streaming. The exit code handling ensures test failures are properly reported.kubernetes/it/state.go (1)
37-56: Well-designed test state struct with proper encapsulation.The
TestStatestruct cleanly aggregates Kubernetes clients, HTTP client, and test context. The mutex-protected port-forward map ensures thread-safe access. Good separation of concerns.kubernetes/it/Makefile (1)
21-49: Comprehensive help target with clear documentation.The help target provides excellent developer UX with usage examples, available tags, and configuration options. This makes the test suite approachable for new contributors.
kubernetes/it/cmd/state.go (3)
37-56: LGTM!The
TestStatestruct is well-designed with proper synchronization for port-forward management and appropriate client initialization for different Kubernetes operations.
108-117: LGTM!The separation between
Reset()(for inter-scenario cleanup) andCleanup()(for final teardown) is appropriate for the test lifecycle.
148-174: LGTM!The port-forward cleanup logic correctly handles process termination with
Kill()+Wait(), and safely modifies the map during iteration.kubernetes/it/steps/http_steps.go (5)
48-69: LGTM!The
HTTPStepsstruct design and constructor with defensive nil check for the client is appropriate.
71-100: LGTM!Comprehensive step definitions covering common HTTP operations, and
Reset()properly clears all mutable state between scenarios.
190-262: LGTM!Well-designed request building with comprehensive debug logging, sensitive header redaction, and helpful JSON content-type auto-detection.
263-295: LGTM!Excellent error diagnostics with actionable suggestions for common failure modes (connection refused, timeout, DNS resolution).
296-323: LGTM!Response handling correctly reads and stores the body separately, then closes it. The
lastBodyfield provides access to the response content after the body is closed.kubernetes/it/steps/helm_steps.go (1)
31-57: LGTM!Clean configuration design with environment variable overrides. Defaults are appropriate for local/CI testing with Kind clusters.
| // Create REST mapper | ||
| restMapper := meta.NewDefaultRESTMapper(nil) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if RestMapper is used anywhere in the codebase
rg -n "restMapper" kubernetes/it/ -A 3 -B 3Repository: wso2/api-platform
Length of output: 2441
🏁 Script executed:
#!/bin/bash
# Check where restMapper field is actually used in K8sSteps
rg -n "\.restMapper" kubernetes/it/steps/ -A 2 -B 2Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check the K8sSteps struct and its methods more carefully
cat kubernetes/it/steps/k8s_steps.go | head -100Repository: wso2/api-platform
Length of output: 4180
🏁 Script executed:
#!/bin/bash
# Search for restMapper usage in method bodies - look for accessing the field
rg "k\.restMapper|m\.restMapper|r\.restMapper" kubernetes/it/steps/Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Get full k8s_steps.go to verify restMapper is never used
wc -l kubernetes/it/steps/k8s_steps.goRepository: wso2/api-platform
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search for any occurrence of "restMapper" in k8s_steps.go (case-insensitive)
rg -i "restmapper" kubernetes/it/steps/k8s_steps.go -nRepository: wso2/api-platform
Length of output: 212
Remove unused restMapper field from K8sSteps.
The restMapper variable is created at line 89 and passed to K8sSteps at line 100, but it is never used in any methods. This dead code should be removed to avoid confusion.
🤖 Prompt for AI Agents
In kubernetes/it/cmd/state.go around lines 88 to 89, the local variable
restMapper is created and passed into K8sSteps but never used; remove the dead
restMapper creation and the corresponding parameter usage so K8sSteps
construction and its type/signature no longer reference restMapper (update the
NewK8sSteps call site and K8sSteps definition/constructor to drop that
parameter) to eliminate the unused field.
| // WaitForCondition waits for a condition on a resource | ||
| func (s *TestState) WaitForCondition(ctx context.Context, gvk, namespace, name, conditionType string, timeout time.Duration) error { | ||
| deadline := time.Now().Add(timeout) | ||
|
|
||
| for time.Now().Before(deadline) { | ||
| // Use kubectl wait for simplicity | ||
| cmd := exec.CommandContext(ctx, "kubectl", "wait", | ||
| fmt.Sprintf("--for=condition=%s", conditionType), | ||
| fmt.Sprintf("%s/%s", gvk, name), | ||
| "-n", namespace, | ||
| "--timeout=10s") | ||
|
|
||
| if err := cmd.Run(); err == nil { | ||
| return nil | ||
| } | ||
|
|
||
| time.Sleep(2 * time.Second) | ||
| } | ||
|
|
||
| return fmt.Errorf("timeout waiting for condition %s on %s/%s", conditionType, gvk, name) | ||
| } |
There was a problem hiding this comment.
Context cancellation not respected in the polling loop.
The loop doesn't check ctx.Done(), so if the context is cancelled, the method continues until the internal deadline. This could cause tests to hang during cleanup.
🔎 Suggested fix
func (s *TestState) WaitForCondition(ctx context.Context, gvk, namespace, name, conditionType string, timeout time.Duration) error {
deadline := time.Now().Add(timeout)
for time.Now().Before(deadline) {
+ select {
+ case <-ctx.Done():
+ return ctx.Err()
+ default:
+ }
+
// Use kubectl wait for simplicity
cmd := exec.CommandContext(ctx, "kubectl", "wait",
fmt.Sprintf("--for=condition=%s", conditionType),🤖 Prompt for AI Agents
In kubernetes/it/cmd/state.go around lines 176 to 196, the polling loop in
WaitForCondition does not respect context cancellation; update the loop to break
immediately when ctx is canceled by selecting on ctx.Done() alongside the
polling/timer (or check ctx.Err() before each iteration and before sleeping), so
you stop invoking kubectl and return ctx.Err() when canceled; keep the existing
kubectl.CommandContext usage but ensure the loop returns early on context
cancellation rather than waiting the internal deadline.
| @jwt-accept-authenticated | ||
| Scenario: JWT protected API accepts valid tokens | ||
| Given I obtain a token from "http://localhost:8081/token" | ||
| When I send a GET request with the token to "http://localhost:8080/secure/get" | ||
| Then the response status code should be 200 |
There was a problem hiding this comment.
Scenario depends on state from previous scenario, breaking BDD independence.
This scenario expects jwt-api RestApi to exist but doesn't create it. It relies on Scenario 1 ("JWT protected API rejects unauthenticated requests") having run first. If tests run in parallel or Scenario 1 is skipped/fails, this will fail.
Each scenario should be self-contained. Consider adding the RestApi CR application step to this scenario as well, or use a Background if the same RestApi is needed across all scenarios.
🤖 Prompt for AI Agents
In kubernetes/it/features/jwt_authentication.feature around lines 85 to 89, the
"JWT protected API accepts valid tokens" scenario assumes the RestApi CR
(jwt-api) exists from a prior scenario, breaking BDD independence; make the
scenario self-contained by either adding the step(s) to apply/create the RestApi
CR (same manifest used in Scenario 1) before obtaining a token, or move the
common RestApi creation into a Background section at the top of the feature so
every scenario starts with the RestApi present; ensure any created resources are
namespaced/unique or cleaned up to avoid cross-scenario interference.
| module github.com/wso2/api-platform/kubernetes/it | ||
|
|
||
| go 1.23.0 | ||
|
|
||
| toolchain go1.23.4 | ||
|
|
||
| require ( | ||
| github.com/cucumber/godog v0.15.0 | ||
| k8s.io/api v0.32.0 | ||
| k8s.io/apimachinery v0.32.0 | ||
| k8s.io/client-go v0.32.0 | ||
| sigs.k8s.io/controller-runtime v0.20.0 | ||
| sigs.k8s.io/yaml v1.4.0 | ||
| ) | ||
|
|
||
| require ( | ||
| github.com/cucumber/gherkin/go/v26 v26.2.0 // indirect | ||
| github.com/cucumber/messages/go/v21 v21.0.1 // indirect | ||
| github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect | ||
| github.com/emicklei/go-restful/v3 v3.11.0 // indirect | ||
| github.com/evanphx/json-patch/v5 v5.9.0 // indirect | ||
| github.com/fxamacker/cbor/v2 v2.7.0 // indirect | ||
| github.com/go-logr/logr v1.4.2 // indirect | ||
| github.com/go-openapi/jsonpointer v0.21.0 // indirect | ||
| github.com/go-openapi/jsonreference v0.20.2 // indirect | ||
| github.com/go-openapi/swag v0.23.0 // indirect | ||
| github.com/gofrs/uuid v4.3.1+incompatible // indirect | ||
| github.com/gogo/protobuf v1.3.2 // indirect | ||
| github.com/golang/protobuf v1.5.4 // indirect | ||
| github.com/google/gnostic-models v0.6.8 // indirect | ||
| github.com/google/go-cmp v0.6.0 // indirect | ||
| github.com/google/gofuzz v1.2.0 // indirect | ||
| github.com/google/uuid v1.6.0 // indirect | ||
| github.com/hashicorp/go-immutable-radix v1.3.1 // indirect | ||
| github.com/hashicorp/go-memdb v1.3.4 // indirect | ||
| github.com/hashicorp/golang-lru v0.5.4 // indirect | ||
| github.com/josharian/intern v1.0.0 // indirect | ||
| github.com/json-iterator/go v1.1.12 // indirect | ||
| github.com/mailru/easyjson v0.7.7 // indirect | ||
| github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect | ||
| github.com/modern-go/reflect2 v1.0.2 // indirect | ||
| github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect | ||
| github.com/onsi/ginkgo/v2 v2.22.1 // indirect | ||
| github.com/onsi/gomega v1.36.2 // indirect | ||
| github.com/pkg/errors v0.9.1 // indirect | ||
| github.com/spf13/pflag v1.0.5 // indirect | ||
| github.com/x448/float16 v0.8.4 // indirect | ||
| golang.org/x/net v0.33.0 // indirect | ||
| golang.org/x/oauth2 v0.23.0 // indirect | ||
| golang.org/x/sys v0.28.0 // indirect | ||
| golang.org/x/term v0.27.0 // indirect | ||
| golang.org/x/text v0.21.0 // indirect | ||
| golang.org/x/time v0.7.0 // indirect | ||
| google.golang.org/protobuf v1.36.1 // indirect | ||
| gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect | ||
| gopkg.in/inf.v0 v0.9.1 // indirect | ||
| gopkg.in/yaml.v3 v3.0.1 // indirect | ||
| k8s.io/klog/v2 v2.130.1 // indirect | ||
| k8s.io/kube-openapi v0.0.0-20241105132330-32ad38e42d3f // indirect | ||
| k8s.io/utils v0.0.0-20241104100929-3ea5e8cea738 // indirect | ||
| sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3 // indirect | ||
| sigs.k8s.io/structured-merge-diff/v4 v4.4.2 // indirect | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GO-2025-3488 golang.org/x/oauth2 vulnerability
💡 Result:
Summary: GO-2025-3488 (CVE-2025-22868) is a high-severity vulnerability in golang.org/x/oauth2 (package golang.org/x/oauth2/jws) where a maliciously malformed token can cause unexpected memory consumption during parsing, enabling potential DoS. It affects versions before v0.27.0 and was fixed in v0.27.0. Credited to jub0bs. References: pkg.go.dev vulnerability entry [1], GitHub Advisory DB entry [2], OSV record [3].
References
[1] https://pkg.go.dev/vuln/GO-2025-3488
[2] GHSA-6v2p-p543-phr9
[3] https://osv.dev/vulnerability/GO-2025-3488
🌐 Web query:
GHSA-6v2p-p543-phr9 golang.org/x/oauth2 oauth2
💡 Result:
Summary — GHSA-6v2p-p543-phr9 (golang.org/x/oauth2)
- ID / aliases: GHSA-6v2p-p543-phr9, CVE-2025-22868, GO-2025-3488. [1][2]
- Affected: golang.org/x/oauth2 versions before v0.27.0 (issue in jws.Verify). [1][3]
- Impact: malformed tokens can trigger unexpected memory consumption during parsing → potential DoS (CVSS 3.1 Base Score 7.5, High). [1][2]
- Fix / mitigation: upgrade to golang.org/x/oauth2 v0.27.0 or later. References: Go CL / issue and official advisories. [1][2][3]
Sources:
[1] GitHub Advisory GHSA-6v2p-p543-phr9 (golang.org/x/oauth2).
[2] OSV / GO-2025-3488 (CVE-2025-22868) entry.
[3] GitLab / NVD summary and go.dev references (vulnerability details & fix).
🏁 Script executed:
# Check golang.org/x/oauth2 versions and vulnerability information
# First, let's see what version information is available in the codebase
cd kubernetes/it && cat go.modRepository: wso2/api-platform
Length of output: 2653
Upgrade golang.org/x/oauth2 to v0.27.0 to address GO-2025-3488 (CVE-2025-22868).
A HIGH severity vulnerability exists in golang.org/x/oauth2 v0.23.0. The vulnerability (GO-2025-3488 / GHSA-6v2p-p543-phr9) is in the jws.Verify function where malformed tokens cause unexpected memory consumption during parsing, enabling potential DoS attacks. Upgrade to v0.27.0 or later.
🧰 Tools
🪛 OSV Scanner (2.3.1)
[HIGH] 1-1: golang.org/x/oauth2 0.23.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 1-1: golang.org/x/oauth2 0.23.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
🤖 Prompt for AI Agents
In kubernetes/it/go.mod lines 1-63 the project requires golang.org/x/oauth2 at
v0.23.0 which is vulnerable (GO-2025-3488/CVE-2025-22868); update the module
requirement to golang.org/x/oauth2 v0.27.0, then run `go get
golang.org/x/[email protected]` (or edit go.mod and run `go mod tidy`) to update
go.sum and module graph, re-run `go test ./...` and `go mod vendor` if you
vendor dependencies to ensure the fix is applied across the build.
| @@ -0,0 +1,63 @@ | |||
| module github.com/wso2/api-platform/kubernetes/it | |||
|
|
|||
| go 1.23.0 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which Go version requirement is correct
echo "=== Checking go.mod requirement ==="
rg -n "^go [0-9]" kubernetes/it/go.mod
echo ""
echo "=== Checking README prerequisite ==="
rg -n "Go [0-9]\." kubernetes/it/README.mdRepository: wso2/api-platform
Length of output: 192
Fix Go version inconsistency between go.mod and README.
The file specifies Go 1.23.0 but the README (line 9) states "Go 1.25+" as a prerequisite. Either update go.mod to 1.25.0 or update the README to reflect 1.23.0+.
🤖 Prompt for AI Agents
In kubernetes/it/go.mod around line 3, the go directive is set to 1.23.0 but the
README requires Go 1.25+, so update the go.mod go version to 1.25.0 (replace "go
1.23.0" with "go 1.25") to match the README, then run go mod tidy and verify the
project builds with Go 1.25; alternatively, if you prefer to support 1.23,
update the README prerequisite to 1.23+ instead—ensure both files agree.
| @echo "Building Mock JWKS..." | ||
| cd $(PROJECT_ROOT)/tests/mock-servers/mock-jwks && \ | ||
| docker build -t $(DOCKER_REGISTRY)/mock-jwks:$(IMAGE_TAG) . | ||
| @echo "All images built!" | ||
| cd $(PROJECT_ROOT)/tests/mock-servers/mock-jwks && \ | ||
| docker build -t $(DOCKER_REGISTRY)/mock-jwks:$(IMAGE_TAG) . | ||
| @echo "All images built!" |
There was a problem hiding this comment.
Duplicate mock-jwks build commands.
Lines 114-115 and 117-118 are identical, building the mock-jwks image twice unnecessarily.
🔎 Proposed fix to remove duplicate
@echo "Building Mock JWKS..."
cd $(PROJECT_ROOT)/tests/mock-servers/mock-jwks && \
docker build -t $(DOCKER_REGISTRY)/mock-jwks:$(IMAGE_TAG) .
@echo "All images built!"
- cd $(PROJECT_ROOT)/tests/mock-servers/mock-jwks && \
- docker build -t $(DOCKER_REGISTRY)/mock-jwks:$(IMAGE_TAG) .
- @echo "All images built!"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @echo "Building Mock JWKS..." | |
| cd $(PROJECT_ROOT)/tests/mock-servers/mock-jwks && \ | |
| docker build -t $(DOCKER_REGISTRY)/mock-jwks:$(IMAGE_TAG) . | |
| @echo "All images built!" | |
| cd $(PROJECT_ROOT)/tests/mock-servers/mock-jwks && \ | |
| docker build -t $(DOCKER_REGISTRY)/mock-jwks:$(IMAGE_TAG) . | |
| @echo "All images built!" | |
| @echo "Building Mock JWKS..." | |
| cd $(PROJECT_ROOT)/tests/mock-servers/mock-jwks && \ | |
| docker build -t $(DOCKER_REGISTRY)/mock-jwks:$(IMAGE_TAG) . | |
| @echo "All images built!" |
🤖 Prompt for AI Agents
In kubernetes/it/Makefile around lines 113 to 119 there is a duplicated
mock-jwks build sequence (the cd + docker build and the "All images built!" echo
appear twice); remove the redundant second block (lines 117-118 and its
duplicate echo) so the Makefile runs the mock-jwks docker build only once and
preserves the surrounding targets and indentation.
|
|
||
| Before running tests, ensure you have the following installed: | ||
|
|
||
| - **Go 1.25+** - For running the test suite |
There was a problem hiding this comment.
Fix Go version inconsistency with go.mod.
The README states "Go 1.25+" as a prerequisite, but kubernetes/it/go.mod specifies Go 1.23.0. This inconsistency will confuse contributors.
🔎 Proposed fix
-- **Go 1.25+** - For running the test suite
+- **Go 1.23+** - For running the test suite📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Go 1.25+** - For running the test suite | |
| - **Go 1.23+** - For running the test suite |
🤖 Prompt for AI Agents
In kubernetes/it/README.md around line 9 the Go version prerequisite ("Go
1.25+") conflicts with kubernetes/it/go.mod which declares Go 1.23.0; make them
consistent by either updating the README to state "Go 1.23+" to match go.mod, or
bump kubernetes/it/go.mod's `go` directive to 1.25 if you intend to require
features from 1.25—apply the chosen change, update any related docs/tests if
raising the version, and ensure the README and go.mod agree.
| // obtainToken gets a JWT token from a URL | ||
| func (h *HTTPSteps) obtainToken(url string) error { | ||
| resp, err := h.client.Get(url) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to obtain token: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read token response: %w", err) | ||
| } | ||
|
|
||
| h.Token = strings.TrimSpace(string(body)) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing HTTP status check in obtainToken.
The method doesn't verify the response status code before treating the body as a valid token. A 4xx/5xx response would result in storing an error message as the token.
🔎 Suggested fix
func (h *HTTPSteps) obtainToken(url string) error {
resp, err := h.client.Get(url)
if err != nil {
return fmt.Errorf("failed to obtain token: %w", err)
}
defer resp.Body.Close()
+ if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+ return fmt.Errorf("failed to obtain token: status %d", resp.StatusCode)
+ }
+
body, err := io.ReadAll(resp.Body)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // obtainToken gets a JWT token from a URL | |
| func (h *HTTPSteps) obtainToken(url string) error { | |
| resp, err := h.client.Get(url) | |
| if err != nil { | |
| return fmt.Errorf("failed to obtain token: %w", err) | |
| } | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return fmt.Errorf("failed to read token response: %w", err) | |
| } | |
| h.Token = strings.TrimSpace(string(body)) | |
| return nil | |
| } | |
| // obtainToken gets a JWT token from a URL | |
| func (h *HTTPSteps) obtainToken(url string) error { | |
| resp, err := h.client.Get(url) | |
| if err != nil { | |
| return fmt.Errorf("failed to obtain token: %w", err) | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | |
| return fmt.Errorf("failed to obtain token: status %d", resp.StatusCode) | |
| } | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return fmt.Errorf("failed to read token response: %w", err) | |
| } | |
| h.Token = strings.TrimSpace(string(body)) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In kubernetes/it/steps/http_steps.go around lines 164 to 179, the obtainToken
function currently reads the response body without verifying HTTP status, which
can store an error page as the token; update the function to check
resp.StatusCode and if it is not in the 200–299 range, read the body (safely)
and return a formatted error including the status code and body snippet,
otherwise proceed to read and trim the token as before; keep the existing defer
resp.Body.Close() and preserve error wrapping for IO errors.
| cmd := exec.Command("kubectl", "port-forward", | ||
| fmt.Sprintf("svc/%s", serviceName), | ||
| fmt.Sprintf("%d:8080", localPort), // Assume remote port is 8080 | ||
| "-n", namespace) |
There was a problem hiding this comment.
Remote port is hardcoded to 8080, limiting flexibility.
The port-forward always targets remote port 8080 (%d:8080), but services like the OCI registry use port 5000, and other services may use different ports. This will cause failures when forwarding to non-8080 services.
🔎 Proposed fix to accept remote port as parameter
-// portForwardService starts port forwarding to a service
-func (k *K8sSteps) portForwardService(serviceName, namespace string, localPort int) error {
+// portForwardService starts port forwarding to a service (assumes remote port 8080)
+// For services with different ports, use portForwardServiceWithRemotePort
+func (k *K8sSteps) portForwardService(serviceName, namespace string, localPort int) error {
+ return k.portForwardServiceWithRemotePort(serviceName, namespace, localPort, 8080)
+}
+
+// portForwardServiceWithRemotePort starts port forwarding with specified remote port
+func (k *K8sSteps) portForwardServiceWithRemotePort(serviceName, namespace string, localPort, remotePort int) error {
key := fmt.Sprintf("%s/%s:%d", namespace, serviceName, localPort)
// Check if already running for this exact service
if _, exists := k.portForwards[key]; exists {
return nil
}
// Kill any existing port-forward on this local port (from previous runs)
killCmd := exec.Command("sh", "-c", fmt.Sprintf("lsof -ti tcp:%d | xargs -r kill 2>/dev/null || true", localPort))
killCmd.Run()
// Small delay to let port be released
time.Sleep(500 * time.Millisecond)
cmd := exec.Command("kubectl", "port-forward",
fmt.Sprintf("svc/%s", serviceName),
- fmt.Sprintf("%d:8080", localPort), // Assume remote port is 8080
+ fmt.Sprintf("%d:%d", localPort, remotePort),
"-n", namespace)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In kubernetes/it/steps/k8s_steps.go around lines 458 to 461 the kubectl
port-forward command hardcodes the remote port to 8080 ("%d:8080"), which breaks
forwarding for services using other ports (e.g., 5000); change the function to
accept a remotePort parameter (or add an optional arg/default) and build the
exec.Command using fmt.Sprintf("%d:%d", localPort, remotePort) so the remote
port is configurable when calling this step.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/operator-integration-test.yml (1)
244-305: Debug section has incomplete resource coverage.The debug section correctly references the main resources from the primary test scenarios (
test-gateway,scoped-gateway,test-api,scoped-api), but it misses resources created by other test features such ascycle-gateway/cycle-api(full_lifecycle),jwt-api(jwt_authentication), andmulti-ns-api(multi_namespace). This limits debugging visibility when those tests fail.Consider adding either:
- Debug commands for all resource names used across all test features
- Generic label-based queries to capture all created resources regardless of name
- A command to list all Gateway and RestApi resources across all namespaces
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/operator-integration-test.yml
🔇 Additional comments (2)
.github/workflows/operator-integration-test.yml (2)
237-242: LGTM! Excellent addition for test result archiving.Using
if: always()ensures test results are captured and uploaded even when tests fail, which is essential for debugging CI failures. The artifact path correctly matches the output file created in line 227.
223-236: Clean refactoring to Cucumber-based tests.The streamlined approach using Go-based Cucumber tests is a significant improvement over inline Helm commands. The
kubernetes/it/cmddirectory structure is properly set up with a valid main package, and all six environment variables are correctly utilized in the test steps with appropriate defaults. The use ofteeto capture output for artifact upload is good practice.
1 participant
Purpose
Summary by CodeRabbit
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.