Merge pull request #959 from CecileRobertMichon/fix-uuid

🐛 Make role assignment name deterministic

Author: k8s-ci-robot

api/v1alpha2/azuremachine_conversion.go

@@ -53,6 +53,7 @@ func restoreAzureMachineSpec(restored, dst *infrav1alpha3.AzureMachineSpec) {
 	if len(restored.UserAssignedIdentities) > 0 {
 		dst.UserAssignedIdentities = restored.UserAssignedIdentities
 	}
+	dst.RoleAssignmentName = restored.RoleAssignmentName
 	if restored.AcceleratedNetworking != nil {
 		dst.AcceleratedNetworking = restored.AcceleratedNetworking
 	}

api/v1alpha2/zz_generated.conversion.go

@@ -18,6 +18,7 @@ package v1alpha3
 
 import (
 	"encoding/base64"
+	"k8s.io/apimachinery/pkg/util/uuid"
 
 	"golang.org/x/crypto/ssh"
 
@@ -73,3 +74,12 @@ func (m *AzureMachine) SetDataDisksDefaults() {
 		}
 	}
 }
+
+// SetIdentityDefaults sets the defaults for VM Identity.
+func (m *AzureMachine) SetIdentityDefaults() {
+	if m.Spec.Identity == VMIdentitySystemAssigned {
+		if m.Spec.RoleAssignmentName == "" {
+			m.Spec.RoleAssignmentName = string(uuid.NewUUID())
+		}
+	}
+}

api/v1alpha3/azuremachine_default.go

