-
Notifications
You must be signed in to change notification settings - Fork 11
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
Changes from 2 commits
c7b196f
1cbba47
0a23778
07d10c7
07d14a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/labels" | ||
webhookutil "k8s.io/apiserver/pkg/util/webhook" | ||
|
@@ -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 | ||
|
||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can undo this and just get the cluster from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ncdc |
||
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 | ||
|
@@ -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 | ||
} | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -193,6 +203,11 @@ type validatingWebhookAccessor struct { | |
clientErr error | ||
} | ||
|
||
func (v *validatingWebhookAccessor) GetLogicalCluster() logicalcluster.LogicalCluster { | ||
return v.cluster | ||
|
||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minimize changes |
||
"k8s.io/apimachinery/pkg/util/diff" | ||
) | ||
|
||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 keeps everything together. 2 makes rebases easier. I think I maybe am voting for 2. @sttts WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
|
@@ -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) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ import ( | |
"fmt" | ||
"io" | ||
|
||
"github.com/kcp-dev/apimachinery/pkg/logicalcluster" | ||
admissionv1 "k8s.io/api/admission/v1" | ||
admissionv1beta1 "k8s.io/api/admission/v1beta1" | ||
"k8s.io/api/admissionregistration/v1" | ||
|
@@ -140,7 +141,10 @@ func (a *Webhook) ValidateInitialization() error { | |
|
||
// ShouldCallHook returns invocation details if the webhook should be called, nil if the webhook should not be called, | ||
// or an error if an error was encountered during evaluation. | ||
func (a *Webhook) ShouldCallHook(h webhook.WebhookAccessor, attr admission.Attributes, o admission.ObjectInterfaces) (*WebhookInvocation, *apierrors.StatusError) { | ||
func (a *Webhook) ShouldCallHook(h webhook.WebhookAccessor, attr admission.Attributes, o admission.ObjectInterfaces, cluster logicalcluster.LogicalCluster) (*WebhookInvocation, *apierrors.StatusError) { | ||
if h.GetLogicalCluster() != cluster { | ||
return nil, nil | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I discussed with Shawn yesterday. I think we might want to split this into
WDYT? |
||
matches, matchNsErr := a.namespaceMatcher.MatchNamespaceSelector(h, attr) | ||
// Should not return an error here for webhooks which do not apply to the request, even if err is an unexpected scenario. | ||
if !matches && matchNsErr == nil { | ||
|
There was a problem hiding this comment.
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.