From 39a728b3a566ee1c9a23a089778acfa1043d69bd Mon Sep 17 00:00:00 2001 From: Ivo Gosemann Date: Tue, 26 Nov 2024 00:49:48 +0100 Subject: [PATCH] fix(teammemberships): improve error handling - do not supress error on getting individual members from SCIM - report status for inactive/malformed members --- .../v1alpha1/teammembership_types.go | 2 + .../teammembership_updater_controller.go | 24 ++++-- pkg/scim/scim_client.go | 48 ++++++------ pkg/scim/scim_client_test.go | 75 +++++++++++++++++-- 4 files changed, 113 insertions(+), 36 deletions(-) diff --git a/pkg/apis/greenhouse/v1alpha1/teammembership_types.go b/pkg/apis/greenhouse/v1alpha1/teammembership_types.go index b52aee446..eaede82ba 100644 --- a/pkg/apis/greenhouse/v1alpha1/teammembership_types.go +++ b/pkg/apis/greenhouse/v1alpha1/teammembership_types.go @@ -12,6 +12,8 @@ const ( SCIMAccessReadyCondition ConditionType = "SCIMAccessReady" // SCIMAPIUnavailableReason is set when the organization has set SCIMAPIAvailableCondition to false. SCIMAPIUnavailableReason ConditionReason = "SCIMAPIUnavailable" + // SCIMAllMembersValidCondition reflects if all members are valid. It is set to false if there are invalid or inactive members. + SCIMAllMembersValidCondition ConditionType = "SCIMAllMembersValid" ) // User specifies a human person. diff --git a/pkg/controllers/teammembership/teammembership_updater_controller.go b/pkg/controllers/teammembership/teammembership_updater_controller.go index 19b3dfdc4..352997141 100644 --- a/pkg/controllers/teammembership/teammembership_updater_controller.go +++ b/pkg/controllers/teammembership/teammembership_updater_controller.go @@ -5,6 +5,7 @@ package teammembership import ( "context" + "fmt" "time" "github.com/pkg/errors" @@ -45,6 +46,7 @@ var ( exposedConditions = []greenhousev1alpha1.ConditionType{ greenhousev1alpha1.ReadyCondition, greenhousev1alpha1.SCIMAccessReadyCondition, + greenhousev1alpha1.SCIMAllMembersValidCondition, } ) @@ -178,14 +180,14 @@ func (r *TeamMembershipUpdaterController) EnsureCreated(ctx context.Context, obj return ctrl.Result{}, lifecycle.Failed, err } - users, err := r.getUsersFromSCIM(scimClient, team.Spec.MappedIDPGroup) + users, membersValidCondition, err := r.getUsersFromSCIM(scimClient, team.Spec.MappedIDPGroup) if err != nil { log.FromContext(ctx).Info("failed processing team-membership for team", "error", err) teamMembershipStatus.SetConditions(greenhousev1alpha1.FalseCondition(greenhousev1alpha1.SCIMAccessReadyCondition, greenhousev1alpha1.SCIMRequestFailedReason, "")) return ctrl.Result{}, lifecycle.Failed, err } - teamMembershipStatus.SetConditions(greenhousev1alpha1.TrueCondition(greenhousev1alpha1.SCIMAccessReadyCondition, "", "")) + teamMembershipStatus.SetConditions(membersValidCondition, greenhousev1alpha1.TrueCondition(greenhousev1alpha1.SCIMAccessReadyCondition, "", "")) membersCountMetric.With(prometheus.Labels{ "namespace": team.Namespace, @@ -248,13 +250,23 @@ func (r *TeamMembershipUpdaterController) createSCIMClient( return scim.NewScimClient(clientConfig) } -func (r *TeamMembershipUpdaterController) getUsersFromSCIM(scimClient *scim.ScimClient, mappedIDPGroup string) ([]greenhousev1alpha1.User, error) { +func (r *TeamMembershipUpdaterController) getUsersFromSCIM(scimClient *scim.ScimClient, mappedIDPGroup string) ([]greenhousev1alpha1.User, greenhousev1alpha1.Condition, error) { + condition := greenhousev1alpha1.UnknownCondition(greenhousev1alpha1.SCIMAllMembersValidCondition, "", "") members, err := scimClient.GetTeamMembers(mappedIDPGroup) if err != nil { - return nil, err + return nil, condition, err + } + users, inactive, malformed, err := scimClient.GetUsers(members) + if err != nil { + return nil, condition, err + } + if inactive+malformed > 0 { + msg := fmt.Sprintf("SCIM members with issues: %d inactive, %d malformed", inactive, malformed) + condition = greenhousev1alpha1.FalseCondition(greenhousev1alpha1.SCIMAllMembersValidCondition, "", msg) + return users, condition, nil } - users := scimClient.GetUsers(members) - return users, nil + condition = greenhousev1alpha1.TrueCondition(greenhousev1alpha1.SCIMAllMembersValidCondition, "", "") + return users, condition, nil } func initTeamMembershipStatus(teamMembership *greenhousev1alpha1.TeamMembership) greenhousev1alpha1.TeamMembershipStatus { diff --git a/pkg/scim/scim_client.go b/pkg/scim/scim_client.go index 710520751..51a8a48ef 100644 --- a/pkg/scim/scim_client.go +++ b/pkg/scim/scim_client.go @@ -11,11 +11,10 @@ import ( "encoding/json" "errors" "fmt" - "log" "net/http" "net/http/httputil" "net/url" - "sync" + "strings" greenhousev1alpha1 "github.com/cloudoperators/greenhouse/pkg/apis/greenhouse/v1alpha1" ) @@ -24,6 +23,9 @@ const ( groupPathName = "Groups" filterQueryKey = "filter" filterQueryExpressionStub = "displayName eq \"%s\"" + + reasonNotActive = "user is not active" + reasonMalformed = "could not create User from memberResponseBody" ) type Config struct { @@ -109,30 +111,28 @@ func (s *ScimClient) GetTeamMembers(teamMappedIDPGroup string) ([]Member, error) return groupResponseBody.Resources[0].Members, nil } -// Returns a full fledged Users array from the members array -func (s *ScimClient) GetUsers(members []Member) []greenhousev1alpha1.User { - var wg sync.WaitGroup - usersBuffer := make(chan *greenhousev1alpha1.User, len(members)) - wg.Add(len(members)) +// GetUsers returns a list of fully qualified Users, the number of malformed users and the number of inactive users +// On error only the error is set +func (s *ScimClient) GetUsers(members []Member) (users []greenhousev1alpha1.User, inactive int, malformed int, err error) { + users = make([]greenhousev1alpha1.User, 0) + malformed = 0 + inactive = 0 for _, member := range members { - go func(member Member) { - defer wg.Done() - user, err := s.getUser(member) - if err != nil { - log.Printf(`failed getting user: %s`, err) + user, err := s.getUser(member) + if err != nil { + if strings.Contains(err.Error(), reasonNotActive) { + inactive += 1 + continue + } + if strings.Contains(err.Error(), reasonMalformed) { + malformed += 1 + continue } - usersBuffer <- user - }(member) - } - wg.Wait() - users := []greenhousev1alpha1.User{} - for range cap(usersBuffer) { - user := <-usersBuffer - if user != nil { - users = append(users, *user) + return nil, 0, 0, err } + users = append(users, *user) } - return users + return users, inactive, malformed, nil } func (s *ScimClient) getUser(member Member) (*greenhousev1alpha1.User, error) { @@ -163,14 +163,14 @@ func (s *ScimClient) getUser(member Member) (*greenhousev1alpha1.User, error) { } if !memberResponseBody.Active { - return nil, fmt.Errorf("user is not active: %v", memberResponseBody) + return nil, fmt.Errorf("%s: %v", reasonNotActive, memberResponseBody) } user := greenhousev1alpha1.User{} if memberResponseBody.UserName != "" && memberResponseBody.Name.GivenName != "" && memberResponseBody.Name.FamilyName != "" && len(memberResponseBody.Emails) > 0 && memberResponseBody.Emails[0].Value != "" { user = greenhousev1alpha1.User{ID: memberResponseBody.UserName, FirstName: memberResponseBody.Name.GivenName, LastName: memberResponseBody.Name.FamilyName, Email: memberResponseBody.Emails[0].Value} } else { - return nil, fmt.Errorf("could not create User from memberResponseBody: %v", memberResponseBody) + return nil, fmt.Errorf("%s: %v", reasonMalformed, memberResponseBody) } return &user, nil diff --git a/pkg/scim/scim_client_test.go b/pkg/scim/scim_client_test.go index 51be19d02..a766fd72a 100644 --- a/pkg/scim/scim_client_test.go +++ b/pkg/scim/scim_client_test.go @@ -9,7 +9,6 @@ import ( "net/http" "net/http/httptest" "os" - "strings" "testing" "gotest.tools/v3/assert" @@ -32,6 +31,7 @@ func TestScim(t *testing.T) { t.Run("GetUsers:Error from upstream", scTest.TestGetUserErrorResponse) t.Run("GetUsers:Malformed response", scTest.TestGetUserMalformedUserResponse) t.Run("GetUsers:2 valid users, 1 malformed, 1 inactive", scTest.TestGetUsers) + t.Run("GetUsers:2 valid users, 1 inactive, 1 rate-limitted", scTest.TestGetUsersWithRateLimit) } func (sc *ScimClientTests) TestNewScimClient(t *testing.T) { @@ -91,7 +91,7 @@ func (sc *ScimClientTests) TestGetTeamMembersErrorResponse(t *testing.T) { statusCode int } - for _, testCase := range []testCase{{500}, {404}, {403}} { + for _, testCase := range []testCase{{http.StatusInternalServerError}, {http.StatusNotFound}, {http.StatusUnauthorized}} { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, r.URL.Path, "/Groups", "Should correctly call groups path") assert.Equal(t, r.URL.RawQuery, "filter=displayName+eq+%22SOME_IDP_GROUP_NAME%22", "Should correctly call filter parameters") @@ -160,7 +160,7 @@ func (sc *ScimClientTests) TestGetUserErrorResponse(t *testing.T) { statusCode int } - for _, testCase := range []testCase{{500}, {404}, {403}} { + for _, testCase := range []testCase{{http.StatusInternalServerError}, {http.StatusTooManyRequests}, {http.StatusNotFound}, {http.StatusUnauthorized}} { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(testCase.statusCode) w.Header().Add("Content-Type", "application/scim+json") @@ -254,10 +254,73 @@ func (sc *ScimClientTests) TestGetUsers(t *testing.T) { log.SetOutput(os.Stderr) }() - users := scimClient.GetUsers(members) + users, inactive, malformed, err := scimClient.GetUsers(members) + assert.NilError(t, err, "Should not error on getting users") expectedUsers := []greenhousesapv1alpha1.User{{ID: "I12345", FirstName: "John", LastName: "Doe", Email: "john.doe@example.com"}, {ID: "I23456", FirstName: "Jane", LastName: "Doe", Email: "jane.doe@example.com"}} assert.NilError(t, err, "Should not error on getting users") - assert.Equal(t, strings.Contains(buf.String(), "failed getting user: could not create User from memberResponseBody:"), true, "Should log error on malformed user") - assert.Equal(t, strings.Contains(buf.String(), "failed getting user: user is not active:"), true, "Should log error on inactive user") + assert.Equal(t, inactive, 1, "Should have 1 inactive user") + assert.Equal(t, malformed, 1, "Should have 1 inactive user") assert.Equal(t, len(users), len(expectedUsers), "Should not error and not return malformed or inactive user") } + +func (sc *ScimClientTests) TestGetUsersWithRateLimit(t *testing.T) { + // valid user 1 + server1 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Header().Add("Content-Type", "application/scim+json") + _, err := w.Write([]byte(UserResponseBodyMock1)) + if err != nil { + log.Printf("error creating mock server: %s", err) + } + })) + defer server1.Close() + + // rate-limitted request + server2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusTooManyRequests) + w.Header().Add("Content-Type", "application/scim+json") + _, err := w.Write([]byte(`{"error":"some-error"}`)) + if err != nil { + log.Printf("error creating mock server: %s", err) + } + })) + defer server2.Close() + + // valid user 2 + server3 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Header().Add("Content-Type", "application/scim+json") + _, err := w.Write([]byte(UserResponseBodyMock2)) + if err != nil { + log.Printf("error creating mock server: %s", err) + } + })) + defer server3.Close() + + // inactive user + server4 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Header().Add("Content-Type", "application/scim+json") + _, err := w.Write([]byte(InactiveUserResponseBodyMock)) + if err != nil { + log.Printf("error creating mock server: %s", err) + } + })) + defer server4.Close() + scimConfig := Config{"https://some-url", Basic, &BasicAuthConfig{"user", "pw"}} + scimClient, err := NewScimClient(scimConfig) + assert.NilError(t, err) + members := []Member{{server1.URL}, {server2.URL}, {server3.URL}, {server4.URL}} + + var buf bytes.Buffer + log.SetOutput(&buf) + defer func() { + log.SetOutput(os.Stderr) + }() + + users, inactive, malformed, err := scimClient.GetUsers(members) + assert.ErrorContains(t, err, "Too Many Requests", "Should get error if request to upstream returned !200 status code") + assert.Equal(t, inactive, 0, "Should not return inactive users on error") + assert.Equal(t, malformed, 0, "Should not return malformed users on error") + assert.Equal(t, len(users), 0, "Should not return users on error") +}