OCPBUGS-74511: remove RouteExternalCertificate feature gate#604
OCPBUGS-74511: remove RouteExternalCertificate feature gate#604jcmoraisjr wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-74511, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRemoved feature-gate handling and config fields for Route External Certificates; route strategy and storage signatures were simplified and validation now unconditionally allows external certificates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenshiftAPIServer
participant RouteAPIServer
participant Strategy
participant Etcd
Client->>OpenshiftAPIServer: Create Route (with ExternalCertificate)
OpenshiftAPIServer->>RouteAPIServer: forward request
RouteAPIServer->>Strategy: validate/prepare Route
Strategy-->>RouteAPIServer: validation passes (ExternalCertificate preserved)
RouteAPIServer->>Etcd: persist Route
Etcd-->>RouteAPIServer: stored
RouteAPIServer-->>Client: respond Created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
RouteExternalCertificate is now enabled by default. This update is removing references to this feature gate, hardcoding its value to true. This update is also a pre-requisite to remove it from openshift/cluster-ingress-operator.
jcmoraisjr
force-pushed
the
OCPBUGS-74511-remove-featuregate
branch
from
17f52c2 to
413f82a
Compare
|
Marking this PR as verified as this openshift/cluster-ingress-operator#1355 (comment) |
openshift-ci-robot
added
the
verified
|
@melvinjoseph86: This PR has been marked as verified by 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. |
|
/retest |
|
/assign |
openshift-ci-robot
removed
the
verified
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-74511, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/route/apiserver/registry/route/strategy_test.go (1)
964-993: Consider whether these test cases are still meaningful.The test cases "certificate-changed-to-external-certificate-denied-and-featuregate-is-not-set" and "certificate-changed-to-external-certificate-allowed-but-featuregate-is-not-set" set
opts.AllowExternalCertificates: false, but sinceNewStrategynow hardcodesAllowExternalCertificates: trueinrouteValidationOptions(), these test cases may no longer exercise the intended code path.If the goal is to test behavior when external certificates are disallowed, note that the strategy will always allow them now. These tests might be retained for historical documentation or to ensure the underlying validation logic still handles the
falsecase, but they won't reflect actual runtime behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/route/apiserver/registry/route/strategy_test.go` around lines 964 - 993, The two test cases (names: "certificate-changed-to-external-certificate-denied-and-featuregate-is-not-set" and "certificate-changed-to-external-certificate-allowed-but-featuregate-is-not-set") are no longer exercising a disallowed-external-certificate path because NewStrategy() hardcodes AllowExternalCertificates: true inside routeValidationOptions(); update the tests to reflect runtime behavior by either (A) setting opts.AllowExternalCertificates to true so the test asserts the actual strategy behavior, or (B) remove/mark these cases as historical and instead add a focused unit test that calls the underlying validation function (route.ValidateRoute or whatever function performs TLS validation) directly with AllowExternalCertificates=false to verify legacy validation logic; refer to NewStrategy, routeValidationOptions, and the two test case names when making the change.
🤖 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/route/apiserver/registry/route/strategy_test.go`:
- Around line 964-993: The two test cases (names:
"certificate-changed-to-external-certificate-denied-and-featuregate-is-not-set"
and
"certificate-changed-to-external-certificate-allowed-but-featuregate-is-not-set")
are no longer exercising a disallowed-external-certificate path because
NewStrategy() hardcodes AllowExternalCertificates: true inside
routeValidationOptions(); update the tests to reflect runtime behavior by either
(A) setting opts.AllowExternalCertificates to true so the test asserts the
actual strategy behavior, or (B) remove/mark these cases as historical and
instead add a focused unit test that calls the underlying validation function
(route.ValidateRoute or whatever function performs TLS validation) directly with
AllowExternalCertificates=false to verify legacy validation logic; refer to
NewStrategy, routeValidationOptions, and the two test case names when making the
change.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
pkg/cmd/openshift-apiserver/openshiftapiserver/openshift_apiserver.gopkg/route/apiserver/apiserver.gopkg/route/apiserver/registry/route/etcd/etcd.gopkg/route/apiserver/registry/route/etcd/etcd_test.gopkg/route/apiserver/registry/route/strategy.gopkg/route/apiserver/registry/route/strategy_test.go
💤 Files with no reviewable changes (1)
- pkg/route/apiserver/apiserver.go
jcmoraisjr
force-pushed
the
OCPBUGS-74511-remove-featuregate
branch
from
ae7242d to
4bb41e2
Compare
AllowExternalCertificates is always true. This is removing all fields, attributes and a unit test specific for this flag.
jcmoraisjr
force-pushed
the
OCPBUGS-74511-remove-featuregate
branch
from
4bb41e2 to
23c556b
Compare
|
/retest |
|
@jcmoraisjr: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/lgtm Thanks! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jcmoraisjr, rikatz 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 |
|
/cc @p0lyn0mial @benluddy @sanchezl @dgrisonnet |
openshift-ci
bot
requested review from
ShudiLi,
benluddy,
lihongan,
p0lyn0mial and
rhamini3
|
re-applying verified label based on #604 (comment) |
openshift-ci-robot
added
the
verified
|
@melvinjoseph86: This PR has been marked as verified by 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. |
5 participants
RouteExternalCertificate is now enabled by default. This update is removing references to this feature gate, hardcoding its value to true. This update is also a pre-requisite to remove it from openshift/cluster-ingress-operator.
Summary by CodeRabbit