Skip to content

Commit aa3e93b

Browse files
theobarberbanytheobarberbany
committed
CAPI MachineSet VAP
1 parent c3c9ca8 commit aa3e93b

File tree

2 files changed

+345
-3
lines changed

2 files changed

+345
-3
lines changed

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

Lines changed: 108 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ data:
277277
variables.newLabels[?k].orValue(null) == variables.oldLabels[?k].orValue(null) ||
278278
variables.newLabels[?k].orValue(null) == variables.paramLabels[k]
279279
)
280-
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."
280+
message: "Cannot modify a Cluster API controlled label except to match the Cluster API mirrored MachineSet. This is because status.authoritativeAPI is set to Cluster API."
281281
---
282282
apiVersion: admissionregistration.k8s.io/v1
283283
kind: ValidatingAdmissionPolicyBinding
@@ -391,6 +391,113 @@ data:
391391
message: "Setting the 'machine-template-hash' label is forbidden.'"
392392
---
393393
apiVersion: admissionregistration.k8s.io/v1
394+
kind: ValidatingAdmissionPolicyBinding
395+
metadata:
396+
name: cluster-api-machine-set-vap
397+
spec:
398+
matchResources:
399+
namespaceSelector:
400+
matchLabels:
401+
kubernetes.io/metadata.name: openshift-cluster-api
402+
paramRef:
403+
namespace: openshift-machine-api
404+
# We 'Allow' here as we don't want to block CAPI Machine functionality
405+
# when no MAPI machine (param) exists. This might happen when a user
406+
# wants to not use MAPI, or is migrating.
407+
parameterNotFoundAction: Allow
408+
selector: {}
409+
policyName: cluster-api-machine-set-vap
410+
validationActions: [Deny]
411+
---
412+
apiVersion: admissionregistration.k8s.io/v1
413+
kind: ValidatingAdmissionPolicy
414+
metadata:
415+
name: cluster-api-machine-set-vap
416+
spec:
417+
failurePolicy: Fail
418+
419+
paramKind:
420+
apiVersion: machine.openshift.io/v1beta1
421+
kind: MachineSet
422+
423+
matchConstraints:
424+
resourceRules:
425+
- apiGroups: ["cluster.x-k8s.io"]
426+
apiVersions: ["v1beta2"]
427+
operations: ["UPDATE"]
428+
resources: ["machinesets"]
429+
430+
# Requests must satisfy every matchCondition to reach the validations
431+
matchConditions:
432+
- name: check-only-non-service-account-requests
433+
expression: >-
434+
!(request.userInfo.username in [
435+
"system:serviceaccount:openshift-machine-api:machine-api-controllers",
436+
"system:serviceaccount:openshift-cluster-api:cluster-capi-operator"
437+
])
438+
- name: check-param-match
439+
expression: 'object.metadata.name == params.metadata.name'
440+
- name: check-authoritativeAPI-machineapi
441+
expression: "params.?status.?authoritativeAPI.orValue(\"\") == \"MachineAPI\""
442+
variables:
443+
# label maps
444+
- name: newLabels
445+
expression: "object.metadata.?labels.orValue({})"
446+
- name: oldLabels
447+
expression: "oldObject.metadata.?labels.orValue({})"
448+
- name: paramLabels
449+
expression: "params.metadata.?labels.orValue({})"
450+
451+
# annotation maps
452+
- name: newAnn
453+
expression: "object.metadata.?annotations.orValue({})"
454+
- name: oldAnn
455+
expression: "oldObject.metadata.?annotations.orValue({})"
456+
457+
# All validations must evaluate to TRUE
458+
validations:
459+
# Only spec.authoritativeAPI may change
460+
- expression: "object.spec == oldObject.spec"
461+
message: "Changing .spec is not allowed. This is because status.authoritativeAPI is set to Machine API."
462+
463+
# Guard machine.openshift.io/* and kubernetes.io/* and cluster.x-k8s.io/* labels
464+
- expression: >
465+
!(
466+
variables.newLabels.exists(k,
467+
(k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) &&
468+
(variables.oldLabels[?k].orValue(null) != variables.newLabels[k])
469+
) ||
470+
variables.oldLabels.exists(k,
471+
(k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) &&
472+
!(k in variables.newLabels)
473+
)
474+
)
475+
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."
476+
477+
# Guard machine.openshift.io/* and cluster.x-k8s.io/* and clusters.x-k8s.io/* annotations
478+
- expression: >
479+
!(
480+
variables.newAnn.exists(k,
481+
(k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) &&
482+
(variables.oldAnn[?k].orValue(null) != variables.newAnn[k])
483+
) ||
484+
variables.oldAnn.exists(k,
485+
(k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) &&
486+
!(k in variables.newAnn)
487+
)
488+
)
489+
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."
490+
491+
# Param-controlled labels (labels on the MAPI machine) may change only to match the value on the MAPI Machine
492+
- expression: >
493+
variables.paramLabels.all(
494+
k,
495+
variables.newLabels[?k].orValue(null) == variables.oldLabels[?k].orValue(null) ||
496+
variables.newLabels[?k].orValue(null) == variables.paramLabels[k]
497+
)
498+
message: "Cannot modify a Machine API controlled label except to match the Machine API mirrored MachineSet. This is because status.authoritativeAPI is set to Machine API."
499+
---
500+
apiVersion: admissionregistration.k8s.io/v1
394501
kind: ValidatingAdmissionPolicy
395502
metadata:
396503
name: openshift-cluster-api-prevent-setting-of-capi-fields-unsupported-by-mapi

