Skip to content

Commit 4b37064

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #393 from chrischdi/pr-machines-labels
NO-JIRA: conversion: CAPI2MAPI set openshift labels for zone, region and instance-type
2 parents 4106e8b + 5f88736 commit 4b37064

File tree

15 files changed

+407
-173
lines changed

15 files changed

+407
-173
lines changed

‎pkg/admissionpolicy/testutils/util.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,14 @@ import (
2525
"path/filepath"
2626
"strings"
2727
"sync"
28+
"time"
2829

2930
corev1 "k8s.io/api/core/v1"
3031

3132
. "github.com/onsi/ginkgo/v2"
3233
. "github.com/onsi/gomega"
3334
"sigs.k8s.io/controller-runtime/pkg/envtest"
35+
"sigs.k8s.io/controller-runtime/pkg/envtest/komega"
3436

3537
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
3638
"k8s.io/apimachinery/pkg/runtime"
@@ -187,6 +189,24 @@ func AddSentinelValidation(vap *admissionregistrationv1.ValidatingAdmissionPolic
187189
})
188190
}
189191

192+
// VerifySentinelValidation tries to update a resource to hit the sentinel validation to see that the VAP is in-place.
193+
func VerifySentinelValidation(k komega.Komega, sentinelObject client.Object, timeout time.Duration) {
194+
Eventually(k.Update(sentinelObject, func() {
195+
SetSentinelValidationLabel(sentinelObject)
196+
}), timeout).Should(MatchError(ContainSubstring("policy in place")))
197+
}
198+
199+
// SetSentinelValidationLabel sets the sentinel validation label on a resource.
200+
func SetSentinelValidationLabel(sentinelObject client.Object) {
201+
currentLabels := sentinelObject.GetLabels()
202+
if currentLabels == nil {
203+
currentLabels = map[string]string{}
204+
}
205+
206+
currentLabels["test-sentinel"] = "fubar"
207+
sentinelObject.SetLabels(currentLabels)
208+
}
209+
190210
// UpdateVAPBindingNamespaces updates a VAP binding's namespace configuration.
191211
//
192212
// Parameters:

‎pkg/controllers/machinesetsync/machineset_vap_test.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -188,9 +188,7 @@ var _ = Describe("MachineSet VAP Tests", func() {
188188
Build()
189189
Eventually(k8sClient.Create(ctx, sentinelMachineSet)).Should(Succeed(), "sentinel machineset should be able to be created")
190190

191-
Eventually(k.Update(sentinelMachineSet, func() {
192-
sentinelMachineSet.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"}
193-
}), timeout).Should(MatchError(ContainSubstring("policy in place")))
191+
admissiontestutils.VerifySentinelValidation(k, sentinelMachineSet, timeout)
194192
})
195193

196194
It("should allow creating a MachineSet without forbidden fields", func() {

‎pkg/controllers/machinesync/machine_sync_controller_test.go‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,9 +1132,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
11321132
Eventually(k.Object(sentinelMachine), timeout).Should(
11331133
HaveField("Status.AuthoritativeAPI", Equal(mapiv1beta1.MachineAuthorityClusterAPI)))
11341134

1135-
Eventually(k.Update(sentinelMachine, func() {
1136-
sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"}
1137-
}), timeout).Should(MatchError(ContainSubstring("policy in place")))
1135+
admissiontestutils.VerifySentinelValidation(k, sentinelMachine, timeout)
11381136
})
11391137

11401138
Context("with status.AuthoritativeAPI: Machine API", func() {
@@ -1318,9 +1316,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
13181316
Eventually(k8sClient.Create(ctx, sentinelMachine)).Should(Succeed())
13191317

13201318
// Continually try to update the capiMachine to a forbidden field until the VAP blocks it
1321-
Eventually(k.Update(sentinelMachine, func() {
1322-
sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"}
1323-
}), timeout).Should(MatchError(ContainSubstring("policy in place")))
1319+
admissiontestutils.VerifySentinelValidation(k, sentinelMachine, timeout)
13241320
})
13251321

13261322
It("updating the spec.Version should not be allowed", func() {
@@ -1383,9 +1379,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
13831379

13841380
Eventually(k.Get(capiSentinelMachine)).Should(Succeed())
13851381

1386-
Eventually(k.Update(sentinelMachine, func() {
1387-
sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"}
1388-
}), timeout).Should(MatchError(ContainSubstring("policy in place")))
1382+
admissiontestutils.VerifySentinelValidation(k, sentinelMachine, timeout)
13891383
})
13901384

