delegated authn/z: optionally opt-out of mandatory authn/authz kubeconfig #67545

sttts · 2018-08-17T14:52:26Z

This adds RemoteKubeConfigFileOptional field to the delegated authn/z option structs. If set to true, the authn/z kubeconfig file flags are optional. If no kubeconfig is given, all token requests are considered to be anonymous and no client CA is looked up in the cluster.

Prerequisite for #64149 and #67069.

k8s-ci-robot · 2018-08-17T14:52:27Z

@sttts: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

k8s-ci-robot · 2018-08-17T14:52:29Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

sttts · 2018-08-27T18:38:14Z

@awly @yue9944882 after the prerequisite merged, this is pretty trivial now. ptal.

awly · 2018-08-27T21:10:06Z

staging/src/k8s.io/apiserver/pkg/server/options/authentication.go

@@ -139,9 +142,14 @@ func (s *DelegatingAuthenticationOptions) AddFlags(fs *pflag.FlagSet) {
 		return
 	}

+	var optionalKubeConfigSentence string
+	if s.RemoteKubeConfigFileOptional {
+		optionalKubeConfigSentence = " This is optional. If empty, all token requests are considered to be anonymous " +


No need to wrap this string

awly · 2018-08-27T21:15:04Z

staging/src/k8s.io/client-go/rest/config.go

@@ -336,6 +336,15 @@ func InClusterConfig() (*Config, error) {
 	}, nil
 }

+type notInCluster struct{ error }


Use a package-level var NotInClusterErr = errors.New("...") instead

yue9944882 · 2018-08-28T05:27:20Z

staging/src/k8s.io/apiserver/pkg/server/options/authorization.go

