🌱 test: add TLS profile unit and e2e tests#2653
🌱 test: add TLS profile unit and e2e tests#2653tmshort wants to merge 1 commit intooperator-framework:mainfrom
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Adds explicit test coverage (unit + e2e) for custom TLS profile flags (--tls-custom-*) to validate version, cipher, and curve enforcement.
Changes:
- Added
tlsprofilesunit tests that stand up a local TLS server and validate negotiation/enforcement behaviors. - Added new e2e Godog steps + a
tls.featurefile that patches thecatalogddeployment TLS args and asserts expected connection outcomes. - Extended the
make test-e2etarget to acceptGODOG_ARGSfor running a subset of features/scenarios.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/tls_steps.go | Implements e2e step helpers for patching deployments and asserting TLS negotiation/rejection against metrics endpoints. |
| test/e2e/steps/steps.go | Registers new TLS-related Godog steps. |
| test/e2e/steps/hooks.go | Adds scenario cleanup support to restore modified deployment args after each scenario. |
| test/e2e/features/tls.feature | New e2e feature scenarios covering TLS min-version, cipher enforcement, and curve enforcement. |
| internal/shared/util/tlsprofiles/tlsprofiles_connection_test.go | New unit tests validating custom TLS profile behavior via real TLS handshakes. |
| Makefile | Adds GODOG_ARGS passthrough to the e2e test runner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // restrictions) serves as the readiness probe; both components require TLS 1.3, so | ||
| // any successful handshake confirms the tunnel is up. |
There was a problem hiding this comment.
The readiness comment here is inaccurate: the port-forward probe is a generic TLS dial, and scenarios later patch the deployment to allow TLS 1.2. Consider updating the comment to avoid implying the components always require TLS 1.3 (the logic is fine, but the explanation is misleading).
| // restrictions) serves as the readiness probe; both components require TLS 1.3, so | |
| // any successful handshake confirms the tunnel is up. | |
| // restrictions) serves as the readiness probe; any successful TLS handshake confirms | |
| // the tunnel is up. |
| // patchDeploymentArgs replaces the args of the "manager" container in the named | ||
| // deployment with args using a JSON patch on the first container index. | ||
| func patchDeploymentArgs(ns, name string, args []string) error { | ||
| argsJSON, err := json.Marshal(args) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| patch := fmt.Sprintf(`[{"op":"replace","path":"/spec/template/spec/containers/0/args","value":%s}]`, string(argsJSON)) |
There was a problem hiding this comment.
This helper claims to patch the args for the container named "manager", but the JSON patch hard-codes /containers/0/args. This is fragile if container ordering changes (or a sidecar is added) and also diverges from getDeploymentContainerArgs, which searches by name. Consider computing the container index by name (or using a strategic merge patch targeting name: manager) and patching that path.
| // patchDeploymentArgs replaces the args of the "manager" container in the named | |
| // deployment with args using a JSON patch on the first container index. | |
| func patchDeploymentArgs(ns, name string, args []string) error { | |
| argsJSON, err := json.Marshal(args) | |
| if err != nil { | |
| return err | |
| } | |
| patch := fmt.Sprintf(`[{"op":"replace","path":"/spec/template/spec/containers/0/args","value":%s}]`, string(argsJSON)) | |
| // getDeploymentContainerIndex returns the index of the container named "manager" | |
| // inside the named deployment. | |
| func getDeploymentContainerIndex(ns, name string) (int, error) { | |
| raw, err := k8sClient("get", "deployment", name, "-n", ns, "-o", "json") | |
| if err != nil { | |
| return -1, fmt.Errorf("getting deployment %s/%s: %w", ns, name, err) | |
| } | |
| var deploy appsv1.Deployment | |
| if err := json.Unmarshal([]byte(raw), &deploy); err != nil { | |
| return -1, fmt.Errorf("parsing deployment %s/%s: %w", ns, name, err) | |
| } | |
| for i, c := range deploy.Spec.Template.Spec.Containers { | |
| if c.Name == "manager" { | |
| return i, nil | |
| } | |
| } | |
| return -1, fmt.Errorf("no container named 'manager' in deployment %s/%s", ns, name) | |
| } | |
| // patchDeploymentArgs replaces the args of the "manager" container in the named | |
| // deployment with args using a JSON patch on that container's actual index. | |
| func patchDeploymentArgs(ns, name string, args []string) error { | |
| containerIndex, err := getDeploymentContainerIndex(ns, name) | |
| if err != nil { | |
| return err | |
| } | |
| argsJSON, err := json.Marshal(args) | |
| if err != nil { | |
| return err | |
| } | |
| patch := fmt.Sprintf( | |
| `[{"op":"replace","path":"/spec/template/spec/containers/%d/args","value":%s}]`, | |
| containerIndex, | |
| string(argsJSON), | |
| ) |
| // Always restore deployments whose args were modified during the scenario, | ||
| // even when the scenario failed, so that a misconfigured TLS profile does | ||
| // not leak into subsequent scenarios. | ||
| for _, dr := range sc.deploymentRestores { | ||
| if err2 := patchDeploymentArgs(dr.namespace, dr.deploymentName, dr.originalArgs); err2 != nil { | ||
| logger.Info("Error restoring deployment args", "name", dr.deploymentName, "error", err2) | ||
| continue | ||
| } | ||
| if _, err2 := k8sClient("rollout", "status", "-n", dr.namespace, | ||
| fmt.Sprintf("deployment/%s", dr.deploymentName), "--timeout=2m"); err2 != nil { | ||
| logger.Info("Timeout waiting for deployment rollout after restore", "name", dr.deploymentName) | ||
| } | ||
| } |
There was a problem hiding this comment.
Deployment restores are applied in the same order they were recorded. If a scenario patches the same deployment multiple times, the final restore will revert to an intermediate args set rather than the original pre-scenario args. Restoring in reverse (LIFO) order or de-duplicating per deployment (keeping the first/original args only) would ensure the deployment returns to its true original state.
tmshort
force-pushed
the
tls-profile-tests
branch
from
1459482 to
6fd8bd1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2653 +/- ##
=======================================
Coverage 68.89% 68.89%
=======================================
Files 141 141
Lines 10009 10009
=======================================
Hits 6896 6896
Misses 2596 2596
Partials 517 517
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Unit tests in tlsprofiles package verify cipher negotiation, cipher rejection, min-version enforcement, and curve acceptance/rejection by starting a local TLS server with a custom profile and connecting to it with a restricted client config. - e2e feature (tls.feature) patches the catalogd deployment with specific custom TLS settings for each scenario, asserts the expected connection behaviour, then restores the original args on cleanup. Covers min-version enforcement (TLSv1.3), cipher negotiation and rejection (TLS 1.2 + ECDHE_ECDSA), and curve enforcement (prime256v1 accepted, secp521r1 rejected). - GODOG_ARGS variable added to the e2e Makefile target so a single feature file can be run with: make test-e2e GODOG_ARGS=features/tls.feature Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
tmshort
force-pushed
the
tls-profile-tests
branch
from
6fd8bd1 to
daed167
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Client offers only AES-128, which the server does not allow. | ||
| clientCfg := &tls.Config{ | ||
| InsecureSkipVerify: true, //nolint:gosec // self-signed cert used only in tests | ||
| MaxVersion: tls.VersionTLS12, | ||
| CipherSuites: []uint16{cipherSuiteId("TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256")}, |
There was a problem hiding this comment.
cipherSuiteId(...) can return 0 if the cipher name is unavailable (e.g., due to platform/FIPS restrictions). In this rejection test, passing 0 into CipherSuites could make the handshake fail for the wrong reason and still satisfy require.Error, reducing the test's signal. It’d be safer to assign the ID to a variable and require.NotZero before using it in clientCfg.
| // Client offers only AES-128, which the server does not allow. | |
| clientCfg := &tls.Config{ | |
| InsecureSkipVerify: true, //nolint:gosec // self-signed cert used only in tests | |
| MaxVersion: tls.VersionTLS12, | |
| CipherSuites: []uint16{cipherSuiteId("TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256")}, | |
| cipherID := cipherSuiteId("TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256") | |
| require.NotZero(t, cipherID) | |
| // Client offers only AES-128, which the server does not allow. | |
| clientCfg := &tls.Config{ | |
| InsecureSkipVerify: true, //nolint:gosec // self-signed cert used only in tests | |
| MaxVersion: tls.VersionTLS12, | |
| CipherSuites: []uint16{cipherID}, |
| clientCfg := &tls.Config{ | ||
| InsecureSkipVerify: true, //nolint:gosec // self-signed cert used only in tests | ||
| MaxVersion: tls.VersionTLS12, | ||
| CurvePreferences: []tls.CurveID{tls.CurveP256}, | ||
| CipherSuites: []uint16{cipherSuiteId("TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256")}, | ||
| } |
There was a problem hiding this comment.
cipherSuiteId(...) can return 0 if the cipher name is unavailable. Here the test expects the connection to succeed; if the ID were 0 this would fail in a non-obvious way. Consider asserting require.NotZero on the cipher ID before building clientCfg so failures clearly indicate an unsupported/renamed cipher.
| clientCfg := &tls.Config{ | ||
| InsecureSkipVerify: true, //nolint:gosec // self-signed cert used only in tests | ||
| MaxVersion: tls.VersionTLS12, | ||
| CurvePreferences: []tls.CurveID{tls.X25519}, | ||
| CipherSuites: []uint16{cipherSuiteId("TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256")}, | ||
| } |
There was a problem hiding this comment.
cipherSuiteId(...) can return 0 if the cipher name is unavailable. In this rejection test, a 0 cipher ID could cause the handshake to fail regardless of curve overlap, making it harder to tell whether the failure was due to curve enforcement. Consider asserting require.NotZero on the cipher ID before using it in clientCfg.
| waitFor(ctx, func() bool { | ||
| conn, err := tls.Dial("tcp", addr, &tls.Config{InsecureSkipVerify: true}) //nolint:gosec | ||
| if err != nil { | ||
| return false | ||
| } | ||
| conn.Close() | ||
| return true |
There was a problem hiding this comment.
tls.Dial uses the default net.Dialer (no timeout) and can block indefinitely if the local port is open (kubectl port-forward is listening) but the upstream connection/handshake never completes. Because this call happens inside waitFor/require.Eventually, a blocked dial prevents the retry loop from progressing and can hang the entire e2e run past the configured timeout. Consider using tls.DialWithDialer (or net.Dialer{Timeout: ...}) and/or setting a per-connection deadline derived from ctx for the readiness probe (and ideally for the subsequent step dials too).
Explicitly tests the
--tls-customarguments, so that we can be sure that they work as expected.Description
Reviewer Checklist