From d407b08a6090de6bc415f34f21c85a07175ad97a Mon Sep 17 00:00:00 2001 From: MuneebAijaz Date: Thu, 10 Mar 2022 16:34:15 +0500 Subject: [PATCH 1/2] Fixed repetitive reconcilation issue on users --- .gitignore | 3 +- api/v1alpha1/zz_generated.deepcopy.go | 1 + controllers/channel_controller.go | 7 ++-- controllers/channel_controller_test.go | 7 ++-- pkg/config/config.go | 3 ++ pkg/slack/service.go | 18 ++++---- pkg/slack/service_test.go | 11 +++-- pkg/util/util.go | 58 ++++++++++++++++++++++++++ 8 files changed, 89 insertions(+), 19 deletions(-) create mode 100644 pkg/util/util.go diff --git a/.gitignore b/.gitignore index a2823c2..418282b 100644 --- a/.gitignore +++ b/.gitignore @@ -23,4 +23,5 @@ bin *.swo *~ .local -testbin \ No newline at end of file +testbin +.vscode \ No newline at end of file diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 93588ab..a197d01 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -1,3 +1,4 @@ +//go:build !ignore_autogenerated // +build !ignore_autogenerated /* diff --git a/controllers/channel_controller.go b/controllers/channel_controller.go index 631bddc..cc9c92d 100644 --- a/controllers/channel_controller.go +++ b/controllers/channel_controller.go @@ -29,6 +29,7 @@ import ( reconcilerUtil "github.com/stakater/operator-utils/util/reconciler" slackv1alpha1 "github.com/stakater/slack-operator/api/v1alpha1" slack "github.com/stakater/slack-operator/pkg/slack" + pkgutil "github.com/stakater/slack-operator/pkg/util" ) var ( @@ -184,10 +185,10 @@ func (r *ChannelReconciler) updateSlackChannel(ctx context.Context, channel *sla return reconcilerUtil.ManageError(r.Client, channel, err, false) } - err = r.SlackService.InviteUsers(channelID, users) - if err != nil { + errList := r.SlackService.InviteUsers(channelID, users) + if len(errList) > 0 { log.Error(err, "Error inviting users to channel") - return reconcilerUtil.ManageError(r.Client, channel, err, false) + return pkgutil.ManageError(ctx, r.Client, channel, pkgutil.MapErrorListToError(errList)) } err = r.SlackService.RemoveUsers(channelID, users) diff --git a/controllers/channel_controller_test.go b/controllers/channel_controller_test.go index 06a7cb4..b414105 100644 --- a/controllers/channel_controller_test.go +++ b/controllers/channel_controller_test.go @@ -2,6 +2,7 @@ package controllers import ( "context" + "fmt" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -84,13 +85,13 @@ var _ = Describe("ChannelController", func() { }) It("should set error condition when user does not exists", func() { - - _ = util.CreateChannel(channelName, true, "", "", []string{"nonexistent@slack.com"}, ns) + emailList := []string{"nonexistent@slack.com"} + _ = util.CreateChannel(channelName, true, "", "", emailList, ns) channel := util.GetChannel(channelName, ns) Expect(len(channel.Status.Conditions)).To(Equal(1)) Expect(channel.Status.Conditions[0].Reason).To(Equal("Failed")) - Expect(channel.Status.Conditions[0].Message).To(Equal("users_not_found")) + Expect(channel.Status.Conditions[0].Message).To(Equal(fmt.Sprintf("Error fetching user by Email %s", emailList[0]))) }) }) }) diff --git a/pkg/config/config.go b/pkg/config/config.go index ef2307c..aa64beb 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -3,6 +3,7 @@ package config import ( "io/ioutil" "os" + "time" util "github.com/stakater/operator-utils/util" secretsUtil "github.com/stakater/operator-utils/util/secrets" @@ -13,6 +14,8 @@ import ( ) const ( + ErrorRequeueTime = 15 * time.Minute + SlackDefaultSecretName string = "slack-secret" SlackAPITokenSecretKey string = "APIToken" ) diff --git a/pkg/slack/service.go b/pkg/slack/service.go index d3be290..4474fc5 100644 --- a/pkg/slack/service.go +++ b/pkg/slack/service.go @@ -21,7 +21,7 @@ type Service interface { SetTopic(string, string) (*slack.Channel, error) RenameChannel(string, string) (*slack.Channel, error) ArchiveChannel(string) error - InviteUsers(string, []string) error + InviteUsers(string, []string) []error RemoveUsers(string, []string) error GetChannel(string) (*slack.Channel, error) GetUsersInChannel(channelID string) ([]string, error) @@ -176,15 +176,17 @@ func (s *SlackService) GetUsersInChannel(channelID string) ([]string, error) { } // InviteUsers invites users to the slack channel -func (s *SlackService) InviteUsers(channelID string, userEmails []string) error { +func (s *SlackService) InviteUsers(channelID string, userEmails []string) []error { log := s.log.WithValues("channelID", channelID) + var errorlist []error + for _, email := range userEmails { user, err := s.api.GetUserByEmail(email) if err != nil { - log.Error(err, "Error getting user by email") - return err + errorlist = append(errorlist, fmt.Errorf(fmt.Sprintf("Error fetching user by Email %s", email))) + continue } log.V(1).Info("Inviting user to Slack Channel", "userID", user.ID) @@ -192,11 +194,11 @@ func (s *SlackService) InviteUsers(channelID string, userEmails []string) error if err != nil && err.Error() != "already_in_channel" { log.Error(err, "Error Inviting user to channel", "userID", user.ID) - return err + errorlist = append(errorlist, err) } } - return nil + return errorlist } // RemoveUsers remove users from the slack channel @@ -285,8 +287,8 @@ func (s *SlackService) IsChannelUpdated(channel *slackv1alpha1.Channel) (bool, e for _, email := range userEmails { user, err := s.api.GetUserByEmail(email) if err != nil { - log.Error(err, "Error fetching user by Email") - return false, err + log.Error(err, fmt.Sprintf("Error fetching user by Email %s", email)) + continue } found := false diff --git a/pkg/slack/service_test.go b/pkg/slack/service_test.go index feb9fc2..2b0fb07 100644 --- a/pkg/slack/service_test.go +++ b/pkg/slack/service_test.go @@ -1,6 +1,7 @@ package slack import ( + "fmt" "testing" "github.com/stakater/slack-operator/pkg/slack/mock" @@ -78,12 +79,14 @@ func TestSlackService_ArchiveChannel_shouldThrowError_whenChannelNotFound(t *tes func TestSlackService_InviteUsers_shouldSendUserInvites_whenUserExists(t *testing.T) { s := NewMockService(log) - err := s.InviteUsers(mock.PublicConversationID, []string{mock.ExistingUserEmail}) - assert.NoError(t, err) + errs := s.InviteUsers(mock.PublicConversationID, []string{mock.ExistingUserEmail}) + assert.Equal(t, 0, len(errs)) } func TestSlackService_InviteUsers_shouldThowError_whenUserDoesNotExists(t *testing.T) { s := NewMockService(log) - err := s.InviteUsers(mock.PublicConversationID, []string{"spengler@ghostbusters.example.com"}) - assert.EqualError(t, err, "users_not_found") + emailList := []string{"spengler@ghostbusters.example.com"} + errs := s.InviteUsers(mock.PublicConversationID, emailList) + assert.Equal(t, 1, len(errs)) + assert.EqualError(t, errs[0], fmt.Sprintf("Error fetching user by Email %s", emailList[0])) } diff --git a/pkg/util/util.go b/pkg/util/util.go new file mode 100644 index 0000000..d4dca3f --- /dev/null +++ b/pkg/util/util.go @@ -0,0 +1,58 @@ +package pkgutil + +import ( + "context" + "fmt" + "strings" + "time" + + "github.com/stakater/slack-operator/pkg/config" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + + k8sClient "sigs.k8s.io/controller-runtime/pkg/client" + + reconcilerUtil "github.com/stakater/operator-utils/util/reconciler" + slackv1alpha1 "github.com/stakater/slack-operator/api/v1alpha1" +) + +// MapErrorListToError maps multiple errors into a single error +func MapErrorListToError(errs []error) error { + + if len(errs) == 0 { + return nil + } + + errMsg := []string{} + for _, err := range errs { + errMsg = append(errMsg, err.Error()) + } + + return fmt.Errorf(strings.Join(errMsg, "\n")) +} + +func ManageError(ctx context.Context, client k8sClient.Client, channelInstance *slackv1alpha1.Channel, issue error) (ctrl.Result, error) { + + // Base object for patch, which patches using the merge-patch strategy with the given object as base. + channelInstancePatchBase := k8sClient.MergeFrom(channelInstance.DeepCopy()) + + // Update status + channelInstance.Status.Conditions = []metav1.Condition{ + { + Type: "ReconcileError", + LastTransitionTime: metav1.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), time.Now().Hour(), time.Now().Minute(), 0, 0, time.Now().Location()), + Message: issue.Error(), + Reason: reconcilerUtil.FailedReason, + Status: metav1.ConditionTrue, + }, + } + + // Patch status + err := client.Status().Patch(ctx, channelInstance, channelInstancePatchBase) + if err != nil { + return ctrl.Result{}, err + } + + return reconcilerUtil.RequeueAfter(config.ErrorRequeueTime) +} From 3f2b6ad00d012a229b8299c96feb44ec355c12ae Mon Sep 17 00:00:00 2001 From: MuneebAijaz Date: Thu, 10 Mar 2022 16:47:42 +0500 Subject: [PATCH 2/2] Synced var names --- controllers/channel_controller.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/controllers/channel_controller.go b/controllers/channel_controller.go index cc9c92d..b27a9ff 100644 --- a/controllers/channel_controller.go +++ b/controllers/channel_controller.go @@ -185,10 +185,10 @@ func (r *ChannelReconciler) updateSlackChannel(ctx context.Context, channel *sla return reconcilerUtil.ManageError(r.Client, channel, err, false) } - errList := r.SlackService.InviteUsers(channelID, users) - if len(errList) > 0 { + errorlist := r.SlackService.InviteUsers(channelID, users) + if len(errorlist) > 0 { log.Error(err, "Error inviting users to channel") - return pkgutil.ManageError(ctx, r.Client, channel, pkgutil.MapErrorListToError(errList)) + return pkgutil.ManageError(ctx, r.Client, channel, pkgutil.MapErrorListToError(errorlist)) } err = r.SlackService.RemoveUsers(channelID, users)