Adding the admistrationregistration api group #52

shawn-hurley · 2022-03-11T19:15:00Z

Add ability to handle validating and mutating webhooks for logical cluster
Attempted to keep the changes localized to admission plugin package

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

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.:

shawn-hurley · 2022-03-11T19:15:46Z

staging/src/k8s.io/apiserver/pkg/util/webhook/client.go

@@ -222,6 +222,7 @@ func (cm *ClientManager) HookClient(cc ClientConfig) (*rest.RESTClient, error) {
 	cfg := rest.CopyConfig(restConfig)
 	cfg.Host = u.Scheme + "://" + u.Host
 	cfg.APIPath = u.Path
+	cfg.TLSClientConfig.Insecure = true


Need to remove, this was to just help me testing a webhook via URL without having to spend too much time on certs.

shawn-hurley · 2022-03-11T19:16:04Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go

+
+	name, err := genericapirequest.ClusterNameFrom(ctx)
+
+	fmt.Printf("\n\nCluster: %v\nerr:%v\n\n", name, err)


Will remove, used for testing

sttts · 2022-03-30T12:24:20Z

pkg/genericcontrolplane/options/options.go

@@ -114,7 +113,7 @@ func NewServerRunOptions() *ServerRunOptions {
 	s.Etcd.EnableWatchCache = false

 	// TODO: turn off the admission webhooks for now
-	s.Admission.DefaultOffPlugins.Insert(validating.PluginName, mutating.PluginName)
+	s.Admission.DefaultOffPlugins.Insert(mutating.PluginName)


why leaving mutating disabled?

sttts · 2022-03-30T12:24:52Z

staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go

@@ -118,7 +118,7 @@ func mergeValidatingWebhookConfigurations(configurations []*v1.ValidatingWebhook
 			n := c.Webhooks[i].Name
 			uid := fmt.Sprintf("%s/%s/%d", c.Name, n, names[n])
 			names[n]++
-			accessors = append(accessors, webhook.NewValidatingWebhookAccessor(uid, c.Name, &c.Webhooks[i]))
+			accessors = append(accessors, webhook.NewValidatingWebhookAccessor(uid, c.Name, c.ObjectMeta.ClusterName, &c.Webhooks[i]))


this needs a rebase to latest kube branch, with logicalcluster.LogicalCluster

sttts · 2022-03-30T12:25:00Z

staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go

@@ -21,7 +21,7 @@ import (
 	"sort"
 	"sync/atomic"

-	"k8s.io/api/admissionregistration/v1"
+	v1 "k8s.io/api/admissionregistration/v1"


drop this change

sttts · 2022-03-30T12:25:29Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go

@@ -31,6 +31,8 @@ type WebhookAccessor interface {
 	// GetUID gets a string that uniquely identifies the webhook.
 	GetUID() string

+	GetCluster() string


GetLogicalCluster() logicalcluster.LogicalCluster

sttts · 2022-03-30T12:25:42Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go

@@ -93,6 +95,10 @@ type mutatingWebhookAccessor struct {
 	clientErr  error
 }

+func (m *mutatingWebhookAccessor) GetCluster() string {
+	return ""


sttts · 2022-03-30T12:25:57Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go

 }

 type validatingWebhookAccessor struct {
 	*v1.ValidatingWebhook
 	uid               string
 	configurationName string
+	cluster           string


logicalcluster.LogicalCluster

sttts · 2022-03-30T12:26:03Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook.go

@@ -23,7 +23,7 @@ import (

 	admissionv1 "k8s.io/api/admission/v1"
 	admissionv1beta1 "k8s.io/api/admission/v1beta1"
-	"k8s.io/api/admissionregistration/v1"
+	v1 "k8s.io/api/admissionregistration/v1"


drop the change

sttts · 2022-03-30T12:26:55Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook.go

+func (a *Webhook) ShouldCallHook(h webhook.WebhookAccessor, attr admission.Attributes, o admission.ObjectInterfaces, clusterName string) (*WebhookInvocation, *apierrors.StatusError) {
+	if h.GetCluster() != clusterName {
+		return nil, nil
+	}


this looks fine for CRDs, but probably not enough for APIBindings. Before merging this PR I would like to see a kcp "proof" PR showing in e2e that admission works for APIBindings.

I discussed with Shawn yesterday. I think we might want to split this into

Just getting webhooks working within a single workspace

Support for webhooks for APIBindings, after we implement identity

WDYT?

Add ability to handle validating and mutating webhooks for logical cluster Attempted to keep the changes localized to admission plugin package

ncdc · 2022-04-06T17:50:30Z

go.mod

@@ -98,7 +98,7 @@ require (
 	gopkg.in/yaml.v2 v2.4.0
 	k8s.io/api v0.0.0
 	k8s.io/apiextensions-apiserver v0.0.0
-	k8s.io/apimachinery v0.0.0
+	k8s.io/apimachinery v0.23.5


This is the thing I was saying a rebase should fix, and needs to be reverted. Let me know if you can't get it undone after a go mod tidy.

ncdc · 2022-04-06T17:51:15Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go

@@ -71,14 +75,15 @@ type WebhookAccessor interface {
 }

 // NewMutatingWebhookAccessor creates an accessor for a MutatingWebhook.
-func NewMutatingWebhookAccessor(uid, configurationName string, h *v1.MutatingWebhook) WebhookAccessor {
-	return &mutatingWebhookAccessor{uid: uid, configurationName: configurationName, MutatingWebhook: h}
+func NewMutatingWebhookAccessor(uid, configurationName string, cluster logicalcluster.LogicalCluster, h *v1.MutatingWebhook) WebhookAccessor {


I think you can undo this and just get the cluster from h inside the function (rather than passing in cluster)

@ncdc h has no ObjectMeta

ncdc · 2022-04-06T17:51:38Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go

@@ -171,14 +180,15 @@ func (m *mutatingWebhookAccessor) GetValidatingWebhook() (*v1.ValidatingWebhook,
 }

 // NewValidatingWebhookAccessor creates an accessor for a ValidatingWebhook.
-func NewValidatingWebhookAccessor(uid, configurationName string, h *v1.ValidatingWebhook) WebhookAccessor {
-	return &validatingWebhookAccessor{uid: uid, configurationName: configurationName, ValidatingWebhook: h}
+func NewValidatingWebhookAccessor(uid, configurationName string, cluster logicalcluster.LogicalCluster, h *v1.ValidatingWebhook) WebhookAccessor {


Ditto re undo

ncdc · 2022-04-06T18:14:36Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors_test.go

@@ -37,7 +38,7 @@ func TestMutatingWebhookAccessor(t *testing.T) {
 			orig.ReinvocationPolicy = nil

 			uid := fmt.Sprintf("test.configuration.admission/%s/0", orig.Name)
-			accessor := NewMutatingWebhookAccessor(uid, "test.configuration.admission", orig)
+			accessor := NewMutatingWebhookAccessor(uid, "test.configuration.admission", logicalcluster.New(""), orig)


(General comment, not line specific)

Would like to ensure the unit test accounts for the new GetLogicalCluster function. I see 2 options:

We modify this file

We create a new kcp_accessors_test.go file where the only thing we test is our changes

1 keeps everything together. 2 makes rebases easier. I think I maybe am voting for 2.

@sttts WDYT?

external file if possible

…apibindings

sttts · 2022-04-27T07:58:17Z

pkg/genericcontrolplane/options/options.go

 	// Overwrite the default for storage data format.
 	s.Etcd.DefaultStorageMediaType = "application/vnd.kubernetes.protobuf"

+	s.Admission.DefaultOffPlugins.Insert(validating.PluginName, mutating.PluginName)
+


why this move? Note that every carry patch costs. We shouldn't do them if they are not strictly necessary.

sttts · 2022-04-27T07:59:18Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go

-	"k8s.io/api/admissionregistration/v1"
+	logicalcluster "github.com/kcp-dev/apimachinery/pkg/logicalcluster"
+
+	v1 "k8s.io/api/admissionregistration/v1"


reduce changes. The alias is not necessary.

sttts · 2022-04-27T08:02:35Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors_test.go

@@ -22,7 +22,8 @@ import (
 	"testing"

 	fuzz "github.com/google/gofuzz"
-	"k8s.io/api/admissionregistration/v1"
+	"github.com/kcp-dev/apimachinery/pkg/logicalcluster"
+	v1 "k8s.io/api/admissionregistration/v1"


minimize changes

sttts · 2022-04-27T08:03:15Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go

+	return v.cluster
+
+}
+


put into patch_accessor.go file

sttts · 2022-04-27T08:21:20Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/patch_file.go

@@ -0,0 +1,10 @@
+package mutating


patch_dispatcher.go

shawn-hurley commented Mar 11, 2022

View reviewed changes

shawn-hurley force-pushed the add-validation-admission-lcluster-aware branch from d14bc9e to ef2bdb0 Compare March 11, 2022 19:21

sttts reviewed Mar 30, 2022

View reviewed changes

shawn-hurley force-pushed the add-validation-admission-lcluster-aware branch from ef2bdb0 to 03721ff Compare March 30, 2022 13:41

shawn-hurley mentioned this pull request Apr 1, 2022

Enabling Validating and Mutating Webhook's for KCP kcp-dev/kcp#818

Merged

Adding the admistrationregistration api group

c7b196f

Add ability to handle validating and mutating webhooks for logical cluster Attempted to keep the changes localized to admission plugin package

shawn-hurley force-pushed the add-validation-admission-lcluster-aware branch from 5d3d7d5 to 7610335 Compare April 6, 2022 17:49

ncdc reviewed Apr 6, 2022

View reviewed changes

shawn-hurley force-pushed the add-validation-admission-lcluster-aware branch from 7610335 to 0513df7 Compare April 6, 2022 17:50

ncdc reviewed Apr 6, 2022

View reviewed changes

addressing feedback

1cbba47

shawn-hurley force-pushed the add-validation-admission-lcluster-aware branch from 0513df7 to 1cbba47 Compare April 6, 2022 17:59

ncdc reviewed Apr 6, 2022

View reviewed changes

Shawn Hurley added 3 commits April 20, 2022 08:58

making changes for new type of admission plugin that can also handle …

0a23778

…apibindings

adding patch to expose mutating webhook dispatcher

07d10c7

removing logical testing check from ShouldCallPlugin method

07d14a4

sttts reviewed Apr 27, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go

return v.cluster

}

Copy link

Member

sttts Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put into patch_accessor.go file

sttts reviewed Apr 27, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/patch_file.go

@@ -0,0 +1,10 @@

package mutating

Copy link

Member

sttts Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

patch_dispatcher.go

sttts mentioned this pull request Apr 27, 2022

apiserver/admission-webhooks: make lcluster aware #60

Merged

shawn-hurley closed this Apr 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the admistrationregistration api group #52

Adding the admistrationregistration api group #52

shawn-hurley commented Mar 11, 2022

shawn-hurley Mar 11, 2022

shawn-hurley Mar 11, 2022

sttts Mar 30, 2022

sttts Mar 30, 2022

sttts Mar 30, 2022

sttts Mar 30, 2022

sttts Mar 30, 2022

sttts Mar 30, 2022

sttts Mar 30, 2022

sttts Mar 30, 2022

ncdc Apr 6, 2022

ncdc Apr 6, 2022

ncdc Apr 6, 2022

sttts Apr 27, 2022

ncdc Apr 6, 2022

ncdc Apr 6, 2022

sttts Apr 27, 2022

sttts Apr 27, 2022

sttts Apr 27, 2022

sttts Apr 27, 2022

sttts Apr 27, 2022

sttts Apr 27, 2022


		name, err := genericapirequest.ClusterNameFrom(ctx)

		fmt.Printf("\n\nCluster: %v\nerr:%v\n\n", name, err)

Adding the admistrationregistration api group #52

Adding the admistrationregistration api group #52

Conversation

shawn-hurley commented Mar 11, 2022

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.:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment