Skip to content

Commit 52c2b64

Browse files
theobarberbanytheobarberbany
committed
Adds CAPI Machine VAP
- Adds CAPI Machine VAP, similar to the MAPI one, but for when MAPI is autoritative. - Adds tests - Updates machine sync reconciler to requeue after 2s of AuthoritativeAPI is empty (rather than the 10m default)
1 parent a18e6a8 commit 52c2b64

File tree

3 files changed

+373
-46
lines changed

3 files changed

+373
-46
lines changed

‎manifests/0000_30_cluster-api_09_admission-policies.yaml‎

Lines changed: 129 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -115,29 +115,29 @@ data:
115115
- expression: "variables.specLockedExceptAuthoritativeAPI"
116116
message: "You may only modify spec.authoritativeAPI. Any other change inside .spec is not allowed. This is because status.authoritativeAPI is set to Cluster API."
117117
118-
# Guard machine.openshift.io/* and kubernetes.io/* labels
118+
# Guard machine.openshift.io/*, kubernetes.io/* and cluster.x-k8s.io labels
119119
- expression: >
120120
!(
121121
variables.newLabels.exists(k,
122-
(k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io')) &&
122+
(k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) &&
123123
(variables.oldLabels[?k].orValue(null) != variables.newLabels[k])
124124
) ||
125125
variables.oldLabels.exists(k,
126-
(k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io')) &&
126+
(k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) &&
127127
!(k in variables.newLabels)
128128
)
129129
)
130130
message: "Cannot add, modify or delete any machine.openshift.io/* or kubernetes.io/* label. This is because status.authoritativeAPI is set to Cluster API."
131131
132-
# Guard machine.openshift.io/* annotations
132+
# Guard machine.openshift.io/* and cluster(s).x-k8s.io annotations
133133
- expression: >
134134
!(
135135
variables.newAnn.exists(k,
136-
k.startsWith('machine.openshift.io') &&
136+
(k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) &&
137137
(variables.oldAnn[?k].orValue(null) != variables.newAnn[k])
138138
) ||
139139
variables.oldAnn.exists(k,
140-
k.startsWith('machine.openshift.io') &&
140+
(k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) &&
141141
!(k in variables.newAnn)
142142
)
143143
)
@@ -151,6 +151,121 @@ data:
151151
variables.newLabels[?k].orValue(null) == variables.paramLabels[k]
152152
)
153153
message: "Cannot modify a Cluster API controlled label except to match the Cluster API mirrored machine. This is because status.authoritativeAPI is set to Cluster API."
154+
155+
# Don't allow setting the 'machine-template-hash' label. It should only be set by the CAPI controllers.
156+
- expression: "!('machine-template-hash' in variables.newLabels)"
157+
message: "Setting the 'machine-template-hash' label is forbidden.'"
158+
---
159+
apiVersion: admissionregistration.k8s.io/v1
160+
kind: ValidatingAdmissionPolicyBinding
161+
metadata:
162+
name: cluster-api-machine-vap
163+
spec:
164+
matchResources:
165+
namespaceSelector:
166+
matchLabels:
167+
kubernetes.io/metadata.name: openshift-cluster-api
168+
paramRef:
169+
namespace: openshift-machine-api
170+
# We 'Allow' here as we don't want to block CAPI Machine functionality
171+
# when no MAPI machine (param) exists. This might happen when a user
172+
# wants to not use MAPI, or is migrating.
173+
parameterNotFoundAction: Allow
174+
selector: {}
175+
policyName: cluster-api-machine-vap
176+
validationActions: [Deny]
177+
---
178+
apiVersion: admissionregistration.k8s.io/v1
179+
kind: ValidatingAdmissionPolicy
180+
metadata:
181+
name: cluster-api-machine-vap
182+
spec:
183+
failurePolicy: Fail
184+
185+
paramKind:
186+
apiVersion: machine.openshift.io/v1beta1
187+
kind: Machine
188+
189+
matchConstraints:
190+
resourceRules:
191+
- apiGroups: ["cluster.x-k8s.io"]
192+
apiVersions: ["v1beta1"]
193+
operations: ["UPDATE"]
194+
resources: ["machines"]
195+
196+
# Requests must satisfy every matchCondition to reach the validations
197+
matchConditions:
198+
- name: check-only-non-service-account-requests
199+
expression: >-
200+
!(request.userInfo.username in [
201+
"system:serviceaccount:openshift-machine-api:machine-api-controllers",
202+
"system:serviceaccount:openshift-cluster-api:cluster-capi-operator"
203+
])
204+
- name: check-param-match
205+
expression: 'object.metadata.name == params.metadata.name'
206+
- name: check-authoritativeAPI-machineapi
207+
expression: "params.?status.?authoritativeAPI.orValue(\"\") == \"MachineAPI\""
208+
variables:
209+
# label maps
210+
- name: newLabels
211+
expression: "object.metadata.?labels.orValue({})"
212+
- name: oldLabels
213+
expression: "oldObject.metadata.?labels.orValue({})"
214+
- name: paramLabels
215+
expression: "params.metadata.?labels.orValue({})"
216+
217+
# annotation maps
218+
- name: newAnn
219+
expression: "object.metadata.?annotations.orValue({})"
220+
- name: oldAnn
221+
expression: "oldObject.metadata.?annotations.orValue({})"
222+
223+
# All validations must evaluate to TRUE
224+
validations:
225+
# Only spec.authoritativeAPI may change
226+
- expression: "object.spec == oldObject.spec"
227+
message: "Changing .spec is not allowed. This is because status.authoritativeAPI is set to Machine API."
228+
229+
# Guard machine.openshift.io/* and kubernetes.io/* and cluster.x-k8s.io/* labels
230+
- expression: >
231+
!(
232+
variables.newLabels.exists(k,
233+
(k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) &&
234+
(variables.oldLabels[?k].orValue(null) != variables.newLabels[k])
235+
) ||
236+
variables.oldLabels.exists(k,
237+
(k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) &&
238+
!(k in variables.newLabels)
239+
)
240+
)
241+
message: "Cannot add, modify or delete any machine.openshift.io/*, kubernetes.io/* or cluster.x-k8s.io/* label. This is because status.authoritativeAPI is set to Machine API."
242+
243+
# Guard machine.openshift.io/* and cluster.x-k8s.io/* and clusters.x-k8s.io/* annotations
244+
- expression: >
245+
!(
246+
variables.newAnn.exists(k,
247+
(k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) &&
248+
(variables.oldAnn[?k].orValue(null) != variables.newAnn[k])
249+
) ||
250+
variables.oldAnn.exists(k,
251+
(k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) &&
252+
!(k in variables.newAnn)
253+
)
254+
)
255+
message: "Cannot add, modify or delete any machine.openshift.io/* or cluster.x-k8s.io or clusters.x-k8s.io annotation. This is because status.authoritativeAPI is set to Machine API."
256+
257+
# Param-controlled labels (labels on the MAPI machine) may change only to match the value on the MAPI Machine
258+
- expression: >
259+
variables.paramLabels.all(
260+
k,
261+
variables.newLabels[?k].orValue(null) == variables.oldLabels[?k].orValue(null) ||
262+
variables.newLabels[?k].orValue(null) == variables.paramLabels[k]
263+
)
264+
message: "Cannot modify a Machine API controlled label except to match the Machine API mirrored machine. This is because status.authoritativeAPI is set to Machine API."
265+
266+
# Don't allow setting the 'machine-template-hash' label. It should only be set by the CAPI controllers.
267+
- expression: "!('machine-template-hash' in variables.newLabels)"
268+
message: "Setting the 'machine-template-hash' label is forbidden.'"
154269
---
155270
apiVersion: admissionregistration.k8s.io/v1
156271
kind: ValidatingAdmissionPolicy
@@ -165,15 +280,15 @@ data:
165280
operations: ["CREATE", "UPDATE"]
166281
resources: ["machines", "machinesets"]
167282
variables:
168-
- name: machineSpec
169-
expression: "object.kind == 'Machine' ? object.spec : object.spec.template.spec"
170-
- name: specPath
171-
expression: "object.kind == 'Machine' ? 'spec' : 'spec.template.spec'"
283+
- name: machineSpec
284+
expression: "object.kind == 'Machine' ? object.spec : object.spec.template.spec"
285+
- name: specPath
286+
expression: "object.kind == 'Machine' ? 'spec' : 'spec.template.spec'"
172287
validations:
173-
- expression: "!has(variables.machineSpec.version)"
174-
messageExpression: "variables.specPath + '.version is a forbidden field'"
175-
- expression: "!has(variables.machineSpec.readinessGates)"
176-
messageExpression: "variables.specPath + '.readinessGates is a forbidden field'"
288+
- expression: "!has(variables.machineSpec.version)"
289+
messageExpression: "variables.specPath + '.version is a forbidden field'"
290+
- expression: "!has(variables.machineSpec.readinessGates)"
291+
messageExpression: "variables.specPath + '.readinessGates is a forbidden field'"
177292
---
178293
apiVersion: admissionregistration.k8s.io/v1
179294
kind: ValidatingAdmissionPolicyBinding

‎pkg/controllers/machinesync/machine_sync_controller.go‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -540,12 +540,12 @@ func (r *MachineSyncReconciler) reconcileMAPIMachinetoCAPIMachine(ctx context.Co
540540
BlockOwnerDeletion: ptr.To(true),
541541
}})
542542

