Skip to content

Commit 1c5952e

Browse files
dkoshkinjimmidyson
dkoshkin
and
jimmidyson
authored
fix: skip UUID annotation webhook for clusters with nil topology (#860)
**What problem does this PR solve?**: We only want this webhook to run on Clusters that are created with cluster topology. This also fixes an issue when an existing Cluster that was created before deploying CAREN and this webhook is moved to the bootstrap cluster. Currently it fails the move with ``` 2024-08-14 11:14:55 ERR err="unable to pivot to the to-cluster: error setting Cluster.Spec.Paused=true: action failed after 10 attempts: error patching Cluster default/e2e-preprovisioned-upgrade-573225644: admission webhook "cluster-validator.caren.nutanix.com" denied the request: missing cluster UUID annotation caren.nutanix.com/cluster-uuid" ``` **Which issue(s) this PR fixes**: Fixes # **How Has This Been Tested?**: <!-- Please describe the tests that you ran to verify your changes. Provide output from the tests and any manual steps needed to replicate the tests. --> **Special notes for your reviewer**: <!-- Use this to provide any additional information to the reviewers. This may include: - Best way to review the PR. - Where the author wants the most review attention on. - etc. --> I'm also now wondering if we want the validate to fail when the annotation is missing. Its possible that an existing Cluster that was created using another ClusterClass that does not use CAREN and then when CAREN gets installed, it prevents those existing Clusters from being updated because of a missing annotation that won't even be used. **Should this webhook be smarter and only validate and default Clusters that are referencing a CC that is using CAREN?** --------- Co-authored-by: Jimmi Dyson <[email protected]>
1 parent d434977 commit 1c5952e

File tree

4 files changed

+228
-63
lines changed

4 files changed

+228
-63
lines changed

‎internal/test/envtest/environment.go

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ func init() {
8383
// RunInput is the input for Run.
8484
type RunInput struct {
8585
M *testing.M
86-
ManagerUncachedObjs []client.Object
86+
EnvironmentOpts []EnvironmentOpt
8787
SetupReconcilers func(ctx context.Context, mgr ctrl.Manager)
8888
SetupEnv func(e *Environment)
8989
WebhookInstallOptions envtest.WebhookInstallOptions
@@ -100,7 +100,7 @@ type RunInput struct {
100100
// to a non-empty value.
101101
func Run(ctx context.Context, input RunInput) int {
102102
// Bootstrapping test environment
103-
env := newEnvironment(input.WebhookInstallOptions, input.ManagerUncachedObjs...)
103+
env := newEnvironment(ctx, input.WebhookInstallOptions, input.EnvironmentOpts...)
104104

105105
if input.SetupReconcilers != nil {
106106
input.SetupReconcilers(ctx, env.Manager)
@@ -154,14 +154,43 @@ type Environment struct {
154154
cancelManager context.CancelFunc
155155
}
156156

157+
type environmentOpts struct {
158+
uncachedObjs []client.Object
159+
preexistingObjs []client.Object
160+
}
161+
162+
// EnvironmentOption is a functional option for configuring the environment.
163+
type EnvironmentOpt func(*environmentOpts)
164+
165+
// WithUncachedObjects sets the list of objects that should not be cached by the controller-runtime client.
166+
func WithUncachedObjects(objs ...client.Object) EnvironmentOpt {
167+
return func(opts *environmentOpts) {
168+
opts.uncachedObjs = objs
169+
}
170+
}
171+
172+
// WithPreexistingObjects sets the list of objects that should be created before the test environment is started.
173+
func WithPreexistingObjects(objs ...client.Object) EnvironmentOpt {
174+
return func(opts *environmentOpts) {
175+
opts.preexistingObjs = objs
176+
}
177+
}
178+
157179
// newEnvironment creates a new environment spinning up a local api-server.
158180
//
159181
// This function should be called only once for each package you're running tests within,
160182
// usually the environment is initialized in a suite_test.go file within a `BeforeSuite` ginkgo block.
161183
func newEnvironment(
184+
ctx context.Context,
162185
webhookInstallOptions envtest.WebhookInstallOptions,
163-
uncachedObjs ...client.Object,
186+
opts ...EnvironmentOpt,
164187
) *Environment {
188+
// Apply options.
189+
envOpts := &environmentOpts{}
190+
for _, opt := range opts {
191+
opt(envOpts)
192+
}
193+
165194
// Create the test environment.
166195
env := &envtest.Environment{
167196
ErrorIfCRDPathMissing: true,
@@ -205,7 +234,7 @@ func newEnvironment(
205234
},
206235
Client: client.Options{
207236
Cache: &client.CacheOptions{
208-
DisableFor: uncachedObjs,
237+
DisableFor: envOpts.uncachedObjs,
209238
},
210239
},
211240
WebhookServer: webhook.NewServer(webhook.Options{
@@ -221,6 +250,13 @@ func newEnvironment(
221250
klog.Fatalf("Failed to start testenv manager: %v", err)
222251
}
223252

253+
// Create pre-existing objects.
254+
for _, obj := range envOpts.preexistingObjs {
255+
if err := mgr.GetClient().Create(ctx, obj); err != nil {
256+
klog.Fatalf("Failed to create pre-existing object: %v", err)
257+
}
258+
}
259+
224260
return &Environment{
225261
Manager: mgr,
226262
Client: mgr.GetClient(),

‎pkg/webhook/cluster/clusteruuidlabeler.go

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,10 @@ func (a *clusterUUIDLabeler) defaulter(
5050
return admission.Errored(http.StatusBadRequest, err)
5151
}
5252

53+
if cluster.Spec.Topology == nil {
54+
return admission.Allowed("")
55+
}
56+
5357
if cluster.Annotations == nil {
5458
cluster.Annotations = make(map[string]string, 1)
5559
}
@@ -60,7 +64,7 @@ func (a *clusterUUIDLabeler) defaulter(
6064
// If this is an update request, copy the UUID from the old object if it exists. This prevents deletion of the
6165
// annotation.
6266
//
63-
// This is also important for move operations where the clusterctl for some reason deletes
67+
// This is especially important for move operations where the clusterctl for some reason deletes
6468
// all annotations (see
6569
// https://github.com/kubernetes-sigs/cluster-api/blob/v1.7.4/cmd/clusterctl/client/cluster/mover.go#L1188).
6670
// Without this logic, the UUID would be deleted and the UUID validation webhook would fail.
@@ -77,6 +81,12 @@ func (a *clusterUUIDLabeler) defaulter(
7781
oldClusterUUID, ok := oldCluster.Annotations[v1alpha1.ClusterUUIDAnnotationKey]
7882
if ok {
7983
cluster.Annotations[v1alpha1.ClusterUUIDAnnotationKey] = oldClusterUUID
84+
} else {
85+
// If the old cluster does not have a UUID, generate a new one. This can happen if the cluster was
86+
// created before this webhook was installed or if the cluster began without spec.topology and was
87+
// later converted to a ClusterClass based cluster.
88+
cluster.Annotations[v1alpha1.ClusterUUIDAnnotationKey] = uuid.Must(uuid.NewV7()).
89+
String()
8090
}
8191
// If this is a create request, generate a new UUID.
8292
case v1.Create:
@@ -102,6 +112,10 @@ func (a *clusterUUIDLabeler) validate(
102112
return admission.Errored(http.StatusBadRequest, err)
103113
}
104114

115+
if cluster.Spec.Topology == nil {
116+
return admission.Allowed("")
117+
}
118+
105119
clusterUUID, ok := cluster.Annotations[v1alpha1.ClusterUUIDAnnotationKey]
106120
if !ok {
107121
return admission.Denied(

‎pkg/webhook/cluster/webhook_suite_test.go

Lines changed: 108 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,15 @@
44
package cluster
55

66
import (
7+
"context"
78
"os"
89
"testing"
910

1011
admissionv1 "k8s.io/api/admissionregistration/v1"
1112
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
13+
"k8s.io/klog/v2"
1214
"k8s.io/utils/ptr"
15+
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
1316
ctrl "sigs.k8s.io/controller-runtime"
1417
ctrlenvtest "sigs.k8s.io/controller-runtime/pkg/envtest"
1518
"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -24,74 +27,121 @@ var (
2427
)
2528

2629
func TestMain(m *testing.M) {
30+
mutatingWebhook := &admissionv1.MutatingWebhookConfiguration{
31+
ObjectMeta: metav1.ObjectMeta{
32+
Name: "cluster-defaulter.caren.nutanix.com",
33+
},
34+
Webhooks: []admissionv1.MutatingWebhook{
35+
{
36+
Name: "cluster-defaulter.caren.nutanix.com",
37+
ClientConfig: admissionv1.WebhookClientConfig{
38+
Service: &admissionv1.ServiceReference{
39+
Path: ptr.To("/mutate-v1beta1-cluster"),
40+
},
41+
},
42+
Rules: []admissionv1.RuleWithOperations{{
43+
Operations: []admissionv1.OperationType{
44+
admissionv1.Create,
45+
admissionv1.Update,
46+
},
47+
Rule: admissionv1.Rule{
48+
APIGroups: []string{"cluster.x-k8s.io"},
49+
APIVersions: []string{"*"},
50+
Resources: []string{"clusters"},
51+
},
52+
}},
53+
SideEffects: ptr.To(admissionv1.SideEffectClassNone),
54+
AdmissionReviewVersions: []string{"v1"},
55+
// Start the environment with this to ignore in order to create the pre-existing objects.
56+
FailurePolicy: ptr.To(admissionv1.Ignore),
57+
},
58+
},
59+
}
60+
validatingWebhook := &admissionv1.ValidatingWebhookConfiguration{
61+
ObjectMeta: metav1.ObjectMeta{
62+
Name: "cluster-validator.caren.nutanix.com",
63+
},
64+
Webhooks: []admissionv1.ValidatingWebhook{
65+
{
66+
Name: "cluster-validator.caren.nutanix.com",
67+
ClientConfig: admissionv1.WebhookClientConfig{
68+
Service: &admissionv1.ServiceReference{
69+
Path: ptr.To("/validate-v1beta1-cluster"),
70+
},
71+
},
72+
Rules: []admissionv1.RuleWithOperations{{
73+
Operations: []admissionv1.OperationType{
74+
admissionv1.Create,
75+
admissionv1.Update,
76+
},
77+
Rule: admissionv1.Rule{
78+
APIGroups: []string{"cluster.x-k8s.io"},
79+
APIVersions: []string{"*"},
80+
Resources: []string{"clusters"},
81+
},
82+
}},
83+
SideEffects: ptr.To(admissionv1.SideEffectClassNone),
84+
AdmissionReviewVersions: []string{"v1"},
85+
// Start the environment with this to ignore in order to create the pre-existing objects.
86+
FailurePolicy: ptr.To(admissionv1.Ignore),
87+
},
88+
},
89+
}
90+
2791
os.Exit(envtest.Run(ctx, envtest.RunInput{
2892
M: m,
93+
EnvironmentOpts: []envtest.EnvironmentOpt{
94+
envtest.WithPreexistingObjects(
95+
// Create a pre-existing object without topology or the UUID annotation.
96+
&clusterv1.Cluster{
97+
ObjectMeta: metav1.ObjectMeta{
98+
Name: "preexisting-without-topology-or-uuid-annotation",
99+
Namespace: metav1.NamespaceDefault,
100+
},
101+
},
102+
// Create a pre-existing object with topology but without the UUID annotation.
103+
&clusterv1.Cluster{
104+
ObjectMeta: metav1.ObjectMeta{
105+
Name: "preexisting-with-topology-without-uuid-annotation",
106+
Namespace: metav1.NamespaceDefault,
107+
},
108+
Spec: clusterv1.ClusterSpec{
109+
Topology: &clusterv1.Topology{},
110+
},
111+
},
112+
),
113+
},
114+
WebhookInstallOptions: ctrlenvtest.WebhookInstallOptions{
115+
MutatingWebhooks: []*admissionv1.MutatingWebhookConfiguration{
116+
mutatingWebhook,
117+
},
118+
ValidatingWebhooks: []*admissionv1.ValidatingWebhookConfiguration{
119+
validatingWebhook,
120+
},
121+
},
29122
SetupEnv: func(e *envtest.Environment) {
30123
e.GetWebhookServer().Register("/mutate-v1beta1-cluster", &webhook.Admission{
31124
Handler: NewDefaulter(e.GetClient(), admission.NewDecoder(e.GetScheme())),
32125
})
33126
e.GetWebhookServer().Register("/validate-v1beta1-cluster", &webhook.Admission{
34127
Handler: NewValidator(e.GetClient(), admission.NewDecoder(e.GetScheme())),
35128
})
129+
130+
// The webhooks are initially installed with Ignore failure policy above to allow creating objects before
131+
// actually registering the webhooks so now the webhooks are installed above we can the webhooks to failure
132+
// policy to ensure they fail if not installed or called correctly.
133+
mutatingWebhook.Webhooks[0].FailurePolicy = ptr.To(admissionv1.Fail)
134+
err := e.GetClient().Update(context.Background(), mutatingWebhook)
135+
if err != nil {
136+
klog.Fatalf("failed to update the mutating webhook failure policy: %v", err)
137+
}
138+
validatingWebhook.Webhooks[0].FailurePolicy = ptr.To(admissionv1.Fail)
139+
err = e.GetClient().Update(context.Background(), validatingWebhook)
140+
if err != nil {
141+
klog.Fatalf("failed to update the validating webhook failure policy: %v", err)
142+
}
143+
36144
env = e
37145
},
38-
WebhookInstallOptions: ctrlenvtest.WebhookInstallOptions{
39-
MutatingWebhooks: []*admissionv1.MutatingWebhookConfiguration{{
40-
ObjectMeta: metav1.ObjectMeta{
41-
Name: "cluster-defaulter.caren.nutanix.com",
42-
},
43-
Webhooks: []admissionv1.MutatingWebhook{
44-
{
45-
Name: "cluster-defaulter.caren.nutanix.com",
46-
ClientConfig: admissionv1.WebhookClientConfig{
47-
Service: &admissionv1.ServiceReference{
48-
Path: ptr.To("/mutate-v1beta1-cluster"),
49-
},
50-
},
51-
Rules: []admissionv1.RuleWithOperations{{
52-
Operations: []admissionv1.OperationType{
53-
admissionv1.Create,
54-
admissionv1.Update,
55-
},
56-
Rule: admissionv1.Rule{
57-
APIGroups: []string{"cluster.x-k8s.io"},
58-
APIVersions: []string{"*"},
59-
Resources: []string{"clusters"},
60-
},
61-
}},
62-
SideEffects: ptr.To(admissionv1.SideEffectClassNone),
63-
AdmissionReviewVersions: []string{"v1"},
64-
},
65-
},
66-
}},
67-
ValidatingWebhooks: []*admissionv1.ValidatingWebhookConfiguration{{
68-
ObjectMeta: metav1.ObjectMeta{
69-
Name: "cluster-validator.caren.nutanix.com",
70-
},
71-
Webhooks: []admissionv1.ValidatingWebhook{
72-
{
73-
Name: "cluster-validator.caren.nutanix.com",
74-
ClientConfig: admissionv1.WebhookClientConfig{
75-
Service: &admissionv1.ServiceReference{
76-
Path: ptr.To("/validate-v1beta1-cluster"),
77-
},
78-
},
79-
Rules: []admissionv1.RuleWithOperations{{
80-
Operations: []admissionv1.OperationType{
81-
admissionv1.Create,
82-
admissionv1.Update,
83-
},
84-
Rule: admissionv1.Rule{
85-
APIGroups: []string{"cluster.x-k8s.io"},
86-
APIVersions: []string{"*"},
87-
Resources: []string{"clusters"},
88-
},
89-
}},
90-
SideEffects: ptr.To(admissionv1.SideEffectClassNone),
91-
AdmissionReviewVersions: []string{"v1"},
92-
},
93-
},
94-
}},
95-
},
96146
}))
97147
}

0 commit comments

Comments
 (0)