13911385
// The Authoritative API defaults to MachineAPI so we can't test if it's unset.
@@ -1451,9 +1445,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
14511445

14521446
Eventually(k.Get(capiSentinelMachine)).Should(Succeed())
14531447

1454-
Eventually(k.Update(sentinelMachine, func() {
1455-
sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"}
1456-
}), timeout).Should(MatchError(ContainSubstring("policy in place")))
1448+
admissiontestutils.VerifySentinelValidation(k, sentinelMachine, timeout)
14571449
})
14581450

14591451
It("denies updating the AuthoritativeAPI when the machine is in Provisioning", func() {
@@ -1566,7 +1558,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
15661558
warnSink.Reset() // keep each probe self-contained
15671559

15681560
err := warnKomega.Update(sentinelMachine, func() {
1569-
sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"}
1561+
admissiontestutils.SetSentinelValidationLabel(sentinelMachine)
15701562
})()
15711563
g.Expect(err).NotTo(HaveOccurred())
15721564

‎pkg/conversion/capi2mapi/aws.go‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"strings"
2222

2323
mapiv1beta1 "github.com/openshift/api/machine/v1beta1"
24+
"github.com/openshift/cluster-capi-operator/pkg/conversion/consts"
2425

2526
corev1 "k8s.io/api/core/v1"
2627
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -49,9 +50,10 @@ const (
4950

5051
// machineAndAWSMachineAndAWSCluster stores the details of a Cluster API Machine and AWSMachine and AWSCluster.
5152
type machineAndAWSMachineAndAWSCluster struct {
52-
machine *clusterv1.Machine
53-
awsMachine *awsv1.AWSMachine
54-
awsCluster *awsv1.AWSCluster
53+
machine *clusterv1.Machine
54+
awsMachine *awsv1.AWSMachine
55+
awsCluster *awsv1.AWSCluster
56+
excludeMachineAPILabelsAndAnnotations bool
5557
}
5658

5759
// machineSetAndAWSMachineTemplateAndAWSCluster stores the details of a Cluster API MachineSet and AWSMachineTemplate and AWSCluster.
@@ -84,7 +86,8 @@ func FromMachineSetAndAWSMachineTemplateAndAWSCluster(ms *clusterv1.MachineSet,
8486
awsMachine: &awsv1.AWSMachine{
8587
Spec: mts.Spec.Template.Spec,
8688
},
87-
awsCluster: ac,
89+
awsCluster: ac,
90+
excludeMachineAPILabelsAndAnnotations: true,
8891
},
8992
}
9093
}
@@ -258,7 +261,20 @@ func (m machineAndAWSMachineAndAWSCluster) ToMachine() (*mapiv1beta1.Machine, []
258261

259262
warnings = append(warnings, warn...)
260263

261-
mapiMachine, err := fromCAPIMachineToMAPIMachine(m.machine)
264+
var additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations map[string]string
265+
if !m.excludeMachineAPILabelsAndAnnotations {
266+
additionalMachineAPIMetadataLabels = map[string]string{
267+
consts.MAPIMachineMetadataLabelInstanceType: m.awsMachine.Spec.InstanceType,
268+
consts.MAPIMachineMetadataLabelRegion: m.awsCluster.Spec.Region,
269+
consts.MAPIMachineMetadataLabelZone: ptr.Deref(m.machine.Spec.FailureDomain, ""),
270+
}
271+
272+
additionalMachineAPIMetadataAnnotations = map[string]string{
273+
consts.MAPIMachineMetadataAnnotationInstanceState: string(ptr.Deref(m.awsMachine.Status.InstanceState, "")),
274+
}
275+
}
276+
277+
mapiMachine, err := fromCAPIMachineToMAPIMachine(m.machine, additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations)
262278
if err != nil {
263279
errors = append(errors, err...)
264280
}

‎pkg/conversion/capi2mapi/common.go‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,13 @@ func RawExtensionFromInterface(spec interface{}) (*runtime.RawExtension, error)
4545

4646
func convertCAPIMachineSetSelectorToMAPI(capiSelector metav1.LabelSelector) metav1.LabelSelector {
4747
mapiSelector := capiSelector.DeepCopy()
48-
mapiSelector.MatchLabels = convertCAPILabelsToMAPILabels(capiSelector.MatchLabels)
48+
mapiSelector.MatchLabels = convertCAPILabelsToMAPILabels(capiSelector.MatchLabels, nil)
4949

5050
return *mapiSelector
5151
}
5252

53-
func convertCAPILabelsToMAPILabels(capiLabels map[string]string) map[string]string {
54-
if len(capiLabels) == 0 {
53+
func convertCAPILabelsToMAPILabels(capiLabels map[string]string, machineAPILabels map[string]string) map[string]string {
54+
if len(capiLabels) == 0 && len(machineAPILabels) == 0 {
5555
return nil
5656
}
5757

@@ -77,6 +77,15 @@ func convertCAPILabelsToMAPILabels(capiLabels map[string]string) map[string]stri
7777
mapiLabels[k] = v
7878
}
7979

80+
for k, v := range machineAPILabels {
81+
// Ignore empty labels to ensure to not overwrite potentially existing labels with empty values.
82+
if v == "" {
83+
continue
84+
}
85+
86+
mapiLabels[k] = v
87+
}
88+
8089
// On the original MAPI object some label fields are nil rather than empty.
8190
// So return nil instead to avoid unnecessary diff being picked up by the diff checker.
8291
if len(mapiLabels) == 0 {
@@ -124,8 +133,8 @@ func convertCAPIMachineAnnotationsToMAPIMachineSpecObjectMetaAnnotations(capiAnn
124133
return mapiAnnotations
125134
}
126135

127-
func convertCAPIAnnotationsToMAPIAnnotations(capiAnnotations map[string]string) map[string]string {
128-
if len(capiAnnotations) == 0 {
136+
func convertCAPIAnnotationsToMAPIAnnotations(capiAnnotations map[string]string, machineAPIAnnotations map[string]string) map[string]string {
137+
if len(capiAnnotations) == 0 && len(machineAPIAnnotations) == 0 {
129138
return nil
130139
}
131140

@@ -155,5 +164,20 @@ func convertCAPIAnnotationsToMAPIAnnotations(capiAnnotations map[string]string)
155164
mapiAnnotations[k] = v
156165
}
157166

167+
for k, v := range machineAPIAnnotations {
168+
// Ignore empty annotations to ensure to not overwrite potentially existing annotations with empty values.
169+
if v == "" {
170+
continue
171+
}
172+
173+
mapiAnnotations[k] = v
174+
}
175+
176+
// On the original MAPI object some label fields are nil rather than empty.
177+
// So return nil instead to avoid unnecessary diff being picked up by the diff checker.
178+
if len(mapiAnnotations) == 0 {
179+
return nil
180+
}
181+
158182
return mapiAnnotations
159183
}

‎pkg/conversion/capi2mapi/machine.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ const (
3434
)
3535

3636
// fromCAPIMachineToMAPIMachine translates a core CAPI Machine to its MAPI Machine correspondent.
37-
func fromCAPIMachineToMAPIMachine(capiMachine *clusterv1.Machine) (*mapiv1beta1.Machine, field.ErrorList) {
37+
func fromCAPIMachineToMAPIMachine(capiMachine *clusterv1.Machine, additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations map[string]string) (*mapiv1beta1.Machine, field.ErrorList) {
3838
errs := field.ErrorList{}
3939

4040
lifecycleHooks, capiMachineNonHookAnnotations := convertCAPILifecycleHookAnnotationsToMAPILifecycleHooksAndAnnotations(capiMachine.Annotations)
@@ -48,8 +48,8 @@ func fromCAPIMachineToMAPIMachine(capiMachine *clusterv1.Machine) (*mapiv1beta1.
4848
ObjectMeta: metav1.ObjectMeta{
4949
Name: capiMachine.Name,
5050
Namespace: mapiNamespace,
51-
Labels: convertCAPILabelsToMAPILabels(capiMachine.Labels),
52-
Annotations: convertCAPIAnnotationsToMAPIAnnotations(capiMachineNonHookAnnotations),
51+
Labels: convertCAPILabelsToMAPILabels(capiMachine.Labels, additionalMachineAPIMetadataLabels),
52+
Annotations: convertCAPIAnnotationsToMAPIAnnotations(capiMachineNonHookAnnotations, additionalMachineAPIMetadataAnnotations),
5353
Finalizers: []string{mapiv1beta1.MachineFinalizer},
5454
OwnerReferences: nil, // OwnerReferences not populated here. They are added later by the machineSync controller.
5555
},

‎pkg/conversion/capi2mapi/machineset.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@ func fromCAPIMachineSetToMAPIMachineSet(capiMachineSet *clusterv1.MachineSet) (*
3232
ObjectMeta: metav1.ObjectMeta{
3333
Name: capiMachineSet.Name,
3434
Namespace: capiMachineSet.Namespace,
35-
Labels: convertCAPILabelsToMAPILabels(capiMachineSet.Labels),
36-
Annotations: convertCAPIAnnotationsToMAPIAnnotations(capiMachineSet.Annotations),
35+
Labels: convertCAPILabelsToMAPILabels(capiMachineSet.Labels, nil),
36+
Annotations: convertCAPIAnnotationsToMAPIAnnotations(capiMachineSet.Annotations, nil),
3737
Finalizers: nil, // MAPI MachineSet does not have finalizers.
3838
OwnerReferences: nil, // OwnerReferences not populated here. They are added later by the machineSetSync controller.
3939
},
@@ -44,8 +44,8 @@ func fromCAPIMachineSetToMAPIMachineSet(capiMachineSet *clusterv1.MachineSet) (*
4444
DeletePolicy: capiMachineSet.Spec.DeletePolicy,
4545
Template: mapiv1beta1.MachineTemplateSpec{
4646
ObjectMeta: mapiv1beta1.ObjectMeta{
47-
Labels: convertCAPILabelsToMAPILabels(capiMachineSet.Spec.Template.Labels),
48-
Annotations: convertCAPIAnnotationsToMAPIAnnotations(capiMachineSet.Spec.Template.Annotations),
47+
Labels: convertCAPILabelsToMAPILabels(capiMachineSet.Spec.Template.Labels, nil),
48+
Annotations: convertCAPIAnnotationsToMAPIAnnotations(capiMachineSet.Spec.Template.Annotations, nil),
4949
},
5050
},
5151
},

‎pkg/conversion/capi2mapi/openstack.go‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,13 @@ import (
2323

2424
mapiv1alpha1 "github.com/openshift/api/machine/v1alpha1"
2525
mapiv1beta1 "github.com/openshift/api/machine/v1beta1"
26+
"github.com/openshift/cluster-capi-operator/pkg/conversion/consts"
2627
corev1 "k8s.io/api/core/v1"
2728
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2829
"k8s.io/apimachinery/pkg/runtime"
2930
utilerrors "k8s.io/apimachinery/pkg/util/errors"
3031
"k8s.io/apimachinery/pkg/util/validation/field"
32+
"k8s.io/utils/ptr"
3133
openstackv1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
3234
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
3335
)
@@ -39,9 +41,10 @@ var (
3941

4042
// machineAndOpenStackMachineAndOpenStackCluster stores the details of a Cluster API Machine and OpenStackMachine and OpenStackCluster.
4143
type machineAndOpenStackMachineAndOpenStackCluster struct {
42-
machine *clusterv1.Machine
43-
openstackMachine *openstackv1.OpenStackMachine
44-
openstackCluster *openstackv1.OpenStackCluster
44+
machine *clusterv1.Machine
45+
openstackMachine *openstackv1.OpenStackMachine
46+
openstackCluster *openstackv1.OpenStackCluster
47+
excludeMachineAPILabelsAndAnnotations bool
4548
}
4649

4750
// machineSetAndOpenStackMachineTemplateAndOpenStackCluster stores the details of a Cluster API MachineSet and OpenStackMachineTemplate and OpenStackCluster.
@@ -74,7 +77,8 @@ func FromMachineSetAndOpenStackMachineTemplateAndOpenStackCluster(ms *clusterv1.
7477
openstackMachine: &openstackv1.OpenStackMachine{
7578
Spec: mts.Spec.Template.Spec,
7679
},
77-
openstackCluster: ac,
80+
openstackCluster: ac,
81+
excludeMachineAPILabelsAndAnnotations: true,
7882
},
7983
}
8084
}
@@ -201,7 +205,22 @@ func (m machineAndOpenStackMachineAndOpenStackCluster) ToMachine() (*mapiv1beta1
201205

202206
warnings = append(warnings, warns...)
203207

204-
mapiMachine, errs := fromCAPIMachineToMAPIMachine(m.machine)
208+
var additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations map[string]string
209+
if !m.excludeMachineAPILabelsAndAnnotations {
210+
additionalMachineAPIMetadataLabels = map[string]string{
211+
// Unable to determine the region without fetching the identity resources as done at:
212+
// https://github.com/openshift/machine-api-provider-openstack/blob/2defb131bd0836beba0a9790de7c9a63137a5cec/pkg/machine/actuator.go#L85-L89
213+
// consts.MAPIMachineMetadataLabelRegion
214+
consts.MAPIMachineMetadataLabelInstanceType: ptr.Deref(m.openstackMachine.Spec.Flavor, ""),
215+
consts.MAPIMachineMetadataLabelZone: ptr.Deref(m.machine.Spec.FailureDomain, ""),
216+
}
217+
218+
additionalMachineAPIMetadataAnnotations = map[string]string{
219+
consts.MAPIMachineMetadataAnnotationInstanceState: string(ptr.Deref(m.openstackMachine.Status.InstanceState, "")),
220+
}
221+
}
222+
223+
mapiMachine, errs := fromCAPIMachineToMAPIMachine(m.machine, additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations)
205224
if errs != nil {
206225
errors = append(errors, errs...)
207226
}

‎pkg/conversion/capi2mapi/powervs.go‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222

2323
mapiv1 "github.com/openshift/api/machine/v1"
2424
mapiv1beta1 "github.com/openshift/api/machine/v1beta1"
25+
"github.com/openshift/cluster-capi-operator/pkg/conversion/consts"
2526

2627
corev1 "k8s.io/api/core/v1"
2728
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -39,9 +40,10 @@ var (
3940

4041
// machineAndPowerVSMachineAndPowerVSCluster stores the details of a Cluster API Machine and PowerVSMachine and PowerVSCluster.
4142
type machineAndPowerVSMachineAndPowerVSCluster struct {
42-
machine *clusterv1.Machine
43-
powerVSMachine *ibmpowervsv1.IBMPowerVSMachine
44-
powerVSCluster *ibmpowervsv1.IBMPowerVSCluster
43+
machine *clusterv1.Machine
44+
powerVSMachine *ibmpowervsv1.IBMPowerVSMachine
45+
powerVSCluster *ibmpowervsv1.IBMPowerVSCluster
46+
excludeMachineAPILabelsAndAnnotations bool
4547
}
4648

4749
// machineSetAndPowerVSMachineTemplateAndPowerVSCluster stores the details of a Cluster API MachineSet and PowerVSMachineTemplate and AWSCluster.
@@ -74,7 +76,8 @@ func FromMachineSetAndPowerVSMachineTemplateAndPowerVSCluster(ms *clusterv1.Mach
7476
powerVSMachine: &ibmpowervsv1.IBMPowerVSMachine{
7577
Spec: mts.Spec.Template.Spec,
7678
},
77-
powerVSCluster: pc,
79+
powerVSCluster: pc,
80+
excludeMachineAPILabelsAndAnnotations: true,
7881
},
7982
}
8083
}
@@ -100,7 +103,20 @@ func (m machineAndPowerVSMachineAndPowerVSCluster) ToMachine() (*mapiv1beta1.Mac
100103
return nil, nil, fmt.Errorf("unable to convert PowerVS providerSpec to raw extension: %w", errRaw)
101104
}
102105

103-
mapiMachine, err := fromCAPIMachineToMAPIMachine(m.machine)
106+
var additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations map[string]string
107+
if !m.excludeMachineAPILabelsAndAnnotations {
108+
additionalMachineAPIMetadataLabels = map[string]string{
109+
consts.MAPIMachineMetadataLabelInstanceType: m.powerVSMachine.Spec.SystemType,
110+
consts.MAPIMachineMetadataLabelRegion: ptr.Deref(m.powerVSMachine.Status.Region, ""),
111+
consts.MAPIMachineMetadataLabelZone: ptr.Deref(m.machine.Spec.FailureDomain, ""),
112+
}
113+
114+
additionalMachineAPIMetadataAnnotations = map[string]string{
115+
consts.MAPIMachineMetadataAnnotationInstanceState: string(m.powerVSMachine.Status.InstanceState),
116+
}
117+
}
118+
119+
mapiMachine, err := fromCAPIMachineToMAPIMachine(m.machine, additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations)
104120
if err != nil {
105121
errors = append(errors, err...)
106122
}

0 commit comments

Comments
 (0)