Always allow converting MySQL[AAD]Users back to v1alpha1

.gitignore

@@ -45,9 +45,6 @@ coverage-existing.txt
 report-existing.xml
 testlogs-existing.txt
 
-# manager output from build
-manager
-
 # terraform artifacts
 .terraform*
 terraform.tfstate*

api/v1alpha1/conversion_stash.go

@@ -0,0 +1,53 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+package v1alpha1
+
+import (
+	"encoding/json"
+
+	"github.com/pkg/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/conversion"
+)
+
+const conversionStashAnnotation = "azure.microsoft.com/convert-stash"
+
+// getStashedAnnotation unmarshals the convert-stash annotation value
+// into the target pointer, if the annotation is present. It returns
+// whether the annotation was present, and any error from unmarshalling.
+func getStashedAnnotation(meta metav1.ObjectMeta, target interface{}) (bool, error) {
+	if _, err := conversion.EnforcePtr(target); err != nil {
+		return false, errors.Errorf("getStashedAnnotation target must be a pointer, not %T", target)
+	}
+	if meta.Annotations == nil {
+		return false, nil
+	}
+	value, found := meta.Annotations[conversionStashAnnotation]
+	if !found {
+		return false, nil
+	}
+	if err := json.Unmarshal([]byte(value), target); err != nil {
+		return false, errors.Wrap(err, "decoding stashed fields")
+	}
+	return true, nil
+}
+
+func setStashedAnnotation(meta *metav1.ObjectMeta, stashValues interface{}) error {
+	if meta.Annotations == nil {
+		meta.Annotations = make(map[string]string)
+	}
+	encoded, err := json.Marshal(stashValues)
+	if err != nil {
+		return errors.Wrap(err, "encoding stashed fields")
+	}
+	meta.Annotations[conversionStashAnnotation] = string(encoded)
+	return nil
+}
+
+func clearStashedAnnotation(meta *metav1.ObjectMeta) {
+	delete(meta.Annotations, conversionStashAnnotation)
+	if len(meta.Annotations) == 0 {
+		meta.Annotations = nil
+	}
+}

api/v1alpha1/mysqlaaduser_conversion.go

@@ -4,30 +4,55 @@
 package v1alpha1
 
 import (
-	"github.com/pkg/errors"
+	"sort"
+
 	"sigs.k8s.io/controller-runtime/pkg/conversion"
 
 	"github.com/Azure/azure-service-operator/api/v1alpha2"
 )
 
 var _ conversion.Convertible = &MySQLAADUser{}
 
