diff --git a/apis/cluster/mysql/v1alpha1/user_types.go b/apis/cluster/mysql/v1alpha1/user_types.go index cae393f8..2086624a 100644 --- a/apis/cluster/mysql/v1alpha1/user_types.go +++ b/apis/cluster/mysql/v1alpha1/user_types.go @@ -68,6 +68,11 @@ type UserParameters struct { // BinLog defines whether the create, delete, update operations of this user are propagated to replicas. Defaults to true // +optional BinLog *bool `json:"binlog,omitempty"` + + // PasswordRotationTrigger triggers rotation of the auto-generated password when set to + // a time after the current LastPasswordChange. Has no effect when passwordSecretRef is set. + // +optional + PasswordRotationTrigger *metav1.Time `json:"passwordRotationTrigger,omitempty"` } // AuthenticationPlugin selects the auth plugin used when creating the user. @@ -115,6 +120,9 @@ type UserObservation struct { // spec.forProvider.authenticationPlugin and what's actually configured // on the DB so plugin changes are not silently ignored. AuthenticationPlugin *AuthenticationPlugin `json:"authenticationPlugin,omitempty"` + + // LastPasswordChange records when the provider last set the user's password. + LastPasswordChange *metav1.Time `json:"lastPasswordChange,omitempty"` } // +kubebuilder:object:root=true diff --git a/apis/cluster/mysql/v1alpha1/zz_generated.deepcopy.go b/apis/cluster/mysql/v1alpha1/zz_generated.deepcopy.go index 078183e1..5a57345c 100644 --- a/apis/cluster/mysql/v1alpha1/zz_generated.deepcopy.go +++ b/apis/cluster/mysql/v1alpha1/zz_generated.deepcopy.go @@ -687,6 +687,10 @@ func (in *UserObservation) DeepCopyInto(out *UserObservation) { *out = new(AuthenticationPlugin) (*in).DeepCopyInto(*out) } + if in.LastPasswordChange != nil { + in, out := &in.LastPasswordChange, &out.LastPasswordChange + *out = (*in).DeepCopy() + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UserObservation. @@ -722,6 +726,10 @@ func (in *UserParameters) DeepCopyInto(out *UserParameters) { *out = new(bool) **out = **in } + if in.PasswordRotationTrigger != nil { + in, out := &in.PasswordRotationTrigger, &out.PasswordRotationTrigger + *out = (*in).DeepCopy() + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UserParameters. diff --git a/apis/namespaced/mysql/v1alpha1/user_types.go b/apis/namespaced/mysql/v1alpha1/user_types.go index fec67c90..006dc398 100644 --- a/apis/namespaced/mysql/v1alpha1/user_types.go +++ b/apis/namespaced/mysql/v1alpha1/user_types.go @@ -69,6 +69,11 @@ type UserParameters struct { // BinLog defines whether the create, delete, update operations of this user are propagated to replicas. Defaults to true // +optional BinLog *bool `json:"binlog,omitempty"` + + // PasswordRotationTrigger triggers rotation of the auto-generated password when set to + // a time after the current LastPasswordChange. Has no effect when passwordSecretRef is set. + // +optional + PasswordRotationTrigger *metav1.Time `json:"passwordRotationTrigger,omitempty"` } // AuthenticationPlugin selects the auth plugin used when creating the user. @@ -116,6 +121,9 @@ type UserObservation struct { // spec.forProvider.authenticationPlugin and what's actually configured // on the DB so plugin changes are not silently ignored. AuthenticationPlugin *AuthenticationPlugin `json:"authenticationPlugin,omitempty"` + + // LastPasswordChange records when the provider last set the user's password. + LastPasswordChange *metav1.Time `json:"lastPasswordChange,omitempty"` } // +kubebuilder:object:root=true diff --git a/apis/namespaced/mysql/v1alpha1/zz_generated.deepcopy.go b/apis/namespaced/mysql/v1alpha1/zz_generated.deepcopy.go index 6c6c349d..c9e7e721 100644 --- a/apis/namespaced/mysql/v1alpha1/zz_generated.deepcopy.go +++ b/apis/namespaced/mysql/v1alpha1/zz_generated.deepcopy.go @@ -805,6 +805,10 @@ func (in *UserObservation) DeepCopyInto(out *UserObservation) { *out = new(AuthenticationPlugin) (*in).DeepCopyInto(*out) } + if in.LastPasswordChange != nil { + in, out := &in.LastPasswordChange, &out.LastPasswordChange + *out = (*in).DeepCopy() + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UserObservation. @@ -840,6 +844,10 @@ func (in *UserParameters) DeepCopyInto(out *UserParameters) { *out = new(bool) **out = **in } + if in.PasswordRotationTrigger != nil { + in, out := &in.PasswordRotationTrigger, &out.PasswordRotationTrigger + *out = (*in).DeepCopy() + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UserParameters. diff --git a/package/crds/mysql.sql.crossplane.io_users.yaml b/package/crds/mysql.sql.crossplane.io_users.yaml index a14e6c68..701a058d 100644 --- a/package/crds/mysql.sql.crossplane.io_users.yaml +++ b/package/crds/mysql.sql.crossplane.io_users.yaml @@ -108,6 +108,12 @@ spec: operations of this user are propagated to replicas. Defaults to true type: boolean + passwordRotationTrigger: + description: |- + PasswordRotationTrigger triggers rotation of the auto-generated password when set to + a time after the current LastPasswordChange. Has no effect when passwordSecretRef is set. + format: date-time + type: string passwordSecretRef: description: |- PasswordSecretRef references the secret that contains the password used @@ -272,6 +278,11 @@ spec: required: - name type: object + lastPasswordChange: + description: LastPasswordChange records when the provider last + set the user's password. + format: date-time + type: string resourceOptionsAsClauses: description: ResourceOptionsAsClauses represents the applied resource options diff --git a/package/crds/mysql.sql.m.crossplane.io_users.yaml b/package/crds/mysql.sql.m.crossplane.io_users.yaml index 8ff1932e..f6daa4a5 100644 --- a/package/crds/mysql.sql.m.crossplane.io_users.yaml +++ b/package/crds/mysql.sql.m.crossplane.io_users.yaml @@ -94,6 +94,12 @@ spec: operations of this user are propagated to replicas. Defaults to true type: boolean + passwordRotationTrigger: + description: |- + PasswordRotationTrigger triggers rotation of the auto-generated password when set to + a time after the current LastPasswordChange. Has no effect when passwordSecretRef is set. + format: date-time + type: string passwordSecretRef: description: |- PasswordSecretRef references the secret that contains the password used @@ -225,6 +231,11 @@ spec: required: - name type: object + lastPasswordChange: + description: LastPasswordChange records when the provider last + set the user's password. + format: date-time + type: string resourceOptionsAsClauses: description: ResourceOptionsAsClauses represents the applied resource options diff --git a/pkg/controller/cluster/mysql/user/reconciler.go b/pkg/controller/cluster/mysql/user/reconciler.go index a7ef0cd9..0bfe0090 100644 --- a/pkg/controller/cluster/mysql/user/reconciler.go +++ b/pkg/controller/cluster/mysql/user/reconciler.go @@ -24,6 +24,7 @@ import ( "github.com/crossplane/crossplane-runtime/v2/pkg/statemetrics" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -51,12 +52,13 @@ const ( errGetSecret = "cannot get credentials Secret" errTLSConfig = "cannot load TLS config" - errSelectUser = "cannot select user" - errCreateUser = "cannot create user" - errDropUser = "cannot drop user" - errUpdateUser = "cannot update user" - errGetPasswordSecretFailed = "cannot get password secret" - errCompareResourceOptions = "cannot compare desired and observed resource options" + errSelectUser = "cannot select user" + errCreateUser = "cannot create user" + errDropUser = "cannot drop user" + errUpdateUser = "cannot update user" + errGetPasswordSecretFailed = "cannot get password secret" + errGetConnectionSecretFailed = "cannot get connection secret" + errCompareResourceOptions = "cannot compare desired and observed resource options" maxConcurrency = 5 ) @@ -525,10 +527,18 @@ func (c *external) UpdatePassword(ctx context.Context, cr *v1alpha1.User, userna } if pwchanged { + if pw == "" { + pw, err = password.Generate() + if err != nil { + return managed.ConnectionDetails{}, err + } + } query := fmt.Sprintf("ALTER USER %s@%s IDENTIFIED BY %s", mysql.QuoteValue(username), mysql.QuoteValue(host), mysql.QuoteValue(pw)) if err := mysql.ExecWrapper(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errUpdateUser}); err != nil { return managed.ConnectionDetails{}, err } + now := metav1.Now() + cr.Status.AtProvider.LastPasswordChange = &now return c.db.GetConnectionDetails(username, pw), nil } diff --git a/pkg/controller/cluster/mysql/user/reconciler_test.go b/pkg/controller/cluster/mysql/user/reconciler_test.go index 0d256a84..60c26d27 100644 --- a/pkg/controller/cluster/mysql/user/reconciler_test.go +++ b/pkg/controller/cluster/mysql/user/reconciler_test.go @@ -19,15 +19,19 @@ package user import ( "context" "database/sql" + "fmt" "strings" "testing" + "time" "github.com/crossplane-contrib/provider-sql/apis/cluster/mysql/v1alpha1" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" xpv1 "github.com/crossplane/crossplane-runtime/v2/apis/common/v1" @@ -36,6 +40,7 @@ import ( "github.com/crossplane/crossplane-runtime/v2/pkg/resource" "github.com/crossplane/crossplane-runtime/v2/pkg/test" + "github.com/crossplane-contrib/provider-sql/pkg/clients/mysql" "github.com/crossplane-contrib/provider-sql/pkg/clients/xsql" ) @@ -1038,6 +1043,285 @@ func TestUpdate(t *testing.T) { } } +func TestGetPassword(t *testing.T) { + type args struct { + ctx context.Context + user *v1alpha1.User + kube client.Client + } + type want struct { + pwd string + changed bool + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "NilLastPasswordChange": { + reason: "nil LastPasswordChange with no connection secret reference should not reset (nowhere to publish a regenerated password)", + args: args{ + user: &v1alpha1.User{}, + }, + want: want{pwd: "", changed: false}, + }, + "RotationTriggerNilLastChangeSecretHasPassword": { + reason: "PasswordRotationTrigger with nil LastPasswordChange must not reset when the connection secret already has a password (Create just ran)", + args: args{ + user: &v1alpha1.User{ + Spec: v1alpha1.UserSpec{ + ForProvider: v1alpha1.UserParameters{ + PasswordRotationTrigger: &v1.Time{Time: time.Now()}, + }, + ResourceSpec: xpv1.ResourceSpec{ + WriteConnectionSecretToReference: &xpv1.SecretReference{ + Name: "test-secret", + Namespace: "test-ns", + }, + }, + }, + }, + kube: &test.MockClient{ + MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error { + secret := corev1.Secret{ + Data: map[string][]byte{ + xpv1.ResourceCredentialsSecretPasswordKey: []byte("existing-password"), + }, + } + secret.DeepCopyInto(obj.(*corev1.Secret)) + return nil + }, + }, + }, + want: want{pwd: "", changed: false}, + }, + "NilLastPasswordChangeSecretNotFound": { + reason: "nil LastPasswordChange with a missing connection secret should trigger a reset (restoration scenario)", + args: args{ + user: &v1alpha1.User{ + Spec: v1alpha1.UserSpec{ + ResourceSpec: xpv1.ResourceSpec{ + WriteConnectionSecretToReference: &xpv1.SecretReference{ + Name: "test-secret", + Namespace: "test-ns", + }, + }, + }, + }, + kube: &test.MockClient{ + MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{Resource: "secrets"}, "test-secret")), + }, + }, + want: want{pwd: "", changed: true}, + }, + "NilLastPasswordChangeSecretHasPassword": { + reason: "nil LastPasswordChange with existing connection secret password should not reset (Create already ran)", + args: args{ + user: &v1alpha1.User{ + Spec: v1alpha1.UserSpec{ + ResourceSpec: xpv1.ResourceSpec{ + WriteConnectionSecretToReference: &xpv1.SecretReference{ + Name: "test-secret", + Namespace: "test-ns", + }, + }, + }, + }, + kube: &test.MockClient{ + MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error { + secret := corev1.Secret{ + Data: map[string][]byte{ + xpv1.ResourceCredentialsSecretPasswordKey: []byte("existing-password"), + }, + } + secret.DeepCopyInto(obj.(*corev1.Secret)) + return nil + }, + }, + }, + want: want{pwd: "", changed: false}, + }, + "LastPasswordChangeSetNoTrigger": { + reason: "LastPasswordChange set and no rotation trigger should not reset", + args: args{ + user: &v1alpha1.User{ + Status: v1alpha1.UserStatus{ + AtProvider: v1alpha1.UserObservation{ + LastPasswordChange: &v1.Time{Time: v1.Now().Time}, + }, + }, + }, + }, + want: want{pwd: "", changed: false}, + }, + "RotationTriggerAfterLastChange": { + reason: "PasswordRotationTrigger set after LastPasswordChange should trigger rotation", + args: args{ + user: &v1alpha1.User{ + Spec: v1alpha1.UserSpec{ + ForProvider: v1alpha1.UserParameters{ + PasswordRotationTrigger: &v1.Time{Time: time.Now()}, + }, + }, + Status: v1alpha1.UserStatus{ + AtProvider: v1alpha1.UserObservation{ + LastPasswordChange: &v1.Time{Time: time.Now().Add(-time.Hour)}, + }, + }, + }, + }, + want: want{pwd: "", changed: true}, + }, + "RotationTriggerBeforeLastChange": { + reason: "PasswordRotationTrigger set before LastPasswordChange should not trigger rotation", + args: args{ + user: &v1alpha1.User{ + Spec: v1alpha1.UserSpec{ + ForProvider: v1alpha1.UserParameters{ + PasswordRotationTrigger: &v1.Time{Time: time.Now().Add(-2 * time.Hour)}, + }, + }, + Status: v1alpha1.UserStatus{ + AtProvider: v1alpha1.UserObservation{ + LastPasswordChange: &v1.Time{Time: time.Now().Add(-time.Hour)}, + }, + }, + }, + }, + want: want{pwd: "", changed: false}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + e := external{kube: tc.args.kube} + pwd, changed, err := e.getPassword(tc.args.ctx, tc.args.user) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\ne.getPassword(...): -want error, +got error:\n%s\n", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.pwd, pwd); diff != "" { + t.Errorf("\n%s\ne.getPassword(...): -want pwd, +got pwd:\n%s\n", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.changed, changed); diff != "" { + t.Errorf("\n%s\ne.getPassword(...): -want changed, +got changed:\n%s\n", tc.reason, diff) + } + }) + } +} + +func TestUpdatePasswordReset(t *testing.T) { + errBoom := errors.New("boom") + + type fields struct { + execQuery *string + } + + type args struct { + ctx context.Context + mg *v1alpha1.User + kube client.Client + } + + type want struct { + err error + passwordGenerated bool + } + + cases := map[string]struct { + reason string + fields fields + args args + want want + }{ + "NilLastPasswordChangeGeneratesPassword": { + reason: "nil LastPasswordChange with a connection secret reference but missing secret should generate a new password (restoration scenario)", + fields: fields{execQuery: new(string)}, + args: args{ + mg: &v1alpha1.User{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + meta.AnnotationKeyExternalName: "example", + }, + }, + Spec: v1alpha1.UserSpec{ + ResourceSpec: xpv1.ResourceSpec{ + WriteConnectionSecretToReference: &xpv1.SecretReference{ + Name: "test-secret", + Namespace: "test-ns", + }, + }, + }, + }, + kube: &test.MockClient{ + MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{Resource: "secrets"}, "test-secret")), + }, + }, + want: want{ + err: nil, + passwordGenerated: true, + }, + }, + "LastPasswordChangeSetNoReset": { + reason: "LastPasswordChange set and no rotation trigger should not reset", + fields: fields{execQuery: nil}, + args: args{ + mg: &v1alpha1.User{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + meta.AnnotationKeyExternalName: "example", + }, + }, + Status: v1alpha1.UserStatus{ + AtProvider: v1alpha1.UserObservation{ + LastPasswordChange: &v1.Time{Time: v1.Now().Time}, + }, + }, + }, + kube: &test.MockClient{}, + }, + want: want{ + err: nil, + passwordGenerated: false, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + execQuery := tc.fields.execQuery + db := &mockDB{ + MockExec: func(ctx context.Context, q xsql.Query) error { + if execQuery == nil { + return errBoom + } + *execQuery = q.String + return nil + }, + } + e := external{ + db: db, + kube: tc.args.kube, + } + got, err := e.Update(tc.args.ctx, tc.args.mg) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\ne.Update(...): -want error, +got error:\n%s\n", tc.reason, diff) + } + if tc.want.passwordGenerated { + if execQuery == nil || *execQuery == "" { + t.Errorf("\n%s\ne.Update(...): expected ALTER USER IDENTIFIED BY query to be executed\n", tc.reason) + } else if *execQuery == fmt.Sprintf("ALTER USER %s@%s IDENTIFIED BY ''", mysql.QuoteValue("example"), mysql.QuoteValue("%")) { + t.Errorf("\n%s\ne.Update(...): ALTER USER IDENTIFIED BY query contained an empty password\n", tc.reason) + } + if pw := got.ConnectionDetails[xpv1.ResourceCredentialsSecretPasswordKey]; len(pw) == 0 { + t.Errorf("\n%s\ne.Update(...): expected non-empty password in ConnectionDetails\n", tc.reason) + } + } + }) + } +} + func TestDelete(t *testing.T) { errBoom := errors.New("boom") diff --git a/pkg/controller/cluster/mysql/user/utils.go b/pkg/controller/cluster/mysql/user/utils.go index 13dd5050..b7678f60 100644 --- a/pkg/controller/cluster/mysql/user/utils.go +++ b/pkg/controller/cluster/mysql/user/utils.go @@ -31,36 +31,70 @@ import ( ) func (c *external) getPassword(ctx context.Context, user *v1alpha1.User) (newPwd string, changed bool, err error) { - if user.Spec.ForProvider.PasswordSecretRef == nil { - return "", false, nil - } - nn := types.NamespacedName{ - Name: user.Spec.ForProvider.PasswordSecretRef.Name, - Namespace: user.Spec.ForProvider.PasswordSecretRef.Namespace, - } - s := &corev1.Secret{} - if err := c.kube.Get(ctx, nn, s); err != nil { - return "", false, errors.Wrap(err, errGetPasswordSecretFailed) + if user.Spec.ForProvider.PasswordSecretRef != nil { + nn := types.NamespacedName{ + Name: user.Spec.ForProvider.PasswordSecretRef.Name, + Namespace: user.Spec.ForProvider.PasswordSecretRef.Namespace, + } + s := &corev1.Secret{} + if err := c.kube.Get(ctx, nn, s); err != nil { + return "", false, errors.Wrap(err, errGetPasswordSecretFailed) + } + newPwd = string(s.Data[user.Spec.ForProvider.PasswordSecretRef.Key]) + + if user.Spec.WriteConnectionSecretToReference == nil { + return newPwd, false, nil + } + + nn = types.NamespacedName{ + Name: user.Spec.WriteConnectionSecretToReference.Name, + Namespace: user.Spec.WriteConnectionSecretToReference.Namespace, + } + s = &corev1.Secret{} + // the output secret may not exist yet, so we can skip returning an + // error if the error is NotFound + if err := c.kube.Get(ctx, nn, s); resource.IgnoreNotFound(err) != nil { + return "", false, err + } + // if newPwd was set to some value, compare value in output secret with + // newPwd + changed = newPwd != "" && newPwd != string(s.Data[xpv1.ResourceCredentialsSecretPasswordKey]) + + return newPwd, changed, nil } - newPwd = string(s.Data[user.Spec.ForProvider.PasswordSecretRef.Key]) + shouldReset, err := c.shouldResetPassword(ctx, user) + return "", shouldReset, err +} + +// shouldResetPassword returns true when a password change is needed for the non-BYOP path. +// When LastPasswordChange is set, only a PasswordRotationTrigger newer than it forces a +// reset. When LastPasswordChange is nil the connection secret decides: a populated secret +// means Create already ran, while an absent or empty secret means the user was restored +// out-of-band and needs a fresh password. Without a connection secret reference there is +// nowhere to publish a regenerated password, so no reset is attempted. +func (c *external) shouldResetPassword(ctx context.Context, user *v1alpha1.User) (bool, error) { + last := user.Status.AtProvider.LastPasswordChange + if last != nil { + if user.Spec.ForProvider.PasswordRotationTrigger != nil { + return user.Spec.ForProvider.PasswordRotationTrigger.After(last.Time), nil + } + return false, nil + } if user.Spec.WriteConnectionSecretToReference == nil { - return newPwd, false, nil + return false, nil } - - nn = types.NamespacedName{ + nn := types.NamespacedName{ Name: user.Spec.WriteConnectionSecretToReference.Name, Namespace: user.Spec.WriteConnectionSecretToReference.Namespace, } - s = &corev1.Secret{} - // the output secret may not exist yet, so we can skip returning an - // error if the error is NotFound - if err := c.kube.Get(ctx, nn, s); resource.IgnoreNotFound(err) != nil { - return "", false, err + s := &corev1.Secret{} + err := c.kube.Get(ctx, nn, s) + if err != nil { + if resource.IgnoreNotFound(err) != nil { + return false, errors.Wrap(err, errGetConnectionSecretFailed) + } + return true, nil } - // if newPwd was set to some value, compare value in output secret with - // newPwd - changed = newPwd != "" && newPwd != string(s.Data[xpv1.ResourceCredentialsSecretPasswordKey]) - - return newPwd, changed, nil + return len(s.Data[xpv1.ResourceCredentialsSecretPasswordKey]) == 0, nil } diff --git a/pkg/controller/namespaced/mysql/user/reconciler.go b/pkg/controller/namespaced/mysql/user/reconciler.go index 07fc1c57..34c76f7e 100644 --- a/pkg/controller/namespaced/mysql/user/reconciler.go +++ b/pkg/controller/namespaced/mysql/user/reconciler.go @@ -23,6 +23,7 @@ import ( "github.com/crossplane/crossplane-runtime/v2/pkg/statemetrics" "github.com/pkg/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -47,12 +48,13 @@ const ( errTrackPCUsage = "cannot track ProviderConfig usage" errTLSConfig = "cannot load TLS config" - errSelectUser = "cannot select user" - errCreateUser = "cannot create user" - errDropUser = "cannot drop user" - errUpdateUser = "cannot update user" - errGetPasswordSecretFailed = "cannot get password secret" - errCompareResourceOptions = "cannot compare desired and observed resource options" + errSelectUser = "cannot select user" + errCreateUser = "cannot create user" + errDropUser = "cannot drop user" + errUpdateUser = "cannot update user" + errGetPasswordSecretFailed = "cannot get password secret" + errGetConnectionSecretFailed = "cannot get connection secret" + errCompareResourceOptions = "cannot compare desired and observed resource options" maxConcurrency = 5 ) @@ -506,10 +508,18 @@ func (c *external) UpdatePassword(ctx context.Context, cr *namespacedv1alpha1.Us } if pwchanged { + if pw == "" { + pw, err = password.Generate() + if err != nil { + return managed.ConnectionDetails{}, err + } + } query := fmt.Sprintf("ALTER USER %s@%s IDENTIFIED BY %s", mysql.QuoteValue(username), mysql.QuoteValue(host), mysql.QuoteValue(pw)) if err := mysql.ExecWrapper(ctx, c.db, mysql.ExecQuery{Query: query, ErrorValue: errUpdateUser}); err != nil { return managed.ConnectionDetails{}, err } + now := metav1.Now() + cr.Status.AtProvider.LastPasswordChange = &now return c.db.GetConnectionDetails(username, pw), nil } diff --git a/pkg/controller/namespaced/mysql/user/reconciler_test.go b/pkg/controller/namespaced/mysql/user/reconciler_test.go index 5f9ce7d6..5b12cfcb 100644 --- a/pkg/controller/namespaced/mysql/user/reconciler_test.go +++ b/pkg/controller/namespaced/mysql/user/reconciler_test.go @@ -19,15 +19,19 @@ package user import ( "context" "database/sql" + "fmt" "strings" "testing" + "time" "github.com/crossplane-contrib/provider-sql/apis/namespaced/mysql/v1alpha1" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/crossplane/crossplane-runtime/v2/apis/common" @@ -38,6 +42,7 @@ import ( "github.com/crossplane/crossplane-runtime/v2/pkg/resource" "github.com/crossplane/crossplane-runtime/v2/pkg/test" + "github.com/crossplane-contrib/provider-sql/pkg/clients/mysql" "github.com/crossplane-contrib/provider-sql/pkg/clients/xsql" provErrors "github.com/crossplane-contrib/provider-sql/pkg/controller/namespaced/errors" ) @@ -1088,6 +1093,291 @@ func TestUpdate(t *testing.T) { } } +func TestGetPassword(t *testing.T) { + type args struct { + ctx context.Context + user *v1alpha1.User + kube client.Client + } + type want struct { + pwd string + changed bool + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "NilLastPasswordChange": { + reason: "nil LastPasswordChange with no connection secret reference should not reset (nowhere to publish a regenerated password)", + args: args{ + user: &v1alpha1.User{}, + }, + want: want{pwd: "", changed: false}, + }, + "RotationTriggerNilLastChangeSecretHasPassword": { + reason: "PasswordRotationTrigger with nil LastPasswordChange must not reset when the connection secret already has a password (Create just ran)", + args: args{ + user: &v1alpha1.User{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-ns", + }, + Spec: v1alpha1.UserSpec{ + ForProvider: v1alpha1.UserParameters{ + PasswordRotationTrigger: &metav1.Time{Time: time.Now()}, + }, + ManagedResourceSpec: xpv2.ManagedResourceSpec{ + WriteConnectionSecretToReference: &common.LocalSecretReference{ + Name: "test-secret", + }, + }, + }, + }, + kube: &test.MockClient{ + MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error { + secret := corev1.Secret{ + Data: map[string][]byte{ + xpv1.ResourceCredentialsSecretPasswordKey: []byte("existing-password"), + }, + } + secret.DeepCopyInto(obj.(*corev1.Secret)) + return nil + }, + }, + }, + want: want{pwd: "", changed: false}, + }, + "NilLastPasswordChangeSecretNotFound": { + reason: "nil LastPasswordChange with a missing connection secret should trigger a reset (restoration scenario)", + args: args{ + user: &v1alpha1.User{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-ns", + }, + Spec: v1alpha1.UserSpec{ + ManagedResourceSpec: xpv2.ManagedResourceSpec{ + WriteConnectionSecretToReference: &common.LocalSecretReference{ + Name: "test-secret", + }, + }, + }, + }, + kube: &test.MockClient{ + MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{Resource: "secrets"}, "test-secret")), + }, + }, + want: want{pwd: "", changed: true}, + }, + "NilLastPasswordChangeSecretHasPassword": { + reason: "nil LastPasswordChange with existing connection secret password should not reset (Create already ran)", + args: args{ + user: &v1alpha1.User{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-ns", + }, + Spec: v1alpha1.UserSpec{ + ManagedResourceSpec: xpv2.ManagedResourceSpec{ + WriteConnectionSecretToReference: &common.LocalSecretReference{ + Name: "test-secret", + }, + }, + }, + }, + kube: &test.MockClient{ + MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error { + secret := corev1.Secret{ + Data: map[string][]byte{ + xpv1.ResourceCredentialsSecretPasswordKey: []byte("existing-password"), + }, + } + secret.DeepCopyInto(obj.(*corev1.Secret)) + return nil + }, + }, + }, + want: want{pwd: "", changed: false}, + }, + "LastPasswordChangeSetNoTrigger": { + reason: "LastPasswordChange set and no rotation trigger should not reset", + args: args{ + user: &v1alpha1.User{ + Status: v1alpha1.UserStatus{ + AtProvider: v1alpha1.UserObservation{ + LastPasswordChange: &metav1.Time{Time: metav1.Now().Time}, + }, + }, + }, + }, + want: want{pwd: "", changed: false}, + }, + "RotationTriggerAfterLastChange": { + reason: "PasswordRotationTrigger set after LastPasswordChange should trigger rotation", + args: args{ + user: &v1alpha1.User{ + Spec: v1alpha1.UserSpec{ + ForProvider: v1alpha1.UserParameters{ + PasswordRotationTrigger: &metav1.Time{Time: time.Now()}, + }, + }, + Status: v1alpha1.UserStatus{ + AtProvider: v1alpha1.UserObservation{ + LastPasswordChange: &metav1.Time{Time: time.Now().Add(-time.Hour)}, + }, + }, + }, + }, + want: want{pwd: "", changed: true}, + }, + "RotationTriggerBeforeLastChange": { + reason: "PasswordRotationTrigger set before LastPasswordChange should not trigger rotation", + args: args{ + user: &v1alpha1.User{ + Spec: v1alpha1.UserSpec{ + ForProvider: v1alpha1.UserParameters{ + PasswordRotationTrigger: &metav1.Time{Time: time.Now().Add(-2 * time.Hour)}, + }, + }, + Status: v1alpha1.UserStatus{ + AtProvider: v1alpha1.UserObservation{ + LastPasswordChange: &metav1.Time{Time: time.Now().Add(-time.Hour)}, + }, + }, + }, + }, + want: want{pwd: "", changed: false}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + e := external{kube: tc.args.kube} + pwd, changed, err := e.getPassword(tc.args.ctx, tc.args.user) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\ne.getPassword(...): -want error, +got error:\n%s\n", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.pwd, pwd); diff != "" { + t.Errorf("\n%s\ne.getPassword(...): -want pwd, +got pwd:\n%s\n", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.changed, changed); diff != "" { + t.Errorf("\n%s\ne.getPassword(...): -want changed, +got changed:\n%s\n", tc.reason, diff) + } + }) + } +} + +func TestUpdatePasswordReset(t *testing.T) { + errBoom := errors.New("boom") + + type fields struct { + execQuery *string + } + + type args struct { + ctx context.Context + mg *v1alpha1.User + kube client.Client + } + + type want struct { + err error + passwordGenerated bool + } + + cases := map[string]struct { + reason string + fields fields + args args + want want + }{ + "NilLastPasswordChangeGeneratesPassword": { + reason: "nil LastPasswordChange with a connection secret reference but missing secret should generate a new password (restoration scenario)", + fields: fields{execQuery: new(string)}, + args: args{ + mg: &v1alpha1.User{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-ns", + Annotations: map[string]string{ + meta.AnnotationKeyExternalName: "example", + }, + }, + Spec: v1alpha1.UserSpec{ + ManagedResourceSpec: xpv2.ManagedResourceSpec{ + WriteConnectionSecretToReference: &common.LocalSecretReference{ + Name: "test-secret", + }, + }, + }, + }, + kube: &test.MockClient{ + MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{Resource: "secrets"}, "test-secret")), + }, + }, + want: want{ + err: nil, + passwordGenerated: true, + }, + }, + "LastPasswordChangeSetNoReset": { + reason: "LastPasswordChange set and no rotation trigger should not reset", + fields: fields{execQuery: nil}, + args: args{ + mg: &v1alpha1.User{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + meta.AnnotationKeyExternalName: "example", + }, + }, + Status: v1alpha1.UserStatus{ + AtProvider: v1alpha1.UserObservation{ + LastPasswordChange: &metav1.Time{Time: metav1.Now().Time}, + }, + }, + }, + kube: &test.MockClient{}, + }, + want: want{ + err: nil, + passwordGenerated: false, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + execQuery := tc.fields.execQuery + db := &mockDB{ + MockExec: func(ctx context.Context, q xsql.Query) error { + if execQuery == nil { + return errBoom + } + *execQuery = q.String + return nil + }, + } + e := external{ + db: db, + kube: tc.args.kube, + } + got, err := e.Update(tc.args.ctx, tc.args.mg) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\ne.Update(...): -want error, +got error:\n%s\n", tc.reason, diff) + } + if tc.want.passwordGenerated { + if execQuery == nil || *execQuery == "" { + t.Errorf("\n%s\ne.Update(...): expected ALTER USER IDENTIFIED BY query to be executed\n", tc.reason) + } else if *execQuery == fmt.Sprintf("ALTER USER %s@%s IDENTIFIED BY ''", mysql.QuoteValue("example"), mysql.QuoteValue("%")) { + t.Errorf("\n%s\ne.Update(...): ALTER USER IDENTIFIED BY query contained an empty password\n", tc.reason) + } + if pw := got.ConnectionDetails[xpv1.ResourceCredentialsSecretPasswordKey]; len(pw) == 0 { + t.Errorf("\n%s\ne.Update(...): expected non-empty password in ConnectionDetails\n", tc.reason) + } + } + }) + } +} + func TestDelete(t *testing.T) { errBoom := errors.New("boom") diff --git a/pkg/controller/namespaced/mysql/user/utils.go b/pkg/controller/namespaced/mysql/user/utils.go index 2e7335c2..e6e37a0a 100644 --- a/pkg/controller/namespaced/mysql/user/utils.go +++ b/pkg/controller/namespaced/mysql/user/utils.go @@ -31,36 +31,70 @@ import ( ) func (c *external) getPassword(ctx context.Context, user *v1alpha1.User) (newPwd string, changed bool, err error) { - if user.Spec.ForProvider.PasswordSecretRef == nil { - return "", false, nil - } - nn := types.NamespacedName{ - Name: user.Spec.ForProvider.PasswordSecretRef.Name, - Namespace: user.Namespace, - } - s := &corev1.Secret{} - if err := c.kube.Get(ctx, nn, s); err != nil { - return "", false, errors.Wrap(err, errGetPasswordSecretFailed) + if user.Spec.ForProvider.PasswordSecretRef != nil { + nn := types.NamespacedName{ + Name: user.Spec.ForProvider.PasswordSecretRef.Name, + Namespace: user.Namespace, + } + s := &corev1.Secret{} + if err := c.kube.Get(ctx, nn, s); err != nil { + return "", false, errors.Wrap(err, errGetPasswordSecretFailed) + } + newPwd = string(s.Data[user.Spec.ForProvider.PasswordSecretRef.Key]) + + if user.Spec.WriteConnectionSecretToReference == nil { + return newPwd, false, nil + } + + nn = types.NamespacedName{ + Name: user.Spec.WriteConnectionSecretToReference.Name, + Namespace: user.Namespace, + } + s = &corev1.Secret{} + // the output secret may not exist yet, so we can skip returning an + // error if the error is NotFound + if err := c.kube.Get(ctx, nn, s); resource.IgnoreNotFound(err) != nil { + return "", false, err + } + // if newPwd was set to some value, compare value in output secret with + // newPwd + changed = newPwd != "" && newPwd != string(s.Data[xpv1.ResourceCredentialsSecretPasswordKey]) + + return newPwd, changed, nil } - newPwd = string(s.Data[user.Spec.ForProvider.PasswordSecretRef.Key]) + shouldReset, err := c.shouldResetPassword(ctx, user) + return "", shouldReset, err +} + +// shouldResetPassword returns true when a password change is needed for the non-BYOP path. +// When LastPasswordChange is set, only a PasswordRotationTrigger newer than it forces a +// reset. When LastPasswordChange is nil the connection secret decides: a populated secret +// means Create already ran, while an absent or empty secret means the user was restored +// out-of-band and needs a fresh password. Without a connection secret reference there is +// nowhere to publish a regenerated password, so no reset is attempted. +func (c *external) shouldResetPassword(ctx context.Context, user *v1alpha1.User) (bool, error) { + last := user.Status.AtProvider.LastPasswordChange + if last != nil { + if user.Spec.ForProvider.PasswordRotationTrigger != nil { + return user.Spec.ForProvider.PasswordRotationTrigger.After(last.Time), nil + } + return false, nil + } if user.Spec.WriteConnectionSecretToReference == nil { - return newPwd, false, nil + return false, nil } - - nn = types.NamespacedName{ + nn := types.NamespacedName{ Name: user.Spec.WriteConnectionSecretToReference.Name, Namespace: user.Namespace, } - s = &corev1.Secret{} - // the output secret may not exist yet, so we can skip returning an - // error if the error is NotFound - if err := c.kube.Get(ctx, nn, s); resource.IgnoreNotFound(err) != nil { - return "", false, err + s := &corev1.Secret{} + err := c.kube.Get(ctx, nn, s) + if err != nil { + if resource.IgnoreNotFound(err) != nil { + return false, errors.Wrap(err, errGetConnectionSecretFailed) + } + return true, nil } - // if newPwd was set to some value, compare value in output secret with - // newPwd - changed = newPwd != "" && newPwd != string(s.Data[xpv1.ResourceCredentialsSecretPasswordKey]) - - return newPwd, changed, nil + return len(s.Data[xpv1.ResourceCredentialsSecretPasswordKey]) == 0, nil }