Promote APIServerIdentity to Beta #113629

andrewsykim · 2022-11-04T15:51:26Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR promotes the APIServerIdentity feature, per KEP-1965 updates for v1.26:

KEP-1965: update with Beta criteria/milestone and PRR questions answered enhancements#3589
KEP-1965: update kube-apiserver identity KEP to reflect current state enhancements#3621
KEP-1965: update apiserver lease identity format enhancements#3635

In addition, it adds e2e tests which were part of the Beta graduation criterias and also removes kube-apiserver flags used to configure the lease duration and renewal interval.

The following Beta criterias have been met:

Appropriate metrics are agreed on and implemented
- See poststarthook SLI metrics added in enable health check SLI metrics for apiserver #112741, which can be used to check the health of the lease controller and lease gc controller running in kube-apiserver
Sufficient integration tests covering basic functionality of this enhancement.
- Integration tests can be found in test/integration/controlplane/apiserver_identity_test.go
- Additional unit tests for the apiserverleasegc controller was also added in https://github.com/kubernetes/kubernetes/pull/113074/files
e2e tests outlined in the test plan are implemented
- This PR adds an e2e test validating behavior for kube-apiserver leases on restart.
SIG consensus on lease identity format, see apiserver identity: use persistent names for lease objects #113307

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Promote the `APIServerIdentity` feature to Beta. By default, each kube-apiserver will now create a Lease in the `kube-system` namespace. These lease objects can be used to identify the number of active API servers in the cluster, and may also be used for future features such as the Storage Version API.

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

sftim · 2022-11-04T15:59:06Z

Changelog entry suggestion

Promote the `APIServerIdentity` feature to Beta. By default, each kube-apiserver will now create a Lease in the `kube-system` namespace. These lease objects can be used to identify the number of active API servers in the cluster, and may also be used for future features such as the Storage Version API.

Not sure what StorageVersionAPI is; if possible, follow https://kubernetes.io/docs/contribute/style/style-guide/ when working out how to refer to it.

andrewsykim · 2022-11-04T16:08:06Z

Changelog entry suggestion

Thanks for the suggestion! Updated :)

andrewsykim · 2022-11-04T23:41:19Z

/hold

#113307 (comment) needs to be addressed first

enj · 2022-11-07T18:25:00Z

pkg/controlplane/instance.go

@@ -480,9 +477,9 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
 				clock.RealClock{},
 				kubeClient,
 				holderIdentity,
-				int32(c.ExtraConfig.IdentityLeaseDurationSeconds),
+				3600,


nit:

Inside this function create a const for this an re-use it in the two spots:

const identityLeaseDurationSeconds = 3600

thanks, updated

enj · 2022-11-07T18:26:40Z

test/e2e/apimachinery/apiserver_identity.go

+	controlPlaneAddress := nodeAddresses[0]
+
+	host := controlPlaneAddress + ":" + e2essh.SSHPort
+	result, err := e2essh.SSH("hostname", host, framework.TestContext.Provider)


Hah, that is one way to do it :)

why not using exec on pods?

apiserver pods generally aren't part of the cluster (static pods managed only by a kubelet)

I mean, exec a pod on that node as hostNetwork , not execing on the apiserver pod ... but maybe this is restricted, right?

kubectl debug has tooling for running a more-privileged Pod on a node to learn about it; maybe we could use some of that approach?

just to clarify, I commented because I was curious, we are close to code freeze and I don't mean to delay this PR

enj · 2022-11-07T18:30:15Z

test/e2e/apimachinery/apiserver_identity.go

+			h := fnv.New32a()
+			h.Write([]byte(strings.TrimSpace(hostname)))
+			leaseName := "kube-apiserver-" + fmt.Sprint(h.Sum32())


Reminder to update this.

updated, but test will fail until the other PR merges

enj · 2022-11-07T18:30:31Z

test/e2e/apimachinery/apiserver_identity.go

+	f.NamespacePodSecurityEnforceLevel = admissionapi.LevelPrivileged
+
+	ginkgo.It("kube-apiserver identity should persist after restart [Disruptive]", func() {
+		e2eskipper.SkipUnlessProviderIs("gce")


Is this something we do often?

Yes, especially on tests where you have to ssh into the nodes or run some external operation against them

enj · 2022-11-07T18:36:03Z

test/e2e/apimachinery/apiserver_identity.go

+			LabelSelector: "k8s.io/component=kube-apiserver",
+		})
+		framework.ExpectNoError(err)
+		framework.ExpectEqual(len(leases.Items), len(controlPlaneNodes), "unexpected number of leases")


Can this flake in CI?

Only if you run it on a cluster while it is still being provisioned. Otherwise the node check above should prevent flakes

I guess it doesn't hurt to add a retry loop here

andrewsykim · 2022-11-07T19:10:15Z

test/integration/controlplane/apiserver_identity_test.go

-		// This shorten the GC check period to make the test run faster.
-		// Since we are testing GC behavior on leases we create, what happens to
-		// the real apiserver lease doesn't matter.
-		[]string{"--identity-lease-duration-seconds=1"},


