Add framework.StartDex and export framework.CreateDexClient#339
Conversation
WalkthroughTests now start and manage a shared Dex process from the framework (StartDex) and register per-test Dex OAuth clients (CreateDexClient). A Makefile e2e target no longer starts or tracks a background Dex process; tests call the new framework helpers directly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as e2e Test
participant Framework as framework
participant DexProc as dex serve
participant DexAPI as Dex gRPC
Test->>Framework: StartDex(t)
activate Framework
Framework->>DexProc: Launch subprocess (dex serve -c config)
Framework-->>Framework: Poll OIDC well-known until ready
deactivate Framework
Test->>Framework: CreateDexClient(t, addr)
activate Framework
Framework->>DexAPI: CreateClient(id=kube-bind-<port>, redirect_uri)
deactivate Framework
Note over Test,DexAPI: Test runs using Dex and registered client
Test-->>Framework: Cleanup
activate Framework
Framework->>DexAPI: DeleteClient(id=kube-bind-<port>) (with timeout)
deactivate Framework
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
ntnn
force-pushed
the
framework-dex
branch
4 times, most recently
from
3596a36 to
5f84e47
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: 1
🧹 Nitpick comments (4)
test/e2e/framework/dex.go (4)
60-68: Consider adding explicit cleanup registration.While
Pdeathsig: syscall.SIGKILLensures Dex is terminated when the parent process exits, adding an explicitt.Cleanupregistration would make the cleanup more robust and idiomatic for Go tests. This ensures cleanup even if the test framework exits abnormally.Apply this diff to add explicit cleanup:
require.NoError(t, dexCmd.Start()) + t.Cleanup(func() { + if dexCmd.Process != nil { + _ = dexCmd.Process.Kill() + } + }) })
71-80: Consider making Dex URLs configurable.The Dex readiness check and gRPC client (in
CreateDexClient) use hardcoded URLs (http://127.0.0.1:5556and127.0.0.1:5557). If the Dex configuration changes to use different ports or addresses, these would need manual updates.Consider reading these from environment variables or the Dex config file for flexibility:
dexHTTPAddr := os.Getenv("DEX_HTTP_ADDR") if dexHTTPAddr == "" { dexHTTPAddr = "http://127.0.0.1:5556" } dexGRPCAddr := os.Getenv("DEX_GRPC_ADDR") if dexGRPCAddr == "" { dexGRPCAddr = "127.0.0.1:5557" }
105-112: Cleanup error handling may hide original test failures.The cleanup function uses
require.NoErrorto assert that client deletion succeeds. If the original test fails and then cleanup also fails, the cleanup failure will be reported instead of the original failure, potentially hiding the root cause.Consider using
t.Erroror logging cleanup failures instead:t.Cleanup(func() { ctx, cancel := context.WithDeadline(context.Background(), metav1.Now().Add(10*time.Second)) defer cancel() conn, err := grpc.NewClient("127.0.0.1:5557", grpc.WithTransportCredentials(grpcinsecure.NewCredentials())) - require.NoError(t, err) + if err != nil { + t.Logf("Failed to create gRPC connection for cleanup: %v", err) + return + } _, err = dexapi.NewDexClient(conn).DeleteClient(ctx, &dexapi.DeleteClientReq{Id: "kube-bind-" + port}) - require.NoError(t, err) + if err != nil { + t.Logf("Failed to delete Dex client during cleanup: %v", err) + } })
106-106: Usingmetav1.Now()instead oftime.Now()is unconventional.The cleanup context deadline uses
metav1.Now().Add(10*time.Second)instead of the standardtime.Now().Add(10*time.Second). While functionally equivalent,metav1.Now()returns ametav1.Timewhich is then implicitly converted totime.Time, making this usage unnecessarily indirect.Use the standard library function for clarity:
- ctx, cancel := context.WithDeadline(context.Background(), metav1.Now().Add(10*time.Second)) + ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(10*time.Second))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(1 hunks)test/e2e/bind/happy-case_test.go(1 hunks)test/e2e/framework/backend.go(1 hunks)test/e2e/framework/dex.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/e2e/framework/dex.go (1)
test/e2e/framework/env.go (1)
WorkDir(26-26)
test/e2e/bind/happy-case_test.go (1)
test/e2e/framework/dex.go (1)
StartDex(42-82)
test/e2e/framework/backend.go (1)
test/e2e/framework/dex.go (1)
CreateDexClient(84-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: verify
- GitHub Check: go-test-e2e
- GitHub Check: go-test
🔇 Additional comments (3)
test/e2e/bind/happy-case_test.go (1)
62-62: LGTM!The call to
framework.StartDex(t)is correctly placed early in the test setup, ensuring Dex is running and ready before any operations that require it. The function usessync.Onceto ensure Dex starts only once across all tests and waits for readiness before returning.test/e2e/framework/backend.go (1)
83-83: LGTM!The replacement of the local
createDexClienthelper with the exportedframework.CreateDexClientconsolidates Dex client management into the framework package, improving code organization and reusability.Makefile (1)
281-281: LGTM!The removal of Dex process management from the Makefile simplifies the e2e test orchestration. Dex lifecycle is now handled by
framework.StartDexwithin the tests, centralizing control and eliminating the need for external process coordination.
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e/framework/dex.go (2)
69-80: Consider moving readiness check insidesync.Oncefor efficiency.The readiness polling happens outside the
sync.Onceblock, so every test callingStartDexwill re-poll the OIDC configuration endpoint. While this is harmless, it adds unnecessary overhead.For efficiency, consider moving the readiness check inside the
sync.Onceblock so that it runs only once when Dex is started.Apply this diff to move the readiness check:
dexOnce.Do(func() { dexConfig := os.Getenv("DEX_CONFIG") if dexConfig == "" { dexConfig = filepath.Clean(filepath.Join(WorkDir, "..", "hack", "dex-config-dev.yaml")) } t.Logf("Starting dex with config %q", dexConfig) dexCmd := exec.Command( "dex", "serve", dexConfig, ) // Ensures that dex is killed when the process ends. dexCmd.SysProcAttr = &syscall.SysProcAttr{ Pdeathsig: syscall.SIGKILL, Setpgid: true, Pgid: 0, } require.NoError(t, dexCmd.Start()) + + t.Log("Wait for Dex to be ready") + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://127.0.0.1:5556/dex/.well-known/openid-configuration", nil) + require.NoError(t, err) + require.Eventually(t, func() bool { + resp, err := http.DefaultClient.Do(req) + if err != nil { + return false + } + defer resp.Body.Close() + return resp.StatusCode == http.StatusOK + }, wait.ForeverTestTimeout, time.Millisecond*100) + t.Log("Dex is ready") }) - - t.Log("Wait for Dex to be ready") - req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://127.0.0.1:5556/dex/.well-known/openid-configuration", nil) - require.NoError(t, err) - require.Eventually(t, func() bool { - resp, err := http.DefaultClient.Do(req) - if err != nil { - return false - } - defer resp.Body.Close() - return resp.StatusCode == http.StatusOK - }, wait.ForeverTestTimeout, time.Millisecond*100) - t.Log("Dex is ready") }
88-111: Consider reusing the gRPC connection for cleanup.The function creates a gRPC connection (line 88), closes it immediately after registration (line 90), and then creates a new connection in the cleanup function (line 107). While this works, it's less efficient than reusing the connection.
Consider storing the connection in a variable accessible to the cleanup function, or deferring the connection close until after the cleanup is registered.
Apply this diff to reuse the connection:
- conn, err := grpc.NewClient("127.0.0.1:5557", grpc.WithTransportCredentials(grpcinsecure.NewCredentials())) - require.NoError(t, err) - defer conn.Close() - client := dexapi.NewDexClient(conn) - - _, err = client.CreateClient(t.Context(), &dexapi.CreateClientReq{ + conn, err := grpc.NewClient("127.0.0.1:5557", grpc.WithTransportCredentials(grpcinsecure.NewCredentials())) + require.NoError(t, err) + client := dexapi.NewDexClient(conn) + + _, err = client.CreateClient(t.Context(), &dexapi.CreateClientReq{ Client: &dexapi.Client{ Id: "kube-bind-" + port, Secret: "ZXhhbXBsZS1hcHAtc2VjcmV0", RedirectUris: []string{fmt.Sprintf("http://%s/callback", addr)}, Public: true, Name: "kube-bind on port " + port, }, }) require.NoError(t, err) t.Cleanup(func() { + defer conn.Close() ctx, cancel := context.WithDeadline(context.Background(), metav1.Now().Add(10*time.Second)) defer cancel() - conn, err := grpc.NewClient("127.0.0.1:5557", grpc.WithTransportCredentials(grpcinsecure.NewCredentials())) - if err != nil { - t.Logf("Failed to create gRPC connection for cleanup: %v", err) - return - } - defer conn.Close() _, err = dexapi.NewDexClient(conn).DeleteClient(ctx, &dexapi.DeleteClientReq{Id: "kube-bind-" + port}) if err != nil { t.Logf("Failed to delete Dex client during cleanup: %v", err) } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(1 hunks)test/e2e/bind/happy-case_test.go(1 hunks)test/e2e/framework/backend.go(1 hunks)test/e2e/framework/dex.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- test/e2e/bind/happy-case_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/framework/backend.go (1)
test/e2e/framework/dex.go (1)
CreateDexClient(83-112)
test/e2e/framework/dex.go (1)
test/e2e/framework/env.go (1)
WorkDir(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: go-test
- GitHub Check: lint
- GitHub Check: verify
- GitHub Check: go-test-e2e
🔇 Additional comments (2)
test/e2e/framework/dex.go (1)
40-40: LGTM!Using a package-level
sync.Onceensures Dex starts only once across all tests in the suite, which is appropriate for a shared test resource.test/e2e/framework/backend.go (1)
83-83: LGTM!The change correctly uses the new exported
CreateDexClientfunction fromframework/dex.go, eliminating code duplication and centralizing Dex client management.
2 participants
Summary
Add
framework.StartDexfor tests to ensure dex is running.What Type of PR Is This?
/kind feature
Related Issue(s)
Fixes #
Release Notes
Summary by CodeRabbit
Tests
Chores