Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down
2 changes: 0 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
6 changes: 2 additions & 4 deletions internal/bootstrap/gcp/gce_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)

Expand All @@ -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())
Expand Down
58 changes: 7 additions & 51 deletions internal/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,71 +6,27 @@ 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.
func GetSSHKeysFromGitHubTeam(client GitHubClient, org, teamSlug string) (string, error) {
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
}
165 changes: 150 additions & 15 deletions internal/github/github_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,39 +4,174 @@
package github

import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"strings"

"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"

// 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 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) {
members(first: 100, after: $after) {
pageInfo { hasNextPage endCursor }
nodes {
login
publicKeys(first: 20) { totalCount 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 {
TotalCount int `json:"totalCount"`
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"`
}

// 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
}
Comment on lines +104 to +106

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drops previously fetched members. We could return the partial results if an error happens.

If partial results are usable?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning a partial result risks giving a false sense of "it worked". Returning all or nothing is IMHO safer.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine for me


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)
}
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
Comment on lines +105 to +128

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this silently succeed with 0 keys if the team/org is misconfigured? Before it was a hard 404 error, now it would be (nil, nil)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

404 is not what I would expect from a list members endpoint. If the team/org exists, but has no members, it should return an empty list (nil or empty slice usually have the same behavior, but can return an empty slice if that's preferred) and not return an error. If the caller considers the empty list an error, it's the caller's responsibility to flag this to their user (via error or warning log).

Or am I missing an unhandled error somewhere?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with that, I just wanted to know if that was an intentional change

}

// 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
// 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 func() { _ = 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 {
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, "; "))
}
Comment thread
gnarlex marked this conversation as resolved.

return &result, nil
}
Loading