Skip to content

Enable overriding service account issuer for bound tokens #569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 2 commits into from
Jan 22, 2020
Merged

Enable overriding service account issuer for bound tokens #569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5637f56
Enable configuration of the issuer of bound service account tokens
marun Jan 17, 2020
5e3a31c
Updated generated
marun Jan 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions config/v1/0000_10_config-operator_01_authentication.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ spec:
description: name is the metadata.name of the referenced config
map
type: string
serviceAccountIssuer:
description: serviceAccountIssuer is the identifier of the bound service
account token issuer. The default is auth.openshift.io.
type: string
type:
description: type identifies the cluster managed, user facing authentication
mode in use. Specifically, it manages the component that responds
Expand Down
6 changes: 6 additions & 0 deletions config/v1/types_authentication.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ type AuthenticationSpec struct {
// The namespace for these secrets is openshift-config.
// +optional
WebhookTokenAuthenticators []WebhookTokenAuthenticator `json:"webhookTokenAuthenticators,omitempty"`

// serviceAccountIssuer is the identifier of the bound service account token
// issuer.
// The default is auth.openshift.io.
// +optional
ServiceAccountIssuer string `json:"serviceAccountIssuer"`
Copy link
Contributor

@sttts sttts Jan 20, 2020

Choose a reason for hiding this comment

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

is there any special format? Is it a URL ?

Copy link
Contributor

@sttts sttts Jan 20, 2020

Choose a reason for hiding this comment

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

Is there a default value?

Copy link
Contributor

@stlaz stlaz Jan 20, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@marun marun Jan 20, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@deads2k deads2k Jan 20, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@sttts sttts Jan 21, 2020

Choose a reason for hiding this comment

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

something like [^:]*|someURLRegEx

Copy link
Contributor Author

@marun marun Jan 21, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@sttts sttts Jan 21, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@marun marun Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@sttts sttts Jan 22, 2020

Choose a reason for hiding this comment

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

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

}

type AuthenticationStatus struct {
Expand Down
1 change: 1 addition & 0 deletions config/v1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.