Skip to content

Commit

Permalink
Always allow converting MySQL[AAD]Users back to v1alpha1 (#1393)
Browse files Browse the repository at this point in the history
* Remove ignore entry for manager

The output of `make manager` goes into bin/ which is already
ignored. This entry was causing very confusing things to happen with
directories called `manager` further down the directory tree.

* Never error when converting v1alpha2 MySQLAADUser -> v1alpha1

(Unless JSON serialisation fails for some reason.) Instead we store
the changed fields in an annotation and allow roundtripping that when
converting in the other direction.

* Never error when converting v1alpha2 MySQLUser -> v1alpha1

(Unless JSON serialisation fails for some reason.) Instead we store
the changed fields in an annotation and allow roundtripping that when
converting in the other direction.

* Review feedback, thanks @matthchr

* Comment out the replica server in the MySQL happy path test

This is perpetually timing out for me at the moment, and testing
manually shows that creation can take more than an hour.
  • Loading branch information
babbageclunk authored Mar 24, 2021
1 parent 1e656f0 commit bfabead
Show file tree
Hide file tree
Showing 7 changed files with 396 additions and 61 deletions.
3 changes: 0 additions & 3 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,6 @@ coverage-existing.txt
report-existing.xml
testlogs-existing.txt

# manager output from build
manager

# terraform artifacts
.terraform*
terraform.tfstate*
Expand Down
53 changes: 53 additions & 0 deletions api/v1alpha1/conversion_stash.go
Original file line number Diff line number Diff line change
@@ -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
}
}
88 changes: 68 additions & 20 deletions api/v1alpha1/mysqlaaduser_conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"`
}
112 changes: 106 additions & 6 deletions api/v1alpha1/mysqlaaduser_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
},
}))
})

})
Expand Down
Loading

0 comments on commit bfabead

Please sign in to comment.