diff --git a/pkg/controller/user/user_controller.go b/pkg/controller/user/user_controller.go index c573d306e..5b882d36f 100644 --- a/pkg/controller/user/user_controller.go +++ b/pkg/controller/user/user_controller.go @@ -198,12 +198,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco // update user status if not managed by kubefed managedByKubefed := user.Labels[constants.KubefedManagedLabel] == "true" if !managedByKubefed { - if user, err = r.encryptPassword(ctx, user); err != nil { + if err = r.encryptPassword(ctx, user); err != nil { klog.Error(err) r.Recorder.Event(user, corev1.EventTypeWarning, failedSynced, fmt.Sprintf(syncFailMessage, err)) return ctrl.Result{}, err } - if user, err = r.syncUserStatus(ctx, user); err != nil { + if err = r.syncUserStatus(ctx, user); err != nil { klog.Error(err) r.Recorder.Event(user, corev1.EventTypeWarning, failedSynced, fmt.Sprintf(syncFailMessage, err)) return ctrl.Result{}, err @@ -238,15 +238,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return ctrl.Result{}, nil } -func (r *Reconciler) encryptPassword(ctx context.Context, user *iamv1alpha2.User) (*iamv1alpha2.User, error) { +// encryptPassword Encrypt and update the user password +func (r *Reconciler) encryptPassword(ctx context.Context, user *iamv1alpha2.User) error { // password is not empty and not encrypted if user.Spec.EncryptedPassword != "" && !isEncrypted(user.Spec.EncryptedPassword) { password, err := encrypt(user.Spec.EncryptedPassword) if err != nil { klog.Error(err) - return nil, err + return err } - user = user.DeepCopy() user.Spec.EncryptedPassword = password if user.Annotations == nil { user.Annotations = make(map[string]string) @@ -256,11 +256,10 @@ func (r *Reconciler) encryptPassword(ctx context.Context, user *iamv1alpha2.User delete(user.Annotations, corev1.LastAppliedConfigAnnotation) err = r.Update(ctx, user, &client.UpdateOptions{}) if err != nil { - return nil, err + return err } - return user, nil } - return user, nil + return nil } func (r *Reconciler) ensureNotControlledByKubefed(ctx context.Context, user *iamv1alpha2.User) error { @@ -268,11 +267,11 @@ func (r *Reconciler) ensureNotControlledByKubefed(ctx context.Context, user *iam if user.Labels == nil { user.Labels = make(map[string]string, 0) } - user = user.DeepCopy() user.Labels[constants.KubefedManagedLabel] = "false" err := r.Update(ctx, user, &client.UpdateOptions{}) if err != nil { klog.Error(err) + return err } } return nil @@ -453,57 +452,51 @@ func (r *Reconciler) deleteLoginRecords(ctx context.Context, user *iamv1alpha2.U return r.Client.DeleteAllOf(ctx, loginRecord, client.MatchingLabels{iamv1alpha2.UserReferenceLabel: user.Name}) } -// syncUserStatus will reconcile user state based on user login records -func (r *Reconciler) syncUserStatus(ctx context.Context, user *iamv1alpha2.User) (*iamv1alpha2.User, error) { +// syncUserStatus Update the user status +func (r *Reconciler) syncUserStatus(ctx context.Context, user *iamv1alpha2.User) error { if user.Spec.EncryptedPassword == "" { if user.Labels[iamv1alpha2.IdentifyProviderLabel] != "" { // mapped user from other identity provider always active until disabled if user.Status.State == nil || *user.Status.State != iamv1alpha2.UserActive { - expected := user.DeepCopy() active := iamv1alpha2.UserActive - expected.Status = iamv1alpha2.UserStatus{ + user.Status = iamv1alpha2.UserStatus{ State: &active, LastTransitionTime: &metav1.Time{Time: time.Now()}, } - err := r.Update(ctx, expected, &client.UpdateOptions{}) + err := r.Update(ctx, user, &client.UpdateOptions{}) if err != nil { - return nil, err + return err } - return expected, nil } } else { // becomes disabled after setting a blank password if user.Status.State == nil || *user.Status.State != iamv1alpha2.UserDisabled { - expected := user.DeepCopy() disabled := iamv1alpha2.UserDisabled - expected.Status = iamv1alpha2.UserStatus{ + user.Status = iamv1alpha2.UserStatus{ State: &disabled, LastTransitionTime: &metav1.Time{Time: time.Now()}, } - err := r.Update(ctx, expected, &client.UpdateOptions{}) + err := r.Update(ctx, user, &client.UpdateOptions{}) if err != nil { - return nil, err + return err } - return expected, nil } } - return user, nil + return nil } // becomes active after password encrypted if isEncrypted(user.Spec.EncryptedPassword) { if user.Status.State == nil || *user.Status.State == iamv1alpha2.UserDisabled { - expected := user.DeepCopy() active := iamv1alpha2.UserActive - expected.Status = iamv1alpha2.UserStatus{ + user.Status = iamv1alpha2.UserStatus{ State: &active, LastTransitionTime: &metav1.Time{Time: time.Now()}, } - err := r.Update(ctx, expected, &client.UpdateOptions{}) + err := r.Update(ctx, user, &client.UpdateOptions{}) if err != nil { - return nil, err + return err } - return expected, nil } } @@ -511,18 +504,17 @@ func (r *Reconciler) syncUserStatus(ctx context.Context, user *iamv1alpha2.User) if user.Status.State != nil && *user.Status.State == iamv1alpha2.UserAuthLimitExceeded { if user.Status.LastTransitionTime != nil && user.Status.LastTransitionTime.Add(r.AuthenticationOptions.AuthenticateRateLimiterDuration).Before(time.Now()) { - expected := user.DeepCopy() // unblock user active := iamv1alpha2.UserActive - expected.Status = iamv1alpha2.UserStatus{ + user.Status = iamv1alpha2.UserStatus{ State: &active, LastTransitionTime: &metav1.Time{Time: time.Now()}, } - err := r.Update(ctx, expected, &client.UpdateOptions{}) + err := r.Update(ctx, user, &client.UpdateOptions{}) if err != nil { - return nil, err + return err } - return expected, nil + return nil } } @@ -531,7 +523,7 @@ func (r *Reconciler) syncUserStatus(ctx context.Context, user *iamv1alpha2.User) err := r.List(ctx, records, client.MatchingLabels{iamv1alpha2.UserReferenceLabel: user.Name}) if err != nil { klog.Error(err) - return nil, err + return err } // count failed login attempts during last AuthenticateRateLimiterDuration @@ -546,22 +538,20 @@ func (r *Reconciler) syncUserStatus(ctx context.Context, user *iamv1alpha2.User) // block user if failed login attempts exceeds maximum tries setting if failedLoginAttempts >= r.AuthenticationOptions.AuthenticateRateLimiterMaxTries { - expected := user.DeepCopy() limitExceed := iamv1alpha2.UserAuthLimitExceeded - expected.Status = iamv1alpha2.UserStatus{ + user.Status = iamv1alpha2.UserStatus{ State: &limitExceed, Reason: fmt.Sprintf("Failed login attempts exceed %d in last %s", failedLoginAttempts, r.AuthenticationOptions.AuthenticateRateLimiterDuration), LastTransitionTime: &metav1.Time{Time: time.Now()}, } - err = r.Update(ctx, expected, &client.UpdateOptions{}) + err = r.Update(ctx, user, &client.UpdateOptions{}) if err != nil { - return nil, err + return err } - return expected, nil } - return user, nil + return nil } func encrypt(password string) (string, error) { diff --git a/pkg/controller/user/user_controller_test.go b/pkg/controller/user/user_controller_test.go index 3c7b5d443..b79e81655 100644 --- a/pkg/controller/user/user_controller_test.go +++ b/pkg/controller/user/user_controller_test.go @@ -99,7 +99,7 @@ func TestDoNothing(t *testing.T) { t.Fatal(err) } - _, err = c.Reconcile(context.Background(), reconcile.Request{ + result, err := c.Reconcile(context.Background(), reconcile.Request{ NamespacedName: types.NamespacedName{Name: user.Name}, }) if err != nil { @@ -108,22 +108,15 @@ func TestDoNothing(t *testing.T) { // append finalizer updateEvent := <-w.ResultChan() - assert.Equal(t, updateEvent.Type, watch.Modified) + assert.Equal(t, watch.Modified, updateEvent.Type) assert.NotNil(t, updateEvent.Object) user = updateEvent.Object.(*iamv1alpha2.User) assert.NotNil(t, user) assert.NotEmpty(t, user.Finalizers) - result, err := c.Reconcile(context.Background(), reconcile.Request{ - NamespacedName: types.NamespacedName{Name: user.Name}, - }) - if err != nil { - t.Fatal(err) - } - updateEvent = <-w.ResultChan() // encrypt password - assert.Equal(t, updateEvent.Type, watch.Modified) + assert.Equal(t, watch.Modified, updateEvent.Type) assert.NotNil(t, updateEvent.Object) user = updateEvent.Object.(*iamv1alpha2.User) assert.NotNil(t, user) @@ -132,12 +125,12 @@ func TestDoNothing(t *testing.T) { // becomes active after password encrypted updateEvent = <-w.ResultChan() user = updateEvent.Object.(*iamv1alpha2.User) - assert.Equal(t, *user.Status.State, iamv1alpha2.UserActive) + assert.Equal(t, iamv1alpha2.UserActive, *user.Status.State) // block user updateEvent = <-w.ResultChan() user = updateEvent.Object.(*iamv1alpha2.User) - assert.Equal(t, *user.Status.State, iamv1alpha2.UserAuthLimitExceeded) + assert.Equal(t, iamv1alpha2.UserAuthLimitExceeded, *user.Status.State) assert.True(t, result.Requeue) time.Sleep(result.RequeueAfter + time.Second) @@ -151,5 +144,5 @@ func TestDoNothing(t *testing.T) { // unblock user updateEvent = <-w.ResultChan() user = updateEvent.Object.(*iamv1alpha2.User) - assert.Equal(t, *user.Status.State, iamv1alpha2.UserActive) + assert.Equal(t, iamv1alpha2.UserActive, *user.Status.State) } diff --git a/pkg/controller/workspacerole/workspacerole_controller.go b/pkg/controller/workspacerole/workspacerole_controller.go index c8c8c2bae..b70d61c17 100644 --- a/pkg/controller/workspacerole/workspacerole_controller.go +++ b/pkg/controller/workspacerole/workspacerole_controller.go @@ -122,7 +122,6 @@ func (r *Reconciler) bindWorkspace(ctx context.Context, logger logr.Logger, work return client.IgnoreNotFound(err) } if !metav1.IsControlledBy(workspaceRole, &workspace) { - workspaceRole = workspaceRole.DeepCopy() workspaceRole.OwnerReferences = k8sutil.RemoveWorkspaceOwnerReference(workspaceRole.OwnerReferences) if err := controllerutil.SetControllerReference(&workspace, workspaceRole, r.Scheme); err != nil { logger.Error(err, "set controller reference failed") @@ -151,6 +150,7 @@ func (r *Reconciler) multiClusterSync(ctx context.Context, logger logr.Logger, w logger.Error(err, "create federated workspace role failed") return err } + return nil } } logger.Error(err, "get federated workspace role failed") @@ -174,10 +174,6 @@ func (r *Reconciler) multiClusterSync(ctx context.Context, logger logr.Logger, w func newFederatedWorkspaceRole(workspaceRole *iamv1alpha2.WorkspaceRole) (*typesv1beta1.FederatedWorkspaceRole, error) { federatedWorkspaceRole := &typesv1beta1.FederatedWorkspaceRole{ - TypeMeta: metav1.TypeMeta{ - Kind: typesv1beta1.FederatedWorkspaceRoleKind, - APIVersion: typesv1beta1.SchemeGroupVersion.String(), - }, ObjectMeta: metav1.ObjectMeta{ Name: workspaceRole.Name, }, @@ -206,7 +202,6 @@ func (r *Reconciler) ensureNotControlledByKubefed(ctx context.Context, logger lo if workspaceRole.Labels == nil { workspaceRole.Labels = make(map[string]string) } - workspaceRole = workspaceRole.DeepCopy() workspaceRole.Labels[constants.KubefedManagedLabel] = "false" if err := r.Update(ctx, workspaceRole); err != nil { logger.Error(err, "update kubefed managed label failed") diff --git a/pkg/controller/workspacerolebinding/workspacerolebinding_controller.go b/pkg/controller/workspacerolebinding/workspacerolebinding_controller.go index 11f436a64..39c845cae 100644 --- a/pkg/controller/workspacerolebinding/workspacerolebinding_controller.go +++ b/pkg/controller/workspacerolebinding/workspacerolebinding_controller.go @@ -123,7 +123,6 @@ func (r *Reconciler) bindWorkspace(ctx context.Context, logger logr.Logger, work } // owner reference not match workspace label if !metav1.IsControlledBy(workspaceRoleBinding, workspace) { - workspaceRoleBinding := workspaceRoleBinding.DeepCopy() workspaceRoleBinding.OwnerReferences = k8sutil.RemoveWorkspaceOwnerReference(workspaceRoleBinding.OwnerReferences) if err := controllerutil.SetControllerReference(workspace, workspaceRoleBinding, r.Scheme); err != nil { logger.Error(err, "set controller reference failed") @@ -145,7 +144,7 @@ func (r *Reconciler) multiClusterSync(ctx context.Context, logger logr.Logger, w federatedWorkspaceRoleBinding := &typesv1beta1.FederatedWorkspaceRoleBinding{} if err := r.Client.Get(ctx, types.NamespacedName{Name: workspaceRoleBinding.Name}, federatedWorkspaceRoleBinding); err != nil { if errors.IsNotFound(err) { - if federatedWorkspaceRoleBinding, err := newFederatedWorkspaceRole(workspaceRoleBinding); err != nil { + if federatedWorkspaceRoleBinding, err := newFederatedWorkspaceRoleBinding(workspaceRoleBinding); err != nil { logger.Error(err, "generate federated workspace role binding failed") return err } else { @@ -153,6 +152,7 @@ func (r *Reconciler) multiClusterSync(ctx context.Context, logger logr.Logger, w logger.Error(err, "create federated workspace role binding failed") return err } + return nil } } logger.Error(err, "get federated workspace role binding failed") @@ -176,12 +176,8 @@ func (r *Reconciler) multiClusterSync(ctx context.Context, logger logr.Logger, w return nil } -func newFederatedWorkspaceRole(workspaceRoleBinding *iamv1alpha2.WorkspaceRoleBinding) (*typesv1beta1.FederatedWorkspaceRoleBinding, error) { - federatedWorkspaceRole := &typesv1beta1.FederatedWorkspaceRoleBinding{ - TypeMeta: metav1.TypeMeta{ - Kind: typesv1beta1.FederatedWorkspaceRoleBindingKind, - APIVersion: typesv1beta1.SchemeGroupVersion.String(), - }, +func newFederatedWorkspaceRoleBinding(workspaceRoleBinding *iamv1alpha2.WorkspaceRoleBinding) (*typesv1beta1.FederatedWorkspaceRoleBinding, error) { + federatedWorkspaceRoleBinding := &typesv1beta1.FederatedWorkspaceRoleBinding{ ObjectMeta: metav1.ObjectMeta{ Name: workspaceRoleBinding.Name, }, @@ -198,10 +194,10 @@ func newFederatedWorkspaceRole(workspaceRoleBinding *iamv1alpha2.WorkspaceRoleBi }, }, } - if err := controllerutil.SetControllerReference(workspaceRoleBinding, federatedWorkspaceRole, scheme.Scheme); err != nil { + if err := controllerutil.SetControllerReference(workspaceRoleBinding, federatedWorkspaceRoleBinding, scheme.Scheme); err != nil { return nil, err } - return federatedWorkspaceRole, nil + return federatedWorkspaceRoleBinding, nil } func (r *Reconciler) ensureNotControlledByKubefed(ctx context.Context, logger logr.Logger, workspaceRoleBinding *iamv1alpha2.WorkspaceRoleBinding) error { @@ -209,7 +205,6 @@ func (r *Reconciler) ensureNotControlledByKubefed(ctx context.Context, logger lo if workspaceRoleBinding.Labels == nil { workspaceRoleBinding.Labels = make(map[string]string) } - workspaceRoleBinding = workspaceRoleBinding.DeepCopy() workspaceRoleBinding.Labels[constants.KubefedManagedLabel] = "false" logger.V(4).Info("update kubefed managed label") if err := r.Update(ctx, workspaceRoleBinding); err != nil { diff --git a/pkg/controller/workspacetemplate/workspacetemplate_controller.go b/pkg/controller/workspacetemplate/workspacetemplate_controller.go index 10017eac3..7e90ffca9 100644 --- a/pkg/controller/workspacetemplate/workspacetemplate_controller.go +++ b/pkg/controller/workspacetemplate/workspacetemplate_controller.go @@ -230,10 +230,6 @@ func (r *Reconciler) multiClusterSync(ctx context.Context, logger logr.Logger, w func newFederatedWorkspace(template *tenantv1alpha2.WorkspaceTemplate) (*typesv1beta1.FederatedWorkspace, error) { federatedWorkspace := &typesv1beta1.FederatedWorkspace{ - TypeMeta: metav1.TypeMeta{ - Kind: typesv1beta1.FederatedWorkspaceRoleKind, - APIVersion: typesv1beta1.SchemeGroupVersion.String(), - }, ObjectMeta: metav1.ObjectMeta{ Name: template.Name, Labels: template.Labels, @@ -301,7 +297,6 @@ func (r *Reconciler) ensureNotControlledByKubefed(ctx context.Context, logger lo if workspaceTemplate.Labels == nil { workspaceTemplate.Labels = make(map[string]string) } - workspaceTemplate = workspaceTemplate.DeepCopy() workspaceTemplate.Labels[constants.KubefedManagedLabel] = "false" logger.V(4).Info("update kubefed managed label") if err := r.Update(ctx, workspaceTemplate); err != nil { @@ -326,8 +321,8 @@ func (r *Reconciler) initWorkspaceRoles(ctx context.Context, logger logr.Logger, expected.Labels = make(map[string]string) } expected.Labels[tenantv1alpha1.WorkspaceLabel] = workspace.Name - var existed iamv1alpha2.WorkspaceRole - if err := r.Get(ctx, types.NamespacedName{Name: expected.Name}, &existed); err != nil { + workspaceRole := &iamv1alpha2.WorkspaceRole{} + if err := r.Get(ctx, types.NamespacedName{Name: expected.Name}, workspaceRole); err != nil { if errors.IsNotFound(err) { logger.V(4).Info("create workspace role", "workspacerole", expected.Name) if err := r.Create(ctx, &expected); err != nil { @@ -340,15 +335,14 @@ func (r *Reconciler) initWorkspaceRoles(ctx context.Context, logger logr.Logger, return err } } - if !reflect.DeepEqual(expected.Labels, existed.Labels) || - !reflect.DeepEqual(expected.Annotations, existed.Annotations) || - !reflect.DeepEqual(expected.Rules, existed.Rules) { - updated := existed.DeepCopy() - updated.Labels = expected.Labels - updated.Annotations = expected.Annotations - updated.Rules = expected.Rules - logger.V(4).Info("update workspace role", "workspacerole", updated.Name) - if err := r.Update(ctx, updated); err != nil { + if !reflect.DeepEqual(expected.Labels, workspaceRole.Labels) || + !reflect.DeepEqual(expected.Annotations, workspaceRole.Annotations) || + !reflect.DeepEqual(expected.Rules, workspaceRole.Rules) { + workspaceRole.Labels = expected.Labels + workspaceRole.Annotations = expected.Annotations + workspaceRole.Rules = expected.Rules + logger.V(4).Info("update workspace role", "workspacerole", workspaceRole.Name) + if err := r.Update(ctx, workspaceRole); err != nil { logger.Error(err, "update workspace role failed") return err }