✨ Update to Kubernetes 1.30 #3140

embik · 2024-07-15T10:16:07Z

Summary

This updates kcp to Kubernetes 1.30.0. The corresponding branch for our Kubernetes fork is kcp-1.30. Mainly this PR adds new APIs or removes feature gates where they have been removed by upstream.

The bigger changes in this PR are:

Updating to the new ValidatingAdmissionPolicy plugin infrastructure.
Rewrite the OpenAPI controller we recently added to accommodate for new upstream types.

The following bigger TODOs exist, but I would like to tackle them separately:

Look at enabling the flags I've disabled in this PR, most notably --authentication-config, which seems like a hugely useful feature for a multi-tenant API server like KCP.

Related issue(s)

Fixes #3068

Release Notes

Update to Kubernetes 1.30.0

mjudeikis · 2024-07-15T10:29:21Z

🥳 🔥 🎆

embik · 2024-07-15T13:35:31Z

/retest

mjudeikis · 2024-07-15T13:45:09Z

/lgtm
/approve
/hold
@sttts wanna read through or fix on master? :)

kcp-ci-bot · 2024-07-15T13:45:15Z

LGTM label has been added.

Git tree hash: 2c7ea837cae12d637bf20e30cd8bef62fb6bd5bc

kcp-ci-bot · 2024-07-15T13:45:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mjudeikis

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:

~~OWNERS~~ [mjudeikis]
~~config/crds/OWNERS~~ [mjudeikis]
~~pkg/admission/OWNERS~~ [mjudeikis]

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

pkg/admission/apiresourceschema/validation_test.go

pkg/admission/plugins.go

pkg/cache/server/config.go

pkg/proxy/options/authentication.go

pkg/server/openapiv3/controller.go

pkg/openapi/value.go

pkg/reconciler/topology/partitionset/resources.go

pkg/server/openapiv3/servicecache.go

pkg/server/options/flags.go

sttts · 2024-07-17T12:54:57Z

pkg/virtual/apiexport/schemas/builtin/builtin.go

@@ -261,6 +272,16 @@ var BuiltInAPIs = []internalapis.InternalAPI{
 		Instance:      &admissionregistrationv1alpha1.ValidatingAdmissionPolicyBinding{},
 		ResourceScope: apiextensionsv1.ClusterScoped,
 	},
