Bug 1906808: configobservation: override service-account-jwks-uri to use LB address #1020

stlaz · 2020-12-10T15:56:56Z

Without the override, the KAS would point to its own IP which is not
covered by the default serving certificate.

marun · 2020-12-10T16:13:58Z

pkg/operator/configobservation/auth/auth_serviceaccountissuer.go

+			"service-account-jwks-uri": []interface{}{
+				// We need to set the URL so that it points to our LB and doesn't
+				// default to KAS IP which isn't included in the serving certificate.
+				internalLBURL + "/openid/v1/jwks",


If sts or cco were to override the default issuer, would they need to be affect the value of service-account-jwks-uri?

Also, maybe an update to the enhancement is suggested?

You're right. If I understand the logic of this observer, this would have to be moved to the condition above when no issuer is set, correct?

Then there would be no need for changes when the default issuer is overriden, although we might consider allowing this modification in the future.

stlaz · 2020-12-11T13:00:39Z

Updated to only set the jwks uri when the issuer is unset, lets' see whether it can pass CI

openshift-ci-robot · 2020-12-11T13:23:59Z

@stlaz: This pull request references Bugzilla bug 1906808, which is invalid:

expected the bug to target the "4.7.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1906808: configobservation: override service-account-jwks-uri to use LB address

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.

stlaz · 2020-12-11T13:25:11Z

/bugzilla refresh

openshift-ci-robot · 2020-12-11T13:25:19Z

@stlaz: This pull request references Bugzilla bug 1906808, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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.

marun · 2020-12-11T16:19:42Z

pkg/operator/configobservation/auth/auth_serviceaccountissuer.go

 ) (map[string]interface{}, []error) {

 	issuer, errs := observedIssuer(existingConfig, authConfigAccessor)
-	config := unstructuredConfigForIssuer(issuer)
+
+	infrastructureConfig, err := infrastructureConfigAccessor("cluster")


Maybe only perform this step when the issuer length is zero? Otherwise an error could occur trying to retrieve something that isn't needed.

What you've done looks fine, but maybe could have been accomplished with less effort?

internalURL := "" if len(issuer) == 0 { ... internalURL = ... }

Without the override, the KAS would point to its own IP which is not covered by the default serving certificate.

stlaz · 2020-12-14T12:41:00Z

@marun I fixed the logic to only grab the infra config when an issuer is not set. PTAL

stlaz · 2020-12-14T13:22:41Z

/retest

marun · 2020-12-14T15:15:13Z

/lgtm

stlaz · 2020-12-15T08:43:19Z

/retest

openshift-merge-robot · 2020-12-15T11:54:13Z

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

Test name	Commit	Details	Rerun command
ci/prow/k8s-e2e-gcp	`14c367c`	link	`/test k8s-e2e-gcp`
ci/prow/e2e-aws	`14c367c`	link	`/test e2e-aws`
ci/prow/e2e-aws-operator	`14c367c`	link	`/test e2e-aws-operator`

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-01-05T08:12:18Z

/retest

sttts · 2021-01-05T09:02:04Z

pkg/operator/configobservation/auth/auth_serviceaccountissuer.go

 }

 // observedConfig returns an unstructured fragment of KubeAPIServerConfig that may
 // include an override of the default service account issuer if one was set in the
 // Authentication resource.
 func observedConfig(
 	existingConfig map[string]interface{},
-	authConfigAccessor func(string) (*v1.Authentication, error),
+	authConfigAccessor func(string) (*configv1.Authentication, error),


nit: accessor is a fixed term in apimachinery. Better: getAuthConfig, getInfrastructureConfig

sttts · 2021-01-05T09:03:59Z

pkg/operator/configobservation/auth/auth_serviceaccountissuer.go

 }

 // observedConfig returns an unstructured fragment of KubeAPIServerConfig that may
 // include an override of the default service account issuer if one was set in the
 // Authentication resource.
 func observedConfig(
 	existingConfig map[string]interface{},
-	authConfigAccessor func(string) (*v1.Authentication, error),
+	authConfigAccessor func(string) (*configv1.Authentication, error),
+	infrastructureConfigAccessor func(string) (*configv1.Infrastructure, error),
 ) (map[string]interface{}, []error) {

 	issuer, errs := observedIssuer(existingConfig, authConfigAccessor)


what can errs be? Keep going with all the logic is a little confusing. At least doc some rational why.

sttts · 2021-01-05T09:04:07Z

pkg/operator/configobservation/auth/auth_serviceaccountissuer.go

+			},
+		},
+	}, errs
+


drop empty line

sttts · 2021-01-05T09:04:33Z

nits

sttts · 2021-01-05T09:07:37Z

/approve

Follow-up for nits.

openshift-ci-robot · 2021-01-05T09:07:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marun, stlaz, sttts

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~~ [sttts]

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

openshift-bot · 2021-01-05T09:42:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-05T11:39:16Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2021-01-05T18:05:34Z

@stlaz: All pull requests linked via external trackers have merged:

openshift/cluster-kube-apiserver-operator#1020

Bugzilla bug 1906808 has been moved to the MODIFIED state.

In response to this:

Bug 1906808: configobservation: override service-account-jwks-uri to use LB address

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 assigned marun Dec 10, 2020

openshift-ci-robot requested a review from tnozicka December 10, 2020 15:56

marun reviewed Dec 10, 2020

View reviewed changes

stlaz force-pushed the sa_issuer_jwt branch from 29ac35a to 3225472 Compare December 11, 2020 12:59

stlaz changed the title ~~Rebase: configobservation: override service-account-jwks-uri to use LB address~~ Bug 1906808: configobservation: override service-account-jwks-uri to use LB address Dec 11, 2020

openshift-ci-robot added the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Dec 11, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Dec 11, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Dec 11, 2020

marun reviewed Dec 11, 2020

View reviewed changes

configobservation: override service-account-jwks-uri to use LB address

14c367c

Without the override, the KAS would point to its own IP which is not covered by the default serving certificate.

stlaz force-pushed the sa_issuer_jwt branch from 3225472 to 14c367c Compare December 14, 2020 12:40

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2020

sttts reviewed Jan 5, 2021

View reviewed changes

pkg/operator/configobservation/auth/auth_serviceaccountissuer.go

},

},

}, errs

Copy link

Contributor

sttts Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop empty line

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2021

stlaz mentioned this pull request Jan 5, 2021

Bug 1906808: follow-up fixes to SA-issuer observer #1025

Merged

openshift-merge-robot merged commit 7ac187a into openshift:master Jan 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1906808: configobservation: override service-account-jwks-uri to use LB address #1020

Bug 1906808: configobservation: override service-account-jwks-uri to use LB address #1020

stlaz commented Dec 10, 2020

marun Dec 10, 2020 •

edited

stlaz Dec 11, 2020 •

edited

stlaz commented Dec 11, 2020

openshift-ci-robot commented Dec 11, 2020

stlaz commented Dec 11, 2020

openshift-ci-robot commented Dec 11, 2020

marun Dec 11, 2020

marun Dec 14, 2020

stlaz commented Dec 14, 2020

stlaz commented Dec 14, 2020

marun commented Dec 14, 2020

stlaz commented Dec 15, 2020

openshift-merge-robot commented Dec 15, 2020

stlaz commented Jan 5, 2021

sttts Jan 5, 2021

sttts Jan 5, 2021

sttts Jan 5, 2021

sttts commented Jan 5, 2021

sttts commented Jan 5, 2021

openshift-ci-robot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-ci-robot commented Jan 5, 2021

Bug 1906808: configobservation: override service-account-jwks-uri to use LB address #1020

Bug 1906808: configobservation: override service-account-jwks-uri to use LB address #1020

Conversation

stlaz commented Dec 10, 2020

marun Dec 10, 2020 • edited

Choose a reason for hiding this comment

stlaz Dec 11, 2020 • edited

Choose a reason for hiding this comment

stlaz commented Dec 11, 2020

openshift-ci-robot commented Dec 11, 2020

stlaz commented Dec 11, 2020

openshift-ci-robot commented Dec 11, 2020

marun Dec 11, 2020

Choose a reason for hiding this comment

marun Dec 14, 2020

Choose a reason for hiding this comment

stlaz commented Dec 14, 2020

stlaz commented Dec 14, 2020

marun commented Dec 14, 2020

stlaz commented Dec 15, 2020

openshift-merge-robot commented Dec 15, 2020

stlaz commented Jan 5, 2021

sttts Jan 5, 2021

Choose a reason for hiding this comment

sttts Jan 5, 2021

Choose a reason for hiding this comment

sttts Jan 5, 2021

Choose a reason for hiding this comment

sttts commented Jan 5, 2021

sttts commented Jan 5, 2021

openshift-ci-robot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-bot commented Jan 5, 2021

openshift-ci-robot commented Jan 5, 2021

marun Dec 10, 2020 •

edited

stlaz Dec 11, 2020 •

edited