Custom Certs for OAuth Route #430

awgreene · 2021-04-06T14:17:40Z

No description provided.

openshift-ci-robot · 2021-04-06T14:20:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: awgreene
To complete the pull request process, please assign sttts after the PR has been reviewed.
You can assign the PR to them by writing /assign @sttts in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

stlaz

Does the host of the route get changed anywhere?

pkg/controllers/configobservation/customroutersecret/observe_custom_router_secret.go

pkg/controllers/configobservation/routersecret/observe_router_secret.go

pkg/controllers/deployment/default_deployment.go

awgreene · 2021-04-06T15:43:53Z

Does the host of the route get changed anywhere?
@stlaz not yet.

stlaz

there now appears to be both the controller and the observer to sync the secret. Keep the controller, add ingress status reporting about the route.

Route hostname change should still happen, right?

edit: I did not notice the observer getting remove in the later commit. Squash please.

pkg/controllers/configobservation/customroutersecret/observe_custom_router_secret.go

pkg/controllers/configobservation/routersecret/observe_router_secret.go

pkg/controllers/metadata/metadata_controller.go

pkg/controllers/componentroutesecretsync/component_route_secret_sync_controller.go

pkg/controllers/componentroutestatus/ingress_component_route.go

pkg/controllers/common/ingress_config.go

stlaz

merge the status and secret sync controllers
the componentRoutes in status should have Conditions describing their current state (see the enhancement)
the componentRoutes in status should have consumingUsers set
don't pass variables by value unless you've got a good reason to do so, your stack might start hating you for that

pkg/controllers/common/ingress_config.go

pkg/controllers/componentroutesecretsync/component_route_secret_sync_controller.go

pkg/controllers/componentroutestatus/ingress_component_route.go

pkg/controllers/configobservation/routersecret/observe_router_secret.go

stlaz · 2021-04-09T14:18:30Z

I just realized: should we use the "CustomRoute" instead of "ComponentRoute" terminology in the controller name(s)? IMO it makes more sense to call it that

awgreene · 2021-04-09T20:36:07Z

I just realized: should we use the "CustomRoute" instead of "ComponentRoute" terminology in the controller name(s)? IMO it makes more sense to call it that

Done

awgreene · 2021-04-09T20:39:07Z

the componentRoutes in status should have consumingUsers set

@stlaz I do not believe that we need to set this value. Doing so would cause the ingress-controller to generate RBAC for the provided users. This controller already has the rbac necessary to read the admin provided secrets.

pkg/controllers/oauthendpoints/oauth_endpoints_controller.go

stlaz

needs an e2e test

pkg/controllers/configobservation/routersecret/observe_router_secret.go

pkg/controllers/configobservation/routersecret/observe_router_secret_test.go

pkg/controllers/configobservation/routersecret/observe_router_secret.go

pkg/controllers/configobservation/configobservercontroller/observe_config_controller.go

pkg/controllers/customroute/custom_route_controller.go

pkg/controllers/oauthclientscontroller/oauthclientscontroller.go

pkg/controllers/oauthendpoints/oauth_endpoints_controller.go

stlaz · 2021-04-14T11:27:18Z

I do not believe that we need to set this value. Doing so would cause the ingress-controller to generate RBAC for the provided users. This controller already has the rbac necessary to read the admin provided secrets.

In the unlikely event we decide to narrow down the RBAC of this operator to only the resources it needs (even though it basically sets up the components that decide who is who), this would still break. Also, this would be helpful if we were to move this controller to library-go.

stlaz · 2021-04-15T13:13:32Z

pkg/controllers/customroute/custom_route_controller.go