+	{
+		Names: apiextensionsv1.CustomResourceDefinitionNames{
+			Plural:   "validatingadmissionpolicybindings",


I wonder whether we better want a quick e2e test that checks that the policies work as expected. Namely, the should of course apply to objects in the same logical cluster, but also next to an APIExport? This would mean we have to replicate them via the cache server.

I definitely want an e2e test for this (there's a note on it in the PR description), but I didn't want to squeeze it into this PR. Do you think it would be better to do this immediately?

If the API is enabled, we should test it. We can disable it and postpone to followup.

Test has been added. It was in fact not working, I had to fix things on k8s and kcp side. Key commits:

kcp-dev/kubernetes@d61cdac

kcp-dev/kubernetes@3b450c7

bcf32ed

6af5c93

sttts · 2024-07-25T10:31:35Z

test/e2e/conformance/validatingadmissionpolicy_test.go

+	policy, err = kubeClusterClient.Cluster(paths[0]).AdmissionregistrationV1().ValidatingAdmissionPolicies().Create(ctx, policy, metav1.CreateOptions{})
+
+	require.NoError(t, err, "failed to create ValidatingAdmissionPolicy")


Suggested change

policy, err = kubeClusterClient.Cluster(paths[0]).AdmissionregistrationV1().ValidatingAdmissionPolicies().Create(ctx, policy, metav1.CreateOptions{})

require.NoError(t, err, "failed to create ValidatingAdmissionPolicy")

policy, err = kubeClusterClient.Cluster(paths[0]).AdmissionregistrationV1().ValidatingAdmissionPolicies().Create(ctx, policy, metav1.CreateOptions{})

require.NoError(t, err, "failed to create ValidatingAdmissionPolicy")

sttts · 2024-07-25T10:34:07Z

test/e2e/conformance/validatingadmissionpolicy_test.go

+	// Policies take a second to compile. So far there doesn't seem to be a way
+	// to query if a policy has been successfully processed and is active, so we
+	// just sleep for a few seconds.
+	time.Sleep(5 * time.Second)


timeouts always break eventually. Better just repeat the check below until the error appears.

sttts · 2024-07-25T10:34:34Z

test/e2e/conformance/validatingadmissionpolicy_test.go

+	}
+
+	// badCowboy violates the policy, so it should be rejected
+	t.Logf("Creating bad cowboy resource in first logical cluster")


style: combine both Log and comment. Having both is noisy.

t.Logf("Creating bad cowboy in first workspace should fail")

sttts · 2024-07-25T10:45:49Z

Modulo the e2e style comments, lgtm.

embik · 2024-07-25T13:38:17Z

Looks like the failing tests aren't a flake:

maximalpermissionpolicy_authorizer_test.go:232: 
        	Error Trace:	/home/prow/go/src/github.com/kcp-dev/kcp/test/e2e/apibinding/maximalpermissionpolicy_authorizer_test.go:404
        	            				/home/prow/go/src/github.com/kcp-dev/kcp/test/e2e/apibinding/maximalpermissionpolicy_authorizer_test.go:232
        	Error:      	Received unexpected error:
        	            	cowboys.wildwest.dev "cowboy-e2e-workspace-fhj5z" is forbidden: ValidatingAdmissionPolicy 'policy-4lr9q' with binding 'binding-4vxvf' denied request: expression 'object.spec.intent != 'bad'' resulted in error: no such key: intent
        	Test:       	TestMaximalPermissionPolicyAuthorizer
        	Messages:   	error creating cowboy in consumer workspace "root:e2e-workspace-jmr78:e2e-workspace-fhj5z"

This isn't always triggered, but it needs to be addressed.

embik · 2024-07-29T09:43:08Z

Addressed the previous error by reworking the internal informer/lister built by the generic policy plugin framework. Should work now. 🤞🏻

sttts · 2024-07-29T10:55:27Z

pkg/admission/validatingadmissionpolicy/validating_admission_policy.go

@@ -136,7 +141,8 @@ func (k *KubeValidatingAdmissionPolicy) Validate(ctx context.Context, a admissio
 		return err
 	}

-	return delegate.Validate(ctx, a, o)
+	err = delegate.Validate(ctx, a, o)
+	return err


nit: the previous was fine

sttts · 2024-07-29T10:57:12Z

pkg/admission/validatingadmissionpolicy/validating_admission_policy.go

+	plugin.SetSourceFactory(func(_ informers.SharedInformerFactory, client kubernetes.Interface, dynamicClient dynamic.Interface, restMapper meta.RESTMapper, clusterName logicalcluster.Name) generic.Source[validating.PolicyHook] {
+		return generic.NewPolicySource(
+			k.globalKubeSharedInformerFactory.Admissionregistration().V1().ValidatingAdmissionPolicies().Informer().Cluster(clusterName),
+			k.globalKubeSharedInformerFactory.Admissionregistration().V1().ValidatingAdmissionPolicyBindings().Informer().Cluster(clusterName),


Did you add replication of policies? I would have expected this to fail if not. The local informers have the local shard objects.

https://github.com/kcp-dev/kcp/pull/3140/files#diff-56ef0431fd7b0e03ab56df4aba923c02c16a6e523bd27230cd3aa856203c5f9eR228

This is where replication is configured. Through InstallIndexers, this is is wired into (s *Server) addIndexersToInformers, which returns the list of replicated GVRs. This is a relatively recent change made in #3139, which was necessary to that informer installation doesn't create a race condition on leader election changes.

sttts · 2024-07-29T10:57:52Z

test/e2e/conformance/validatingadmissionpolicy_test.go

+		},
+	}
+	policy, err = kubeClusterClient.Cluster(ws1Path).AdmissionregistrationV1().ValidatingAdmissionPolicies().Create(ctx, policy, metav1.CreateOptions{})
+


nit/style: no empty line before error checks.

sttts · 2024-07-29T10:59:35Z

test/e2e/conformance/validatingadmissionpolicy_test.go

+					return true
+				}
+			}
+			t.Logf("Unexpected error when trying to create bad cowboy: %s", err)


nit: a success is not unexpected while waiting for the policy to apply.

This is within a check for err != nil, so it only logs any error that's not the policy rejection. If resource creation succeeds, it just returns false but doesn't log this line.

…add e2e conformance test Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

embik · 2024-07-29T13:34:15Z

/retest

flake.

sttts · 2024-07-29T14:20:32Z

/lgtm

kcp-ci-bot · 2024-07-29T14:20:38Z

LGTM label has been added.

Git tree hash: 4e71586e672cb06c291d6f5a0b9ff4367f665ba1

embik · 2024-07-29T14:32:11Z

/hold cancel

embik added this to the 1.30-rebase milestone Jul 15, 2024

embik requested review from xrstf, sttts and mjudeikis July 15, 2024 10:16

kcp-ci-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 15, 2024

embik force-pushed the k8s-1.30 branch from 8a061f0 to 92f52e3 Compare July 15, 2024 13:05

kcp-ci-bot assigned mjudeikis Jul 15, 2024

kcp-ci-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jul 15, 2024

kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2024