‎pkg/controllers/machinesetsync/machineset_vap_test.go‎

Lines changed: 237 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,7 @@ var _ = Describe("MachineSet VAP Tests", func() {
418418

419419
Eventually(k.Get(capiSentinelMachineSet)).Should(Succeed())
420420

421-
admissiontestutils.VerifySentinelValidation(k, sentinelMachineSet, 2*timeout)
421+
admissiontestutils.VerifySentinelValidation(k, sentinelMachineSet, timeout)
422422

423423
By("Creating a shared machineset pair to be used across the tests")
424424
mapiMachineSetBuilder = machinev1resourcebuilder.MachineSet().
@@ -586,7 +586,7 @@ var _ = Describe("MachineSet VAP Tests", func() {
586586
It("rejects changing a label to differ from the param machine", func() {
587587
Eventually(k.Update(mapiMachineSet, func() {
588588
mapiMachineSet.Labels["capi-param-controlled-label"] = "foo"
589-
}), timeout).Should(MatchError(ContainSubstring("Cannot modify a Cluster API controlled label except to match the Cluster API mirrored machine")))
589+
}), timeout).Should(MatchError(ContainSubstring("Cannot modify a Cluster API controlled label except to match the Cluster API mirrored MachineSet")))
590590
})
591591
})
592592

