allow multiple of --service-account-issuer #101155

zshihang · 2021-04-15T16:59:44Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

change the issuer in a non-disruptive way

Special notes for your reviewer:

Does this PR introduce a user-facing change?

-->

kube-apiserver: `--service-account-issuer` can be specified multiple times now, to enable non-disruptive change of issuer.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/1205-bound-service-account-tokens

zshihang · 2021-04-15T17:45:16Z

/assign @liggitt
/sig auth
/triage accepted
/priority important-soon

liggitt · 2021-04-15T17:51:17Z

cmd/kube-apiserver/app/server.go

@@ -416,7 +416,7 @@ func CreateKubeAPIServerConfig(
 		pubKeys = append(pubKeys, keys...)
 	}
 	// Plumb the required metadata through ExtraConfig.
-	config.ExtraConfig.ServiceAccountIssuerURL = s.Authentication.ServiceAccounts.Issuer
+	config.ExtraConfig.ServiceAccountIssuerURL = s.Authentication.ServiceAccounts.Issuers[0]


are we guaranteed len(s.Authentication.ServiceAccounts.Issuers) > 0 here so this doesn't panic?

it is guaranteed as long as the validation is called. in the integration tests, the test server doesn't provide those options and the validation is not called.

done. i added the validation logic in the test server initialization and the missing options.

liggitt · 2021-04-15T17:53:42Z

pkg/kubeapiserver/options/authentication.go

+			"This flag can be specified multiple times with different values but only the first one"+
+			"is used to generate tokens.")


clarify that when specified multiple times, the first is used to generate tokens and all are used to determine which issuers are accepted

liggitt · 2021-04-15T17:57:58Z

looks like a fair number of panics in integration tests, a few questions/clarifications, lgtm otherwise

liggitt · 2021-04-15T17:58:00Z

looks like a fair number of panics in integration tests, a few questions/clarifications, lgtm otherwise

fejta-bot · 2021-04-15T19:22:58Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

liggitt · 2021-04-16T14:46:15Z

integration test failures look relevant

zshihang · 2021-04-16T19:56:18Z

/test pull-kubernetes-e2e-kind

zshihang · 2021-04-16T21:19:05Z

/test pull-kubernetes-e2e-kind

zshihang · 2021-04-16T22:00:45Z

/retest

liggitt · 2021-04-19T13:37:58Z

test/integration/apiserver/admissionwebhook/admission_test.go