@@ -41,6 +42,9 @@ type DelegatingAuthorizationOptions struct {
 	// RemoteKubeConfigFile is the file to use to connect to a "normal" kube API server which hosts the
 	// SubjectAccessReview.authorization.k8s.io endpoint for checking tokens.
 	RemoteKubeConfigFile string
+	// RemoteKubeConfigFileOptional is specifying whether not specifying the kubeconfig or
+	// a missing in-cluster config will be fatal.
+	RemoteKubeConfigFileOptional bool


if we don’t add it to the flagset，where can we set its value?

the component using the option struct sets it.

i thought that every field in option struct should be added to flagset

No, why? We have many which are not.

look into SecureServing as an example.

yue9944882 · 2018-08-28T05:34:34Z

staging/src/k8s.io/client-go/rest/config.go

+// IsNotInCluster checks whether the error means that this process is not in a in-cluster context with
+// KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT set as environment variables.
+func IsNotInCluster(err error) bool {
+	_, ok := err.(notInCluster)


what about do a type switch? and we can handle nil check here

see @awly suggestion #67545 (comment). That's even simpler.

yue9944882 · 2018-08-28T05:35:45Z

staging/src/k8s.io/apiserver/pkg/server/options/authentication.go

 	} else {
 		// without the remote kubeconfig file, try to use the in-cluster config.  Most addon API servers will
 		// use this path
 		clientConfig, err = rest.InClusterConfig()
+		if err != nil && rest.IsNotInCluster(err) && s.RemoteKubeConfigFileOptional {


yue9944882 · 2018-08-28T05:41:38Z

staging/src/k8s.io/apiserver/pkg/server/options/authentication.go

+		if len(s.ClientCert.ClientCA) == 0 {
+			glog.Warningf("No authentication-kubeconfig provided in order to lookup client-ca-file in configmap/%s in %s, so client certificate authentication to extension api-server won't work.", authenticationConfigMapName, authenticationConfigMapNamespace)
+		}
+		if len(s.RequestHeader.ClientCAFile) > 0 {


yue9944882 · 2018-08-28T05:46:23Z

staging/src/k8s.io/apiserver/pkg/server/options/authorization.go

-	if err != nil {
-		return nil, err
+	if client == nil {
+		glog.Warningf("No authorization-kubeconfig provided, so SubjectAccessReview of authorization tokens won't work.")


nit: what about straightly return the union here？a long else block looks odd to me 👀

I prefer the if block. The authorizers slice is constructed one by one by appending new authorizers. I think it is easier to reason about the code the way it is. The pattern is clear when another authorizer is added with another if block. Then an early return wouldn't work anymore.

yue9944882 · 2018-08-28T05:48:57Z

/retest

sttts · 2018-08-28T08:10:03Z

@yue9944882 @awly addressed all comments. Anything left?

sttts · 2018-08-28T09:40:18Z

/retest

sttts · 2018-08-28T10:23:26Z

@awly lgty?

k8s-ci-robot · 2018-08-28T11:26:23Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gke	74f594e302fc51086046ca8f45615da5a7dea76f	link	`/test pull-kubernetes-e2e-gke`
pull-kubernetes-cross	74f594e302fc51086046ca8f45615da5a7dea76f	link	`/test pull-kubernetes-cross`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

sttts · 2018-08-28T12:36:56Z

/retest

awly

Small nits, but LGTM

awly · 2018-08-28T16:45:03Z

staging/src/k8s.io/client-go/rest/config.go

@@ -313,7 +318,7 @@ func DefaultKubernetesUserAgent() string {
 func InClusterConfig() (*Config, error) {


Update the above comment to mention ErrNotInCluster

awly · 2018-08-28T16:45:22Z

staging/src/k8s.io/client-go/rest/config.go

@@ -44,6 +45,10 @@ const (
 	DefaultBurst int     = 10
 )

+var (


Remove parens, just var ErrNotInCluster = ...

awly · 2018-08-28T17:17:47Z

/lgtm

k8s-ci-robot · 2018-08-28T17:18:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awly, 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:

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]
~~staging/src/k8s.io/client-go/OWNERS~~ [sttts]

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

k8s-github-robot · 2018-08-28T20:47:17Z

Automatic merge from submit-queue (batch tested with PRs 66960, 67545). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 17, 2018

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 17, 2018

k8s-ci-robot requested review from davidopp and ericchiang August 17, 2018 14:52

sttts assigned liggitt Aug 17, 2018

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Aug 17, 2018

sttts force-pushed the sttts-auth-optional-kubeconfig branch from fb07de6 to 9d43cb1 Compare August 17, 2018 14:57

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 17, 2018

sttts force-pushed the sttts-auth-optional-kubeconfig branch from 9d43cb1 to c19485b Compare August 17, 2018 15:06

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2018

sttts force-pushed the sttts-auth-optional-kubeconfig branch 2 times, most recently from 2ac1d50 to 3f34458 Compare August 17, 2018 15:40

sttts added sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. labels Aug 17, 2018

sttts force-pushed the sttts-auth-optional-kubeconfig branch from 3f34458 to 8a384e0 Compare August 21, 2018 14:45

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Aug 21, 2018

sttts force-pushed the sttts-auth-optional-kubeconfig branch from 8a384e0 to 74f594e Compare August 21, 2018 14:51

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2018

sttts force-pushed the sttts-auth-optional-kubeconfig branch from 74f594e to c9925dc Compare August 21, 2018 14:51

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2018

sttts force-pushed the sttts-auth-optional-kubeconfig branch from c9925dc to 75976d9 Compare August 21, 2018 19:50

sttts added this to the v1.12 milestone Aug 27, 2018

awly reviewed Aug 27, 2018

View reviewed changes

yue9944882 reviewed Aug 28, 2018

View reviewed changes

sttts force-pushed the sttts-auth-optional-kubeconfig branch from 4a752b7 to dcb9b10 Compare August 28, 2018 08:09

sttts force-pushed the sttts-auth-optional-kubeconfig branch from dcb9b10 to 7675587 Compare August 28, 2018 08:11

yue9944882 approved these changes Aug 28, 2018

View reviewed changes

sttts force-pushed the sttts-auth-optional-kubeconfig branch from 7675587 to b996971 Compare August 28, 2018 10:44

awly reviewed Aug 28, 2018

View reviewed changes

authn/z: optionally opt-out of mandatory authn/authz kubeconfig

a671d65

sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2018

sttts force-pushed the sttts-auth-optional-kubeconfig branch from b996971 to a671d65 Compare August 28, 2018 17:08

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2018

k8s-github-robot merged commit 1b3a2dd into kubernetes:master Aug 28, 2018

yue9944882 mentioned this pull request Dec 2, 2018

Chore(openapi): make delegated authn authz kubeconfig optional kubernetes-sigs/apiserver-builder-alpha#271

Merged

aojea mentioned this pull request May 11, 2022

apiextensions-apiserver integration test don't use a nonexisting apiserver address #109973

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delegated authn/z: optionally opt-out of mandatory authn/authz kubeconfig #67545

delegated authn/z: optionally opt-out of mandatory authn/authz kubeconfig #67545

sttts commented Aug 17, 2018 •

edited

Loading

k8s-ci-robot commented Aug 17, 2018

k8s-ci-robot commented Aug 17, 2018

sttts commented Aug 27, 2018

awly Aug 27, 2018

sttts Aug 28, 2018

awly Aug 27, 2018

sttts Aug 28, 2018

yue9944882 Aug 28, 2018

sttts Aug 28, 2018

yue9944882 Aug 28, 2018

sttts Aug 28, 2018

sttts Aug 28, 2018

yue9944882 Aug 28, 2018

sttts Aug 28, 2018

yue9944882 Aug 28, 2018

yue9944882 Aug 28, 2018 •

edited

Loading

sttts Aug 28, 2018

yue9944882 Aug 28, 2018

sttts Aug 28, 2018

yue9944882 commented Aug 28, 2018

sttts commented Aug 28, 2018

sttts commented Aug 28, 2018

sttts commented Aug 28, 2018

k8s-ci-robot commented Aug 28, 2018 •

edited

Loading

sttts commented Aug 28, 2018

awly left a comment

awly Aug 28, 2018

sttts Aug 28, 2018

awly Aug 28, 2018

sttts Aug 28, 2018

awly commented Aug 28, 2018

k8s-ci-robot commented Aug 28, 2018

k8s-github-robot commented Aug 28, 2018

		@@ -313,7 +318,7 @@ func DefaultKubernetesUserAgent() string {
		func InClusterConfig() (*Config, error) {

delegated authn/z: optionally opt-out of mandatory authn/authz kubeconfig #67545

delegated authn/z: optionally opt-out of mandatory authn/authz kubeconfig #67545

Conversation

sttts commented Aug 17, 2018 • edited Loading

k8s-ci-robot commented Aug 17, 2018

k8s-ci-robot commented Aug 17, 2018

sttts commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yue9944882 Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yue9944882 commented Aug 28, 2018

sttts commented Aug 28, 2018

sttts commented Aug 28, 2018

sttts commented Aug 28, 2018

k8s-ci-robot commented Aug 28, 2018 • edited Loading

sttts commented Aug 28, 2018

awly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awly commented Aug 28, 2018

k8s-ci-robot commented Aug 28, 2018

k8s-github-robot commented Aug 28, 2018

sttts commented Aug 17, 2018 •

edited

Loading

yue9944882 Aug 28, 2018 •

edited

Loading

k8s-ci-robot commented Aug 28, 2018 •

edited

Loading