+// To avoid losing information converting a v1alpha2 instance into
+// v1alpha1, we stash the affected fields (roles and database roles)
+// into a json-serialised struct in the annotations. When converting
+// back to a v1alpha2 instance we use any stashed values, layering any
+// changes to the database roles back over the top. The conversions
+// will only return errors if JSON marshalling/unmarshalling fails.
+
 func (src *MySQLAADUser) ConvertTo(dstRaw conversion.Hub) error {
 	dst := dstRaw.(*v1alpha2.MySQLAADUser)
 
 	// ObjectMeta
 	dst.ObjectMeta = src.ObjectMeta
 
+	// If there are stashed values in the annotations then we need to
+	// retrieve them first.
+	var stashedValues stashedMySQLAADUserFields
+	found, err := getStashedAnnotation(src.ObjectMeta, &stashedValues)
+	if err != nil {
+		return err
+	}
+	if found {
+		dst.Spec.Roles = stashedValues.Roles
+		dst.Spec.DatabaseRoles = stashedValues.DatabaseRoles
+
+		// Clear out the annotation to avoid confusion.
+		clearStashedAnnotation(&dst.ObjectMeta)
+	}
+
 	// Spec
 	dst.Spec.ResourceGroup = src.Spec.ResourceGroup
 	dst.Spec.Server = src.Spec.Server
 	dst.Spec.AADID = src.Spec.AADID
 	dst.Spec.Username = src.Spec.Username
 
-	// v1alpha1 doesn't support server-level roles, only
-	// database-level ones, so we move them into the new field.
-	dst.Spec.Roles = []string{}
-	dst.Spec.DatabaseRoles = make(map[string][]string)
+	if dst.Spec.Roles == nil {
+		dst.Spec.Roles = []string{}
+	}
+	if dst.Spec.DatabaseRoles == nil {
+		dst.Spec.DatabaseRoles = make(map[string][]string)
+	}
 	dst.Spec.DatabaseRoles[src.Spec.DBName] = append([]string(nil), src.Spec.Roles...)
 
 	// Status
@@ -39,34 +64,57 @@ func (src *MySQLAADUser) ConvertTo(dstRaw conversion.Hub) error {
 func (dst *MySQLAADUser) ConvertFrom(srcRaw conversion.Hub) error {
 	src := srcRaw.(*v1alpha2.MySQLAADUser)
 
-	// Converting a v1alpha2 user into a v1alpha1 one is only allowed
-	// if it has exactly one database...
-	if len(src.Spec.DatabaseRoles) != 1 {
-		return errors.Errorf("can't convert user %q to %T because it has privileges in %d databases", src.ObjectMeta.Name, dst, len(src.Spec.DatabaseRoles))
-	}
-	// ...and no server-level roles.
-	if len(src.Spec.Roles) != 0 {
-		return errors.Errorf("can't convert user %q to %T because it has server-level roles", src.ObjectMeta.Name, dst)
-	}
-
 	// ObjectMeta
 	dst.ObjectMeta = src.ObjectMeta
 
+	if len(src.Spec.DatabaseRoles) != 1 || len(src.Spec.Roles) != 0 {
+		// If this can't be represented exactly as a v1alpha1, store
+		// the original server-level and database roles in an
+		// annotation.
+		err := setStashedAnnotation(&dst.ObjectMeta, stashedMySQLAADUserFields{
+			DatabaseRoles: src.Spec.DatabaseRoles,
+			Roles:         src.Spec.Roles,
+		})
+		if err != nil {
+			return err
+		}
+	}
+
 	// Spec
 	dst.Spec.ResourceGroup = src.Spec.ResourceGroup
 	dst.Spec.Server = src.Spec.Server
 	dst.Spec.AADID = src.Spec.AADID
 	dst.Spec.Username = src.Spec.Username
 
-	for dbName, roles := range src.Spec.DatabaseRoles {
-		dst.Spec.DBName = dbName
-		dst.Spec.Roles = append(dst.Spec.Roles, roles...)
-		break
+	// Pick the first database name to include as the DBName.
+	var dbNames []string
+	for dbName := range src.Spec.DatabaseRoles {
+		dbNames = append(dbNames, dbName)
 	}
+	// Sorting the list of names for testing (and so that a client
+	// gets a consistent value back for a resource).
+	sort.Strings(dbNames)
+	var (
+		dbName string
+		roles  []string
+	)
+	if len(dbNames) != 0 {
+		dbName = dbNames[0]
+		roles = src.Spec.DatabaseRoles[dbName]
+	}
+
+	dst.Spec.DBName = dbName
+	dst.Spec.Roles = append(dst.Spec.Roles, roles...)
 
 	// Status
 	dst.Status = ASOStatus(src.Status)
-
 	return nil
+}
 
+// stashedMySQLAADUserFields stores values that can't be represented
+// directly on a v1alpha1 spec struct, so that they can be stored in
+// an annotation and used when converting to v1alpha2.
+type stashedMySQLAADUserFields struct {
+	DatabaseRoles map[string][]string `json:"databaseRoles,omitempty"`
+	Roles         []string            `json:"roles,omitempty"`
 }

api/v1alpha1/mysqlaaduser_types_test.go

@@ -86,7 +86,7 @@ var _ = Describe("MySQLAADUser", func() {
 			}))
 		})
 