@@ -18,6 +18,7 @@ package v1alpha3
 
 import (
 	"encoding/json"
+	"github.com/google/uuid"
 	"reflect"
 	"testing"
 
@@ -45,6 +46,38 @@ func TestAzureMachine_SetDefaultSSHPublicKey(t *testing.T) {
 	g.Expect(publicKeyNotExistTest.machine.Spec.SSHPublicKey).To(Not(BeEmpty()))
 }
 
+func TestAzureMachine_SetIdentityDefaults(t *testing.T) {
+	g := NewWithT(t)
+
+	type test struct {
+		machine *AzureMachine
+	}
+
+	existingRoleAssignmentName := "42862306-e485-4319-9bf0-35dbc6f6fe9c"
+	roleAssignmentExistTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{
+		Identity:           VMIdentitySystemAssigned,
+		RoleAssignmentName: existingRoleAssignmentName,
+	}}}
+	roleAssignmentEmptyTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{
+		Identity:           VMIdentitySystemAssigned,
+		RoleAssignmentName: "",
+	}}}
+	notSystemAssignedTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{
+		Identity: VMIdentityUserAssigned,
+	}}}
+
+	roleAssignmentExistTest.machine.SetIdentityDefaults()
+	g.Expect(roleAssignmentExistTest.machine.Spec.RoleAssignmentName).To(Equal(existingRoleAssignmentName))
+
+	roleAssignmentEmptyTest.machine.SetIdentityDefaults()
+	g.Expect(roleAssignmentEmptyTest.machine.Spec.RoleAssignmentName).To(Not(BeEmpty()))
+	_, err := uuid.Parse(roleAssignmentEmptyTest.machine.Spec.RoleAssignmentName)
+	g.Expect(err).To(Not(HaveOccurred()))
+
+	notSystemAssignedTest.machine.SetIdentityDefaults()
+	g.Expect(notSystemAssignedTest.machine.Spec.RoleAssignmentName).To(BeEmpty())
+}
+
 func TestAzureMachine_SetDataDisksDefaults(t *testing.T) {
 	cases := []struct {
 		name   string

api/v1alpha3/azuremachine_default_test.go

@@ -66,6 +66,11 @@ type AzureMachineSpec struct {
 	// +optional
 	UserAssignedIdentities []UserAssignedIdentity `json:"userAssignedIdentities,omitempty"`
 
+	// RoleAssignmentName is the name of the role assignment to create for a system assigned identity. It can be any valid GUID.
+	// If not specified, a random GUID will be generated.
+	// +optional
+	RoleAssignmentName string `json:"roleAssignmentName,omitempty"`
+
 	// OSDisk specifies the parameters for the operating system disk of the machine
 	OSDisk OSDisk `json:"osDisk"`
 

api/v1alpha3/azuremachine_types.go

@@ -19,6 +19,7 @@ package v1alpha3
 import (
 	"encoding/base64"
 	"fmt"
+	"github.com/google/uuid"
 
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2020-06-01/compute"
 	"golang.org/x/crypto/ssh"
@@ -43,6 +44,24 @@ func ValidateSSHKey(sshKey string, fldPath *field.Path) field.ErrorList {
 	return allErrs
 }
 
+// ValidateUserAssignedIdentity validates the user-assigned identities list
+func ValidateSystemAssignedIdentity(identityType VMIdentity, old, new string, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	if identityType == VMIdentitySystemAssigned {
+		if _, err := uuid.Parse(new); err != nil {
+			allErrs = append(allErrs, field.Invalid(fldPath, new, "Role assignment name must be a valid GUID. It is optional and will be auto-generated when not specified."))
+		}
+		if old != "" && old != new {
+			allErrs = append(allErrs, field.Invalid(fldPath, new, "Role assignment name should not be modified after AzureMachine creation."))
+		}
+	} else if len(new) != 0 {
+		allErrs = append(allErrs, field.Forbidden(fldPath, "Role assignment name should only be set when using system assigned identity."))
+	}
+
+	return allErrs
+}
+
 // ValidateUserAssignedIdentity validates the user-assigned identities list
 func ValidateUserAssignedIdentity(identityType VMIdentity, userAssignedIdenteties []UserAssignedIdentity, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}

api/v1alpha3/azuremachine_validation.go

@@ -21,6 +21,7 @@ import (
 	"crypto/rsa"
 	"encoding/base64"
 	"fmt"
+	"github.com/google/uuid"
 	"testing"
 
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2020-06-01/compute"
@@ -341,3 +342,58 @@ func TestAzureMachine_ValidateDataDisks(t *testing.T) {
 		})
 	}
 }
+
+func TestAzureMachine_ValidateSystemAssignedIdentity(t *testing.T) {
+	g := NewWithT(t)
+
+	tests := []struct {
+		name               string
+		roleAssignmentName string
+		old                string
+		Identity           VMIdentity
+		wantErr            bool
+	}{
+		{
+			name:               "valid UUID",
+			roleAssignmentName: uuid.New().String(),
+			Identity:           VMIdentitySystemAssigned,
+			wantErr:            false,
+		},
+		{
+			name:               "wrong Identity type",
+			roleAssignmentName: uuid.New().String(),
+			Identity:           VMIdentityNone,
+			wantErr:            true,
+		},
+		{
+			name:               "not a valid UUID",
+			roleAssignmentName: "notaguid",
+			Identity:           VMIdentitySystemAssigned,
+			wantErr:            true,
+		},
+		{
+			name:               "empty",
+			roleAssignmentName: "",
+			Identity:           VMIdentitySystemAssigned,
+			wantErr:            true,
+		},
+		{
+			name:               "changed",
+			roleAssignmentName: uuid.New().String(),
+			old:                uuid.New().String(),
+			Identity:           VMIdentitySystemAssigned,
+			wantErr:            true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			err := ValidateSystemAssignedIdentity(tc.Identity, tc.old, tc.roleAssignmentName, field.NewPath("sshPublicKey"))
+			if tc.wantErr {
+				g.Expect(err).ToNot(HaveLen(0))
+			} else {
+				g.Expect(err).To(HaveLen(0))
+			}
+		})
+	}
+}

api/v1alpha3/azuremachine_validation_test.go

@@ -57,6 +57,10 @@ func (m *AzureMachine) ValidateCreate() error {
 		allErrs = append(allErrs, errs...)
 	}
 
+	if errs := ValidateSystemAssignedIdentity(m.Spec.Identity, "", m.Spec.RoleAssignmentName, field.NewPath("roleAssignmentName")); len(errs) > 0 {
+		allErrs = append(allErrs, errs...)
+	}
+
 	if errs := ValidateUserAssignedIdentity(m.Spec.Identity, m.Spec.UserAssignedIdentities, field.NewPath("userAssignedIdentities")); len(errs) > 0 {
 		allErrs = append(allErrs, errs...)
 	}
@@ -75,6 +79,7 @@ func (m *AzureMachine) ValidateCreate() error {
 func (m *AzureMachine) ValidateUpdate(oldRaw runtime.Object) error {
 	machinelog.Info("validate update", "name", m.Name)
 	var allErrs field.ErrorList
+	old := oldRaw.(*AzureMachine)
 
 	if errs := ValidateImage(m.Spec.Image, field.NewPath("image")); len(errs) > 0 {
 		allErrs = append(allErrs, errs...)
@@ -88,6 +93,10 @@ func (m *AzureMachine) ValidateUpdate(oldRaw runtime.Object) error {
 		allErrs = append(allErrs, errs...)
 	}
 
+	if errs := ValidateSystemAssignedIdentity(m.Spec.Identity, old.Spec.RoleAssignmentName, m.Spec.RoleAssignmentName, field.NewPath("roleAssignmentName")); len(errs) > 0 {
+		allErrs = append(allErrs, errs...)
+	}
+
 	if errs := ValidateUserAssignedIdentity(m.Spec.Identity, m.Spec.UserAssignedIdentities, field.NewPath("userAssignedIdentities")); len(errs) > 0 {
 		allErrs = append(allErrs, errs...)
 	}
@@ -96,8 +105,6 @@ func (m *AzureMachine) ValidateUpdate(oldRaw runtime.Object) error {
 		allErrs = append(allErrs, errs...)
 	}
 
-	old := oldRaw.(*AzureMachine)
-
 	if errs := ValidateManagedDisk(old.Spec.OSDisk.ManagedDisk, m.Spec.OSDisk.ManagedDisk, field.NewPath("osDisk").Child("managedDisk")); len(errs) > 0 {
 		allErrs = append(allErrs, errs...)
 	}
@@ -134,4 +141,6 @@ func (m *AzureMachine) Default() {
 	}
 
 	m.SetDataDisksDefaults()
+
+	m.SetIdentityDefaults()
 }

api/v1alpha3/azuremachine_webhook.go

@@ -20,13 +20,11 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
-
 	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/uuid"
 	"k8s.io/klog/klogr"
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha3"
 	azure "sigs.k8s.io/cluster-api-provider-azure/cloud"
@@ -226,7 +224,7 @@ func (m *MachineScope) RoleAssignmentSpecs() []azure.RoleAssignmentSpec {
 		return []azure.RoleAssignmentSpec{
 			{
 				MachineName: m.Name(),
-				UUID:        string(uuid.NewUUID()),
+				Name:        m.AzureMachine.Spec.RoleAssignmentName,
 			},
 		}
 	}

cloud/scope/machine.go

@@ -43,7 +43,7 @@ func (s *Service) Reconcile(ctx context.Context) error {
 				PrincipalID:      resultVM.Identity.PrincipalID,
 			},
 		}
-		_, err = s.Client.Create(ctx, scope, roleSpec.UUID, params)
+		_, err = s.Client.Create(ctx, scope, roleSpec.Name, params)
 		if err != nil {
 			return errors.Wrapf(err, "cannot assign role to VM system assigned identity")
 		}