CFE-882: Route external certificate validation by thejasn · Pull Request #1549 · openshift/library-go

thejasn · 2023-07-05T12:39:23Z

Description

Updates route validations based on openshift/enhancements#1307 which introduces a new field in the route API externalCertificate behind the TP feature gate.

Changes

ValidateRoute() has been updated to pass additional args (secrets lister and subjectaccessreviewer) to validate
externalCertificate and the rbac required. Now it also does additional subject access review authorization checks to verify if the router SA has the correct permissions to parse externalCertificate. It also adds a hard requirement that the secret referenced in externalCertificate be present prior to creating the route.
ValidateHostUpdate() has also been updated since updating route host/subdomain is also affected by updating
certificates on the route. Now certificateChangeRequiresAuth() will always return true if the route has a externalCertificate set (check EP for additional info).
ValidateHostExternalCertificate() has been introduced as part of the validations done during route updates, this function specifically checks if the user has the correct permissions on the custom-host sub-resource. Additional details can be found on the EP.

openshift-ci-robot · 2023-07-05T12:39:26Z

@thejasn: This pull request references CFE-882 which is a valid jira issue.

Details

In 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 kubernetes/test-infra repository.

openshift-ci · 2023-07-05T12:39:28Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2023-08-02T06:10:02Z

@thejasn: This pull request references CFE-882 which is a valid jira issue.

Details

In response to this:

Description

Updates route validations based on openshift/enhancements#1307 which introduces a new field in the route API externalCertificate behind the TP feature gate.

Changes

ValidateRoute() has been updated to pass additional args (secrets lister and subjectaccessreviewer) to validate
externalCertificate and the rbac required. Now it also does additional subject access review authorization checks to verify if the router SA has the correct permissions to parse externalCertificate. It also adds a hard requirement that the secret referenced in externalCertificate be present prior to creating the route.

ValidateHostUpdate() has also been updated since updating route host/subdomain is also affected by updating
certificates on the route. Now certificateChangeRequiresAuth() will always return true if the route has a externalCertificate set (check EP for additional info).

ValidateHostExternalCertificate() has been introduced as part of the validations done during route updates, this function specifically checks if the user has the correct permissions on the custom-host sub-resource. Additional details can be found on the EP.

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 kubernetes/test-infra repository.

p0lyn0mial · 2023-08-03T12:56:55Z

/cc @p0lyn0mial

thejasn · 2023-10-11T08:33:41Z

/cc @alebedev87

Introduce new external certificate validation function to check if user has the required rbac when updating routes and using the ExternalCertificate field on the route.

Update all route validation functions to parse TP featuregate and validate ExternalCertificate field on the route.

openshift-ci · 2023-10-11T09:45:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: thejasn
Once this PR has been reviewed and has the lgtm label, please assign miciah for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

pkg/route/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alebedev87

First run: code oriented, not logic oriented.

pkg/route/common.go

alebedev87 · 2023-10-12T12:46:31Z

pkg/route/common.go

+	AllowExternalCertificates bool
+}
+
+func CheckRouteCustomHostSAR(ctx context.Context, fldPath *field.Path, sarc SubjectAccessReviewCreator) field.ErrorList {


Can you please provide a comment which verbs and sub resources are supposed to be checked?

pkg/route/validation/validation.go

alebedev87 · 2023-10-12T20:24:52Z

pkg/route/hostassignment/assignment_test.go

+		validType  bool
+		secretData map[string]string
+		validNS    bool


Same as in validation package. What do you think about providing corev1.Secret as a test case input?

pkg/route/hostassignment/assignment_test.go

pkg/route/hostassignment/externalcertificate.go

pkg/route/hostassignment/externalcertificate_test.go

openshift-ci · 2023-10-17T11:29:47Z

@thejasn: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes/test-infra repository. I understand the commands that are listed here.

chiragkyal · 2024-01-17T07:52:21Z

/close
Being covered in #1625

openshift-ci · 2024-01-17T07:52:38Z

@chiragkyal: Closed this PR.

Details

In response to this:

/close
Being covered in #1625

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 kubernetes/test-infra repository.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 5, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2023

thejasn changed the title ~~[WIP] CFE-882: Feature/route external certificate validation~~ [WIP] CFE-882: Route external certificate validation Jul 5, 2023

thejasn force-pushed the feature/route-external-certificate branch from bc7829a to 679f3fc Compare July 27, 2023 13:01

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2023

thejasn force-pushed the feature/route-external-certificate branch from 679f3fc to e6f2ca7 Compare July 27, 2023 13:08

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2023

thejasn changed the title ~~[WIP] CFE-882: Route external certificate validation~~ CFE-882: Route external certificate validation Jul 27, 2023

thejasn force-pushed the feature/route-external-certificate branch from e6f2ca7 to 31a4e26 Compare July 27, 2023 13:27

thejasn marked this pull request as ready for review July 27, 2023 14:03

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2023

openshift-ci bot requested review from Miciah and soltysh July 27, 2023 14:05

thejasn force-pushed the feature/route-external-certificate branch 2 times, most recently from f340708 to 2245efc Compare July 31, 2023 08:22

thejasn mentioned this pull request Jul 31, 2023

CFE-885:Feature route external certificate validation openshift/openshift-apiserver#385

Closed

thejasn force-pushed the feature/route-external-certificate branch from b9b6aac to 9e93e53 Compare August 1, 2023 09:23

thejasn force-pushed the feature/route-external-certificate branch from 9e93e53 to 90c9115 Compare August 3, 2023 05:02

thejasn mentioned this pull request Aug 8, 2023

UPSTREAM: <carry>: CFE-910: Update route external certificate validation openshift/kubernetes#1659

Closed

thejasn force-pushed the feature/route-external-certificate branch from 90c9115 to 2805597 Compare October 10, 2023 13:40

openshift-ci bot requested a review from alebedev87 October 11, 2023 08:33

thejasn added 2 commits October 11, 2023 15:12

Add ValidateHostExternalCertificate to validate routes

7b9e876

Introduce new external certificate validation function to check if user has the required rbac when updating routes and using the ExternalCertificate field on the route.

Update route validations to honour ExternalCertificate

971698e

Update all route validation functions to parse TP featuregate and validate ExternalCertificate field on the route.

thejasn force-pushed the feature/route-external-certificate branch from 2805597 to 971698e Compare October 11, 2023 09:42

alebedev87 reviewed Oct 12, 2023

View reviewed changes

Apply code review suggestions

186073d

thejasn force-pushed the feature/route-external-certificate branch from f276dc4 to 186073d Compare October 17, 2023 11:22

chiragkyal mentioned this pull request Nov 22, 2023

CFE-882: Route external certificate validation #1625

Merged

openshift-ci bot closed this Jan 17, 2024

Comments

Conversation

thejasn commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Uh oh!

openshift-ci-robot commented Jul 5, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jul 5, 2023

Uh oh!

openshift-ci-robot commented Aug 2, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Uh oh!

p0lyn0mial commented Aug 3, 2023

Uh oh!

thejasn commented Oct 11, 2023

Uh oh!

openshift-ci bot commented Oct 11, 2023

Uh oh!

alebedev87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alebedev87 Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

alebedev87 Nov 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alebedev87 Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openshift-ci bot commented Oct 17, 2023

Uh oh!

chiragkyal commented Jan 17, 2024

Uh oh!

openshift-ci bot commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

thejasn commented Jul 5, 2023 •

edited

Loading

openshift-ci-robot commented Jul 5, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 2, 2023 •

edited by openshift-ci bot

Loading