From 93ca5a103932e01a96a2735a0fcd72b01f1f5408 Mon Sep 17 00:00:00 2001 From: Alex Klein Date: Fri, 21 Aug 2026 10:26:23 +0200 Subject: [PATCH 1/2] refac(internal/github): single graphql query to fetch all SSH keys at once --- internal/bootstrap/gcp/gce_test.go | 6 +- internal/github/github.go | 58 ++--------- internal/github/github_client.go | 151 ++++++++++++++++++++++++++--- internal/github/github_test.go | 49 ++++------ internal/github/mocks.go | 117 ++++------------------ 5 files changed, 187 insertions(+), 194 deletions(-) diff --git a/internal/bootstrap/gcp/gce_test.go b/internal/bootstrap/gcp/gce_test.go index 033a11aab..352608bfb 100644 --- a/internal/bootstrap/gcp/gce_test.go +++ b/internal/bootstrap/gcp/gce_test.go @@ -12,7 +12,6 @@ import ( "github.com/codesphere-cloud/oms/internal/bootstrap/gcp" "github.com/codesphere-cloud/oms/internal/github" "github.com/codesphere-cloud/oms/internal/util" - gh "github.com/google/go-github/v74/github" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/stretchr/testify/mock" @@ -678,8 +677,7 @@ var _ = Describe("GCE", func() { csEnv.GitHubTeamSlug = "dev" }) It("fetches GitHub team keys", func() { - mockGitHubClient.EXPECT().ListTeamMembersBySlug(mock.Anything, csEnv.GitHubTeamOrg, csEnv.GitHubTeamSlug, mock.Anything).Return([]*gh.User{{Login: gh.Ptr("alice")}}, nil).Maybe() - mockGitHubClient.EXPECT().ListUserKeys(mock.Anything, "alice").Return([]*gh.Key{{Key: gh.Ptr("ssh-rsa AAALICE...")}}, nil).Maybe() + mockGitHubClient.EXPECT().GetTeamMemberSSHKeys(mock.Anything, csEnv.GitHubTeamOrg, csEnv.GitHubTeamSlug).Return([]github.TeamMemberKeys{{Login: "alice", Keys: []string{"ssh-rsa AAALICE..."}}}, nil).Maybe() ipResp := makeRunningInstance("10.0.0.x", "1.2.3.x") mockGetInstanceNotFoundThenRunning(gc, csEnv.ProjectID, csEnv.Zone, ipResp, 8) @@ -703,7 +701,7 @@ var _ = Describe("GCE", func() { It("fails when GitHub client fails to list team members", func() { gc.EXPECT().GetInstance(csEnv.ProjectID, csEnv.Zone, mock.Anything).Return(nil, grpcstatus.Errorf(codes.NotFound, "not found")).Maybe() - mockGitHubClient.EXPECT().ListTeamMembersBySlug(mock.Anything, csEnv.GitHubTeamOrg, csEnv.GitHubTeamSlug, mock.Anything).Return(nil, fmt.Errorf("list members error")).Maybe() + mockGitHubClient.EXPECT().GetTeamMemberSSHKeys(mock.Anything, csEnv.GitHubTeamOrg, csEnv.GitHubTeamSlug).Return(nil, fmt.Errorf("list members error")).Maybe() err := bs.EnsureComputeInstances() Expect(err).To(HaveOccurred()) diff --git a/internal/github/github.go b/internal/github/github.go index be6cc4b08..540cc5109 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -6,8 +6,6 @@ package github import ( "context" "fmt" - - "github.com/google/go-github/v74/github" ) // GetSSHKeysFromGitHubTeam fetches the public SSH keys of all members of the specified GitHub team and formats them for inclusion in instance metadata. @@ -15,62 +13,20 @@ func GetSSHKeysFromGitHubTeam(client GitHubClient, org, teamSlug string) (string if org == "" || teamSlug == "" { return "", fmt.Errorf("GitHub team slug and org must be specified to fetch SSH keys from GitHub team") } - allKeys := "" - allMembers, err := listAllGitHubTeamMembers(client, org, teamSlug) + members, err := client.GetTeamMemberSSHKeys(context.Background(), org, teamSlug) if err != nil { - return "", fmt.Errorf("failed to list GitHub team members: %w", err) + return "", fmt.Errorf("failed to fetch SSH keys from GitHub team: %w", err) } - fmt.Printf("Found %d members in team '%s'\n", len(allMembers), teamSlug) + fmt.Printf("Found %d members in team '%s'\n", len(members), teamSlug) - for _, user := range allMembers { - username := user.GetLogin() - keys, err := client.ListUserKeys(context.Background(), username) - if err != nil { - fmt.Printf("Could not fetch keys for %s: %v\n", username, err) - continue - } - - for _, key := range keys { - allKeys += fmt.Sprintf("root:%s %sroot\nubuntu:%s %subuntu\n", key.GetKey(), username, key.GetKey(), username) + allKeys := "" + for _, member := range members { + for _, key := range member.Keys { + allKeys += fmt.Sprintf("root:%s %sroot\nubuntu:%s %subuntu\n", key, member.Login, key, member.Login) } } return allKeys, nil } - -// listAllGitHubTeamMembers retrieves all members of the specified GitHub team, handling pagination to ensure all members are fetched. -func listAllGitHubTeamMembers(client GitHubClient, org string, teamSlug string) ([]*github.User, error) { - perPage := 100 - page := 1 - var allMembers []*github.User - - for { - opts := &github.TeamListTeamMembersOptions{ - ListOptions: github.ListOptions{ - Page: page, - PerPage: perPage, - }, - } - - members, err := client.ListTeamMembersBySlug(context.Background(), org, teamSlug, opts) - if err != nil { - return nil, fmt.Errorf("failed to fetch team members from GitHub: %w", err) - } - - if len(members) == 0 { - break - } - - allMembers = append(allMembers, members...) - - if len(members) < perPage { - break - } - - page++ - } - - return allMembers, nil -} diff --git a/internal/github/github_client.go b/internal/github/github_client.go index 0e25e8a8a..bb8ed682c 100644 --- a/internal/github/github_client.go +++ b/internal/github/github_client.go @@ -4,39 +4,160 @@ package github import ( + "bytes" "context" + "encoding/json" + "fmt" + "io" + "net/http" - "github.com/google/go-github/v74/github" "golang.org/x/oauth2" ) -// GitHubClient abstracts the GitHub API calls used to fetch team SSH keys. +const githubGraphQLEndpoint = "https://api.github.com/graphql" + +// teamMemberSSHKeysQuery fetches every member of a team together with their public SSH keys in a +// single request. Members are paginated with the $after cursor; publicKeys are assumed to fit in +// the first page (a user is extremely unlikely to have more than 20 keys). +const teamMemberSSHKeysQuery = `query($org: String!, $team: String!, $after: String) { + organization(login: $org) { + team(slug: $team) { + members(first: 100, after: $after) { + pageInfo { hasNextPage endCursor } + nodes { + login + publicKeys(first: 20) { nodes { key } } + } + } + } + } +}` + +// TeamMemberKeys holds a team member's login and their public SSH keys. +type TeamMemberKeys struct { + Login string + Keys []string +} + +// GitHubClient abstracts the GitHub API call used to fetch team SSH keys. // //mockery:generate: true type GitHubClient interface { - ListTeamMembersBySlug(ctx context.Context, org, teamSlug string, opts *github.TeamListTeamMembersOptions) ([]*github.User, error) - ListUserKeys(ctx context.Context, username string) ([]*github.Key, error) + GetTeamMemberSSHKeys(ctx context.Context, org, teamSlug string) ([]TeamMemberKeys, error) } type RealGitHubClient struct { - client *github.Client + httpClient *http.Client + endpoint string } // NewGitHubClient creates a new RealGitHubClient with the provided OAuth token. func NewGitHubClient(ctx context.Context, token string) *RealGitHubClient { ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token}) - tc := oauth2.NewClient(ctx, ts) - return &RealGitHubClient{client: github.NewClient(tc)} + return &RealGitHubClient{ + httpClient: oauth2.NewClient(ctx, ts), + endpoint: githubGraphQLEndpoint, + } } -// ListTeamMembersBySlug lists the members of a GitHub team identified by its slug. -func (c *RealGitHubClient) ListTeamMembersBySlug(ctx context.Context, org, teamSlug string, opts *github.TeamListTeamMembersOptions) ([]*github.User, error) { - members, _, err := c.client.Teams.ListTeamMembersBySlug(ctx, org, teamSlug, opts) - return members, err +// graphQLResponse mirrors the shape of the teamMemberSSHKeysQuery response. +type graphQLResponse struct { + Data struct { + Organization struct { + Team struct { + Members struct { + PageInfo struct { + HasNextPage bool `json:"hasNextPage"` + EndCursor string `json:"endCursor"` + } `json:"pageInfo"` + Nodes []struct { + Login string `json:"login"` + PublicKeys struct { + Nodes []struct { + Key string `json:"key"` + } `json:"nodes"` + } `json:"publicKeys"` + } `json:"nodes"` + } `json:"members"` + } `json:"team"` + } `json:"organization"` + } `json:"data"` + Errors []struct { + Message string `json:"message"` + } `json:"errors"` } -// ListUserKeys lists the public SSH keys of a GitHub user. -func (c *RealGitHubClient) ListUserKeys(ctx context.Context, username string) ([]*github.Key, error) { - keys, _, err := c.client.Users.ListKeys(ctx, username, nil) - return keys, err +// GetTeamMemberSSHKeys fetches all members of the team and their public SSH keys via the GitHub +// GraphQL API, following member pagination until every member has been retrieved. +func (c *RealGitHubClient) GetTeamMemberSSHKeys(ctx context.Context, org, teamSlug string) ([]TeamMemberKeys, error) { + var members []TeamMemberKeys + var after *string + + for { + resp, err := c.queryTeamMembers(ctx, org, teamSlug, after) + if err != nil { + return nil, err + } + + team := resp.Data.Organization.Team + for _, node := range team.Members.Nodes { + keys := make([]string, 0, len(node.PublicKeys.Nodes)) + for _, k := range node.PublicKeys.Nodes { + keys = append(keys, k.Key) + } + members = append(members, TeamMemberKeys{Login: node.Login, Keys: keys}) + } + + if !team.Members.PageInfo.HasNextPage { + break + } + cursor := team.Members.PageInfo.EndCursor + after = &cursor + } + + return members, nil +} + +// queryTeamMembers executes a single page of the teamMemberSSHKeysQuery. +func (c *RealGitHubClient) queryTeamMembers(ctx context.Context, org, teamSlug string, after *string) (*graphQLResponse, error) { + variables := map[string]any{"org": org, "team": teamSlug} + if after != nil { + variables["after"] = *after + } + + body, err := json.Marshal(map[string]any{"query": teamMemberSSHKeysQuery, "variables": variables}) + if err != nil { + return nil, fmt.Errorf("failed to marshal GraphQL request: %w", err) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.endpoint, bytes.NewReader(body)) + if err != nil { + return nil, fmt.Errorf("failed to create GraphQL request: %w", err) + } + req.Header.Set("Content-Type", "application/json") + + httpResp, err := c.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("failed to execute GraphQL request: %w", err) + } + defer httpResp.Body.Close() + + respBody, err := io.ReadAll(httpResp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read GraphQL response: %w", err) + } + + if httpResp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("GraphQL request failed with status %d: %s", httpResp.StatusCode, string(respBody)) + } + + var result graphQLResponse + if err := json.Unmarshal(respBody, &result); err != nil { + return nil, fmt.Errorf("failed to unmarshal GraphQL response: %w", err) + } + if len(result.Errors) > 0 { + return nil, fmt.Errorf("GraphQL query returned errors: %s", result.Errors[0].Message) + } + + return &result, nil } diff --git a/internal/github/github_test.go b/internal/github/github_test.go index f4357c848..cefe81630 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -10,8 +10,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/stretchr/testify/mock" - - gh "github.com/google/go-github/v74/github" ) var _ = Describe("Github", func() { @@ -30,8 +28,9 @@ var _ = Describe("Github", func() { }) It("fetches GitHub team keys", func() { - mockGitHubClient.EXPECT().ListTeamMembersBySlug(mock.Anything, org, teamSlug, mock.Anything).Return([]*gh.User{{Login: gh.Ptr("alice")}}, nil).Once() - mockGitHubClient.EXPECT().ListUserKeys(mock.Anything, "alice").Return([]*gh.Key{{Key: gh.Ptr("ssh-rsa AAALICE...")}}, nil).Once() + mockGitHubClient.EXPECT().GetTeamMemberSSHKeys(mock.Anything, org, teamSlug).Return([]github.TeamMemberKeys{ + {Login: "alice", Keys: []string{"ssh-rsa AAALICE..."}}, + }, nil).Once() keys, err := github.GetSSHKeysFromGitHubTeam(mockGitHubClient, org, teamSlug) Expect(err).ToNot(HaveOccurred()) @@ -39,20 +38,21 @@ var _ = Describe("Github", func() { Expect(keys).To(ContainSubstring("ubuntu:ssh-rsa AAALICE... alice")) }) - Context("when fetching team members fails", func() { + Context("when fetching team member keys fails", func() { It("returns an error", func() { - mockGitHubClient.EXPECT().ListTeamMembersBySlug(mock.Anything, org, teamSlug, mock.Anything).Return(nil, fmt.Errorf("GitHub API error")).Once() + mockGitHubClient.EXPECT().GetTeamMemberSSHKeys(mock.Anything, org, teamSlug).Return(nil, fmt.Errorf("GitHub API error")).Once() keys, err := github.GetSSHKeysFromGitHubTeam(mockGitHubClient, org, teamSlug) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("failed to list GitHub team members")) + Expect(err.Error()).To(ContainSubstring("failed to fetch SSH keys from GitHub team")) Expect(keys).To(BeEmpty()) }) }) - Context("when fetching user keys fails", func() { - It("skips the user and continues", func() { - mockGitHubClient.EXPECT().ListTeamMembersBySlug(mock.Anything, org, teamSlug, mock.Anything).Return([]*gh.User{{Login: gh.Ptr("alice")}}, nil).Once() - mockGitHubClient.EXPECT().ListUserKeys(mock.Anything, "alice").Return(nil, fmt.Errorf("GitHub API error")).Once() + Context("when a member has no keys", func() { + It("skips the member and continues", func() { + mockGitHubClient.EXPECT().GetTeamMemberSSHKeys(mock.Anything, org, teamSlug).Return([]github.TeamMemberKeys{ + {Login: "alice", Keys: nil}, + }, nil).Once() keys, err := github.GetSSHKeysFromGitHubTeam(mockGitHubClient, org, teamSlug) Expect(err).ToNot(HaveOccurred()) Expect(keys).To(BeEmpty()) @@ -61,31 +61,24 @@ var _ = Describe("Github", func() { Context("when team has no members", func() { It("returns an empty string", func() { - mockGitHubClient.EXPECT().ListTeamMembersBySlug(mock.Anything, org, teamSlug, mock.Anything).Return([]*gh.User{}, nil).Once() + mockGitHubClient.EXPECT().GetTeamMemberSSHKeys(mock.Anything, org, teamSlug).Return([]github.TeamMemberKeys{}, nil).Once() keys, err := github.GetSSHKeysFromGitHubTeam(mockGitHubClient, org, teamSlug) Expect(err).ToNot(HaveOccurred()) Expect(keys).To(BeEmpty()) }) }) - Context("when team has more than 100 members", func() { - It("handles pagination correctly", func() { - // Simulate 150 members to trigger pagination - membersPage1 := make([]*gh.User, 100) - for i := 0; i < 100; i++ { - membersPage1[i] = &gh.User{Login: gh.Ptr(fmt.Sprintf("user%d", i+1))} - } - membersPage2 := make([]*gh.User, 50) - for i := 0; i < 50; i++ { - membersPage2[i] = &gh.User{Login: gh.Ptr(fmt.Sprintf("user%d", i+101))} + Context("when the team has many members", func() { + It("formats keys for every member", func() { + members := make([]github.TeamMemberKeys, 150) + for i := 0; i < 150; i++ { + members[i] = github.TeamMemberKeys{ + Login: fmt.Sprintf("user%d", i+1), + Keys: []string{fmt.Sprintf("ssh-rsa AAAUSER%d...", i+1)}, + } } - mockGitHubClient.EXPECT().ListTeamMembersBySlug(mock.Anything, org, teamSlug, mock.Anything).Return(membersPage1, nil).Once() - mockGitHubClient.EXPECT().ListTeamMembersBySlug(mock.Anything, org, teamSlug, mock.Anything).Return(membersPage2, nil).Once() - - for i := 1; i <= 150; i++ { - mockGitHubClient.EXPECT().ListUserKeys(mock.Anything, fmt.Sprintf("user%d", i)).Return([]*gh.Key{{Key: gh.Ptr(fmt.Sprintf("ssh-rsa AAAUSER%d...", i))}}, nil).Once() - } + mockGitHubClient.EXPECT().GetTeamMemberSSHKeys(mock.Anything, org, teamSlug).Return(members, nil).Once() keys, err := github.GetSSHKeysFromGitHubTeam(mockGitHubClient, org, teamSlug) Expect(err).ToNot(HaveOccurred()) diff --git a/internal/github/mocks.go b/internal/github/mocks.go index 228efba51..9e746c75e 100644 --- a/internal/github/mocks.go +++ b/internal/github/mocks.go @@ -6,7 +6,6 @@ package github import ( "context" - "github.com/google/go-github/v74/github" mock "github.com/stretchr/testify/mock" ) @@ -37,49 +36,48 @@ func (_m *MockGitHubClient) EXPECT() *MockGitHubClient_Expecter { return &MockGitHubClient_Expecter{mock: &_m.Mock} } -// ListTeamMembersBySlug provides a mock function for the type MockGitHubClient -func (_mock *MockGitHubClient) ListTeamMembersBySlug(ctx context.Context, org string, teamSlug string, opts *github.TeamListTeamMembersOptions) ([]*github.User, error) { - ret := _mock.Called(ctx, org, teamSlug, opts) +// GetTeamMemberSSHKeys provides a mock function for the type MockGitHubClient +func (_mock *MockGitHubClient) GetTeamMemberSSHKeys(ctx context.Context, org string, teamSlug string) ([]TeamMemberKeys, error) { + ret := _mock.Called(ctx, org, teamSlug) if len(ret) == 0 { - panic("no return value specified for ListTeamMembersBySlug") + panic("no return value specified for GetTeamMemberSSHKeys") } - var r0 []*github.User + var r0 []TeamMemberKeys var r1 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string, string, *github.TeamListTeamMembersOptions) ([]*github.User, error)); ok { - return returnFunc(ctx, org, teamSlug, opts) + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) ([]TeamMemberKeys, error)); ok { + return returnFunc(ctx, org, teamSlug) } - if returnFunc, ok := ret.Get(0).(func(context.Context, string, string, *github.TeamListTeamMembersOptions) []*github.User); ok { - r0 = returnFunc(ctx, org, teamSlug, opts) + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) []TeamMemberKeys); ok { + r0 = returnFunc(ctx, org, teamSlug) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).([]*github.User) + r0 = ret.Get(0).([]TeamMemberKeys) } } - if returnFunc, ok := ret.Get(1).(func(context.Context, string, string, *github.TeamListTeamMembersOptions) error); ok { - r1 = returnFunc(ctx, org, teamSlug, opts) + if returnFunc, ok := ret.Get(1).(func(context.Context, string, string) error); ok { + r1 = returnFunc(ctx, org, teamSlug) } else { r1 = ret.Error(1) } return r0, r1 } -// MockGitHubClient_ListTeamMembersBySlug_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ListTeamMembersBySlug' -type MockGitHubClient_ListTeamMembersBySlug_Call struct { +// MockGitHubClient_GetTeamMemberSSHKeys_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetTeamMemberSSHKeys' +type MockGitHubClient_GetTeamMemberSSHKeys_Call struct { *mock.Call } -// ListTeamMembersBySlug is a helper method to define mock.On call +// GetTeamMemberSSHKeys is a helper method to define mock.On call // - ctx context.Context // - org string // - teamSlug string -// - opts *github.TeamListTeamMembersOptions -func (_e *MockGitHubClient_Expecter) ListTeamMembersBySlug(ctx any, org any, teamSlug any, opts any) *MockGitHubClient_ListTeamMembersBySlug_Call { - return &MockGitHubClient_ListTeamMembersBySlug_Call{Call: _e.mock.On("ListTeamMembersBySlug", ctx, org, teamSlug, opts)} +func (_e *MockGitHubClient_Expecter) GetTeamMemberSSHKeys(ctx any, org any, teamSlug any) *MockGitHubClient_GetTeamMemberSSHKeys_Call { + return &MockGitHubClient_GetTeamMemberSSHKeys_Call{Call: _e.mock.On("GetTeamMemberSSHKeys", ctx, org, teamSlug)} } -func (_c *MockGitHubClient_ListTeamMembersBySlug_Call) Run(run func(ctx context.Context, org string, teamSlug string, opts *github.TeamListTeamMembersOptions)) *MockGitHubClient_ListTeamMembersBySlug_Call { +func (_c *MockGitHubClient_GetTeamMemberSSHKeys_Call) Run(run func(ctx context.Context, org string, teamSlug string)) *MockGitHubClient_GetTeamMemberSSHKeys_Call { _c.Call.Run(func(args mock.Arguments) { var arg0 context.Context if args[0] != nil { @@ -93,94 +91,21 @@ func (_c *MockGitHubClient_ListTeamMembersBySlug_Call) Run(run func(ctx context. if args[2] != nil { arg2 = args[2].(string) } - var arg3 *github.TeamListTeamMembersOptions - if args[3] != nil { - arg3 = args[3].(*github.TeamListTeamMembersOptions) - } run( arg0, arg1, arg2, - arg3, - ) - }) - return _c -} - -func (_c *MockGitHubClient_ListTeamMembersBySlug_Call) Return(users []*github.User, err error) *MockGitHubClient_ListTeamMembersBySlug_Call { - _c.Call.Return(users, err) - return _c -} - -func (_c *MockGitHubClient_ListTeamMembersBySlug_Call) RunAndReturn(run func(ctx context.Context, org string, teamSlug string, opts *github.TeamListTeamMembersOptions) ([]*github.User, error)) *MockGitHubClient_ListTeamMembersBySlug_Call { - _c.Call.Return(run) - return _c -} - -// ListUserKeys provides a mock function for the type MockGitHubClient -func (_mock *MockGitHubClient) ListUserKeys(ctx context.Context, username string) ([]*github.Key, error) { - ret := _mock.Called(ctx, username) - - if len(ret) == 0 { - panic("no return value specified for ListUserKeys") - } - - var r0 []*github.Key - var r1 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string) ([]*github.Key, error)); ok { - return returnFunc(ctx, username) - } - if returnFunc, ok := ret.Get(0).(func(context.Context, string) []*github.Key); ok { - r0 = returnFunc(ctx, username) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).([]*github.Key) - } - } - if returnFunc, ok := ret.Get(1).(func(context.Context, string) error); ok { - r1 = returnFunc(ctx, username) - } else { - r1 = ret.Error(1) - } - return r0, r1 -} - -// MockGitHubClient_ListUserKeys_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ListUserKeys' -type MockGitHubClient_ListUserKeys_Call struct { - *mock.Call -} - -// ListUserKeys is a helper method to define mock.On call -// - ctx context.Context -// - username string -func (_e *MockGitHubClient_Expecter) ListUserKeys(ctx any, username any) *MockGitHubClient_ListUserKeys_Call { - return &MockGitHubClient_ListUserKeys_Call{Call: _e.mock.On("ListUserKeys", ctx, username)} -} - -func (_c *MockGitHubClient_ListUserKeys_Call) Run(run func(ctx context.Context, username string)) *MockGitHubClient_ListUserKeys_Call { - _c.Call.Run(func(args mock.Arguments) { - var arg0 context.Context - if args[0] != nil { - arg0 = args[0].(context.Context) - } - var arg1 string - if args[1] != nil { - arg1 = args[1].(string) - } - run( - arg0, - arg1, ) }) return _c } -func (_c *MockGitHubClient_ListUserKeys_Call) Return(keys []*github.Key, err error) *MockGitHubClient_ListUserKeys_Call { - _c.Call.Return(keys, err) +func (_c *MockGitHubClient_GetTeamMemberSSHKeys_Call) Return(teamMemberKeys []TeamMemberKeys, err error) *MockGitHubClient_GetTeamMemberSSHKeys_Call { + _c.Call.Return(teamMemberKeys, err) return _c } -func (_c *MockGitHubClient_ListUserKeys_Call) RunAndReturn(run func(ctx context.Context, username string) ([]*github.Key, error)) *MockGitHubClient_ListUserKeys_Call { +func (_c *MockGitHubClient_GetTeamMemberSSHKeys_Call) RunAndReturn(run func(ctx context.Context, org string, teamSlug string) ([]TeamMemberKeys, error)) *MockGitHubClient_GetTeamMemberSSHKeys_Call { _c.Call.Return(run) return _c } From 6d5824586d82445162ac9c69ea305a8a6a544227 Mon Sep 17 00:00:00 2001 From: Alex Klein Date: Fri, 21 Aug 2026 17:13:21 +0200 Subject: [PATCH 2/2] address feedback and lint failure --- go.mod | 1 - go.sum | 2 - internal/github/github_client.go | 26 +++- internal/portal/http_retry_internal_test.go | 150 ++++++++++++++++++++ 4 files changed, 170 insertions(+), 9 deletions(-) create mode 100644 internal/portal/http_retry_internal_test.go diff --git a/go.mod b/go.mod index 66efba610..bb4875014 100644 --- a/go.mod +++ b/go.mod @@ -62,7 +62,6 @@ require ( require ( github.com/DATA-DOG/go-sqlmock v1.5.2 github.com/argoproj/argo-cd/v3 v3.5.1 - github.com/google/go-github/v74 v74.0.0 github.com/lib/pq v1.12.3 github.com/rook/rook/pkg/apis v0.0.0-20260818165109-3fc7fa0ca1cb ) diff --git a/go.sum b/go.sum index a1367df29..a2c8e1136 100644 --- a/go.sum +++ b/go.sum @@ -3801,8 +3801,6 @@ github.com/google/go-containerregistry v0.21.7 h1:/vPFuVXDjtFREsVArW+0h1CIl5urnO github.com/google/go-containerregistry v0.21.7/go.mod h1:kjSbt7/zMsKLWfnHrIvKvhXHUw91jbe9DNjPPJ32gXE= github.com/google/go-github/v69 v69.2.0 h1:wR+Wi/fN2zdUx9YxSmYE0ktiX9IAR/BeePzeaUUbEHE= github.com/google/go-github/v69 v69.2.0/go.mod h1:xne4jymxLR6Uj9b7J7PyTpkMYstEMMwGZa0Aehh1azM= -github.com/google/go-github/v74 v74.0.0 h1:yZcddTUn8DPbj11GxnMrNiAnXH14gNs559AsUpNpPgM= -github.com/google/go-github/v74 v74.0.0/go.mod h1:ubn/YdyftV80VPSI26nSJvaEsTOnsjrxG3o9kJhcyak= github.com/google/go-github/v86 v86.0.0 h1:S/6aANJhwRm8EQmGKVML3j41yq0h2BsTP8FnDkO7kcA= github.com/google/go-github/v86 v86.0.0/go.mod h1:zKv1l4SwDXNFMGByi2FWkq71KwSXqj/eQRZuqtmcot8= github.com/google/go-github/v88 v88.0.0 h1:dZA9IKkPK1eXZj4ypngnpRj5FwdpTv4whix2PrQMP7M= diff --git a/internal/github/github_client.go b/internal/github/github_client.go index bb8ed682c..12188c3f9 100644 --- a/internal/github/github_client.go +++ b/internal/github/github_client.go @@ -10,15 +10,20 @@ import ( "fmt" "io" "net/http" + "strings" "golang.org/x/oauth2" ) const githubGraphQLEndpoint = "https://api.github.com/graphql" +// publicKeysPageSize is how many public SSH keys we request per team member. A user is very +// unlikely to have this many keys; totalCount lets us detect and log the rare case where they do. +const publicKeysPageSize = 20 + // teamMemberSSHKeysQuery fetches every member of a team together with their public SSH keys in a -// single request. Members are paginated with the $after cursor; publicKeys are assumed to fit in -// the first page (a user is extremely unlikely to have more than 20 keys). +// single request. Members are paginated with the $after cursor; publicKeys are fetched in a single +// page of publicKeysPageSize and totalCount is used to detect truncation. const teamMemberSSHKeysQuery = `query($org: String!, $team: String!, $after: String) { organization(login: $org) { team(slug: $team) { @@ -26,7 +31,7 @@ const teamMemberSSHKeysQuery = `query($org: String!, $team: String!, $after: Str pageInfo { hasNextPage endCursor } nodes { login - publicKeys(first: 20) { nodes { key } } + publicKeys(first: 20) { totalCount nodes { key } } } } } @@ -73,7 +78,8 @@ type graphQLResponse struct { Nodes []struct { Login string `json:"login"` PublicKeys struct { - Nodes []struct { + TotalCount int `json:"totalCount"` + Nodes []struct { Key string `json:"key"` } `json:"nodes"` } `json:"publicKeys"` @@ -101,6 +107,10 @@ func (c *RealGitHubClient) GetTeamMemberSSHKeys(ctx context.Context, org, teamSl team := resp.Data.Organization.Team for _, node := range team.Members.Nodes { + if node.PublicKeys.TotalCount > publicKeysPageSize { + fmt.Printf("User %s has %d public keys but only the first %d were fetched\n", + node.Login, node.PublicKeys.TotalCount, publicKeysPageSize) + } keys := make([]string, 0, len(node.PublicKeys.Nodes)) for _, k := range node.PublicKeys.Nodes { keys = append(keys, k.Key) @@ -140,7 +150,7 @@ func (c *RealGitHubClient) queryTeamMembers(ctx context.Context, org, teamSlug s if err != nil { return nil, fmt.Errorf("failed to execute GraphQL request: %w", err) } - defer httpResp.Body.Close() + defer func() { _ = httpResp.Body.Close() }() respBody, err := io.ReadAll(httpResp.Body) if err != nil { @@ -156,7 +166,11 @@ func (c *RealGitHubClient) queryTeamMembers(ctx context.Context, org, teamSlug s return nil, fmt.Errorf("failed to unmarshal GraphQL response: %w", err) } if len(result.Errors) > 0 { - return nil, fmt.Errorf("GraphQL query returned errors: %s", result.Errors[0].Message) + msgs := make([]string, len(result.Errors)) + for i, e := range result.Errors { + msgs[i] = e.Message + } + return nil, fmt.Errorf("GraphQL query returned errors: %s", strings.Join(msgs, "; ")) } return &result, nil diff --git a/internal/portal/http_retry_internal_test.go b/internal/portal/http_retry_internal_test.go new file mode 100644 index 000000000..d1fa9f66f --- /dev/null +++ b/internal/portal/http_retry_internal_test.go @@ -0,0 +1,150 @@ +// Copyright (c) Codesphere Inc. +// SPDX-License-Identifier: Apache-2.0 + +package portal + +import ( + "errors" + "io" + "net/http" + "strings" + "testing" + "time" +) + +// stubClient returns queued responses in order, one per Do call. +type stubClient struct { + responses []stubResponse + calls int +} + +type stubResponse struct { + resp *http.Response + err error +} + +func (s *stubClient) Do(*http.Request) (*http.Response, error) { + i := s.calls + s.calls++ + if i >= len(s.responses) { + return nil, errors.New("unexpected extra call") + } + return s.responses[i].resp, s.responses[i].err +} + +func okResp(status int, body string) *http.Response { + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(strings.NewReader(body)), + } +} + +func newTestRetryingClient(inner HttpClient) *retryingClient { + return &retryingClient{ + inner: inner, + attempts: maxHttpAttempts, + baseWait: 0, + sleep: func(time.Duration) {}, + } +} + +func mustGetReq(t *testing.T) *http.Request { + t.Helper() + req, err := http.NewRequest(http.MethodGet, "https://portal.example.com/api/packages", nil) + if err != nil { + t.Fatalf("failed to build request: %v", err) + } + return req +} + +func TestRetryingClient_RetriesTransportErrorThenSucceeds(t *testing.T) { + stub := &stubClient{responses: []stubResponse{ + {nil, errors.New("unexpected EOF")}, + {okResp(http.StatusOK, "ok"), nil}, + }} + c := newTestRetryingClient(stub) + + resp, err := c.Do(mustGetReq(t)) + if err != nil { + t.Fatalf("expected success after retry, got error: %v", err) + } + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected 200, got %d", resp.StatusCode) + } + if stub.calls != 2 { + t.Fatalf("expected 2 calls, got %d", stub.calls) + } +} + +func TestRetryingClient_RetriesRetryable5xxThenSucceeds(t *testing.T) { + stub := &stubClient{responses: []stubResponse{ + {okResp(http.StatusBadGateway, "bad gateway"), nil}, + {okResp(http.StatusOK, "ok"), nil}, + }} + c := newTestRetryingClient(stub) + + resp, err := c.Do(mustGetReq(t)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected 200, got %d", resp.StatusCode) + } + if stub.calls != 2 { + t.Fatalf("expected 2 calls, got %d", stub.calls) + } +} + +func TestRetryingClient_DoesNotRetryNon5xxStatus(t *testing.T) { + stub := &stubClient{responses: []stubResponse{ + {okResp(http.StatusBadRequest, "bad request"), nil}, + }} + c := newTestRetryingClient(stub) + + resp, err := c.Do(mustGetReq(t)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("expected 400 to pass through, got %d", resp.StatusCode) + } + if stub.calls != 1 { + t.Fatalf("expected 1 call (no retry), got %d", stub.calls) + } +} + +func TestRetryingClient_DoesNotRetryNonIdempotentMethod(t *testing.T) { + req, err := http.NewRequest(http.MethodPost, "https://portal.example.com/keys", strings.NewReader("body")) + if err != nil { + t.Fatalf("failed to build request: %v", err) + } + stub := &stubClient{responses: []stubResponse{ + {nil, errors.New("unexpected EOF")}, + }} + c := newTestRetryingClient(stub) + + _, doErr := c.Do(req) + if doErr == nil { + t.Fatal("expected the transport error to propagate for a POST") + } + if stub.calls != 1 { + t.Fatalf("expected 1 call (POST not retried), got %d", stub.calls) + } +} + +func TestRetryingClient_ExhaustsAttemptsAndReturnsLastError(t *testing.T) { + stub := &stubClient{responses: []stubResponse{ + {nil, errors.New("unexpected EOF")}, + {nil, errors.New("unexpected EOF")}, + {nil, errors.New("unexpected EOF")}, + }} + c := newTestRetryingClient(stub) + + _, err := c.Do(mustGetReq(t)) + if err == nil { + t.Fatal("expected an error after exhausting attempts") + } + if stub.calls != maxHttpAttempts { + t.Fatalf("expected %d calls, got %d", maxHttpAttempts, stub.calls) + } +}