NO-JIRA: Implement -external-certificate flag to edge and re-encrypt route#2256
NO-JIRA: Implement -external-certificate flag to edge and re-encrypt route#2256lunarwhite wants to merge 1 commit intoopenshift:mainfrom
-external-certificate flag to edge and re-encrypt route#2256Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@lunarwhite: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lunarwhite 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @lunarwhite. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/cli/create/routeedge.go (1)
100-106: Extract shared TLS external-certificate validation/wiring logic.The mutual-exclusivity checks and
ExternalCertificateassignment here are duplicated withpkg/cli/create/routereenecrypt.go, which risks behavioral drift over time. Consider moving this into shared helpers used by both commands.Refactor sketch
+// pkg/cli/create/route_tls_helpers.go +func validateExternalCertMutualExclusion(certPath, keyPath, externalName string) error { + if len(externalName) == 0 { + return nil + } + if len(certPath) > 0 { + return fmt.Errorf("--cert and --external-certificate are mutually exclusive") + } + if len(keyPath) > 0 { + return fmt.Errorf("--key and --external-certificate are mutually exclusive") + } + return nil +} + +func setExternalCertificate(tls *routev1.TLSConfig, externalName string) { + if len(externalName) == 0 { + return + } + tls.ExternalCertificate = &routev1.LocalObjectReference{Name: externalName} +}- if len(o.Cert) > 0 && len(o.ExternalCertificate) > 0 { ... } - if len(o.Key) > 0 && len(o.ExternalCertificate) > 0 { ... } + if err := validateExternalCertMutualExclusion(o.Cert, o.Key, o.ExternalCertificate); err != nil { + return err + } ... - if len(o.ExternalCertificate) > 0 { - route.Spec.TLS.ExternalCertificate = &routev1.LocalObjectReference{ - Name: o.ExternalCertificate, - } - } + setExternalCertificate(route.Spec.TLS, o.ExternalCertificate)Also applies to: 142-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/create/routeedge.go` around lines 100 - 106, The mutual-exclusivity TLS checks and ExternalCertificate assignment duplicated in CreateEdgeRouteOptions.Run (and the similar block in routereenecrypt.go) should be extracted into a shared helper (e.g., ValidateAndWireExternalCertificate or WireExternalCert) in a common package (pkg/cli/create or pkg/cli/create/internal); implement the helper to accept the options struct fields (Cert, Key, ExternalCertificate) or the options pointer, perform the mutual-exclusion checks, return a wired ExternalCertificate value or an error, and replace the duplicated logic in CreateEdgeRouteOptions.Run and the corresponding routereenecrypt.Run by calling this helper and using its returned value; ensure tests and callers preserve existing error messages ("--cert and --external-certificate are mutually exclusive", "--key and --external-certificate are mutually exclusive").pkg/cli/create/routeedge_test.go (1)
48-77: Consider consolidating duplicated test harness between edge/reencrypt suites.
newTestEdgeRouteOptions, setup, and many assertions mirrorpkg/cli/create/routereenecrypt_test.go. A shared helper would reduce maintenance overhead.Also applies to: 79-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/create/routeedge_test.go` around lines 48 - 77, Tests duplicate the route options setup in newTestEdgeRouteOptions and the reencrypt suite; extract the common setup into a single helper (e.g., newTestRouteOptions) that performs genericiooptions.NewTestIOStreams, fakekubernetes.NewClientset(service), routefake.NewClientset(), meta.NewDefaultRESTMapper(nil), genericclioptions.NewPrintFlags(...).ToPrinter(), and returns the shared CreateRouteSubcommandOptions (with Mapper, Client from fakeRouteClientset.RouteV1(), CoreClient, Printer, IOStreams), the fakeRouteClientset, and the output buffer; then rewrite newTestEdgeRouteOptions to call newTestRouteOptions and wrap/augment its result into a CreateEdgeRouteOptions (setting Service, Name, Namespace or TLS-specific fields) so both edge and reencrypt tests reuse the same setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cli/create/routeedge_test.go`:
- Around line 48-77: Tests duplicate the route options setup in
newTestEdgeRouteOptions and the reencrypt suite; extract the common setup into a
single helper (e.g., newTestRouteOptions) that performs
genericiooptions.NewTestIOStreams, fakekubernetes.NewClientset(service),
routefake.NewClientset(), meta.NewDefaultRESTMapper(nil),
genericclioptions.NewPrintFlags(...).ToPrinter(), and returns the shared
CreateRouteSubcommandOptions (with Mapper, Client from
fakeRouteClientset.RouteV1(), CoreClient, Printer, IOStreams), the
fakeRouteClientset, and the output buffer; then rewrite newTestEdgeRouteOptions
to call newTestRouteOptions and wrap/augment its result into a
CreateEdgeRouteOptions (setting Service, Name, Namespace or TLS-specific fields)
so both edge and reencrypt tests reuse the same setup.
In `@pkg/cli/create/routeedge.go`:
- Around line 100-106: The mutual-exclusivity TLS checks and ExternalCertificate
assignment duplicated in CreateEdgeRouteOptions.Run (and the similar block in
routereenecrypt.go) should be extracted into a shared helper (e.g.,
ValidateAndWireExternalCertificate or WireExternalCert) in a common package
(pkg/cli/create or pkg/cli/create/internal); implement the helper to accept the
options struct fields (Cert, Key, ExternalCertificate) or the options pointer,
perform the mutual-exclusion checks, return a wired ExternalCertificate value or
an error, and replace the duplicated logic in CreateEdgeRouteOptions.Run and the
corresponding routereenecrypt.Run by calling this helper and using its returned
value; ensure tests and callers preserve existing error messages ("--cert and
--external-certificate are mutually exclusive", "--key and
--external-certificate are mutually exclusive").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c10a931c-5e0b-498d-92e6-592877a11d7e
📒 Files selected for processing (4)
pkg/cli/create/routeedge.gopkg/cli/create/routeedge_test.gopkg/cli/create/routereenecrypt.gopkg/cli/create/routereenecrypt_test.go
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
lunarwhite
force-pushed
the
ext-cert-route-flag
branch
from
9225a5f to
e0ac307
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/cli/create/routeedge_test.go (2)
86-87: Usefilepath.Joinfor OS-safe test paths.These paths are currently string-concatenated;
filepath.Joinis safer and idiomatic across platforms.Proposed diff
import ( "bytes" "context" "os" + "path/filepath" "strings" "testing" @@ - certFile := tmpDir + "/tls.crt" - keyFile := tmpDir + "/tls.key" + certFile := filepath.Join(tmpDir, "tls.crt") + keyFile := filepath.Join(tmpDir, "tls.key")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/create/routeedge_test.go` around lines 86 - 87, The test builds file paths by string-concatenating tmpDir with "/tls.crt" and "/tls.key" (variables certFile and keyFile); replace those concatenations with filepath.Join(tmpDir, "tls.crt") and filepath.Join(tmpDir, "tls.key") to make the paths OS-safe and idiomatic (ensure you add/import "path/filepath" if not already present).
122-124: Consider addingtt := ttfor explicit loop variable capture clarity.This is optional defensive coding. Go 1.22+ (required by go.mod
1.25.0) fixed range-loop variable capture semantics in closures, so this change is not needed for correctness—only for code readability if preferred.Proposed diff
for _, tt := range tests { + tt := tt t.Run(tt.name, func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/create/routeedge_test.go` around lines 122 - 124, In the table-driven test loop over "tests" before launching the subtest with t.Run, shadow the loop variable by adding "tt := tt" so the closure captures the per-iteration value explicitly; update the range loop in pkg/cli/create/routeedge_test.go (the block that calls newTestEdgeRouteOptions and t.Run) to introduce this local assignment immediately before t.Run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cli/create/routeedge_test.go`:
- Around line 86-87: The test builds file paths by string-concatenating tmpDir
with "/tls.crt" and "/tls.key" (variables certFile and keyFile); replace those
concatenations with filepath.Join(tmpDir, "tls.crt") and filepath.Join(tmpDir,
"tls.key") to make the paths OS-safe and idiomatic (ensure you add/import
"path/filepath" if not already present).
- Around line 122-124: In the table-driven test loop over "tests" before
launching the subtest with t.Run, shadow the loop variable by adding "tt := tt"
so the closure captures the per-iteration value explicitly; update the range
loop in pkg/cli/create/routeedge_test.go (the block that calls
newTestEdgeRouteOptions and t.Run) to introduce this local assignment
immediately before t.Run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d51fb9d4-775c-4649-b669-6e4834eae93d
📒 Files selected for processing (4)
pkg/cli/create/routeedge.gopkg/cli/create/routeedge_test.gopkg/cli/create/routereenecrypt.gopkg/cli/create/routereenecrypt_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cli/create/routereenecrypt_test.go
- pkg/cli/create/routereenecrypt.go
- pkg/cli/create/routeedge.go
lunarwhite
force-pushed
the
ext-cert-route-flag
branch
from
e0ac307 to
d821dc3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/cli/create/routereenecrypt_test.go (2)
26-30: Temp file setup is unnecessary in this unit test.
Validate()only checks whether strings are non-empty, so creating real cert/key files adds noise and I/O without increasing confidence.♻️ Simplify test setup
- tmpDir := t.TempDir() - certFile := filepath.Join(tmpDir, "tls.crt") - keyFile := filepath.Join(tmpDir, "tls.key") - writeTestFile(t, certFile, "test-cert-data") - writeTestFile(t, keyFile, "test-key-data") + certFile := "dummy-cert" + keyFile := "dummy-key"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/create/routereenecrypt_test.go` around lines 26 - 30, The test currently creates real temp files and writes cert/key data even though Validate() only checks non-empty strings; remove the tmpDir, filepath.Join and writeTestFile calls and instead set certFile and keyFile to simple non-empty string literals (e.g., "test-cert-data", "test-key-data") so the test avoids unnecessary I/O; update the variables used by the Validate() call in the test in routereenecrypt_test.go accordingly.
122-123: Prefer typed termination constant over string literal.The test at lines 122-123 uses string literal
"reencrypt"instead of the available typed constant. Replace withroutev1.TLSTerminationReencrypt(already used in the same package atroutereenecrypt.go:139) to avoid literal drift and align with the existing codebase pattern.Import
routev1fromgithub.com/openshift/api/route/v1if not already present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/create/routereenecrypt_test.go` around lines 122 - 123, The test checks route.Spec.TLS.Termination against the string literal "reencrypt"—replace that literal with the typed constant routev1.TLSTerminationReencrypt and add/import routev1 from github.com/openshift/api/route/v1 if missing; update the assertion in the test (the comparison of route.Spec.TLS.Termination) to use routev1.TLSTerminationReencrypt to match the usage in routereenecrypt.go (where TLSTerminationReencrypt is already used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cli/create/routereenecrypt_test.go`:
- Around line 26-30: The test currently creates real temp files and writes
cert/key data even though Validate() only checks non-empty strings; remove the
tmpDir, filepath.Join and writeTestFile calls and instead set certFile and
keyFile to simple non-empty string literals (e.g., "test-cert-data",
"test-key-data") so the test avoids unnecessary I/O; update the variables used
by the Validate() call in the test in routereenecrypt_test.go accordingly.
- Around line 122-123: The test checks route.Spec.TLS.Termination against the
string literal "reencrypt"—replace that literal with the typed constant
routev1.TLSTerminationReencrypt and add/import routev1 from
github.com/openshift/api/route/v1 if missing; update the assertion in the test
(the comparison of route.Spec.TLS.Termination) to use
routev1.TLSTerminationReencrypt to match the usage in routereenecrypt.go (where
TLSTerminationReencrypt is already used).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 91b6ed89-c258-4dd1-ba99-49ed399362a6
📒 Files selected for processing (4)
pkg/cli/create/routeedge.gopkg/cli/create/routeedge_test.gopkg/cli/create/routereenecrypt.gopkg/cli/create/routereenecrypt_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/create/routeedge_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cli/create/routereenecrypt.go
- pkg/cli/create/routeedge.go
Adds support for referencing TLS certificates from a TLS Secret when creating edge and reencrypt routes. The new '--external-certificate' flag accepts a Secret name and populates 'route.Spec.TLS.ExternalCertificate' as a LocalObjectReference. This flag is mutually exclusive with '--cert' and '--key', since inline certificates and secret-backed certificates are alternative sources. The '--ca-cert' flag (and '--dest-ca-cert' for reencrypt routes) remains compatible with '--external-certificate'.
lunarwhite
force-pushed
the
ext-cert-route-flag
branch
from
d821dc3 to
34b1b7d
Compare
2 participants
Closes #2254
Summary
Adds support for referencing TLS certificates from a TLS Secret when creating edge and reencrypt routes. The new
--external-certificateflag accepts a Secret name and populatesroute.Spec.TLS.ExternalCertificateas a LocalObjectReference.This flag is mutually exclusive with
--certand--key, since inline certificates and secret-backed certificates are alternative sources. The--ca-certflag (and--dest-ca-certfor reencrypt routes) remains compatible with--external-certificate.E2E Verification
edge route with --external-certificate
edge route with --external-certificate + --ca-cert
reencrypt route with --external-certificate + --dest-ca-cert
edge route w/o --external-certificate
[negative] edge route with --cert + --external-certificate
[negative] reencrypt route with --key + --external-certificate
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests