Enable overriding service account issuer for bound tokens #569

marun · 2020-01-18T08:43:40Z

Implements support for overriding the service account issuer as per openshift/enhancements#150:

sttts · 2020-01-20T08:58:17Z

config/v1/types_authentication.go

+
+	// serviceAccountIssuer is the identifier of the bound service account token issuer.
+	// +optional
+	ServiceAccountIssuer string `json:"serviceAccountIssuer"`


is there any special format? Is it a URL ?

Is there a default value?

maybe serviceAccountIssuer configures the issuer for bound service account tokens would make more sense as an explanation for a common user?

It's just a string. It can be configured with a URL to source the public key to verify issued tokens (as is the case for the EKS webhook), but it can also just be a textual value used by the verifier in kube-apiserver to identify that a token was issued by it.

As to the question of a default value, the operator will set auth.openshift.io in defaultconfig.yaml. Does that need to be documented here given that the API isn't the one defaulting it?

It's just a string. It can be configured with a URL to source the public key to verify issued tokens (as is the case for the EKS webhook), but it can also just be a textual value used by the verifier in kube-apiserver to identify that a token was issued by it.

Provide the recommended options in the godoc.

As to the question of a default value, the operator will set auth.openshift.io in defaultconfig.yaml. Does that need to be documented here given that the API isn't the one defaulting it?

Yes, the behavior of the system and the repercussions of choices needs to be described here.

something like [^:]*|someURLRegEx

Wait, why don't I want to allow an empty string given that will be the default?

We could. But then we have two ways to express that: empty and undefined. Kind of ugly.

A 'url regex' is a non-trivial thing (as per https://mathiasbynens.be/demo/url-regex). I get that it's desirable to validate up-front, but having to validate complex data declaratively like this seems error prone. Do you still want regex valiation, or would it make sense to instead do a url check in code when the field is observed and report an error in the operator logs or an event?

am fine with later validation and reporting via e.g. a condition

stlaz · 2020-01-20T12:37:17Z

kubecontrolplane/v1/types.go

+	// issuer.
+	ServiceAccountIssuer string `json:"serviceAccountIssuer"`
+
+	// apiAudiences is a list of identifies of the API. The service account token


nit: identifiers

deads2k · 2020-01-20T21:18:38Z

kubecontrolplane/v1/types.go


+	// serviceAccountIssuer is the identifier of the bound service account token
+	// issuer. If not provided, a default value of "auth.openshift.io" will be used.
+	ServiceAccountIssuer string `json:"serviceAccountIssuer"`


these can be passed as flags, right? If we can use flags, then we don't add the types here. We want to eliminate this type over time and only use the upstream provided values.

That sound preferable, but it is not clear from reading the code how exactly these flags would be specified by the operator (or overridden by an administrator). Are there docs/examples/code/etc you can point me to?

sttts · 2020-01-22T11:26:52Z

/lgtm
/approve

openshift-ci-robot · 2020-01-22T11:27:16Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

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

In particular, to get openshift/api#585: config: disable IPv6DualStack feature flag so we don't launch kube-controller-manager with that (still-broken) feature Also includes: openshift/api#557: create the IBMCLoudPlatform type for the ingress operator try2 openshift/api#570: Clarify image config doc openshift/api#569: Enable overriding service account issuer for bound tokens openshift/api#527: Add kubebuilder annotations to the network types openshift/api#574: add deprecaction notice for build pipeline strategy openshift/api#582: config/v1/types_proxy: Clarify trustedCA semantics openshift/api#583: Clarify FROM behavior in builds openshift/api#573: Add CRD generator documentation to Readme openshift/api#576: Remove Description from CLI output to improve its display openshift/api#589: Add missing enum validations openshift/api#583: operator/ingress: add dnsrecord type

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 18, 2020

openshift-ci-robot requested review from abhinavdahiya and adambkaplan January 18, 2020 08:44

This was referenced Jan 18, 2020

WIP Enable configuration of bound service account tokens openshift/origin#24416

Closed

Enable bound SA tokens openshift/cluster-kube-apiserver-operator#718

Merged

sttts reviewed Jan 20, 2020

View reviewed changes

stlaz reviewed Jan 20, 2020

View reviewed changes

deads2k reviewed Jan 20, 2020

View reviewed changes

Enable configuration of the issuer of bound service account tokens

5637f56

marun changed the title ~~Enable bound service account tokens~~ Enable overriding service account issuer for bound tokens Jan 21, 2020

Updated generated

5e3a31c

openshift-ci-robot assigned sttts Jan 22, 2020

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

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

openshift-merge-robot merged commit 1108c9a into openshift:master Jan 22, 2020

danwinship mentioned this pull request Mar 2, 2020

Bug 1809368: bump openshift/api to remove IPv6DualStack feature flag openshift/cluster-kube-controller-manager-operator#355

Closed

soltysh mentioned this pull request Mar 4, 2020

Bug 1809368: bump openshift/api to remove IPv6DualStack feature flag openshift/cluster-kube-controller-manager-operator#359

Merged

Enable overriding service account issuer for bound tokens #569

Enable overriding service account issuer for bound tokens #569

Uh oh!

Conversation

marun commented Jan 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marun Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts commented Jan 22, 2020

Uh oh!

openshift-ci-robot commented Jan 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marun commented Jan 18, 2020 •

edited

Loading

marun Jan 22, 2020 •

edited

Loading