feat: add per-proxy production integration tests#715
Open
steveiliop56 wants to merge 9 commits intomainfrom
Open
feat: add per-proxy production integration tests#715steveiliop56 wants to merge 9 commits intomainfrom
steveiliop56 wants to merge 9 commits intomainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a full integration test harness and CI workflow to run Tinyauth end-to-end tests across Traefik, Nginx, and Envoy using docker-compose, with test orchestration, proxy configs, env files, and conditional CI build/push steps. No public API changes. Changes
Sequence DiagramsequenceDiagram
actor TestRunner as Test Runner
participant Docker as Docker Compose
participant Proxy as Proxy (Traefik/Nginx/Envoy)
participant Tinyauth as Tinyauth Service
participant Whoami as Whoami Service
participant Client as HTTP Client
TestRunner->>Docker: Start docker-compose up (--build, env file)
activate Docker
Docker->>Proxy: Launch proxy service
Docker->>Whoami: Launch whoami service
Docker->>Tinyauth: Launch tinyauth service
TestRunner->>Tinyauth: Poll /health (retry up to 5 min)
loop Health Check
Tinyauth-->>TestRunner: Health status
end
Note over TestRunner: Services ready
TestRunner->>Client: Test 1: Unauthorized
Client->>Proxy: GET /whoami (no auth)
Proxy->>Tinyauth: ext_authz / auth_request
Tinyauth-->>Proxy: 401
Proxy-->>Client: 401 / login page
Client-->>TestRunner: result
TestRunner->>Client: Test 2: Logged In
Client->>Proxy: GET /whoami (BasicAuth)
Proxy->>Tinyauth: ext_authz
Tinyauth-->>Proxy: 200
Proxy->>Whoami: forward request
Whoami-->>Proxy: 200
Proxy-->>Client: 200
Client-->>TestRunner: result
TestRunner->>Client: Test 3: ACL Allowed
Client->>Proxy: GET /whoami/allow
Proxy->>Tinyauth: ext_authz
Tinyauth-->>Proxy: 200 (ACL match)
Proxy->>Whoami: forward request
Whoami-->>Proxy: 200
Proxy-->>Client: 200
Client-->>TestRunner: result
TestRunner->>Docker: Send SIGINT & cleanup
deactivate Docker
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #715 +/- ##
==========================================
- Coverage 16.85% 16.29% -0.57%
==========================================
Files 50 52 +2
Lines 3820 3953 +133
==========================================
Hits 644 644
- Misses 3112 3245 +133
Partials 64 64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 5
🧹 Nitpick comments (2)
integration/docker-compose.traefik.yml (1)
10-11: Avoid direct Docker socket exposure where possible.Line [11] mounts
/var/run/docker.sockdirectly into Traefik, which grants broad Docker API access. Consider using a Docker socket proxy (or another constrained discovery approach) to reduce risk in CI/integration environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/docker-compose.traefik.yml` around lines 10 - 11, The docker-compose currently mounts /var/run/docker.sock directly into the Traefik service via the volumes entry, which exposes the Docker API; replace this by running a Docker socket proxy (e.g., docker-socket-proxy) as a separate service and point Traefik to the proxy socket/endpoint instead of /var/run/docker.sock, or configure Traefik to use a constrained discovery provider (TCP with TLS) so the volumes entry no longer mounts /var/run/docker.sock; update the Traefik service's volumes and provider endpoint references accordingly and ensure the proxy service exposes only the minimal API scopes needed.integration/.env (1)
5-16: Pin Docker image versions to ensure reproducible CI integration tests.Lines 5, 13, and 16 use floating tags (
latest,*-latest), which allow images to change unpredictably between runs and make test failures difficult to reproduce. Pin to exact version tags instead:
WHOAMI_VERSION=v1.11(or check traefik/whoami releases for latest patch)ENVOY_VERSION=v1.33.5(use exact patch tag;v1.33-latestfloats across 1.33.x releases)TINYAUTH_VERSION=v3orv4(per tinyauth documentation)Optionally pin to image digests (
@sha256:...) for maximum immutability in CI pipelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/.env` around lines 5 - 16, The .env uses floating Docker tags causing non-reproducible CI; update the WHOAMI_VERSION, ENVOY_VERSION, and TINYAUTH_VERSION variables to pinned exact tags (e.g., WHOAMI_VERSION=v1.11, ENVOY_VERSION=v1.33.5, and TINYAUTH_VERSION=v3 or v4) or preferably to image digests to lock versions; locate and replace the values for the WHOAMI_VERSION, ENVOY_VERSION, and TINYAUTH_VERSION entries in the integration/.env file so CI uses fixed immutable images.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/integration.yml:
- Around line 1-10: The workflow "Proxy Integration Tests" triggered by
workflow_dispatch currently uses default token permissions; add an explicit
top-level permissions block to grant only the minimal GITHUB_TOKEN scopes needed
(e.g., permissions: contents: read or whatever specific scopes your tests
require) so the workflow no longer inherits full default permissions. Update the
YAML near the top-level (alongside name: Proxy Integration Tests and
workflow_dispatch) to include a permissions section that enumerates only
required scopes for the jobs run in this file, and apply the same change to the
other similar workflow blocks referenced in the review.
In `@integration/integrarion_tests.go`:
- Line 26: Fix the typo in the assertion error message inside the fmt.Errorf
call that currently reads "or to to contain" — update the string to "or to
contain" so the error message is clean; locate the fmt.Errorf call in
integrarion_tests.go (the line that constructs the error about expected status
code http.StatusUnauthorized and presence of '<div id=\"root\"></div>') and
correct the duplicated "to".
In `@integration/integration.go`:
- Around line 130-132: The HTTP client created as client := &http.Client{} in
integration.go lacks a timeout and the health-check loop leaks resources: modify
the client instantiation used by runTests (and any other HTTP clients in this
file) to set a sensible Timeout (e.g., http.Client{Timeout: ...}) and/or
configure Transport timeouts; in the health-check loop ensure the ticker started
for retries is stopped (call ticker.Stop() when exiting the loop) and always
close the HTTP response body (defer resp.Body.Close() or explicitly close
resp.Body after each request) to prevent connection leaks.
- Around line 147-150: The setupCmdLogging function currently ignores errors
from cmd.StdoutPipe() and cmd.StderrPipe(); change setupCmdLogging to return an
error (e.g., func setupCmdLogging(cmd *exec.Cmd) (/*old return values if any*/,
error)) and propagate any errors from StdoutPipe and StderrPipe instead of
discarding them, and update the caller createInstanceAndRunTests to handle the
returned error at the callsite (where setupCmdLogging is invoked around line
110) by checking the error and returning or logging it appropriately; reference
the StdoutPipe, StderrPipe symbols and the setupCmdLogging and
createInstanceAndRunTests function names to locate the changes.
In `@Makefile`:
- Line 91: The Makefile target currently runs "go run ./integration --
--log=false" which uses the "--" separator and prevents the integration program
from parsing the --log flag; remove the separator so the command is "go run
./integration --log=false" (i.e., edit the Makefile line containing go run
./integration to drop the standalone "--") so the flag is parsed by the Go
program.
---
Nitpick comments:
In `@integration/.env`:
- Around line 5-16: The .env uses floating Docker tags causing non-reproducible
CI; update the WHOAMI_VERSION, ENVOY_VERSION, and TINYAUTH_VERSION variables to
pinned exact tags (e.g., WHOAMI_VERSION=v1.11, ENVOY_VERSION=v1.33.5, and
TINYAUTH_VERSION=v3 or v4) or preferably to image digests to lock versions;
locate and replace the values for the WHOAMI_VERSION, ENVOY_VERSION, and
TINYAUTH_VERSION entries in the integration/.env file so CI uses fixed immutable
images.
In `@integration/docker-compose.traefik.yml`:
- Around line 10-11: The docker-compose currently mounts /var/run/docker.sock
directly into the Traefik service via the volumes entry, which exposes the
Docker API; replace this by running a Docker socket proxy (e.g.,
docker-socket-proxy) as a separate service and point Traefik to the proxy
socket/endpoint instead of /var/run/docker.sock, or configure Traefik to use a
constrained discovery provider (TCP with TLS) so the volumes entry no longer
mounts /var/run/docker.sock; update the Traefik service's volumes and provider
endpoint references accordingly and ensure the proxy service exposes only the
minimal API scopes needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24fd5ee6-4181-4f1d-8edd-5a9286360a14
📒 Files selected for processing (12)
.github/workflows/integration.yml.github/workflows/release.ymlMakefileintegration/.envintegration/config.ymlintegration/docker-compose.envoy.ymlintegration/docker-compose.nginx.ymlintegration/docker-compose.traefik.ymlintegration/envoy.ymlintegration/integrarion_tests.gointegration/integration.gointegration/nginx.conf
💤 Files with no reviewable changes (1)
- .github/workflows/release.yml
| } | ||
| // nginx and envoy will throw us at the frontend | ||
| if resp.StatusCode != http.StatusUnauthorized && !strings.Contains(string(body), "<div id=\"root\"></div>") { | ||
| return fmt.Errorf("expected status code %d or to to contain '<div id=\"root\"></div>', got %d", http.StatusUnauthorized, resp.StatusCode) |
Contributor
There was a problem hiding this comment.
Fix typo in assertion error message.
Line [26] has "or to to contain"; this makes failure output noisier than needed in CI logs.
Suggested fix
- return fmt.Errorf("expected status code %d or to to contain '<div id=\"root\"></div>', got %d", http.StatusUnauthorized, resp.StatusCode)
+ return fmt.Errorf("expected status code %d or to contain '<div id=\"root\"></div>', got %d", http.StatusUnauthorized, resp.StatusCode)📝 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.
Suggested change
| return fmt.Errorf("expected status code %d or to to contain '<div id=\"root\"></div>', got %d", http.StatusUnauthorized, resp.StatusCode) | |
| return fmt.Errorf("expected status code %d or to contain '<div id=\"root\"></div>', got %d", http.StatusUnauthorized, resp.StatusCode) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration/integrarion_tests.go` at line 26, Fix the typo in the assertion
error message inside the fmt.Errorf call that currently reads "or to to contain"
— update the string to "or to contain" so the error message is clean; locate the
fmt.Errorf call in integrarion_tests.go (the line that constructs the error
about expected status code http.StatusUnauthorized and presence of '<div
id=\"root\"></div>') and correct the duplicated "to".
Comment on lines
+130
to
+132
| client := &http.Client{} | ||
|
|
||
| if err := runTests(client, name); err != nil { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n integration/integration.go | head -200Repository: steveiliop56/tinyauth
Length of output: 5430
Add timeouts to HTTP clients and close response bodies in health-check loop.
Line 130 and line 171 create HTTP clients without timeouts. Line 170's ticker is never stopped. Line 182's response body is never closed. These issues cause connection leaks during retries and can hang CI.
Proposed fix
- client := &http.Client{}
+ client := &http.Client{Timeout: 15 * time.Second}
@@
func waitForHealthy() error {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
defer cancel()
ticker := time.NewTicker(10 * time.Second)
+ defer ticker.Stop()
- client := http.Client{}
+ client := http.Client{Timeout: 3 * time.Second}
@@
res, err := client.Do(req)
if err != nil {
continue
}
+ defer res.Body.Close()
if res.StatusCode == http.StatusOK {
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration/integration.go` around lines 130 - 132, The HTTP client created
as client := &http.Client{} in integration.go lacks a timeout and the
health-check loop leaks resources: modify the client instantiation used by
runTests (and any other HTTP clients in this file) to set a sensible Timeout
(e.g., http.Client{Timeout: ...}) and/or configure Transport timeouts; in the
health-check loop ensure the ticker started for retries is stopped (call
ticker.Stop() when exiting the loop) and always close the HTTP response body
(defer resp.Body.Close() or explicitly close resp.Body after each request) to
prevent connection leaks.
Comment on lines
+147
to
+150
| func setupCmdLogging(cmd *exec.Cmd) { | ||
| stdout, _ := cmd.StdoutPipe() | ||
| stderr, _ := cmd.StderrPipe() | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the integration.go file
fd integration.goRepository: steveiliop56/tinyauth
Length of output: 91
🏁 Script executed:
# Read the file to see the actual code around lines 147-150
cat -n integration/integration.go | head -160 | tail -30Repository: steveiliop56/tinyauth
Length of output: 835
🏁 Script executed:
# Also search for the callsite in createInstanceAndRunTests
rg -n "createInstanceAndRunTests" integration/integration.go -A 30 | head -50Repository: steveiliop56/tinyauth
Length of output: 1565
🏁 Script executed:
# Check for other calls to setupCmdLogging
rg "setupCmdLogging" integration/integration.goRepository: steveiliop56/tinyauth
Length of output: 125
🏁 Script executed:
# See the complete setupCmdLogging function
sed -n '147,170p' integration/integration.goRepository: steveiliop56/tinyauth
Length of output: 606
Handle StdoutPipe/StderrPipe errors instead of discarding them.
Ignoring these errors makes logging setup brittle and can lead to runtime issues when log mode is enabled. The function should return an error and the callsite at line 110 in createInstanceAndRunTests should handle it.
🛠️ Proposed fix
-func setupCmdLogging(cmd *exec.Cmd) {
- stdout, _ := cmd.StdoutPipe()
- stderr, _ := cmd.StderrPipe()
+func setupCmdLogging(cmd *exec.Cmd) error {
+ stdout, err := cmd.StdoutPipe()
+ if err != nil {
+ return err
+ }
+ stderr, err := cmd.StderrPipe()
+ if err != nil {
+ return err
+ }
go func() {
scanner := bufio.NewScanner(stdout)
for scanner.Scan() {
slog.Info("docker out", "stdout", scanner.Text())
}
}()
go func() {
scanner := bufio.NewScanner(stderr)
for scanner.Scan() {
slog.Error("docker out", "stderr", scanner.Text())
}
}()
+
+ return nil
}And at the callsite in createInstanceAndRunTests:
if log {
- setupCmdLogging(cmd)
+ if err := setupCmdLogging(cmd); err != nil {
+ return err
+ }
}📝 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.
Suggested change
| func setupCmdLogging(cmd *exec.Cmd) { | |
| stdout, _ := cmd.StdoutPipe() | |
| stderr, _ := cmd.StderrPipe() | |
| func setupCmdLogging(cmd *exec.Cmd) error { | |
| stdout, err := cmd.StdoutPipe() | |
| if err != nil { | |
| return err | |
| } | |
| stderr, err := cmd.StderrPipe() | |
| if err != nil { | |
| return err | |
| } | |
| go func() { | |
| scanner := bufio.NewScanner(stdout) | |
| for scanner.Scan() { | |
| slog.Info("docker out", "stdout", scanner.Text()) | |
| } | |
| }() | |
| go func() { | |
| scanner := bufio.NewScanner(stderr) | |
| for scanner.Scan() { | |
| slog.Error("docker out", "stderr", scanner.Text()) | |
| } | |
| }() | |
| return nil | |
| } |
Suggested change
| func setupCmdLogging(cmd *exec.Cmd) { | |
| stdout, _ := cmd.StdoutPipe() | |
| stderr, _ := cmd.StderrPipe() | |
| if log { | |
| if err := setupCmdLogging(cmd); err != nil { | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration/integration.go` around lines 147 - 150, The setupCmdLogging
function currently ignores errors from cmd.StdoutPipe() and cmd.StderrPipe();
change setupCmdLogging to return an error (e.g., func setupCmdLogging(cmd
*exec.Cmd) (/*old return values if any*/, error)) and propagate any errors from
StdoutPipe and StderrPipe instead of discarding them, and update the caller
createInstanceAndRunTests to handle the returned error at the callsite (where
setupCmdLogging is invoked around line 110) by checking the error and returning
or logging it appropriately; reference the StdoutPipe, StderrPipe symbols and
the setupCmdLogging and createInstanceAndRunTests function names to locate the
changes.
| # Proxy integration tests | ||
| .PHONY: integration | ||
| integration: | ||
| go run ./integration -- --log=false |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how integration CLI parses arguments/flags (read-only reconnaissance).
# Expected: determine whether a leading standalone "--" would stop parsing --log=false.
rg -n -C3 --type go 'flag\.|pflag|cobra|urfave|os\.Args|log' integrationRepository: steveiliop56/tinyauth
Length of output: 6362
Remove the -- separator so the --log=false flag is properly parsed.
The -- separator in Go's flag package stops flag parsing, leaving --log=false as a positional argument instead of being processed as a flag. Remove it to allow the flag to be recognized.
Fix
integration:
- go run ./integration -- --log=false
+ go run ./integration --log=false📝 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.
Suggested change
| go run ./integration -- --log=false | |
| integration: | |
| go run ./integration --log=false |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 91, The Makefile target currently runs "go run
./integration -- --log=false" which uses the "--" separator and prevents the
integration program from parsing the --log flag; remove the separator so the
command is "go run ./integration --log=false" (i.e., edit the Makefile line
containing go run ./integration to drop the standalone "--") so the flag is
parsed by the Go program.
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
1 participant
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.
Depends on #714
Summary by CodeRabbit
New Features
Tests
Chores