I had to remove this test, because of the dependence to this flag. But I think the unit tests added in #113074 and the additional unit test I added in this PR should make up for the coverage we lose here.

In other places we just let the integration test override the "constant"

kubernetes/staging/src/k8s.io/client-go/transport/cert_rotation.go

Lines 37 to 38 in f8750e2

// CertCallbackRefreshDuration is exposed so that integration tests can crank up the reload speed.

var CertCallbackRefreshDuration = 5 * time.Minute

Hmmm -- updating the lease parameters into global vars seems worthwhile so we can run integration tests.

Updated to use public vars

enj · 2022-11-07T22:09:51Z

/lgtm

andrewsykim · 2022-11-07T23:32:06Z

/retest

andrewsykim · 2022-11-07T23:34:46Z

/assign @lavalamp @deads2k

lavalamp · 2022-11-08T00:22:07Z

test/integration/controlplane/apiserver_identity_test.go

+	// the real apiserver lease doesn't matter.
+	controlplane.IdentityLeaseDurationSeconds = 1
+	controlplane.IdentityLeaseGCPeriod = time.Second
+	controlplane.IdentityLeaseRenewIntervalPeriod = time.Second


Can you set them back when you're done to avoid weird things in other tests?

Good catch -- updated

lavalamp · 2022-11-08T00:24:47Z

Looks good except for the one comment

… behavior when restarting kube-apiserver Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

…identity-lease-renew-interval-seconds Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

…ey can be set by integration tests Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

…public lease parameter vars Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

lavalamp · 2022-11-08T00:49:25Z

/lgtm
/approve

k8s-ci-robot · 2022-11-08T00:49:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, lavalamp

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~~ [lavalamp]
~~pkg/controlplane/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiserver/pkg/features/OWNERS~~ [andrewsykim,lavalamp]
~~test/OWNERS~~ [andrewsykim,lavalamp]

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

aojea · 2022-11-08T10:36:41Z

/test pull-kubernetes-e2e-gce-alpha-features

k8s-ci-robot · 2022-11-08T16:31:11Z

@andrewsykim: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-alpha-features	`196a3b9`	link	false	`/test pull-kubernetes-e2e-gce-alpha-features`

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.

lavalamp · 2022-11-08T18:47:41Z

/hold cancel

fedebongio · 2022-11-08T21:24:53Z

/triage accepted

k8s-ci-robot requested review from apelisse and derekwaynecarr November 4, 2022 15:52

andrewsykim force-pushed the apiserver-identity-beta branch 2 times, most recently from 669e27a to d781f1d Compare November 4, 2022 16:01

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 4, 2022

andrewsykim force-pushed the apiserver-identity-beta branch from d781f1d to 79b7b2e Compare November 7, 2022 18:21

enj reviewed Nov 7, 2022

View reviewed changes

andrewsykim force-pushed the apiserver-identity-beta branch from 79b7b2e to 70faaf6 Compare November 7, 2022 19:07

andrewsykim commented Nov 7, 2022

View reviewed changes

andrewsykim force-pushed the apiserver-identity-beta branch 5 times, most recently from 899fc01 to b423794 Compare November 7, 2022 21:56

k8s-ci-robot assigned enj Nov 7, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2022

k8s-ci-robot assigned deads2k and lavalamp Nov 7, 2022

lavalamp reviewed Nov 8, 2022

View reviewed changes

andrewsykim added 5 commits November 7, 2022 19:36

test/e2e/apimachinery: add e2e test for APIServerIdentity, validating…

368f9f9

… behavior when restarting kube-apiserver Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

kube-apiserver: remove flags --identity-lease-duration-seconds and --…

02020b2

…identity-lease-renew-interval-seconds Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

pkg/controlplane: update lease parameters into public variables so th…

d24f93e

…ey can be set by integration tests Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

test/integration/controlplane: update APIServerIdentity tests to use …

c2d387c

…public lease parameter vars Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

kube-apiserver: promote APIServerIdentity to Beta and enabled by default

196a3b9

Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

andrewsykim force-pushed the apiserver-identity-beta branch from b423794 to 196a3b9 Compare November 8, 2022 00:42

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2022

k8s-ci-robot merged commit 3a99a59 into kubernetes:master Nov 8, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 8, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 8, 2022

	// CertCallbackRefreshDuration is exposed so that integration tests can crank up the reload speed.
	var CertCallbackRefreshDuration = 5 * time.Minute

Promote APIServerIdentity to Beta #113629

Promote APIServerIdentity to Beta #113629

Conversation

andrewsykim commented Nov 4, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

sftim commented Nov 4, 2022

andrewsykim commented Nov 4, 2022

andrewsykim commented Nov 4, 2022

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

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

enj commented Nov 7, 2022

andrewsykim commented Nov 7, 2022

andrewsykim commented Nov 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Nov 8, 2022

lavalamp commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

aojea commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

lavalamp commented Nov 8, 2022

fedebongio commented Nov 8, 2022

andrewsykim commented Nov 4, 2022 •

edited