From 012bea77578fef2c939d15ba79fbe9e831f9b5a9 Mon Sep 17 00:00:00 2001 From: Robert Farr Date: Thu, 30 Aug 2018 13:09:03 -0500 Subject: [PATCH] =?UTF-8?q?fix(roles/github):=20Reduce=20burden=20of=20git?= =?UTF-8?q?hub=20reads=20for=20teams=20and=20make=20re=E2=80=A6=20(#247)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(roles/github): Reduce burden of github reads for teams and make resilient to rate-limiting. * Address maximumSize with comment --- .../fiat/roles/github/GitHubProperties.java | 4 +- .../github/GithubTeamsUserRolesProvider.java | 212 ++++++++++++++---- .../roles/github/client/GitHubClient.java | 11 + .../fiat/roles/github/model/Member.java | 26 +++ 4 files changed, 211 insertions(+), 42 deletions(-) create mode 100644 fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/Member.java diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GitHubProperties.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GitHubProperties.java index 121668595..2df02d9c8 100644 --- a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GitHubProperties.java +++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GitHubProperties.java @@ -29,5 +29,7 @@ public class GitHubProperties { @Min(1L) Integer paginationValue = 100; @NotNull - Integer membershipCacheTTLSeconds = 60 * 60; // 1 hour + Integer membershipCacheTTLSeconds = 60 * 10; // 10 min time to refresh + @NotNull + Integer membershipCacheTeamsSize = 1000; // 1000 github teams } diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java index 96e0d861d..cade6c761 100644 --- a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java +++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java @@ -19,11 +19,14 @@ import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.ListenableFutureTask; import com.netflix.spinnaker.fiat.model.resources.Role; import com.netflix.spinnaker.fiat.permissions.ExternalUser; import com.netflix.spinnaker.fiat.roles.UserRolesProvider; import com.netflix.spinnaker.fiat.roles.github.client.GitHubClient; import com.netflix.spinnaker.fiat.roles.github.model.Team; +import com.netflix.spinnaker.fiat.roles.github.model.Member; import com.netflix.spinnaker.fiat.roles.github.model.TeamMembership; import lombok.Data; import lombok.Setter; @@ -42,9 +45,15 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; @@ -68,7 +77,13 @@ public class GithubTeamsUserRolesProvider implements UserRolesProvider, Initiali @Setter private GitHubProperties gitHubProperties; - private LoadingCache teamMembershipCache; + private ExecutorService executor = Executors.newSingleThreadExecutor(); + + private LoadingCache> membersCache; + + private LoadingCache> teamsCache; + + private LoadingCache> teamMembershipCache; private static final String ACTIVE = "active"; @@ -77,29 +92,116 @@ public void afterPropertiesSet() throws Exception { Assert.state(gitHubProperties.getOrganization() != null, "Supply an organization"); Assert.state(gitHubProperties.getBaseUrl() != null, "Supply a base url"); + this.initializeMembersCache(); + this.initializeTeamsCache(); this.initializeTeamMembershipCache(); } - private void initializeTeamMembershipCache() { - this.teamMembershipCache = CacheBuilder.newBuilder() - .maximumSize(1000) - .expireAfterWrite(this.gitHubProperties.getMembershipCacheTTLSeconds(), TimeUnit.SECONDS) + private void initializeMembersCache() { + // Note if multiple github orgs is ever supported the maximumSize will need to change + this.membersCache = CacheBuilder.newBuilder() + .maximumSize(1) //This will only be a cache of one entry keyed by org name. + .refreshAfterWrite(this.gitHubProperties.getMembershipCacheTTLSeconds(), TimeUnit.SECONDS) + .build(new CacheLoader>() { + public Set load(String key) { + Set members = new HashSet<>(); + int page = 1; + boolean hasMorePages = true; + + do { + List membersPage = getMembersInOrgPaginated(key, page++); + membersPage.forEach(m -> members.add(m.getLogin().toLowerCase())); + if (membersPage.size() != gitHubProperties.paginationValue) { + hasMorePages = false; + } + log.debug("Got " + membersPage.size() + " members back. hasMorePages: " + hasMorePages); + } while (hasMorePages); + + return members; + } + + public ListenableFuture> reload(final String key, final Set prev) { + ListenableFutureTask> task = ListenableFutureTask.create(new Callable>() { + public Set call() { + return load(key); + } + }); + executor.execute(task); + return task; + } + }); + } + + private void initializeTeamsCache() { + // Note if multiple github orgs is ever supported the maximumSize will need to change + this.teamsCache = CacheBuilder.newBuilder() + .maximumSize(1) // This will only be a cache of one entry keyed by org name. + .refreshAfterWrite(this.gitHubProperties.getMembershipCacheTTLSeconds(), TimeUnit.SECONDS) .build( - new CacheLoader() { - public Boolean load(CacheKey key) { - try { - TeamMembership response = gitHubClient.isMemberOfTeam(key.getTeamId(), key.getUsername()); - return (response.getState().equals(GithubTeamsUserRolesProvider.ACTIVE)); - } catch (RetrofitError e) { - if (e.getResponse().getStatus() != 404) { - handleNon404s(e); + new CacheLoader>() { + public List load(String key) { + List teams = new ArrayList<>(); + int page = 1; + boolean hasMorePages = true; + + do { + List teamsPage = getTeamsInOrgPaginated(key, page++); + teams.addAll(teamsPage); + if (teamsPage.size() != gitHubProperties.paginationValue) { + hasMorePages = false; + } + log.debug("Got " + teamsPage.size() + " teams back. hasMorePages: " + hasMorePages); + } while (hasMorePages); + + return teams; + } + + public ListenableFuture> reload(final String key, final List prev) { + ListenableFutureTask> task = ListenableFutureTask.create(new Callable>() { + public List call() { + return load(key); } - } - return false; + }); + executor.execute(task); + return task; } }); } + private void initializeTeamMembershipCache() { + this.teamMembershipCache = CacheBuilder.newBuilder() + .maximumSize(this.gitHubProperties.getMembershipCacheTeamsSize()) + .refreshAfterWrite(this.gitHubProperties.getMembershipCacheTTLSeconds(), TimeUnit.SECONDS) + .build(new CacheLoader>() + { + public Set load(Long key) { + Set memberships = new HashSet<>(); + int page = 1; + boolean hasMorePages = true; + do { + List members = getMembersInTeamPaginated(key, page++); + members.forEach(m -> memberships.add(m.getLogin().toLowerCase())); + if (members.size() != gitHubProperties.paginationValue) { + hasMorePages = false; + } + log.debug("Got " + members.size() + " teams back. hasMorePages: " + hasMorePages); + } while (hasMorePages); + + return memberships; + } + + public ListenableFuture> reload(final Long key, final Set prev) { + ListenableFutureTask> task = ListenableFutureTask.create(new Callable>() { + public Set call() { + return load(key); + } + }); + executor.execute(task); + return task; + } + }); + } + @Override public List loadRoles(ExternalUser user) { String username = user.getId(); @@ -139,45 +241,33 @@ public List loadRoles(ExternalUser user) { private boolean isMemberOfOrg(String username) { boolean isMemberOfOrg = false; try { - Response response = gitHubClient.isMemberOfOrganization(gitHubProperties.getOrganization(), - username); - isMemberOfOrg = (response.getStatus() == 204); - } catch (RetrofitError e) { - if (e.getResponse().getStatus() != 404) { - handleNon404s(e); - } + isMemberOfOrg = this.membersCache.get(gitHubProperties.getOrganization()).contains(username.toLowerCase()); + } catch (ExecutionException e) { + log.error("Failed to read from cache when getting org membership", e); } - return isMemberOfOrg; } private List getTeams() { - List teams = new ArrayList<>(); - int page = 1; - boolean hasMorePages = true; - - do { - List teamsPage = getTeamsInOrgPaginated(page++); - teams.addAll(teamsPage); - if (teamsPage.size() != gitHubProperties.paginationValue) { - hasMorePages = false; - } - log.debug("Got " + teamsPage.size() + " teams back. hasMorePages: " + hasMorePages); - } while (hasMorePages); - - return teams; + try { + return this.teamsCache.get(gitHubProperties.getOrganization()); + } catch(ExecutionException e) { + log.error("Failed to read from cache when getting teams", e); + } + return Collections.emptyList(); } - private List getTeamsInOrgPaginated(int page) { + private List getTeamsInOrgPaginated(String organization, int page) { List teams = new ArrayList<>(); try { log.debug("Requesting page " + page + " of teams."); - teams = gitHubClient.getOrgTeams(gitHubProperties.getOrganization(), + teams = gitHubClient.getOrgTeams(organization, page, gitHubProperties.paginationValue); } catch (RetrofitError e) { if (e.getResponse().getStatus() != 404) { handleNon404s(e); + throw e; } else { log.error("404 when getting teams", e); } @@ -186,9 +276,43 @@ private List getTeamsInOrgPaginated(int page) { return teams; } + private List getMembersInOrgPaginated(String organization, int page) { + List members = new ArrayList<>(); + try { + log.debug("Requesting page " + page + " of members."); + members = gitHubClient.getOrgMembers(organization, page, gitHubProperties.paginationValue); + } catch (RetrofitError e) { + if (e.getResponse().getStatus() != 404) { + handleNon404s(e); + throw e; + } else { + log.error("404 when getting members", e); + } + } + + return members; + } + + private List getMembersInTeamPaginated(Long teamId, int page) { + List members = new ArrayList<>(); + try { + log.debug("Requesting page " + page + " of members team " + teamId + "."); + members = gitHubClient.getMembersOfTeam(teamId, page, gitHubProperties.paginationValue); + } catch (RetrofitError e) { + if (e.getResponse().getStatus() != 404) { + handleNon404s(e); + throw e; + } else { + log.error("404 when getting members of team", e); + } + } + + return members; + } + private boolean isMemberOfTeam(Team t, String username) { try { - return this.teamMembershipCache.get(new CacheKey(t.getId(), username)); + return this.teamMembershipCache.get(t.getId()).contains(username.toLowerCase()); } catch (ExecutionException e) { log.error("Failed to read from cache when getting team membership", e); } @@ -231,7 +355,13 @@ public Map> multiLoadRoles(Collection use } @Data - private class CacheKey { + private class OrgMembershipKey { + private final String organization; + private final String username; + } + + @Data + private class TeamMembershipKey { private final Long teamId; private final String username; } diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java index 93f019483..b71aa5b5f 100644 --- a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java +++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java @@ -17,6 +17,7 @@ package com.netflix.spinnaker.fiat.roles.github.client; +import com.netflix.spinnaker.fiat.roles.github.model.Member; import com.netflix.spinnaker.fiat.roles.github.model.Team; import com.netflix.spinnaker.fiat.roles.github.model.TeamMembership; import retrofit.client.Response; @@ -46,6 +47,16 @@ List getOrgTeams(@Path("org") String org, @Query("page") int page, @Query("per_page") int paginationValue); + @GET("/orgs/{org}/members") + List getOrgMembers(@Path("org") String org, + @Query("page") int page, + @Query("per_page") int paginationValue); + + @GET("/teams/{idTeam}/members") + List getMembersOfTeam(@Path("idTeam") Long idTeam, + @Query("page") int page, + @Query("per_page") int paginationValue); + @GET("/teams/{idTeam}/memberships/{username}") TeamMembership isMemberOfTeam(@Path("idTeam") Long idTeam, @Path("username") String username); diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/Member.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/Member.java new file mode 100644 index 000000000..aa2b717bf --- /dev/null +++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/Member.java @@ -0,0 +1,26 @@ +/* + * Copyright 2017 Google, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License") + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.netflix.spinnaker.fiat.roles.github.model; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import lombok.Data; + +@Data +@JsonIgnoreProperties(ignoreUnknown = true) +public class Member { + private String login; +} \ No newline at end of file