543-
result, syncronizationIsProgressing, err := r.createOrUpdateCAPIInfraMachine(ctx, mapiMachine, infraMachine, newCAPIInfraMachine)
543+
result, synchronizationIsProgressing, err := r.createOrUpdateCAPIInfraMachine(ctx, mapiMachine, infraMachine, newCAPIInfraMachine)
544544
if err != nil {
545545
return result, fmt.Errorf("unable to ensure Cluster API infra machine: %w", err)
546546
}
547547

548-
if syncronizationIsProgressing {
548+
if synchronizationIsProgressing {
549549
return ctrl.Result{RequeueAfter: time.Second * 1}, r.applySynchronizedConditionWithPatch(ctx, mapiMachine, corev1.ConditionUnknown,
550550
reasonProgressingToCreateCAPIInfraMachine, progressingToSynchronizeMAPItoCAPI, nil)
551551
}
@@ -605,9 +605,9 @@ func (r *MachineSyncReconciler) convertCAPIToMAPIMachine(capiMachine *clusterv1.
605605
func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Context, mapiMachine *machinev1beta1.Machine, infraMachine client.Object, newCAPIInfraMachine client.Object) (ctrl.Result, bool, error) { //nolint:unparam
606606
logger := log.FromContext(ctx)
607607
// This variable tracks whether or not we are still progressing
608-
// towards syncronizing the MAPI machine with the CAPI infra machine.
608+
// towards synchronized the MAPI machine with the CAPI infra machine.
609609
// It is then passed up the stack so the syncronized condition can be set accordingly.
610-
syncronizationIsProgressing := false
610+
synchronizationIsProgressing := false
611611

612612
alreadyExists := false
613613

@@ -618,10 +618,10 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Conte
618618
createErr := fmt.Errorf("failed to create Cluster API infra machine: %w", err)
619619

620620
if condErr := r.applySynchronizedConditionWithPatch(ctx, mapiMachine, corev1.ConditionFalse, reasonFailedToCreateCAPIInfraMachine, createErr.Error(), nil); condErr != nil {
621-
return ctrl.Result{}, syncronizationIsProgressing, utilerrors.NewAggregate([]error{createErr, condErr})
621+
return ctrl.Result{}, synchronizationIsProgressing, utilerrors.NewAggregate([]error{createErr, condErr})
622622
}
623623

624-
return ctrl.Result{}, syncronizationIsProgressing, createErr
624+
return ctrl.Result{}, synchronizationIsProgressing, createErr
625625
} else if apierrors.IsAlreadyExists(err) {
626626
// this handles the case where the CAPI Machine is not present, so we can't resolve the
627627
// infraMachine ref from it - but the InfraMachine exists. (e.g a user deletes the CAPI machine manually).
@@ -630,7 +630,7 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Conte
630630
} else {
631631
logger.Info("Successfully created Cluster API infra machine")
632632

633-
return ctrl.Result{}, syncronizationIsProgressing, nil
633+
return ctrl.Result{}, synchronizationIsProgressing, nil
634634
}
635635
}
636636