@@ -746,4 +746,239 @@ var _ = Describe("MachineSet VAP Tests", func() {
746746
})
747747
})
748748
})
749+
750+
Context("Prevent changes to non-authoritative CAPI MachineSets except from sync controller", func() {
751+
var mapiMachineSetBuilder machinev1resourcebuilder.MachineSetBuilder
752+
var mapiMachineSet *mapiv1beta1.MachineSet
753+
754+
const (
755+
vapName string = "cluster-api-machine-set-vap"
756+
testLabelValue string = "test-value"
757+
)
758+
759+
BeforeEach(func() {
760+
By("Waiting for VAP to be ready")
761+
machineSetVap = &admissionregistrationv1.ValidatingAdmissionPolicy{}
762+
Eventually(k8sClient.Get(ctx, client.ObjectKey{Name: vapName}, machineSetVap), timeout).Should(Succeed())
763+
764+
Eventually(k.Update(machineSetVap, func() {
765+
admissiontestutils.AddSentinelValidation(machineSetVap)
766+
})).Should(Succeed())
767+
768+
Eventually(k.Object(machineSetVap), timeout).Should(
769+
HaveField("Status.ObservedGeneration", BeNumerically(">=", 2)),
770+
)
771+
772+
By("Updating the VAP binding")
773+
policyBinding = &admissionregistrationv1.ValidatingAdmissionPolicyBinding{}
774+
Eventually(k8sClient.Get(ctx, client.ObjectKey{
775+
Name: vapName}, policyBinding), timeout).Should(Succeed())
776+
777+
Eventually(k.Update(policyBinding, func() {
778+
// paramNamespace=mapiNamespace (MAPI resources are params)
779+
// targetNamespace=capiNamespace (CAPI resources are validated)
780+
admissiontestutils.UpdateVAPBindingNamespaces(policyBinding, mapiNamespace.GetName(), capiNamespace.GetName())
781+
}), timeout).Should(Succeed())
782+
783+
// Wait until the binding shows the patched values
784+
Eventually(k.Object(policyBinding), timeout).Should(
785+
SatisfyAll(
786+
HaveField("Spec.MatchResources.NamespaceSelector.MatchLabels",
787+
HaveKeyWithValue("kubernetes.io/metadata.name",
788+
capiNamespace.GetName())),
789+
),
790+
)
791+
792+
By("Creating throwaway MachineSet pair for sentinel validation")
793+
sentinelMachineSet := machinev1resourcebuilder.MachineSet().
794+
WithNamespace(mapiNamespace.Name).
795+
WithName("sentinel-machineset").
796+
WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityMachineAPI).
797+
Build()
798+
Eventually(k8sClient.Create(ctx, sentinelMachineSet), timeout).Should(Succeed())
799+
800+
Eventually(k.UpdateStatus(sentinelMachineSet, func() {
801+
sentinelMachineSet.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI
802+
})).Should(Succeed())
803+
804+
Eventually(k.Object(sentinelMachineSet), timeout).Should(
805+
HaveField("Status.AuthoritativeAPI", Equal(mapiv1beta1.MachineAuthorityMachineAPI)))
806+
807+
capiSentinelMachineSet := clusterv1resourcebuilder.MachineSet().
808+
WithName("sentinel-machineset").
809+
WithNamespace(capiNamespace.Name).
810+
WithTemplate(clusterv1.MachineTemplateSpec{
811+
Spec: clusterv1.MachineSpec{
812+
ProviderID: "force-having-a-spec",
813+
},
814+
}).
815+
Build()
816+
Eventually(k8sClient.Create(ctx, capiSentinelMachineSet)).Should(Succeed())
817+
818+
Eventually(k.Get(capiSentinelMachineSet)).Should(Succeed())
819+
820+
admissiontestutils.VerifySentinelValidation(k, capiSentinelMachineSet, timeout)
821+
822+
By("Creating a shared machineset pair to be used across the tests")
823+
mapiMachineSetBuilder = machinev1resourcebuilder.MachineSet().
824+
WithNamespace(mapiNamespace.Name).
825+
WithName(capiMachineSet.Name).
826+
WithProviderSpecBuilder(machinev1resourcebuilder.AWSProviderSpec().WithLoadBalancers(nil)).
827+
WithLabels(map[string]string{
828+
"machine.openshift.io/cluster-api-cluster": "ci-op-gs2k97d6-c9e33-2smph",
829+
"mapi-param-controlled-label": "param-controlled-key",
830+
}).WithAnnotations(map[string]string{
831+
"capacity.cluster-autoscaler.kubernetes.io/labels": "kubernetes.io/arch=amd64",
832+
"machine.openshift.io/GPU": "0",
833+
"machine.openshift.io/memoryMb": "16384",
834+
"machine.openshift.io/vCPU": "4",
835+
})
836+
mapiMachineSet = mapiMachineSetBuilder.Build()
837+
Eventually(k8sClient.Create(ctx, mapiMachineSet), timeout).Should(Succeed())
838+
839+
capiMachineSet = capiMachineSetBuilder.WithLabels(map[string]string{
840+
"machine.openshift.io/cluster-api-cluster": "ci-op-gs2k97d6-c9e33-2smph",
841+
"cluster.x-k8s.io/cluster-name": "ci-op-gs2k97d6-c9e33-2smph",
842+
843+
"capi-param-controlled-label": "param-controlled-key",
844+
}).WithAnnotations(map[string]string{
845+
"capacity.cluster-autoscaler.kubernetes.io/labels": "kubernetes.io/arch=amd64",
846+
"machine.openshift.io/GPU": "0",
847+
"machine.openshift.io/memoryMb": "16384",
848+
"machine.openshift.io/vCPU": "4",
849+
}).Build()
850+
851+
Eventually(k8sClient.Create(ctx, capiMachineSet), timeout).Should(Succeed())
852+
853+
})
854+
Context("with status.authoritativeAPI: Machine API (on MAPI MachineSet)", func() {
855+
BeforeEach(func() {
856+
By("Setting the MAPI MachineSet AuthoritativeAPI to Machine API")
857+
Eventually(k.UpdateStatus(mapiMachineSet, func() {
858+
mapiMachineSet.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI
859+
})).Should(Succeed())
860+
861+
Eventually(k.Object(mapiMachineSet), timeout).Should(
862+
HaveField("Status.AuthoritativeAPI", Equal(mapiv1beta1.MachineAuthorityMachineAPI)))
863+
})
864+
865+
It("updating the spec should be prevented", func() {
866+
Eventually(k.Update(capiMachineSet, func() {
867+
replicas := int32(5)
868+
capiMachineSet.Spec.Replicas = &replicas
869+
}), timeout).Should(MatchError(ContainSubstring("Changing .spec is not allowed")))
870+
})
871+
872+
It("updating the spec.AuthoritativeAPI (on the MAPI MachineSet) should be allowed", func() {
873+
Eventually(k.Update(mapiMachineSet, func() {
874+
mapiMachineSet.Spec.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI
875+
}), timeout).Should(Succeed())
876+
})
877+
878+
Context("when trying to update metadata.labels", func() {
879+
It("rejects modification of the protected machine.openshift.io label", func() {
880+
Eventually(k.Update(capiMachineSet, func() {
881+
capiMachineSet.Labels["machine.openshift.io/cluster-api-cluster"] = "different-cluster"
882+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/*, kubernetes.io/* or cluster.x-k8s.io/* label")))
883+
})
884+
885+
It("rejects deletion of the protected machine.openshift.io label", func() {
886+
Eventually(k.Update(capiMachineSet, func() {
887+
delete(capiMachineSet.Labels, "machine.openshift.io/cluster-api-cluster")
888+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/*, kubernetes.io/* or cluster.x-k8s.io/* label")))
889+
})
890+
891+
It("rejects setting of the protected machine.openshift.io label to the empty string ''", func() {
892+
Eventually(k.Update(capiMachineSet, func() {
893+
capiMachineSet.Labels["machine.openshift.io/cluster-api-cluster"] = ""
894+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/*, kubernetes.io/* or cluster.x-k8s.io/* label")))
895+
})
896+
897+
It("rejects adding a new machine.openshift.io label", func() {
898+
Eventually(k.Update(capiMachineSet, func() {
899+
capiMachineSet.Labels["machine.openshift.io/foo"] = testLabelValue
900+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/*, kubernetes.io/* or cluster.x-k8s.io/* label")))
901+
})
902+
903+
It("rejects adding a new machine.openshift.io label with an empty string value", func() {
904+
Eventually(k.Update(capiMachineSet, func() {
905+
capiMachineSet.Labels["machine.openshift.io/foo"] = ""
906+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/*, kubernetes.io/* or cluster.x-k8s.io/* label")))
907+
})
908+
909+
It("rejects modification of the protected cluster.x-k8s.io label", func() {
910+
Eventually(k.Update(capiMachineSet, func() {
911+
capiMachineSet.Labels["cluster.x-k8s.io/cluster-name"] = "different-cluster"
912+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/*, kubernetes.io/* or cluster.x-k8s.io/* label")))
913+
})
914+
915+
It("rejects deletion of the protected cluster.x-k8s.io label", func() {
916+
Eventually(k.Update(capiMachineSet, func() {
917+
delete(capiMachineSet.Labels, "cluster.x-k8s.io/cluster-name")
918+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/*, kubernetes.io/* or cluster.x-k8s.io/* label")))
919+
})
920+
921+
It("allows modification of a non-protected label", func() {
922+
Eventually(k.Update(capiMachineSet, func() {
923+
capiMachineSet.Labels["test"] = "val"
924+
}), timeout).Should(Succeed(), "expected success when modifying unrelated labels")
925+
})
926+
})
927+
928+
Context("when trying to update metadata.Annotations", func() {
929+
It("rejects modification of a protected machine.openshift.io annotation", func() {
930+
Eventually(k.Update(capiMachineSet, func() {
931+
capiMachineSet.Annotations["machine.openshift.io/vCPU"] = "8"
932+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/* or cluster.x-k8s.io or clusters.x-k8s.io annotation")))
933+
})
934+
935+
It("rejects deletion of a protected machine.openshift.io annotation", func() {
936+
Eventually(k.Update(capiMachineSet, func() {
937+
delete(capiMachineSet.Annotations, "machine.openshift.io/vCPU")
938+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/* or cluster.x-k8s.io or clusters.x-k8s.io annotation")))
939+
})
940+
941+
It("rejects modification of a protected machine.openshift.io annotation to the empty string ''", func() {
942+
Eventually(k.Update(capiMachineSet, func() {
943+
capiMachineSet.Annotations["machine.openshift.io/vCPU"] = ""
944+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/* or cluster.x-k8s.io or clusters.x-k8s.io annotation")))
945+
})
946+
947+
It("rejects adding a new protected machine.openshift.io annotation", func() {
948+
Eventually(k.Update(capiMachineSet, func() {
949+
capiMachineSet.Annotations["machine.openshift.io/foo"] = testLabelValue
950+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/* or cluster.x-k8s.io or clusters.x-k8s.io annotation")))
951+
})
952+
953+
It("rejects adding a new protected machine.openshift.io annotation with an empty string value", func() {
954+
Eventually(k.Update(capiMachineSet, func() {
955+
capiMachineSet.Annotations["machine.openshift.io/foo"] = ""
956+
}), timeout).Should(MatchError(ContainSubstring("Cannot add, modify or delete any machine.openshift.io/* or cluster.x-k8s.io or clusters.x-k8s.io annotation")))
957+
})
958+
959+
It("allows modification of a non-protected annotation", func() {
960+
Eventually(k.Update(capiMachineSet, func() {
961+
capiMachineSet.Annotations["bar"] = "baz"
962+
}), timeout).Should(Succeed(), "expected success when modifying unrelated annotations")
963+
})
964+
})
965+
966+
Context("when trying to update Machine API owned metadata.labels", func() {
967+
It("allows changing a metadata label to match the param MachineSet", func() {
968+
Eventually(k.Object(mapiMachineSet), timeout).Should(
969+
HaveField("Labels", HaveKeyWithValue("mapi-param-controlled-label", "param-controlled-key")))
970+
971+
Eventually(k.Update(capiMachineSet, func() {
972+
capiMachineSet.Labels["mapi-param-controlled-label"] = "param-controlled-key"
973+
}), timeout).Should(Succeed(), "expected success when updating label to match MAPI MachineSet")
974+
})
975+
976+
It("rejects changing a label to differ from the param MachineSet", func() {
977+
Eventually(k.Update(capiMachineSet, func() {
978+
capiMachineSet.Labels["mapi-param-controlled-label"] = testLabelValue
979+
}), timeout).Should(MatchError(ContainSubstring("Cannot modify a Machine API controlled label except to match the Machine API mirrored MachineSet")))
980+
})
981+
})
982+
})
983+
})
749984
})

0 commit comments

Comments
 (0)