feat(cli): add --external-certificate flag to oc create route edge/reencrypt#2299
feat(cli): add --external-certificate flag to oc create route edge/reencrypt#2299Dasmat13 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Dasmat13 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 |
|
Hi @Dasmat13. 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 Regular contributors should join the org to skip this step. 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. |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughAdds ChangesExternal certificate support for route creation
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CreateEdgeRouteOptions
participant RouteSpec
User->>CreateEdgeRouteOptions: oc create route edge --external-certificate=secret
CreateEdgeRouteOptions->>CreateEdgeRouteOptions: Validate()
alt ExternalCertificate set
CreateEdgeRouteOptions->>RouteSpec: set TLS.ExternalCertificate
else Inline TLS files
CreateEdgeRouteOptions->>RouteSpec: set TLS.Certificate, Key, CACertificate
end
Related issues: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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: 2
♻️ Duplicate comments (2)
pkg/cli/create/routereenecrypt.go (2)
131-152: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame validation-placement and silent
--ca-certdrop issue asrouteedge.go.The mutual-exclusivity check should live in
Validate(), notRun(), and--ca-certcombined with--external-certificateis silently dropped without warning.As per coding guidelines, "
Validatechecks invariants without side effects,Runexecutes."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/create/routereenecrypt.go` around lines 131 - 152, Move the TLS flag invariant checks out of the Run path in createRoutereenecrypt and into its Validate method, matching the pattern used elsewhere. In the Run logic that builds route.Spec.TLS, keep only the data-loading/assignment work and ensure --ca-cert is not silently ignored when --external-certificate is set; instead, Validate should reject that combination alongside --cert/--key. Use the existing createRoutereenecrypt command type and its Validate/Run methods to locate the change.Source: Coding guidelines
135-135: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftSame
LocalObjectReferencetype mismatch asrouteedge.go.Same issue as flagged in
pkg/cli/create/routeedge.goLine 124:corev1.LocalObjectReferenceis not the field's declared type;route/v1.TLSConfig.ExternalCertificateexpects*route/v1.LocalObjectReference, a distinct type defined within theroute/v1package.🐛 Proposed fix
- route.Spec.TLS.ExternalCertificate = &corev1.LocalObjectReference{Name: o.ExternalCertificate} + route.Spec.TLS.ExternalCertificate = &routev1.LocalObjectReference{Name: o.ExternalCertificate}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/create/routereenecrypt.go` at line 135, The assignment to route.Spec.TLS.ExternalCertificate uses the wrong LocalObjectReference type, since TLSConfig.ExternalCertificate expects the route/v1 version rather than corev1. Update the routereenecrypt.go logic that sets ExternalCertificate to construct the reference from the route/v1 package so the field type matches what route.Spec.TLS requires, mirroring the fix needed in routeedge.go.
🧹 Nitpick comments (2)
pkg/cli/create/routeedge.go (2)
100-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for new flag/branching logic.
No test changes accompany this behavioral change. As per path instructions, all changes must include unit test additions/changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/create/routeedge.go` around lines 100 - 159, Add or update unit tests for CreateEdgeRouteOptions.Run to cover the new branching introduced around ExternalCertificate versus Cert/Key/CACert and the TLS setup path. Verify the CreateEdgeRouteOptions and route.UnsecuredRoute flow handles the mutually exclusive flag case, the external-certificate branch, and the file loading branch correctly, including the expected error when incompatible flags are combined. Ensure the tests exercise the updated route.Spec.TLS fields and any DryRun/Create behavior affected by this logic.Source: Path instructions
119-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated cert/external-certificate branching logic with
routereenecrypt.go.The same 20-line block (mutual-exclusivity check + TLS field assignment) is duplicated verbatim in
pkg/cli/create/routereenecrypt.go(Lines 131-152). Consider extracting a shared helper (e.g. in a common file underpkg/cli/create/) that bothedgeandreencryptcall, takingexternalCertificate, cert, key, caCert stringand returning a populated*routev1.TLSConfigfragment or error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/create/routeedge.go` around lines 119 - 141, The TLS certificate/external-certificate branching in the route creation flow is duplicated between the edge and reencrypt paths, so extract the shared mutual-exclusivity and TLS assignment logic into a common helper under pkg/cli/create and have both RouteEdge and RouteReencrypt call it. Keep the helper focused on taking the certificate-related inputs, performing the --external-certificate vs --cert/--key validation, loading CA/cert/key data, and returning the populated TLS config fragment or an error so both callers reuse the same implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cli/create/routeedge.go`:
- Around line 119-141: Move the TLS flag conflict checks out of `Run()` in
`CreateRouteEdge.Run` and into `CreateRouteEdge.Validate`, following the
Complete/Validate/Run convention so invariants are rejected before any setup or
mutation occurs. Add explicit validation for `--external-certificate` being
incompatible with `--cert`, `--key`, and also `--ca-cert`, since that
combination is currently accepted and then ignored. Keep `Run()` focused on
building the route spec and loading files only after `Validate()` has approved
the inputs.
- Line 124: The assignment to route.Spec.TLS.ExternalCertificate is using the
wrong reference type, causing a compile-time mismatch. Update the code in the
route edge creation path to use routev1.LocalObjectReference instead of
corev1.LocalObjectReference when setting ExternalCertificate, and keep the fix
scoped to the relevant route.Spec.TLS setup logic in routeedge.go.
---
Duplicate comments:
In `@pkg/cli/create/routereenecrypt.go`:
- Around line 131-152: Move the TLS flag invariant checks out of the Run path in
createRoutereenecrypt and into its Validate method, matching the pattern used
elsewhere. In the Run logic that builds route.Spec.TLS, keep only the
data-loading/assignment work and ensure --ca-cert is not silently ignored when
--external-certificate is set; instead, Validate should reject that combination
alongside --cert/--key. Use the existing createRoutereenecrypt command type and
its Validate/Run methods to locate the change.
- Line 135: The assignment to route.Spec.TLS.ExternalCertificate uses the wrong
LocalObjectReference type, since TLSConfig.ExternalCertificate expects the
route/v1 version rather than corev1. Update the routereenecrypt.go logic that
sets ExternalCertificate to construct the reference from the route/v1 package so
the field type matches what route.Spec.TLS requires, mirroring the fix needed in
routeedge.go.
---
Nitpick comments:
In `@pkg/cli/create/routeedge.go`:
- Around line 100-159: Add or update unit tests for CreateEdgeRouteOptions.Run
to cover the new branching introduced around ExternalCertificate versus
Cert/Key/CACert and the TLS setup path. Verify the CreateEdgeRouteOptions and
route.UnsecuredRoute flow handles the mutually exclusive flag case, the
external-certificate branch, and the file loading branch correctly, including
the expected error when incompatible flags are combined. Ensure the tests
exercise the updated route.Spec.TLS fields and any DryRun/Create behavior
affected by this logic.
- Around line 119-141: The TLS certificate/external-certificate branching in the
route creation flow is duplicated between the edge and reencrypt paths, so
extract the shared mutual-exclusivity and TLS assignment logic into a common
helper under pkg/cli/create and have both RouteEdge and RouteReencrypt call it.
Keep the helper focused on taking the certificate-related inputs, performing the
--external-certificate vs --cert/--key validation, loading CA/cert/key data, and
returning the populated TLS config fragment or an error so both callers reuse
the same implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 76b79101-ddac-4210-a3bd-111bc338ba0e
📒 Files selected for processing (2)
pkg/cli/create/routeedge.gopkg/cli/create/routereenecrypt.go
…encrypt Add support for spec.tls.externalCertificate (GA in OCP 4.19) to both 'oc create route edge' and 'oc create route reencrypt' commands. This allows users to reference a kubernetes.io/tls Secret by name at route creation time, enabling integration with external certificate managers like cert-manager, without resorting to post-creation patching. Behavior: - --external-certificate=<secret-name> sets spec.tls.externalCertificate - Mutually exclusive with --cert and --key (returns error if combined) - Passthrough routes are out of scope (no TLS termination at router) Closes: openshift#2254 Signed-off-by: Dasmat13 <dasmat@smail.iitm.ac.in>
Dasmat13
force-pushed
the
feature/external-certificate-route
branch
from
6313a82 to
e9a1881
Compare
1 participant
Summary
Adds support for
spec.tls.externalCertificate(GA in OCP 4.19) to bothoc create route edgeandoc create route reencryptcommands via a new--external-certificateflag.Closes #2254
Problem
oc create route edgeandoc create route reencrypthave no way to setspec.tls.externalCertificateat creation time. Users who integrate cert-manager or other external certificate managers must resort tooc patch,oc edit, or raw YAML after creating the route.Solution
Added
--external-certificate=<secret-name>flag to both subcommands:spec.tls.externalCertificate = &LocalObjectReference{Name: secretName}--certand--key(returns an error if combined)Usage
Changes
pkg/cli/create/routeedge.go: AddedExternalCertificatefield, flag registration, validation, andspec.tls.externalCertificateassignmentpkg/cli/create/routereenecrypt.go: Same changes as aboveTesting
spec.tls.externalCertificatepopulationoc create route edge --external-certificate=<secret>produces correct Route manifestSummary by CodeRabbit