@@ -640,10 +640,10 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Conte
640640
getErr := fmt.Errorf("failed to get Cluster API infra machine: %w", err)
641641

642642
if condErr := r.applySynchronizedConditionWithPatch(ctx, mapiMachine, corev1.ConditionFalse, reasonFailedToGetCAPIInfraResources, getErr.Error(), nil); condErr != nil {
643-
return ctrl.Result{}, syncronizationIsProgressing, utilerrors.NewAggregate([]error{getErr, condErr})
643+
return ctrl.Result{}, synchronizationIsProgressing, utilerrors.NewAggregate([]error{getErr, condErr})
644644
}
645645

646-
return ctrl.Result{}, syncronizationIsProgressing, getErr
646+
return ctrl.Result{}, synchronizationIsProgressing, getErr
647647
}
648648
}
649649

@@ -654,15 +654,15 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Conte
654654

655655
if condErr := r.applySynchronizedConditionWithPatch(
656656
ctx, mapiMachine, corev1.ConditionFalse, reasonFailedToUpdateCAPIInfraMachine, updateErr.Error(), nil); condErr != nil {
657-
return ctrl.Result{}, syncronizationIsProgressing, utilerrors.NewAggregate([]error{updateErr, condErr})
657+
return ctrl.Result{}, synchronizationIsProgressing, utilerrors.NewAggregate([]error{updateErr, condErr})
658658
}
659659