+	if len(conditions) == 0 {
+		return []metav1.Condition{
+			{
+				Type:               "Available",


the enhancement and the API do not speak of an "Available" condition

can you think of a scenario where we could go "progressing=true"?

can you think of a scenario where we could go "progressing=true"?

Degraded could be set to true if the secret is invalid.
Progressing could be set to true if the secret is valid and the deployment is rolling out with the new secrets,

pkg/controllers/configobservation/configobservercontroller/observe_config_controller.go

pkg/controllers/customroute/custom_route_controller.go

stlaz · 2021-04-15T14:18:19Z

pkg/controllers/customroute/custom_route_controller.go

+		hostname := ""
+		if route != nil {
+			hostname = route.Spec.Host
+		}
+		conditions = append(conditions, c.updateIngressConfigStatus(ctx, hostname, componentRouteConditions(conditions), ingress)...)


this should be outside this function

pkg/controllers/metadata/metadata_controller.go

pkg/operator/assets/bindata.go

slaskawi

One possible (small) issue found. Otherwise LGTM

/lgtm

slaskawi · 2021-06-25T08:31:17Z

test/e2e/custom_route_test.go

+
+	// check that the hostname was updated
+	err = checkRouteHostname(t, routeClient, "openshift-authentication", "oauth-openshift", "foo.bar.com")
+


I think we need require.NoError(t, err) here.

stlaz · 2021-06-28T11:28:03Z

/lgtm cancel
Unit tests are failing:

FAIL: github.com/openshift/cluster-authentication-operator/pkg/controllers/routercerts TestValidateRouterCertificates/CustomSecretMissingRouterCertsPEM 0s
FAIL: github.com/openshift/cluster-authentication-operator/pkg/controllers/routercerts TestValidateRouterCertificates/CustomSecretMalformedRouterCertsPEM 0s
FAIL: github.com/openshift/cluster-authentication-operator/pkg/controllers/routercerts TestValidateRouterCertificates/CustomSecretNoServerCertRouterCerts 0s
FAIL: github.com/openshift/cluster-authentication-operator/pkg/controllers/routercerts TestValidateRouterCertificates/CustomSecretInvalidServerCertRouterCertsBadChain 0s
FAIL: github.com/openshift/cluster-authentication-operator/pkg/controllers/routercerts TestValidateRouterCertificates 19.49s

awgreene · 2021-07-08T17:34:46Z

/retest

awgreene · 2021-07-08T17:49:38Z

/retest

awgreene · 2021-07-09T01:07:18Z

/retest

awgreene · 2021-07-09T05:12:46Z

/retest

slaskawi

/lgtm

stlaz · 2021-07-09T10:56:15Z

pkg/operator/datasync/validation.go

+	if numCerts := len(certs); numCerts != 1 {
+		return append(errs, fmt.Errorf("expected a single server certificate, got %d", numCerts))
+	}


The server cert PEM will likely contain the full certificate chain so I would remove this check

Will update the check to expect at least one cert.

This commit introduces a number of changes that allow the Cluster-Authentication-Operator to customize the openshift-authentication/oauth-openshift route's hostname and serving certificate when a user provides this information via the cluster ingress config.

stlaz · 2021-07-09T13:18:11Z

/lgtm

openshift-ci · 2021-07-09T13:18:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, slaskawi, stlaz

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [stlaz]

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

openshift-ci · 2021-07-09T15:33:51Z

@awgreene: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-operator-encryption-perf	`14554e5`	link	`/test e2e-aws-operator-encryption-perf`
ci/prow/e2e-agnostic-ipv6	`7c29d66`	link	`/test e2e-agnostic-ipv6`

Full PR test history. Your PR dashboard.

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.

stlaz · 2021-07-12T13:32:33Z

/hold cancel

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 6, 2021

openshift-ci-robot requested review from stlaz and sttts April 6, 2021 14:21

awgreene force-pushed the custom-routes branch from cf74f49 to 1ca92c0 Compare April 6, 2021 14:21

stlaz requested changes Apr 6, 2021

View reviewed changes

awgreene force-pushed the custom-routes branch from 1ca92c0 to 47207ac Compare April 6, 2021 15:40

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2021

awgreene force-pushed the custom-routes branch 2 times, most recently from e049f49 to 1b61037 Compare April 7, 2021 16:05

stlaz requested changes Apr 8, 2021

View reviewed changes

awgreene force-pushed the custom-routes branch 2 times, most recently from 547c451 to de2397e Compare April 8, 2021 18:12

awgreene commented Apr 8, 2021

View reviewed changes

pkg/controllers/componentroutestatus/ingress_component_route.go Outdated Show resolved Hide resolved

EmilyM1 reviewed Apr 8, 2021

View reviewed changes

pkg/controllers/common/ingress_config.go Outdated Show resolved Hide resolved

stlaz requested changes Apr 9, 2021

View reviewed changes

awgreene force-pushed the custom-routes branch from 405dee8 to 5c19dae Compare April 9, 2021 20:02

awgreene force-pushed the custom-routes branch from 5c19dae to 8b456e3 Compare April 12, 2021 19:10

awgreene commented Apr 12, 2021

View reviewed changes

pkg/controllers/oauthendpoints/oauth_endpoints_controller.go Outdated Show resolved Hide resolved

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 12, 2021

awgreene force-pushed the custom-routes branch from 8b456e3 to 87efa7d Compare April 12, 2021 20:17

stlaz requested changes Apr 14, 2021

View reviewed changes

awgreene force-pushed the custom-routes branch from 87efa7d to bcfdf63 Compare April 15, 2021 13:00

stlaz requested changes Apr 15, 2021

View reviewed changes

slaskawi approved these changes Jun 25, 2021

View reviewed changes

openshift-ci bot assigned slaskawi Jun 25, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2021

openshift-ci bot assigned stlaz Jun 28, 2021

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2021

slaskawi mentioned this pull request Jun 29, 2021

[enterprise-4.9] Custom certs for the oAuth server route openshift/openshift-docs#34077

Closed

awgreene force-pushed the custom-routes branch from 69542f6 to e88a8ae Compare June 29, 2021 12:58

update bindata

5cc62d0

awgreene force-pushed the custom-routes branch 2 times, most recently from 4a6a3fa to 1f247db Compare July 8, 2021 15:25

slaskawi approved these changes Jul 9, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2021

stlaz requested changes Jul 9, 2021

View reviewed changes

awgreene force-pushed the custom-routes branch from 1f247db to 7c29d66 Compare July 9, 2021 13:10

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2021

openshift-merge-robot merged commit 65cd3f0 into openshift:master Jul 12, 2021

slaskawi mentioned this pull request Jul 20, 2021

Custom CA Certs for Route helpers openshift/library-go#1138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Certs for OAuth Route #430

Custom Certs for OAuth Route #430

awgreene commented Apr 6, 2021

openshift-ci-robot commented Apr 6, 2021

stlaz left a comment

awgreene commented Apr 6, 2021

stlaz left a comment •

edited

stlaz left a comment

stlaz commented Apr 9, 2021

awgreene commented Apr 9, 2021

awgreene commented Apr 9, 2021

stlaz left a comment

stlaz commented Apr 14, 2021

stlaz Apr 15, 2021

stlaz Apr 15, 2021

awgreene Apr 15, 2021

stlaz Apr 15, 2021

slaskawi left a comment

slaskawi Jun 25, 2021

stlaz commented Jun 28, 2021

awgreene commented Jul 8, 2021

awgreene commented Jul 8, 2021

awgreene commented Jul 9, 2021

awgreene commented Jul 9, 2021

slaskawi left a comment

stlaz Jul 9, 2021

awgreene Jul 9, 2021

stlaz commented Jul 9, 2021

openshift-ci bot commented Jul 9, 2021

openshift-ci bot commented Jul 9, 2021

stlaz commented Jul 12, 2021


		// check that the hostname was updated
		err = checkRouteHostname(t, routeClient, "openshift-authentication", "oauth-openshift", "foo.bar.com")

Custom Certs for OAuth Route #430

Custom Certs for OAuth Route #430

Conversation

awgreene commented Apr 6, 2021

openshift-ci-robot commented Apr 6, 2021

stlaz left a comment

Choose a reason for hiding this comment

awgreene commented Apr 6, 2021

stlaz left a comment • edited

Choose a reason for hiding this comment

stlaz left a comment

Choose a reason for hiding this comment

stlaz commented Apr 9, 2021

awgreene commented Apr 9, 2021

awgreene commented Apr 9, 2021

stlaz left a comment

Choose a reason for hiding this comment

stlaz commented Apr 14, 2021

stlaz Apr 15, 2021

Choose a reason for hiding this comment

stlaz Apr 15, 2021

Choose a reason for hiding this comment

awgreene Apr 15, 2021

Choose a reason for hiding this comment

stlaz Apr 15, 2021

Choose a reason for hiding this comment

slaskawi left a comment

Choose a reason for hiding this comment

slaskawi Jun 25, 2021

Choose a reason for hiding this comment

stlaz commented Jun 28, 2021

awgreene commented Jul 8, 2021

awgreene commented Jul 8, 2021

awgreene commented Jul 9, 2021

awgreene commented Jul 9, 2021

slaskawi left a comment

Choose a reason for hiding this comment

stlaz Jul 9, 2021

Choose a reason for hiding this comment

awgreene Jul 9, 2021

Choose a reason for hiding this comment

stlaz commented Jul 9, 2021

openshift-ci bot commented Jul 9, 2021

openshift-ci bot commented Jul 9, 2021

stlaz commented Jul 12, 2021

stlaz left a comment •

edited