-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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.
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
[StructuredAuthenticationConfig] Create struct for authn config and re-wire OIDC flags to use it #118984
[StructuredAuthenticationConfig] Create struct for authn config and re-wire OIDC flags to use it #118984
Conversation
/kind feature |
66a2fc0
to
7af3258
Compare
7af3258
to
ee29ae5
Compare
/assign @enj Not ready for review yet, just adding it to your list! |
5ee20d2
to
621fb42
Compare
f9483a1
to
f74783a
Compare
staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go
Outdated
Show resolved
Hide resolved
// prefix is prepended to claim's value to prevent clashes with existing names. | ||
// +required | ||
Prefix *string `json:"prefix"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liggitt I assume this won't be required once we add expression field, so what should the tag be set to optional
or required
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can leave it +required for now and switch to +optional once we have another field, or just make it +optional with good docs now
the tag doesn't drive any behavior and we don't have openapi for this config file API, so it doesn't really matter... we just don't want to be confusing
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go
Outdated
Show resolved
Hide resolved
11822f0
to
58d0422
Compare
ret.OIDCIssuerURL = o.OIDC.IssuerURL | ||
ret.OIDCUsernameClaim = o.OIDC.UsernameClaim | ||
ret.OIDCUsernamePrefix = o.OIDC.UsernamePrefix | ||
if o.OIDC != nil && len(o.OIDC.IssuerURL) > 0 && len(o.OIDC.ClientID) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no action required now, but we'll eventually need to make this path mutually exclusive with the flag for providing the jwt authentication config ... didn't see a TODO and wanted to make sure that was tracked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm doing that as part of #119142. I'll rebase that on top of this PR now.
// prefix is prepended to claim's value to prevent clashes with existing names. | ||
// +required | ||
Prefix *string `json:"prefix"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can leave it +required for now and switch to +optional once we have another field, or just make it +optional with good docs now
the tag doesn't drive any behavior and we don't have openapi for this config file API, so it doesn't really matter... we just don't want to be confusing
staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go
Outdated
Show resolved
Hide resolved
/approve /hold |
LGTM label has been added. Git tree hash: f21705ad5b23ff348287177c4f212ba252d16012
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aramase, liggitt 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:
Approvers can indicate their approval by writing |
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
58d0422
to
1bad3cb
Compare
/hold cancel @liggitt fixed the godoc comments! |
/lgtm |
LGTM label has been added. Git tree hash: 2e02c6fb0593ba35e1ec2b97379fa6130e268e88
|
v1alpha1
API forAuthenticationConfiguration
--oidc-*
flags with the internal structAuthenticationConfiguration
validation is used after the struct is created.Fixes #118832
Fixes #119384
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: