From 2997571a6f8175fe611702006532c4d40ba66b1c Mon Sep 17 00:00:00 2001 From: Igor Rodionov Date: Thu, 20 Apr 2017 23:52:24 +0600 Subject: [PATCH] Use Pager for GitHub API calls (#22) * Use pager for api call * Fix bug with teams * Did right refactoring * Did right refactoring * Fix some tests * Added right support of paginators * Address comments in PR * Fix etcd tests * Made tests to check pagination api calls * Added check for team members > 1 --- .github-authorized-keys-demo.default.yml | 1 + .travis.yml | 3 +- api/github.go | 116 ++++++++++++++++++++--- api/github_test.go | 7 +- api/linux_test.go | 31 +++--- key_storages/github_keys.go | 22 ++--- 6 files changed, 131 insertions(+), 49 deletions(-) diff --git a/.github-authorized-keys-demo.default.yml b/.github-authorized-keys-demo.default.yml index af24c5c..b1e66e2 100644 --- a/.github-authorized-keys-demo.default.yml +++ b/.github-authorized-keys-demo.default.yml @@ -1,3 +1,4 @@ + github_api_token: {token api} github_organization: {organization} github_team: {team} \ No newline at end of file diff --git a/.travis.yml b/.travis.yml index 5e0736b..958ece3 100644 --- a/.travis.yml +++ b/.travis.yml @@ -11,7 +11,6 @@ env: - TEST_GITHUB_TEAM=${GITHUB_TEAM} - TEST_GITHUB_TEAM_ID=${GITHUB_TEAM_ID} - TEST_GITHUB_USER=${GITHUB_USER} - - TEST_ETCD_ENDPOINT=http://${ETCD_IP}:2379 services: - docker @@ -24,6 +23,8 @@ script: - |- export ETCD_IP=$(ifconfig eth0 | grep 'inet addr' | cut -d: -f2 | awk '{print $1}') + - export TEST_ETCD_ENDPOINT=http://${ETCD_IP}:2379 + - docker-compose -f docker-compose-test.yaml up -d - |- diff --git a/api/github.go b/api/github.go index 144c6b1..2fecc92 100644 --- a/api/github.go +++ b/api/github.go @@ -23,6 +23,8 @@ import ( "github.com/google/go-github/github" "golang.org/x/oauth2" + log "github.com/Sirupsen/logrus" + "github.com/spf13/viper" ) var ( @@ -31,8 +33,16 @@ var ( // ErrorGitHubAccessDenied - returned when there was access denied to github.com resource ErrorGitHubAccessDenied = errors.New("Access denied") + + // ErrorGitHubNotFound - returned when github.com resource not found + ErrorGitHubNotFound = errors.New("Not found") + ) +func init() { + viper.SetDefault("github_api_max_page_size", 100) +} + // Naive oauth setup func newAccessToken(token string) oauth2.TokenSource { return oauth2.StaticTokenSource( @@ -58,22 +68,34 @@ func (c *GithubClient) GetTeam(name string, id int) (team *github.Team, err erro team = nil err = nil - teams, response, _ := c.client.Organizations.ListTeams(c.owner, nil) + var opt = &github.ListOptions{ + PerPage: viper.GetInt("github_api_max_page_size"), + } - if response.StatusCode != 200 { - err = ErrorGitHubAccessDenied - - } else { - for _, localTeam := range teams { - if *localTeam.ID == id || *localTeam.Slug == name { - team = localTeam - // team found - return + for { + teams, response, _ := c.client.Organizations.ListTeams(c.owner, opt) + + if response.StatusCode != 200 { + err = ErrorGitHubAccessDenied + return + } else { + for _, localTeam := range teams { + if *localTeam.ID == id || *localTeam.Slug == name { + team = localTeam + // team found + return + } } } - err = errors.New("Team with such name or id not found") + + if response.LastPage == 0 { + break + } + opt.Page = response.NextPage } // Exit with error + + err = errors.New("No such team name or id could be found") return } @@ -94,20 +116,84 @@ func (c *GithubClient) IsTeamMember(user string, team *github.Team) (bool, error } // GetKeys - return array of user's {userName} public keys -func (c *GithubClient) GetKeys(userName string) ([]*github.Key, *github.Response, error) { - return c.client.Users.ListKeys(userName, nil) +func (c *GithubClient) GetKeys(userName string) (keys []*github.Key, err error) { + defer func() { + if r := recover(); r != nil { + keys = make([]*github.Key, 0) + err = ErrorGitHubConnectionFailed + } + }() + + logger := log.WithFields(log.Fields{"class": "GithubClient", "method": "Get"}) + + var opt = &github.ListOptions{ + PerPage: viper.GetInt("github_api_max_page_size"), + } + + for { + items, response, local_err := c.client.Users.ListKeys(userName, opt) + + logger.Debugf("Response: %v", response) + logger.Debugf("Response.StatusCode: %v", response.StatusCode) + + switch response.StatusCode { + case 200: + keys = append(keys, items...) + case 404: + err = ErrorGitHubNotFound + return + default: + err = ErrorGitHubAccessDenied + return + } + + if local_err != nil { + err = local_err + return + } + + if response.LastPage == 0 { + break + } + opt.Page = response.NextPage + } + + return } // GetTeamMembers - return array of user's that are {team} members func (c *GithubClient) GetTeamMembers(team *github.Team) (users []*github.User, err error) { defer func() { if r := recover(); r != nil { - users = make([]*github.User, 0) + users = make([]*github.User, 0) err = ErrorGitHubConnectionFailed } }() - users, _, err = c.client.Organizations.ListTeamMembers(*team.ID, nil) + var opt = &github.OrganizationListTeamMembersOptions{ + ListOptions: github.ListOptions{ + PerPage: viper.GetInt("github_api_max_page_size"), + }, + } + + for { + members, resp, local_err := c.client.Organizations.ListTeamMembers(*team.ID, opt) + if resp.StatusCode != 200 { + return nil, ErrorGitHubAccessDenied + } + if local_err != nil { + err = local_err + return + } + + users = append(users, members...) + + if resp.LastPage == 0 { + break + } + opt.Page = resp.NextPage + } + return } diff --git a/api/github_test.go b/api/github_test.go index 42ce346..25ae793 100644 --- a/api/github_test.go +++ b/api/github_test.go @@ -39,6 +39,8 @@ var _ = Describe("GithubClient", func() { validTeamName = viper.GetString("github_team") validTeamID = viper.GetInt("github_team_id") validUser = viper.GetString("github_user") + // Set max page size to 1 for test pagination code + viper.Set("github_api_max_page_size", 1) }) Describe("getTeam()", func() { @@ -74,7 +76,7 @@ var _ = Describe("GithubClient", func() { team, err := c.GetTeam("dasdasd", 0) Expect(err).NotTo(BeNil()) - Expect(err.Error()).To(Equal("Team with such name or id not found")) + Expect(err.Error()).To(Equal("No such team name or id could be found")) Expect(team).To(BeNil()) }) @@ -163,7 +165,7 @@ var _ = Describe("GithubClient", func() { It("should return nil error and no empty list of keys", func() { c := NewGithubClient(validToken, validOrg) user, _ := c.getUser(validUser) - keys, _, err := c.GetKeys(*user.Login) + keys, err := c.GetKeys(*user.Login) Expect(err).To(BeNil()) @@ -183,6 +185,7 @@ var _ = Describe("GithubClient", func() { Expect(err).To(BeNil()) Expect(len(members) > 0).To(BeTrue()) + Expect(len(members) > 1).To(BeTrue()) }) }) }) diff --git a/api/linux_test.go b/api/linux_test.go index a6e8d6e..0185320 100644 --- a/api/linux_test.go +++ b/api/linux_test.go @@ -24,6 +24,7 @@ import ( . "github.com/onsi/gomega" "os/user" "strconv" + model "github.com/cloudposse/github-authorized-keys/model/linux" ) var _ = Describe("Linux", func() { @@ -83,12 +84,12 @@ var _ = Describe("Linux", func() { Describe("userCreate()", func() { Context("call without GID", func() { var ( - userName LinuxUser + userName model.User linux Linux ) BeforeEach(func() { - userName = LinuxUser{Gid: "", Name: "test", Shell: "/bin/bash", Groups: []string{"wheel", "root"}} + userName = model.NewUser("test", "", []string{"wheel", "root"}, "/bin/bash") linux = NewLinux("/") }) @@ -101,35 +102,35 @@ var _ = Describe("Linux", func() { Expect(err).To(BeNil()) - osUser, _ := user.Lookup(userName.Name) + osUser, _ := user.Lookup(userName.Name()) - Expect(osUser.Username).To(Equal(userName.Name)) + Expect(osUser.Username).To(Equal(userName.Name())) value, _ := strconv.ParseInt(osUser.Gid, 10, 64) Expect(value > 0).To(BeTrue()) gids, _ := osUser.GroupIds() - for _, group := range userName.Groups { + for _, group := range userName.Groups() { linuxGroup, err := user.LookupGroup(group) Expect(err).To(BeNil()) Expect(gids).To(ContainElement(string(linuxGroup.Gid))) } - shell := linux.userShell(userName.Name) + shell := linux.userShell(userName.Name()) - Expect(shell).To(Equal(userName.Shell)) + Expect(shell).To(Equal(userName.Shell())) }) }) Context("call with GID", func() { var ( - userName LinuxUser + userName model.User linux Linux ) BeforeEach(func() { - userName = LinuxUser{Gid: "42", Name: "test", Shell: "/bin/bash", Groups: []string{"root"}} + userName = model.NewUser("test", "42", []string{"root"}, "/bin/bash") linux = NewLinux("/") }) @@ -142,23 +143,23 @@ var _ = Describe("Linux", func() { Expect(err).To(BeNil()) - osUser, _ := user.Lookup(userName.Name) + osUser, _ := user.Lookup(userName.Name()) - Expect(osUser.Username).To(Equal(userName.Name)) + Expect(osUser.Username).To(Equal(userName.Name())) - Expect(string(osUser.Gid)).To(Equal(userName.Gid)) + Expect(string(osUser.Gid)).To(Equal(userName.Gid())) gids, _ := osUser.GroupIds() - for _, group := range userName.Groups { + for _, group := range userName.Groups() { linuxGroup, err := user.LookupGroup(group) Expect(err).To(BeNil()) Expect(gids).To(ContainElement(string(linuxGroup.Gid))) } - shell := linux.userShell(userName.Name) + shell := linux.userShell(userName.Name()) - Expect(shell).To(Equal(userName.Shell)) + Expect(shell).To(Equal(userName.Shell())) }) }) }) diff --git a/key_storages/github_keys.go b/key_storages/github_keys.go index b425c4b..6465aef 100644 --- a/key_storages/github_keys.go +++ b/key_storages/github_keys.go @@ -2,7 +2,6 @@ package keyStorages import ( "errors" - log "github.com/Sirupsen/logrus" "strings" "github.com/cloudposse/github-authorized-keys/api" @@ -26,8 +25,6 @@ func (s *GithubKeys) Get(user string) (value string, err error) { value = "" - logger := log.WithFields(log.Fields{"class": "GithubClient", "method": "Get"}) - // Load team team, err := s.client.GetTeam(s.team, s.teamID) if err != nil { @@ -55,30 +52,23 @@ func (s *GithubKeys) Get(user string) (value string, err error) { return } - keys, response, err := s.client.GetKeys(user) + keys, err := s.client.GetKeys(user) - logger.Debugf("Response: %v", response) - logger.Debugf("Response.StatusCode: %v", response.StatusCode) + if err == nil { - switch response.StatusCode { - case 200: result := []string{} for _, value := range keys { result = append(result, *value.Key) } value = strings.Join(result, "\n") - return - case 404: - value = "" + } else if err == api.ErrorGitHubNotFound { err = ErrStorageKeyNotFound - return - - default: - value = "" + } else { err = errors.New("Access denied") - return } + + return } // NewGithubKeys - constructor for github key storage