-		It("can't downgrade with roles for multiple databases", func() {
+		It("can downgrade with roles for multiple databases", func() {
 			v2 := v1alpha2.MySQLAADUser{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "foo",
@@ -99,13 +99,34 @@ var _ = Describe("MySQLAADUser", func() {
 						"mydb":    {"role1", "role2", "role3"},
 						"otherdb": {"otherrole"},
 					},
+					AADID:    "aadid",
+					Username: "username",
 				},
 			}
 			var v1 MySQLAADUser
-			Expect(v1.ConvertFrom(&v2)).To(MatchError("can't convert user \"foo\" to *v1alpha1.MySQLAADUser because it has privileges in 2 databases"))
+			Expect(v1.ConvertFrom(&v2)).To(Succeed())
+			Expect(v1).To(Equal(MySQLAADUser{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "default",
+					// Might need to account for ordering here -
+					// compare deserialised instead.
+					Annotations: map[string]string{
+						"azure.microsoft.com/convert-stash": `{"databaseRoles":{"mydb":["role1","role2","role3"],"otherdb":["otherrole"]}}`,
+					},
+				},
+				Spec: MySQLAADUserSpec{
+					Server:        "myserver",
+					ResourceGroup: "foo-group",
+					DBName:        "mydb",
+					Roles:         []string{"role1", "role2", "role3"},
+					AADID:         "aadid",
+					Username:      "username",
+				},
+			}))
 		})
 
-		It("can't downgrade with roles for no databases", func() {
+		It("can downgrade with roles for no databases", func() {
 			v2 := v1alpha2.MySQLAADUser{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "foo",
@@ -115,13 +136,32 @@ var _ = Describe("MySQLAADUser", func() {
 					Server:        "myserver",
 					ResourceGroup: "foo-group",
 					DatabaseRoles: nil,
+					AADID:         "aadid",
+					Username:      "username",
 				},
 			}
 			var v1 MySQLAADUser
-			Expect(v1.ConvertFrom(&v2)).To(MatchError("can't convert user \"foo\" to *v1alpha1.MySQLAADUser because it has privileges in 0 databases"))
+			Expect(v1.ConvertFrom(&v2)).To(Succeed())
+			Expect(v1).To(Equal(MySQLAADUser{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "default",
+					Annotations: map[string]string{
+						"azure.microsoft.com/convert-stash": `{}`,
+					},
+				},
+				Spec: MySQLAADUserSpec{
+					Server:        "myserver",
+					ResourceGroup: "foo-group",
+					DBName:        "",
+					Roles:         nil,
+					AADID:         "aadid",
+					Username:      "username",
+				},
+			}))
 		})
 
-		It("can't downgrade with server roles", func() {
+		It("can downgrade with server roles", func() {
 			v2 := v1alpha2.MySQLAADUser{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "foo",
@@ -134,10 +174,70 @@ var _ = Describe("MySQLAADUser", func() {
 					DatabaseRoles: map[string][]string{
 						"mydb": {"role1", "role2", "role3"},
 					},
+					AADID:    "aadid",
+					Username: "username",
 				},
 			}
 			var v1 MySQLAADUser
-			Expect(v1.ConvertFrom(&v2)).To(MatchError("can't convert user \"foo\" to *v1alpha1.MySQLAADUser because it has server-level roles"))
+			Expect(v1.ConvertFrom(&v2)).To(Succeed())
+			Expect(v1).To(Equal(MySQLAADUser{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "default",
+					Annotations: map[string]string{
+						"azure.microsoft.com/convert-stash": `{"databaseRoles":{"mydb":["role1","role2","role3"]},"roles":["somekindofsuperuser"]}`,
+					},
+				},
+				Spec: MySQLAADUserSpec{
+					Server:        "myserver",
+					ResourceGroup: "foo-group",
+					DBName:        "mydb",
+					Roles:         []string{"role1", "role2", "role3"},
+					AADID:         "aadid",
+					Username:      "username",
+				},
+			}))
+		})
+
+		It("can incorporate annotations when upgrading", func() {
+			// Server-level roles should just be carried forwards.
+			// Database roles need to be applied to the ones in the annotation.
+			v1 := MySQLAADUser{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "default",
+					Annotations: map[string]string{
+						"azure.microsoft.com/convert-stash": `{"databaseRoles":{"mydb":["role1","role2","role3"],"otherdb":["anotherrole"]},"roles":["somekindofsuperuser"]}`,
+					},
+				},
+				Spec: MySQLAADUserSpec{
+					Server:        "myserver",
+					ResourceGroup: "foo-group",
+					DBName:        "mydb",
+					Roles:         []string{"role3", "role4"},
+					AADID:         "aadid",
+					Username:      "username",
+				},
+			}
+			var v2 v1alpha2.MySQLAADUser
+			Expect(v1.ConvertTo(&v2)).To(Succeed())
+			Expect(v2).To(Equal(v1alpha2.MySQLAADUser{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "default",
+				},
+				Spec: v1alpha2.MySQLAADUserSpec{
+					Server:        "myserver",
+					ResourceGroup: "foo-group",
+					Roles:         []string{"somekindofsuperuser"},
+					DatabaseRoles: map[string][]string{
+						"mydb":    {"role3", "role4"},
+						"otherdb": {"anotherrole"},
+					},
+					AADID:    "aadid",
+					Username: "username",
+				},
+			}))
 		})
 
 	})