@@ -159,6 +160,7 @@ var (
 		gvr("authorization.k8s.io", "v1beta1", "selfsubjectrulesreviews"):   `{"metadata": {"name": "", "namespace":"` + testNamespace + `"}, "spec": {"namespace":"` + testNamespace + `"}}`,

 		// Other Non persistent resources
+		gvr("", "v1", "serviceaccounts/token"): `{"metadata": {"name": "sa1name"}, "spec": {"audience": ["api"]}}`,


this is probably better done as an addition to customTestFuncs (similar to something like testPodBindingEviction)... I don't think the subresource create implementation is likely to work well as a generic implementation

liggitt · 2021-04-19T13:39:23Z

test/integration/etcd/server.go

@@ -70,12 +77,25 @@ func StartRealMasterOrDie(t *testing.T, configFuncs ...func(*options.ServerRunOp
 		t.Fatal(err)
 	}

+	saSigningKeyFile, err := ioutil.TempFile("/tmp", "key")


nit: name the file so it is clear it is a test key (insecure_test_key or something), since we're setting permission file permissions on it. applies to all the places we're persisting the test keys

liggitt · 2021-04-19T13:40:26Z

test/integration/etcd/server.go

@@ -53,6 +54,12 @@ import (
 	_ "k8s.io/kubernetes/pkg/controlplane"
 )

+const ecdsaPrivateKey = `-----BEGIN EC PRIVATE KEY-----


Add a comment to all the places we're inlining private keys that they are for test purposes only and are not considered secure (This key is for testing purposes only and is not considered secure.). Applies to all the places you added test key data

We've had issues with people reporting keys in testdata as security vulnerabilities

liggitt · 2021-04-19T13:41:56Z

a few comments on tests and testdata, lgtm otherwise

liggitt · 2021-04-19T17:07:22Z

/lgtm
/approve

k8s-ci-robot · 2021-04-19T17:07:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, zshihang

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:

~~cmd/kube-apiserver/OWNERS~~ [liggitt]
~~pkg/kubeapiserver/OWNERS~~ [liggitt]
~~pkg/serviceaccount/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt]

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

zshihang · 2021-04-19T17:38:36Z

/retest

zshihang · 2021-04-19T19:22:40Z

/retest

zshihang · 2021-04-19T19:59:35Z

/retest

zshihang · 2021-04-19T22:21:15Z

/retest

k8s-ci-robot requested review from deads2k and mikedanese April 15, 2021 16:59

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 15, 2021

zshihang force-pushed the bound branch from 6a87df4 to e2706be Compare April 15, 2021 17:16

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Apr 15, 2021

k8s-ci-robot assigned liggitt Apr 15, 2021

zshihang added this to In progress in SIG-Auth: Projected Service Account Tokens Apr 15, 2021

liggitt reviewed Apr 15, 2021

View reviewed changes

zshihang force-pushed the bound branch from e2706be to 469ef4e Compare April 15, 2021 21:25

zshihang force-pushed the bound branch 2 times, most recently from a4a1de7 to e137547 Compare April 16, 2021 19:08

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Apr 16, 2021

zshihang force-pushed the bound branch from e137547 to 4ac4aad Compare April 16, 2021 20:35

liggitt reviewed Apr 19, 2021

View reviewed changes

zshihang force-pushed the bound branch from 4ac4aad to ea6195a Compare April 19, 2021 16:42

ehashman added this to Triage in SIG Node PR Triage Apr 19, 2021

allow multiple of --service-account-issuer

9259003

zshihang force-pushed the bound branch from ea6195a to 9259003 Compare April 19, 2021 16:54

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2021

k8s-ci-robot merged commit 6157361 into kubernetes:master Apr 19, 2021

SIG Node PR Triage automation moved this from Triage to Done Apr 19, 2021

SIG-Auth: Projected Service Account Tokens automation moved this from In progress to 1.22 - Done Apr 19, 2021

k8s-ci-robot added this to the v1.22 milestone Apr 19, 2021

github-actions bot mentioned this pull request Apr 21, 2021

Week Ending April 14, 2021 dev-obs/actus#357

Open

This was referenced Jan 7, 2022

Document that specifying --service-account-issuer multiple times requires k8s v1.22 or later kubernetes/website#31235

Closed

Added the note for --service-account-issuer flag. kubernetes/website#31237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow multiple of --service-account-issuer #101155

allow multiple of --service-account-issuer #101155

zshihang commented Apr 15, 2021 •

edited by liggitt

Loading

zshihang commented Apr 15, 2021

liggitt Apr 15, 2021

zshihang Apr 15, 2021 •

edited

Loading

liggitt Apr 15, 2021

zshihang Apr 15, 2021

liggitt commented Apr 15, 2021

liggitt commented Apr 15, 2021

fejta-bot commented Apr 15, 2021

liggitt commented Apr 16, 2021

zshihang commented Apr 16, 2021

zshihang commented Apr 16, 2021

zshihang commented Apr 16, 2021

liggitt Apr 19, 2021

zshihang Apr 19, 2021

liggitt Apr 19, 2021

zshihang Apr 19, 2021

liggitt Apr 19, 2021 •

edited

Loading

zshihang Apr 19, 2021

liggitt commented Apr 19, 2021

liggitt commented Apr 19, 2021

k8s-ci-robot commented Apr 19, 2021

zshihang commented Apr 19, 2021

zshihang commented Apr 19, 2021

zshihang commented Apr 19, 2021

zshihang commented Apr 19, 2021

		"This flag can be specified multiple times with different values but only the first one"+
		"is used to generate tokens.")

allow multiple of --service-account-issuer #101155

allow multiple of --service-account-issuer #101155

Conversation

zshihang commented Apr 15, 2021 • edited by liggitt Loading

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

zshihang commented Apr 15, 2021

Choose a reason for hiding this comment

zshihang Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Apr 15, 2021

liggitt commented Apr 15, 2021

fejta-bot commented Apr 15, 2021

liggitt commented Apr 16, 2021

zshihang commented Apr 16, 2021

zshihang commented Apr 16, 2021

zshihang commented Apr 16, 2021

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

liggitt Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Apr 19, 2021

liggitt commented Apr 19, 2021

k8s-ci-robot commented Apr 19, 2021

zshihang commented Apr 19, 2021

zshihang commented Apr 19, 2021

zshihang commented Apr 19, 2021

zshihang commented Apr 19, 2021

zshihang commented Apr 15, 2021 •

edited by liggitt

Loading

zshihang Apr 15, 2021 •

edited

Loading

liggitt Apr 19, 2021 •

edited

Loading