Tag based gateway deployment #572
Conversation
WalkthroughAdds per-scenario gateway configuration profiles and runtime controls: a ConfigProfile registry and GatewayConfigManager to switch profiles via scenario tags, ComposeManager methods to restart/health-check gateway components, OpenTelemetry collector integration, BDD tracing steps, and a tracing feature to assert traces in collector logs. Changes
Sequence DiagramsequenceDiagram
actor TestSuite as Test Suite
participant GCM as GatewayConfigManager
participant CM as ComposeManager
participant GC as Gateway Controller
participant OC as OTel Collector
TestSuite->>GCM: EnsureConfig(ctx, scenario)
activate GCM
GCM->>GCM: extractConfigTag -> profile name
GCM->>GCM: registry.Get(profile)
alt profile found & differs
GCM->>CM: RestartGatewayController(ctx, profile.EnvVars)
activate CM
CM->>GC: stop & remove container(s)
CM->>GC: start container(s) with env vars
CM->>CM: WaitForGatewayControllerHealthy(ctx)
CM->>CM: WaitForRouterReady(ctx)
GC-->>CM: healthy / ready
deactivate CM
GCM->>GCM: set currentProfile
else already on profile
GCM-->>TestSuite: no-op
else profile not found
GCM-->>TestSuite: error
end
deactivate GCM
TestSuite->>GC: execute scenario requests
GC->>OC: export trace via OTLP
OC->>OC: receive & log trace
TestSuite->>CM: CheckLogsForText(ctx,"it-otel-collector", text)
activate CM
CM->>OC: fetch container logs (poll)
OC-->>CM: logs contain trace text
CM-->>TestSuite: found -> pass
deactivate CM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
gateway/it/docker-compose.test.yaml (1)
64-75: Consider adding a healthcheck for the otel-collector service.The
otel-collectorservice lacks a healthcheck, which could lead to timing issues if tests try to verify traces before the collector is ready. The OTEL collector exposes health endpoints that can be used.🔎 Suggested healthcheck configuration
otel-collector: image: otel/opentelemetry-collector-contrib:0.88.0 container_name: it-otel-collector command: ["--config=/etc/otel-collector-config.yaml"] volumes: - ./otel-config.yaml:/etc/otel-collector-config.yaml:ro ports: - "4317:4317" # OTLP gRPC receiver - "4318:4318" # OTLP HTTP receiver + healthcheck: + test: ["CMD", "wget", "--quiet", "--tries=1", "-O", "-", "http://localhost:13133/"] + interval: 5s + timeout: 3s + retries: 5 + start_period: 5s networks: - it-gateway-networkNote: The health extension needs to be enabled in
otel-config.yaml:extensions: health_check: service: extensions: [health_check] pipelines: # ...existing configgateway/it/tracing_steps.go (1)
32-46: Consider checking logs immediately before first ticker interval.The current implementation waits 1 second before the first log check. If traces are already present, this adds unnecessary delay. Checking immediately on entry would improve responsiveness.
🔎 Suggested improvement
func verifyTraceInLogs(cm *ComposeManager, text string) error { // Retry for a few seconds as logs might be delayed ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() ticker := time.NewTicker(1 * time.Second) defer ticker.Stop() + // Check immediately first + if found, err := cm.CheckLogsForText(ctx, "it-otel-collector", text); err == nil && found { + return nil + } + for { select { case <-ctx.Done(): return fmt.Errorf("timed out waiting for trace logs containing '%s'", text) case <-ticker.C: found, err := cm.CheckLogsForText(ctx, "it-otel-collector", text) if err != nil { - // Don't fail immediately on log retrieval error (container might be starting?) continue } if found { return nil } } } }gateway/it/config_profiles.go (2)
10-14: ThedefaultProfilefield appears unused.The
defaultProfilefield is set to"default"but there's no method to retrieve or use it. Consider adding aGetDefault()method or removing the field if not needed.🔎 Suggested addition
// GetDefault returns the default profile func (r *ConfigProfileRegistry) GetDefault() (*ConfigProfile, bool) { return r.Get(r.defaultProfile) }
65-68: Consider adding nil check in Register method.If a nil profile is passed, accessing
profile.Namewill cause a panic. While this may be internal code, a defensive check could prevent debugging headaches.🔎 Suggested fix
// Register adds a profile to the registry func (r *ConfigProfileRegistry) Register(profile *ConfigProfile) { + if profile == nil { + return + } r.profiles[profile.Name] = profile }gateway/it/config_manager.go (1)
63-71: Consider validating for multiple config tags.The current implementation returns the first matching config tag when multiple are present. While this works, it could lead to confusion if test authors accidentally apply multiple config tags to a scenario.
Consider either:
- Returning an error if multiple config tags are detected
- Adding a comment documenting the "first match wins" behavior
🔎 Proposed enhancement to detect multiple config tags
// extractConfigTag finds the first tag starting with @config- and returns the suffix func (m *GatewayConfigManager) extractConfigTag(sc *godog.Scenario) string { + var found string for _, tag := range sc.Tags { if strings.HasPrefix(tag.Name, ConfigTagPrefix) { - return strings.TrimPrefix(tag.Name, ConfigTagPrefix) + if found != "" { + log.Printf("Warning: Multiple config tags found in scenario '%s'. Using first: %s", sc.Name, found) + return found + } + found = strings.TrimPrefix(tag.Name, ConfigTagPrefix) } } - return "" + return found }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
gateway/it/config_manager.gogateway/it/config_profiles.gogateway/it/docker-compose.test.yamlgateway/it/features/tracing.featuregateway/it/otel-config.yamlgateway/it/setup.gogateway/it/suite_test.gogateway/it/tracing_steps.go
🧰 Additional context used
🧬 Code graph analysis (3)
gateway/it/tracing_steps.go (1)
gateway/it/setup.go (1)
ComposeManager(68-78)
gateway/it/suite_test.go (2)
gateway/it/config_manager.go (2)
GatewayConfigManager(16-21)NewGatewayConfigManager(24-30)gateway/it/tracing_steps.go (1)
RegisterTracingSteps(12-22)
gateway/it/config_manager.go (2)
gateway/it/config_profiles.go (2)
ConfigProfileRegistry(11-14)NewConfigProfileRegistry(17-63)gateway/it/setup.go (1)
ComposeManager(68-78)
🔇 Additional comments (15)
gateway/it/features/tracing.feature (1)
1-12: Feature file structure looks good.The BDD scenario is well-defined with clear intent. The
@config-tracingtag correctly triggers the tracing profile configuration before the scenario runs.gateway/it/tracing_steps.go (1)
11-22: LGTM!Clean step registration. The no-op step with explanatory comment is a reasonable pattern when the actual setup is handled via scenario tags.
gateway/it/config_profiles.go (1)
51-59: Tracing profile configuration looks correct.The tracing profile sets
GATEWAY_TRACING_ENABLED=truewhich will be applied when scenarios with@config-tracingtag run.gateway/it/suite_test.go (4)
92-93: Setting env var at runtime may not affect already-started containers.
os.Setenv("TESTCONTAINERS_RYUK_DISABLED", "true")is set inBeforeSuite, which runs after the test process starts. For testcontainers to pick this up, it should be set before any testcontainers code executes. The current placement inBeforeSuitebeforeNewComposeManagershould work, but consider moving toinit()for guaranteed early execution.
172-180: Configuration switch happens per-scenario; verify gateway restart timing.The
EnsureConfigcall before each scenario will restart the gateway-controller if the config profile changes. This is a potentially slow operation (30s health timeout perWaitForGatewayControllerHealthy). Ensure scenarios with the same config tag are grouped to minimize restarts.
215-218: Tracing steps registration is correctly conditional.The guard
if composeManager != nilensures tracing steps are only registered when compose infrastructure is available.
56-63: Godog options initialization follows standard patterns.Using
BindCommandLineFlagsininit()allows test customization via command-line flags.gateway/it/setup.go (3)
213-242: Health check implementation is correct.The polling pattern with context timeout, ticker, and proper cleanup is well-implemented. The 30-second timeout is reasonable for container startup.
344-358: StreamLogs implementation is clean.Running in a goroutine with proper context cancellation handling. The error check for
cm.ctx.Err() == nilcorrectly suppresses expected errors during shutdown.
360-372: CheckLogsForText implementation is straightforward and effective.Simple log retrieval with string search. Works well for the trace verification use case.
gateway/it/config_manager.go (5)
1-11: LGTM!The package declaration and imports are appropriate for the file's functionality.
13-13: LGTM!The constant is well-named and follows Godog tag conventions.
15-21: LGTM!The struct is well-designed with appropriate fields for configuration management and includes a mutex for thread safety.
32-61: Code logic is sound; method signature verification recommended but not blocking.The mutex protection, early return optimization, and error handling are all correctly implemented. Signature appears correct based on usage context (
ctxandprofile.EnvVarsmapping), but final confirmation ofRestartGatewayController(ctx context.Context, envVars map[string]string) errorwould be ideal to ensure env var updates are properly handled.
24-29: No action needed. The gateway initializes with environment variables (GATEWAY_STORAGE_TYPE=sqlite,GATEWAY_LOGGING_LEVEL=info) that exactly match the "default" profile definition. The assumption is valid and will not cause test flakiness.Likely an incorrect or invalid review comment.
gateway/it/features/tracing.feature
Outdated
| Scenario: API invocation generates a trace | ||
| Given the Gateway is running with tracing enabled | ||
| And I have a valid API Key for the "Sales API" | ||
| When I send a GET request to "http://localhost:9090/api/v1/sales/orders" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine docker-compose.test.yaml
find . -name "docker-compose.test.yaml" -o -name "docker-compose*.yaml" | head -20Repository: wso2/api-platform
Length of output: 265
🏁 Script executed:
#!/bin/bash
# Find the tracing.feature file and examine it
find . -name "tracing.feature" -type f | head -20Repository: wso2/api-platform
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Search for port definitions in key files
rg -n "9090|8080" --max-count=20Repository: wso2/api-platform
Length of output: 50375
🏁 Script executed:
#!/bin/bash
# Read docker-compose.test.yaml to check port mappings
cat gateway/it/docker-compose.test.yaml | grep -A5 -B5 "ports:"Repository: wso2/api-platform
Length of output: 1616
🏁 Script executed:
#!/bin/bash
# Read tracing.feature to see line 10 and surrounding context
head -20 gateway/it/features/tracing.feature | cat -nRepository: wso2/api-platform
Length of output: 657
🏁 Script executed:
#!/bin/bash
# Look for port constants in setup.go or similar
find gateway -name "setup.go" -o -name "config*.go" | head -5Repository: wso2/api-platform
Length of output: 235
🏁 Script executed:
#!/bin/bash
# Check setup.go to understand how the test harness is configured
cat gateway/it/setup.go | grep -A5 -B5 "9090\|8080" | head -50Repository: wso2/api-platform
Length of output: 795
Port 9090 is for the gateway-controller REST API, not for API requests.
The request should use http://localhost:8080/api/v1/sales/orders instead. The router service exposes HTTP traffic on port 8080, while port 9090 is reserved for the gateway-controller management API.
🤖 Prompt for AI Agents
gateway/it/features/tracing.feature around line 10: the test sends the GET
request to http://localhost:9090/api/v1/sales/orders but port 9090 is for the
gateway-controller management API; update the request URL to use port 8080
(http://localhost:8080/api/v1/sales/orders) so the test targets the router
service HTTP endpoint.
| exporters: | ||
| logging: | ||
| loglevel: debug |
There was a problem hiding this comment.
Replace deprecated loglevel with verbosity configuration.
The loglevel configuration key is deprecated in favor of verbosity. Update the logging exporter configuration to use verbosity: detailed instead of loglevel: debug.
🤖 Prompt for AI Agents
In gateway/it/otel-config.yaml around lines 7 to 9, the logging exporter uses
the deprecated key `loglevel: debug`; replace that key with `verbosity:
detailed` so the exporter configuration reads the new `verbosity` setting
(ensure indentation and YAML formatting remain unchanged and no other keys are
modified).
gateway/it/setup.go
Outdated
| // Start with new env vars | ||
| args := []string{"compose", "-f", cm.composeFile, "-p", cm.projectName, "up", "-d", "gateway-controller"} | ||
| cmd := execCommandContext(ctx, "docker", args...) | ||
|
|
||
| // Copy existing env and append new ones | ||
| cmd.Env = os.Environ() | ||
| for k, v := range envVars { | ||
| cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", k, v)) | ||
| log.Printf("Setting env: %s=%s", k, v) | ||
| } | ||
|
|
||
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to start gateway-controller: %w\nOutput: %s", err, string(output)) | ||
| } |
There was a problem hiding this comment.
Environment variables set via cmd.Env will not propagate to the container.
Setting environment variables on the docker compose up command only affects the Docker CLI process, not the container itself. Docker Compose reads environment variables for the container from the compose file's environment section or from .env files.
To dynamically set container environment variables, you need to either:
- Use
docker compose runwith-eflags - Modify the compose file dynamically
- Use an
.envfile that gets updated - Use
docker rundirectly with-eflags
🔎 Possible fix using docker run
One approach is to use docker run directly instead of docker compose up:
// Build args with environment variables
args := []string{"run", "-d", "--name", "it-gateway-controller",
"--network", "gateway-it_it-gateway-network",
// Add other necessary mounts and ports from compose file
}
for k, v := range envVars {
args = append(args, "-e", fmt.Sprintf("%s=%s", k, v))
}
args = append(args, "ghcr.io/wso2/api-platform/gateway-controller-coverage:0.1.0-SNAPSHOT")
cmd := execCommandContext(ctx, "docker", args...)However, this requires duplicating the container configuration. A better approach might be to use a templated compose file or environment file.
🤖 Prompt for AI Agents
In gateway/it/setup.go around lines 193-207, environment variables appended to
cmd.Env won't be passed into containers by "docker compose up"; instead create a
temporary .env file containing each "KEY=VALUE" from envVars, update the docker
compose command args to include "--env-file", "-f", cm.composeFile, "-p",
cm.projectName, "up", "-d", "gateway-controller" (removing the manual cmd.Env
population), write the envVars to that file before running the command, ensure
the file is closed and removed afterward (or reused appropriately), and keep
cmd.Env minimal (e.g., os.Environ()) so the Docker CLI inherits normal
environment.
| EnvoyAdminPort, // 9901 | ||
| "9002", // Policy engine | ||
| "5000", // Sample backend | ||
| "5050", // Sample backend |
There was a problem hiding this comment.
Port check mismatch with docker-compose.test.yaml.
The port check uses 5050 but docker-compose.test.yaml shows sample-backend on port 5000:5000. This inconsistency will cause false port conflict detection.
🔎 Suggested fix
- "5050", // Sample backend
+ "5000", // Sample backend📝 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.
| "5050", // Sample backend | |
| "5000", // Sample backend |
🤖 Prompt for AI Agents
In gateway/it/setup.go around line 396, the hard-coded sample backend port is
set to "5050" but docker-compose.test.yaml maps the sample-backend to 5000:5000;
update the port check to use "5000" (or better, derive the expected port from
the docker-compose/test config or an environment variable) so the test's port
conflict detection matches docker-compose.test.yaml.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
gateway/it/setup.go (2)
396-396: Port check mismatch with docker-compose.test.yaml.The sample backend port is set to
"5050", but according to previous review comments,docker-compose.test.yamlmaps the sample-backend to5000:5000. This inconsistency will cause false port conflict detection.🔎 Suggested fix
- "5050", // Sample backend + "5000", // Sample backendBased on past review comments, this issue was previously identified but remains unresolved.
193-207: Environment variables won't propagate to the container.Setting
cmd.Envonly affects thedocker composeCLI process environment, not the container's runtime environment. Docker Compose reads container environment variables from the compose file'senvironmentsection,.envfiles, or variable substitution (${VAR}).Since the purpose of this method is to dynamically restart the gateway-controller with profile-specific environment variables, the current implementation will fail to apply those variables.
🔎 Recommended fix using temporary .env file
// Start with new env vars +// Create temporary .env file for docker compose +envFile, err := os.CreateTemp("", "gateway-it-*.env") +if err != nil { + return fmt.Errorf("failed to create temp env file: %w", err) +} +defer os.Remove(envFile.Name()) +defer envFile.Close() + +for k, v := range envVars { + if _, err := fmt.Fprintf(envFile, "%s=%s\n", k, v); err != nil { + return fmt.Errorf("failed to write env file: %w", err) + } + log.Printf("Setting env: %s=%s", k, v) +} +if err := envFile.Close(); err != nil { + return fmt.Errorf("failed to close env file: %w", err) +} + -args := []string{"compose", "-f", cm.composeFile, "-p", cm.projectName, "up", "-d", "gateway-controller"} +args := []string{"compose", "-f", cm.composeFile, "--env-file", envFile.Name(), "-p", cm.projectName, "up", "-d", "gateway-controller"} cmd := execCommandContext(ctx, "docker", args...) - -// Copy existing env and append new ones -cmd.Env = os.Environ() -for k, v := range envVars { - cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", k, v)) - log.Printf("Setting env: %s=%s", k, v) -} output, err := cmd.CombinedOutput()Note: This approach requires the compose file's
environmentsection to reference the variables (e.g.,MY_VAR: ${MY_VAR}), or useenv_filedirective. Verify the compose file is configured accordingly.Based on past review comments, this issue was previously identified but remains unresolved.
🧹 Nitpick comments (1)
gateway/it/setup.go (1)
344-358: Consider logging successful streaming start.The implementation correctly handles context cancellation and error suppression during shutdown. For enhanced observability, consider logging when streaming successfully starts, not just when it stops.
🔎 Suggested enhancement
func (cm *ComposeManager) StreamLogs() { go func() { log.Println("Streaming logs from containers...") cmd := execCommandContext(cm.ctx, "docker", "compose", "-f", cm.composeFile, "-p", cm.projectName, "logs", "-f") cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr + log.Println("Log streaming started successfully") if err := cmd.Run(); err != nil { // Don't log error on context cancellation (standard shutdown) if cm.ctx.Err() == nil { log.Printf("Background log streaming stopped: %v", err) } } }() }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gateway/it/setup.go
🔇 Additional comments (5)
gateway/it/setup.go (5)
31-31: LGTM!The
stringsimport is appropriately added to support thestrings.Containscall in the newCheckLogsForTextmethod.
143-144: LGTM!Starting log streaming immediately after bringing up services provides valuable observability for debugging test failures.
213-242: LGTM!The health check implementation correctly uses a ticker-based polling loop with context timeout and proper resource cleanup.
335-342: LGTM!The cleanup method properly uses
RemoveOrphansandRemoveImagesLocaloptions with an appropriate timeout to ensure thorough resource cleanup.
360-372: LGTM!The log inspection method correctly retrieves container logs and performs text matching. Note that
strings.Containsis case-sensitive, which should be acceptable for trace verification use cases.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
gateway/it/setup.go (1)
513-513: Port check now correctly uses 5050.This aligns with the updated
docker-compose.test.yamlsample-backend port mapping. The earlier review concern is resolved.
🧹 Nitpick comments (4)
gateway/it/docker-compose.test.yaml (1)
67-77: Consider adding a healthcheck forotel-collector.The otel-collector service lacks a healthcheck, which could cause flaky tests if dependent services send traces before the collector is ready. The collector exposes a health endpoint that can be used.
🔎 Suggested healthcheck configuration
otel-collector: image: otel/opentelemetry-collector-contrib:0.88.0 container_name: it-otel-collector command: ["--config=/etc/otel-collector-config.yaml"] volumes: - ./otel-config.yaml:/etc/otel-collector-config.yaml:ro ports: - "4317:4317" # OTLP gRPC receiver - "4318:4318" # OTLP HTTP receiver + healthcheck: + test: ["CMD", "wget", "--quiet", "--tries=1", "--spider", "http://localhost:13133/"] + interval: 5s + timeout: 3s + retries: 5 + start_period: 5s networks: - it-gateway-networkNote: This requires enabling the
health_checkextension inotel-config.yaml.gateway/it/configs/config.yaml (2)
371-384: Consider using placeholder values for KeyManager configurations.The JWKS and issuer URLs reference real Asgardeo tenant names (
tharsanan1995,testorgforsecurity). While these are public OAuth endpoints, using real tenant identifiers in test configurations could:
- Cause test failures if these tenants are modified/deleted
- Expose organization identifiers in the codebase
Consider using placeholder URLs or mock endpoints for test configurations.
316-317: Placeholder token needs clear documentation.The
<MOESIF_APPLICATION_ID>placeholder is good, but consider adding a comment indicating this must be replaced or that analytics will fail gracefully when unconfigured.gateway/it/setup.go (1)
263-265: Consider making xDS propagation delay configurable.The hardcoded 3-second sleep is a magic number. Consider extracting this as a constant or making it configurable for environments with different performance characteristics.
🔎 Suggested refactor
+const ( + // XDSPropagationDelay is the time to wait for xDS configuration to propagate + XDSPropagationDelay = 3 * time.Second +) // Additional wait for xDS configuration to propagate log.Println("Waiting for xDS configuration to propagate...") -time.Sleep(3 * time.Second) +time.Sleep(XDSPropagationDelay)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
gateway/it/Makefilegateway/it/configs/config.yamlgateway/it/docker-compose.test.yamlgateway/it/features/tracing.featuregateway/it/setup.gogateway/it/state.gogateway/it/suite_test.gogateway/it/tracing_steps.go
🚧 Files skipped from review as they are similar to previous changes (3)
- gateway/it/features/tracing.feature
- gateway/it/tracing_steps.go
- gateway/it/suite_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:15.812Z
Learnt from: RakhithaRR
Repo: wso2/api-platform PR: 504
File: gateway/policies/mcp-auth/v0.1.0/policy-definition.yaml:118-191
Timestamp: 2025-12-18T08:49:15.812Z
Learning: The policy engine in the gateway validates configuration paths (e.g., `${config.policy_configurations.jwtauth_v010.*}`) in a case-insensitive manner, so references using lowercase (jwtauth_v010) will correctly match configuration keys using mixed case (JWTAuth_v010) in the Helm values or config files.
Applied to files:
gateway/it/configs/config.yaml
📚 Learning: 2025-12-19T06:57:38.504Z
Learnt from: tharindu1st
Repo: wso2/api-platform PR: 514
File: gateway/gateway-controller/config/config.yaml:0-0
Timestamp: 2025-12-19T06:57:38.504Z
Learning: In gateway/gateway-controller/config/config.yaml, the default admin user with plaintext password "admin" is intentionally provided for testing purposes only and must be changed to secure hashed passwords for production deployments.
Applied to files:
gateway/it/configs/config.yaml
🔇 Additional comments (12)
gateway/it/state.go (1)
50-50: LGTM!The
SampleBackendURLport update to5050correctly aligns with thedocker-compose.test.yamlport mapping change (5050:5000).gateway/it/docker-compose.test.yaml (3)
27-34: Port and tracing configuration looks correct.The gateway-controller port mapping change to
9099:9090aligns with theGatewayControllerPortconstant insetup.go. TheGATEWAY_TRACING_ENABLEDenvironment variable with a default offalsefollows a good pattern for optional feature toggling.
57-58: LGTM!The
GATEWAY_TRACING_ENABLEDenvironment variable is correctly propagated to the policy-engine service with the same default value pattern.
106-106: Port mapping change is consistent.The sample-backend port mapping update to
5050:5000correctly aligns with theSampleBackendURLinstate.go.gateway/it/configs/config.yaml (2)
203-206: Plain text admin credentials are acceptable for testing.Based on learnings, the default admin user with plaintext password "admin" is intentionally provided for testing purposes only. This should not be used in production deployments.
252-254: Overly permissiveallowed_ipsconfiguration.The
allowed_ipslist includes"*"which permits access from any IP. While acceptable for local integration tests, ensure this doesn't accidentally propagate to production configurations.gateway/it/setup.go (6)
50-50: Port constant correctly updated.
GatewayControllerPortnow matches thedocker-compose.test.yamlmapping (9099:9090).
95-111: Good use of explicit project name for network consistency.Using
tc.StackIdentifier("gateway-it")ensures all containers share the same network, which is essential forRestartGatewayControllerto reconnect services properly.
213-224: Environment variable propagation is correct for compose variable substitution.Setting
cmd.Envwith the environment variables works correctly here becausedocker composereads shell-style variable substitutions (like${GATEWAY_TRACING_ENABLED:-false}) from its parent process environment. This addresses the earlier review concern — the pattern is valid for variable substitution in the compose file.
271-332: Health check methods follow good patterns.Both
WaitForGatewayControllerHealthyandWaitForRouterReadyproperly use context timeouts, tickers, and resource cleanup. The 30-second timeout and 500ms polling interval are reasonable defaults.Minor observation: These methods share similar structure and could be consolidated into a generic helper, but the current implementation is clear and maintainable.
425-447: Enhanced cleanup handles edge cases well.The dual cleanup approach (testcontainers-go down + direct docker compose down) ensures containers restarted via
RestartGatewayControllerare properly cleaned up. Explicit volume removal is a good defensive measure.
467-478: Log inspection methods are straightforward and correct.
CheckLogsForTextproperly handles the container name (using explicitcontainer_namefrom compose) and returns useful error context.
| # Uses script to force unbuffered output on macOS | ||
| test: | ||
| go test -v ./... | ||
| script -q /dev/null go test -v -count=1 ./... |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Makefile" -path "*/gateway/it/*" -type f | head -5Repository: wso2/api-platform
Length of output: 82
🏁 Script executed:
if [ -f "gateway/it/Makefile" ]; then
cat -n "gateway/it/Makefile" | head -50
else
find . -name "Makefile" | grep -E "(gateway|it)" | head -10
fiRepository: wso2/api-platform
Length of output: 2154
🏁 Script executed:
ls -la gateway/it/ 2>/dev/null || find . -type d -name "it" | head -5Repository: wso2/api-platform
Length of output: 1571
🏁 Script executed:
find . -path "*/gateway/it/Makefile" -type f 2>/dev/null && cat -n ./gateway/it/Makefile | sed -n '20,40p'Repository: wso2/api-platform
Length of output: 790
Portability issue: script -q /dev/null syntax is macOS/BSD-specific and will fail on Linux.
The test target uses BSD-style script -q /dev/null syntax, which is incompatible with GNU script on Linux systems. This breaks cross-platform development and CI/CD pipelines running on Linux.
Additionally, the test-verbose target (line 36) runs tests without the script wrapper, creating inconsistent behavior between the two targets.
The -count=1 flag is correct for disabling test caching in integration tests. However, go test -v already provides line-buffered streaming output; the script wrapper may be unnecessary. Consider either:
- Removing the wrapper and relying on
go test -voutput buffering - Using OS detection to apply the wrapper only on macOS (if unbuffered output is critical)
🤖 Prompt for AI Agents
In gateway/it/Makefile lines 23-25 the test target uses macOS/BSD-specific
`script -q /dev/null` which fails on Linux; remove the script wrapper and run
`go test -v -count=1 ./...` directly (or add OS detection to apply the wrapper
only on macOS if truly required), and make the test-verbose target consistent
with the change so both targets behave the same across platforms.
1 participant
Purpose
Summary by CodeRabbit
New Features
Tests / Reliability
Chores
✏️ Tip: You can customize this high-level summary in your review settings.