fix(teammemberships): improve error handling

- do not supress error on getting individual members from SCIM
- report status for inactive/malformed members

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.

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 {

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

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: "[email protected]"}, {ID: "I23456", FirstName: "Jane", LastName: "Doe", Email: "[email protected]"}}
 	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")
+}