Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding the admistrationregistration api group #52

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/genericcontrolplane/apis/apis.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"net/http"
"time"

admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
apiserverinternalv1alpha1 "k8s.io/api/apiserverinternal/v1alpha1"
authenticationv1 "k8s.io/api/authentication/v1"
authorizationapiv1 "k8s.io/api/authorization/v1"
Expand Down Expand Up @@ -57,6 +58,7 @@ import (
"k8s.io/utils/clock"

// RESTStorage installers
admissionregistrationrest "k8s.io/kubernetes/pkg/registry/admissionregistration/rest"
apiserverinternalrest "k8s.io/kubernetes/pkg/registry/apiserverinternal/rest"
authenticationrest "k8s.io/kubernetes/pkg/registry/authentication/rest"
authorizationrest "k8s.io/kubernetes/pkg/registry/authorization/rest"
Expand Down Expand Up @@ -229,6 +231,7 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
rbacrest.RESTStorageProvider{Authorizer: c.GenericConfig.Authorization.Authorizer},
flowcontrolrest.RESTStorageProvider{},
eventsrest.RESTStorageProvider{TTL: c.ExtraConfig.EventTTL},
admissionregistrationrest.RESTStorageProvider{},
}
if err := m.InstallAPIs(c.ExtraConfig.APIResourceConfigSource, c.GenericConfig.RESTOptionsGetter, restStorageProviders...); err != nil {
return nil, err
Expand Down Expand Up @@ -439,6 +442,7 @@ func DefaultAPIResourceConfigSource() *serverstorage.ResourceConfig {
rbacv1.SchemeGroupVersion,
flowcontrolv1beta1.SchemeGroupVersion,
flowcontrolv1beta2.SchemeGroupVersion,
admissionregistrationv1.SchemeGroupVersion,
)
// disable alpha versions explicitly so we have a full list of what's possible to serve
ret.DisableVersions(
Expand Down
2 changes: 2 additions & 0 deletions pkg/genericcontrolplane/apis/install/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package install

import (
"k8s.io/kubernetes/pkg/api/genericcontrolplanescheme"
admissionregistrationinstall "k8s.io/kubernetes/pkg/apis/admissionregistration/install"
authenticationinstall "k8s.io/kubernetes/pkg/apis/authentication/install"
authorizationinstall "k8s.io/kubernetes/pkg/apis/authorization/install"
certificatesinstall "k8s.io/kubernetes/pkg/apis/certificates/install"
Expand All @@ -37,4 +38,5 @@ func init() {
rbacinstall.Install(genericcontrolplanescheme.Scheme)
flowcontrolinstall.Install(genericcontrolplanescheme.Scheme)
eventsinstall.Install(genericcontrolplanescheme.Scheme)
admissionregistrationinstall.Install(genericcontrolplanescheme.Scheme)
}
5 changes: 2 additions & 3 deletions pkg/genericcontrolplane/options/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,11 @@ func NewServerRunOptions() *ServerRunOptions {
// disable the watch cache
s.Etcd.EnableWatchCache = false

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

return &s
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"sort"
"sync/atomic"

"github.com/kcp-dev/apimachinery/pkg/logicalcluster"

"k8s.io/api/admissionregistration/v1"
"k8s.io/apimachinery/pkg/labels"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
Expand Down Expand Up @@ -120,7 +122,7 @@ func mergeMutatingWebhookConfigurations(configurations []*v1.MutatingWebhookConf
n := c.Webhooks[i].Name
uid := fmt.Sprintf("%s/%s/%d", c.Name, n, names[n])
names[n]++
accessors = append(accessors, webhook.NewMutatingWebhookAccessor(uid, c.Name, &c.Webhooks[i]))
accessors = append(accessors, webhook.NewMutatingWebhookAccessor(uid, c.Name, logicalcluster.From(c), &c.Webhooks[i]))
}
}
return accessors
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"sort"
"sync/atomic"

logicalcluster "github.com/kcp-dev/apimachinery/pkg/logicalcluster"

"k8s.io/api/admissionregistration/v1"
"k8s.io/apimachinery/pkg/labels"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
Expand Down Expand Up @@ -118,7 +120,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, logicalcluster.From(c), &c.Webhooks[i]))
}
}
return accessors
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ package webhook
import (
"sync"

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

v1 "k8s.io/api/admissionregistration/v1"
Copy link
Member

Choose a reason for hiding this comment

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

reduce changes. The alias is not necessary.

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
webhookutil "k8s.io/apiserver/pkg/util/webhook"
Expand All @@ -31,6 +33,8 @@ type WebhookAccessor interface {
// GetUID gets a string that uniquely identifies the webhook.
GetUID() string

GetLogicalCluster() logicalcluster.LogicalCluster

// GetConfigurationName gets the name of the webhook configuration that owns this webhook.
GetConfigurationName() string

Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@ncdc h has no ObjectMeta

return &mutatingWebhookAccessor{uid: uid, configurationName: configurationName, cluster: cluster, MutatingWebhook: h}
}

type mutatingWebhookAccessor struct {
*v1.MutatingWebhook
uid string
configurationName string
cluster logicalcluster.LogicalCluster

initObjectSelector sync.Once
objectSelector labels.Selector
Expand All @@ -93,6 +98,10 @@ type mutatingWebhookAccessor struct {
clientErr error
}

func (m *mutatingWebhookAccessor) GetLogicalCluster() logicalcluster.LogicalCluster {
return m.cluster
}

func (m *mutatingWebhookAccessor) GetUID() string {
return m.uid
}
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto re undo

return &validatingWebhookAccessor{uid: uid, configurationName: configurationName, cluster: cluster, ValidatingWebhook: h}
}

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

initObjectSelector sync.Once
objectSelector labels.Selector
Expand All @@ -193,6 +203,11 @@ type validatingWebhookAccessor struct {
clientErr error
}

func (v *validatingWebhookAccessor) GetLogicalCluster() logicalcluster.LogicalCluster {
return v.cluster

}

Copy link
Member

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

func (v *validatingWebhookAccessor) GetUID() string {
return v.uid
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

minimize changes

"k8s.io/apimachinery/pkg/util/diff"
)

Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

(General comment, not line specific)

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

  1. We modify this file
  2. 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?

Copy link
Member

Choose a reason for hiding this comment

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

external file if possible

if uid != accessor.GetUID() {
t.Errorf("expected GetUID to return %s, but got %s", accessor.GetUID(), uid)
}
Expand Down Expand Up @@ -77,7 +78,7 @@ func TestValidatingWebhookAccessor(t *testing.T) {
orig := &v1.ValidatingWebhook{}
f.Fuzz(orig)
uid := fmt.Sprintf("test.configuration.admission/%s/0", orig.Name)
accessor := NewValidatingWebhookAccessor(uid, "test.configuration.admission", orig)
accessor := NewValidatingWebhookAccessor(uid, "test.configuration.admission", logicalcluster.New("testing"), orig)
if uid != accessor.GetUID() {
t.Errorf("expected GetUID to return %s, but got %s", accessor.GetUID(), uid)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

drop the change

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apiserver/pkg/admission"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ limitations under the License.
package generic

import (
"context"
"fmt"
"strings"
"testing"

"k8s.io/api/admissionregistration/v1"
"github.com/kcp-dev/apimachinery/pkg/logicalcluster"
v1 "k8s.io/api/admissionregistration/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -69,6 +71,7 @@ func TestShouldCallHook(t *testing.T) {

webhook *v1.ValidatingWebhook
attrs admission.Attributes
cluster logicalcluster.LogicalCluster

expectCall bool
expectErr string
Expand Down Expand Up @@ -296,7 +299,7 @@ func TestShouldCallHook(t *testing.T) {

for i, testcase := range testcases {
t.Run(testcase.name, func(t *testing.T) {
invocation, err := a.ShouldCallHook(webhook.NewValidatingWebhookAccessor(fmt.Sprintf("webhook-%d", i), fmt.Sprintf("webhook-cfg-%d", i), testcase.webhook), testcase.attrs, interfaces)
invocation, err := a.ShouldCallHook(webhook.NewValidatingWebhookAccessor(fmt.Sprintf("webhook-%d", i), fmt.Sprintf("webhook-cfg-%d", i), testcase.cluster, testcase.webhook), testcase.attrs, interfaces, testcase.cluster)
if err != nil {
if len(testcase.expectErr) == 0 {
t.Fatal(err)
Expand Down Expand Up @@ -347,6 +350,12 @@ func (f fakeNamespaceLister) Get(name string) (*corev1.Namespace, error) {
}
return nil, errors.NewNotFound(corev1.Resource("namespaces"), name)
}
func (f fakeNamespaceLister) GetWithContext(ctx context.Context, name string) (*corev1.Namespace, error) {
return f.Get(name)
}
func (f fakeNamespaceLister) ListWithContext(ctx context.Context, selector labels.Selector) ([]*corev1.Namespace, error) {
return f.List(selector)
}

func BenchmarkShouldCallHookWithComplexSelector(b *testing.B) {
allScopes := v1.AllScopes
Expand Down Expand Up @@ -407,13 +416,13 @@ func BenchmarkShouldCallHookWithComplexSelector(b *testing.B) {
},
}

wbAccessor := webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", wb)
wbAccessor := webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", logicalcluster.New("cluster"), wb)
attrs := admission.NewAttributesRecord(nil, nil, schema.GroupVersionKind{"autoscaling", "v1", "Scale"}, "ns", "name", schema.GroupVersionResource{"apps", "v1", "deployments"}, "scale", admission.Create, &metav1.CreateOptions{}, false, nil)
interfaces := &admission.RuntimeObjectInterfaces{EquivalentResourceMapper: mapper}
a := &Webhook{namespaceMatcher: &namespace.Matcher{NamespaceLister: namespaceLister}, objectMatcher: &object.Matcher{}}

for i := 0; i < b.N; i++ {
a.ShouldCallHook(wbAccessor, attrs, interfaces)
a.ShouldCallHook(wbAccessor, attrs, interfaces, logicalcluster.New("cluster"))
}
}

Expand Down Expand Up @@ -475,13 +484,13 @@ func BenchmarkShouldCallHookWithComplexRule(b *testing.B) {
wb.Rules = append(wb.Rules, rule)
}

wbAccessor := webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", wb)
wbAccessor := webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", logicalcluster.New("cluster"), wb)
attrs := admission.NewAttributesRecord(nil, nil, schema.GroupVersionKind{"autoscaling", "v1", "Scale"}, "ns", "name", schema.GroupVersionResource{"apps", "v1", "deployments"}, "scale", admission.Create, &metav1.CreateOptions{}, false, nil)
interfaces := &admission.RuntimeObjectInterfaces{EquivalentResourceMapper: mapper}
a := &Webhook{namespaceMatcher: &namespace.Matcher{NamespaceLister: namespaceLister}, objectMatcher: &object.Matcher{}}

for i := 0; i < b.N; i++ {
a.ShouldCallHook(wbAccessor, attrs, interfaces)
a.ShouldCallHook(wbAccessor, attrs, interfaces, logicalcluster.New("cluster"))
}
}

Expand Down Expand Up @@ -548,12 +557,12 @@ func BenchmarkShouldCallHookWithComplexSelectorAndRule(b *testing.B) {
wb.Rules = append(wb.Rules, rule)
}

wbAccessor := webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", wb)
wbAccessor := webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", logicalcluster.New("cluster"), wb)
attrs := admission.NewAttributesRecord(nil, nil, schema.GroupVersionKind{"autoscaling", "v1", "Scale"}, "ns", "name", schema.GroupVersionResource{"apps", "v1", "deployments"}, "scale", admission.Create, &metav1.CreateOptions{}, false, nil)
interfaces := &admission.RuntimeObjectInterfaces{EquivalentResourceMapper: mapper}
a := &Webhook{namespaceMatcher: &namespace.Matcher{NamespaceLister: namespaceLister}, objectMatcher: &object.Matcher{}}

for i := 0; i < b.N; i++ {
a.ShouldCallHook(wbAccessor, attrs, interfaces)
a.ShouldCallHook(wbAccessor, attrs, interfaces, logicalcluster.New("cluster"))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib
defer func() {
webhookReinvokeCtx.SetLastWebhookInvocationOutput(attr.GetObject())
}()

var versionedAttr *generic.VersionedAttributes
for i, hook := range hooks {
attrForCheck := attr
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package mutating
Copy link
Member

Choose a reason for hiding this comment

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

patch_dispatcher.go


import (
"k8s.io/apiserver/pkg/admission/plugin/webhook/generic"
webhookutil "k8s.io/apiserver/pkg/util/webhook"
)

func NewMutatingDispatcher(p *Plugin) func(cm *webhookutil.ClientManager) generic.Dispatcher {
return newMutatingDispatcher(p)
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ limitations under the License.
package namespace

import (
"context"
"reflect"
"testing"

"github.com/kcp-dev/apimachinery/pkg/logicalcluster"
registrationv1 "k8s.io/api/admissionregistration/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
Expand All @@ -44,6 +46,12 @@ func (f fakeNamespaceLister) Get(name string) (*corev1.Namespace, error) {
}
return nil, errors.NewNotFound(corev1.Resource("namespaces"), name)
}
func (f fakeNamespaceLister) GetWithContext(ctx context.Context, name string) (*corev1.Namespace, error) {
return f.Get(name)
}
func (f fakeNamespaceLister) ListWithContext(ctx context.Context, selector labels.Selector) ([]*corev1.Namespace, error) {
return f.List(selector)
}

func TestGetNamespaceLabels(t *testing.T) {
namespace1Labels := map[string]string{
Expand Down Expand Up @@ -120,7 +128,7 @@ func TestNotExemptClusterScopedResource(t *testing.T) {
}
attr := admission.NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "mock-name", schema.GroupVersionResource{Version: "v1", Resource: "nodes"}, "", admission.Create, &metav1.CreateOptions{}, false, nil)
matcher := Matcher{}
matches, err := matcher.MatchNamespaceSelector(webhook.NewValidatingWebhookAccessor("mock-hook", "mock-cfg", hook), attr)
matches, err := matcher.MatchNamespaceSelector(webhook.NewValidatingWebhookAccessor("mock-hook", "mock-cfg", logicalcluster.New("mock-cluster"), hook), attr)
if err != nil {
t.Fatal(err)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ package object
import (
"testing"

"k8s.io/api/admissionregistration/v1"
"github.com/kcp-dev/apimachinery/pkg/logicalcluster"
v1 "k8s.io/api/admissionregistration/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
Expand Down Expand Up @@ -115,7 +116,7 @@ func TestObjectSelector(t *testing.T) {
}}}

t.Run(testcase.name, func(t *testing.T) {
match, err := matcher.MatchObjectSelector(webhook.NewValidatingWebhookAccessor("mock-hook", "mock-cfg", hook), testcase.attrs)
match, err := matcher.MatchObjectSelector(webhook.NewValidatingWebhookAccessor("mock-hook", "mock-cfg", logicalcluster.New("mock-cluster"), hook), testcase.attrs)
if err != nil {
t.Error(err)
}
Expand Down
Loading