660-
return ctrl.Result{}, syncronizationIsProgressing, updateErr
660+
return ctrl.Result{}, synchronizationIsProgressing, updateErr
661661
}
662662

663663
if len(capiInfraMachinesDiff) == 0 {
664664
logger.Info("No changes detected in Cluster API infra machine")
665-
return ctrl.Result{}, syncronizationIsProgressing, nil
665+
return ctrl.Result{}, synchronizationIsProgressing, nil
666666
}
667667

668668
logger.Info("Deleting the corresponding Cluster API infra machine as it is out of date, it will be recreated", "diff", fmt.Sprintf("%+v", capiInfraMachinesDiff))
@@ -674,10 +674,10 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Conte
674674

675675
if condErr := r.applySynchronizedConditionWithPatch(
676676
ctx, mapiMachine, corev1.ConditionFalse, reasonFailedToUpdateCAPIInfraMachine, deleteErr.Error(), nil); condErr != nil {
677-
return ctrl.Result{}, syncronizationIsProgressing, utilerrors.NewAggregate([]error{deleteErr, condErr})
677+
return ctrl.Result{}, synchronizationIsProgressing, utilerrors.NewAggregate([]error{deleteErr, condErr})
678678
}
679679

680-
return ctrl.Result{}, syncronizationIsProgressing, deleteErr
680+
return ctrl.Result{}, synchronizationIsProgressing, deleteErr
681681
}
682682

683683
// Remove finalizers from the deleting CAPI infraMachine, it is not authoritative.
@@ -690,10 +690,10 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Conte
690690

691691
if condErr := r.applySynchronizedConditionWithPatch(
692692
ctx, mapiMachine, corev1.ConditionFalse, reasonFailedToUpdateCAPIInfraMachine, deleteErr.Error(), nil); condErr != nil {
693-
return ctrl.Result{}, syncronizationIsProgressing, utilerrors.NewAggregate([]error{deleteErr, condErr})
693+
return ctrl.Result{}, synchronizationIsProgressing, utilerrors.NewAggregate([]error{deleteErr, condErr})
694694
}
695695

696-
return ctrl.Result{}, syncronizationIsProgressing, deleteErr
696+
return ctrl.Result{}, synchronizationIsProgressing, deleteErr
697697
}
698698

699699
// The outdated outdated CAPI infra machine has been deleted.
@@ -702,9 +702,9 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Conte
702702

703703
// Set the syncronized as progressing to signal the caller
704704
// we are still progressing and aren't fully synced yet.
705-
syncronizationIsProgressing = true
705+
synchronizationIsProgressing = true
706706

707-
return ctrl.Result{}, syncronizationIsProgressing, nil
707+
return ctrl.Result{}, synchronizationIsProgressing, nil
708708
}
709709

710710
// createOrUpdateCAPIMachine creates a CAPI machine from a MAPI one, or updates if it exists and it is out of date.

0 commit comments

Comments
 (0)