From 15628d87ec762923db954c07d4af6e55f4c17299 Mon Sep 17 00:00:00 2001 From: spinnakerbot Date: Fri, 29 Mar 2019 12:24:46 -0400 Subject: [PATCH 01/10] chore(dependencies): Autobump spinnaker-dependencies (#362) --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index c2c6b710d..61aa10123 100644 --- a/build.gradle +++ b/build.gradle @@ -32,7 +32,7 @@ allprojects { apply plugin: 'groovy' ext { - spinnakerDependenciesVersion = '1.36.0' + spinnakerDependenciesVersion = '1.37.0' if (project.hasProperty('spinnakerDependenciesVersion')) { spinnakerDependenciesVersion = project.property('spinnakerDependenciesVersion') } From 54aacf5defd5450c3f1983a66844fc53d26a2421 Mon Sep 17 00:00:00 2001 From: Rob Fletcher Date: Mon, 1 Apr 2019 12:43:51 -0700 Subject: [PATCH 02/10] fix(health): remove potentially expensive Redis health indicator (#363) Arg --- .../spinnaker/fiat/config/RedisConfig.java | 36 ------------------- 1 file changed, 36 deletions(-) diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/config/RedisConfig.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/config/RedisConfig.java index 8957a73c3..acdfa403b 100644 --- a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/config/RedisConfig.java +++ b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/config/RedisConfig.java @@ -70,40 +70,4 @@ private static JedisPool createPool(GenericObjectPoolConfig redisPoolConfig, return new JedisPool(redisPoolConfig, host, port, timeout, password, database, null); } - - @Bean - public HealthIndicator redisHealth(final JedisPool jedisPool) { - return new HealthIndicator() { - @Override - public Health health() { - Health.Builder health; - try (Jedis jedis = jedisPool.getResource()) { - if ("PONG".equals(jedis.ping())) { - health = Health.up(); - } else { - health = Health.down(); - } - } catch (Exception ex) { - health = Health.down(ex); - } - - try { - Field f = FieldUtils.getField(JedisPool.class, "internalPool", true /*forceAccess*/); - Object o = FieldUtils.readField(f, jedisPool, true /*forceAccess*/); - if (o instanceof GenericObjectPool) { - GenericObjectPool internal = (GenericObjectPool) o; - health.withDetail("maxIdle", internal.getMaxIdle()); - health.withDetail("minIdle", internal.getMinIdle()); - health.withDetail("numActive", internal.getNumActive()); - health.withDetail("numIdle", internal.getNumIdle()); - health.withDetail("numWaiters", internal.getNumWaiters()); - } - } catch (IllegalAccessException iae) { - log.debug("Can't access jedis' internal pool", iae); - } - - return health.build(); - } - }; - } } From 737cc742e3861eccb603519747b22e26bfc41e97 Mon Sep 17 00:00:00 2001 From: Rob Fletcher Date: Mon, 1 Apr 2019 12:54:18 -0700 Subject: [PATCH 03/10] chore(redis): bump Jedis to version that fixes block/wait issue (#364) See https://github.com/xetorthio/jedis/issues/1136 --- fiat-roles/fiat-roles.gradle | 1 + 1 file changed, 1 insertion(+) diff --git a/fiat-roles/fiat-roles.gradle b/fiat-roles/fiat-roles.gradle index 070fb3a9a..27247ea7f 100644 --- a/fiat-roles/fiat-roles.gradle +++ b/fiat-roles/fiat-roles.gradle @@ -27,6 +27,7 @@ dependencies { compile spinnaker.dependency("korkDynomite") compile spinnaker.dependency("korkHystrix") compile spinnaker.dependency("korkJedis") + compile "redis.clients:jedis:2.9.3" testCompile spinnaker.dependency("korkJedisTest") testCompile "org.apache.commons:commons-collections4:4.1" From c311a3d70d0e2221899f040d7e1789576b0776b1 Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Wed, 10 Apr 2019 13:28:06 -0400 Subject: [PATCH 04/10] test(core): Add a basic startup test. (#367) --- .../fiat/config/ResourcesConfig.java | 6 +---- .../com/netflix/spinnaker/fiat/MainSpec.java | 25 +++++++++++++++++++ fiat-web/src/test/resources/fiat-test.yml | 7 ++++++ 3 files changed, 33 insertions(+), 5 deletions(-) create mode 100644 fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/MainSpec.java create mode 100644 fiat-web/src/test/resources/fiat-test.yml diff --git a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java index a79a34bbb..528f8fd53 100644 --- a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java +++ b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java @@ -65,10 +65,6 @@ public class ResourcesConfig { @Setter private String clouddriverEndpoint; - @Value("${services.igor.baseUrl}") - @Setter - private String igorEndpoint; - @Bean Front50Api front50Api() { return new RestAdapter.Builder() @@ -105,7 +101,7 @@ ClouddriverService clouddriverService(ClouddriverApi clouddriverApi) { @Bean @ConditionalOnProperty("services.igor.enabled") - IgorApi igorApi() { + IgorApi igorApi(@Value("${services.igor.enabled}") String igorEndpoint) { return new RestAdapter.Builder() .setEndpoint(Endpoints.newFixedEndpoint(igorEndpoint)) .setClient(okClient) diff --git a/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/MainSpec.java b/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/MainSpec.java new file mode 100644 index 000000000..b03a42ff7 --- /dev/null +++ b/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/MainSpec.java @@ -0,0 +1,25 @@ +package com.netflix.spinnaker.fiat; + +import com.netflix.hystrix.strategy.HystrixPlugins; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; + +@RunWith(SpringJUnit4ClassRunner.class) +@SpringBootTest(classes = {Main.class}) +@TestPropertySource(properties = {"spring.config.location=classpath:fiat-test.yml"}) +public class MainSpec { + + @Before + public void setUp() throws Exception { + HystrixPlugins.reset(); + } + + @Test + public void startupTest(){ + + } +} \ No newline at end of file diff --git a/fiat-web/src/test/resources/fiat-test.yml b/fiat-web/src/test/resources/fiat-test.yml new file mode 100644 index 000000000..bd8e960ca --- /dev/null +++ b/fiat-web/src/test/resources/fiat-test.yml @@ -0,0 +1,7 @@ +services: + clouddriver: + baseUrl: 'http://localhost:7003' + front50: + baseUrl: 'http://localhost:8080' + igor: + enabled: false \ No newline at end of file From c37b2832ce3b3d0c63465f0024d31c098b53494c Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Wed, 10 Apr 2019 16:16:15 -0400 Subject: [PATCH 05/10] feat(admin): Implement isAdmin check in FiatPermissionEvaluator. (#366) --- .../fiat/shared/FiatPermissionEvaluator.java | 9 +++++++++ .../shared/FiatPermissionEvaluatorSpec.groovy | 17 +++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java index 11aa27b7e..3c9435535 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java @@ -350,6 +350,15 @@ public boolean isAdmin() { return true; // TODO(ttomsu): Chosen by fair dice roll. Guaranteed to be random. } + + public boolean isAdmin(Authentication authentication) { + if (!fiatStatus.isEnabled()) { + return true; + } + UserPermission.View permission = getPermission(getUsername(authentication)); + return permission != null && permission.isAdmin(); + } + public class AuthorizationFailure { private final Authorization authorization; private final ResourceType resourceType; diff --git a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy index c9231971a..b62c23935 100644 --- a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy +++ b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy @@ -251,6 +251,23 @@ class FiatPermissionEvaluatorSpec extends Specification { resourceType << ResourceType.values()*.toString() } + def "should support isAdmin check for a user"() { + given: + 1 * fiatService.getUserPermission("testUser") >> { + return new UserPermission.View() + .setApplications(Collections.emptySet()) + .setAdmin(isAdmin) + } + + expect: + evaluator.isAdmin(authentication) == expectedIsAdmin + + where: + isAdmin || expectedIsAdmin + false || false + true || true + } + private static FiatClientConfigurationProperties buildConfigurationProperties() { FiatClientConfigurationProperties configurationProperties = new FiatClientConfigurationProperties(); configurationProperties.enabled = true From 2734743c09c28b031dd41c5b99c1878650084622 Mon Sep 17 00:00:00 2001 From: Emily Burns Date: Wed, 10 Apr 2019 17:14:00 -0700 Subject: [PATCH 06/10] fix(igor): endpoint config picks up endpoint (#368) --- .../java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java index 528f8fd53..40acf7790 100644 --- a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java +++ b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java @@ -101,7 +101,7 @@ ClouddriverService clouddriverService(ClouddriverApi clouddriverApi) { @Bean @ConditionalOnProperty("services.igor.enabled") - IgorApi igorApi(@Value("${services.igor.enabled}") String igorEndpoint) { + IgorApi igorApi(@Value("${services.igor.base-url}") String igorEndpoint) { return new RestAdapter.Builder() .setEndpoint(Endpoints.newFixedEndpoint(igorEndpoint)) .setClient(okClient) From 9645925c31b8d12d66db4420524c22d72d923174 Mon Sep 17 00:00:00 2001 From: Adam Jordens Date: Thu, 11 Apr 2019 12:10:33 -0700 Subject: [PATCH 07/10] fix(roles): Better logging (and metrics) if a user roles sync fails (#369) --- .../spinnaker/fiat/roles/UserRolesSyncer.java | 17 ++++++++++++++++- .../fiat/roles/UserRolesSyncerSpec.groovy | 7 +++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/roles/UserRolesSyncer.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/roles/UserRolesSyncer.java index c188200e8..a7bcbc065 100644 --- a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/roles/UserRolesSyncer.java +++ b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/roles/UserRolesSyncer.java @@ -18,6 +18,8 @@ import com.netflix.appinfo.InstanceInfo; import com.netflix.discovery.DiscoveryClient; +import com.netflix.spectator.api.Gauge; +import com.netflix.spectator.api.Registry; import com.netflix.spinnaker.fiat.config.ResourceProvidersHealthIndicator; import com.netflix.spinnaker.fiat.config.UnrestrictedResourceConfig; import com.netflix.spinnaker.fiat.model.UserPermission; @@ -71,8 +73,11 @@ public class UserRolesSyncer implements ApplicationListener discoveryClient, + Registry registry, LockManager lockManager, PermissionsRepository permissionsRepository, PermissionsResolver permissionsResolver, @@ -99,6 +104,8 @@ public UserRolesSyncer(Optional discoveryClient, // default to enabled iff discovery is not available !discoveryClient.isPresent() ); + + this.userRolesSyncCount = registry.gauge("fiat.userRoles.syncCount"); } @Override @@ -109,6 +116,7 @@ public void onApplicationEvent(RemoteStatusChangedEvent event) { @Scheduled(fixedDelay = 30000L) public void schedule() { if (syncDelayMs < 0 || !isEnabled.get()) { + log.warn("User roles syncing is disabled (syncDelayMs: {}, isEnabled: {})", syncDelayMs, isEnabled.get()); return; } @@ -118,7 +126,14 @@ public void schedule() { .withSuccessInterval(Duration.ofMillis(syncDelayMs)) .withFailureInterval(Duration.ofMillis(syncFailureDelayMs)); - lockManager.acquireLock(lockOptions, () -> this.syncAndReturn(new ArrayList<>())); + lockManager.acquireLock(lockOptions, () -> { + try { + userRolesSyncCount.set(this.syncAndReturn(new ArrayList<>())); + } catch (Exception e) { + log.error("User roles synchronization failed", e); + userRolesSyncCount.set(-1); + } + }); } public long syncAndReturn(List roles) { diff --git a/fiat-roles/src/test/groovy/com/netflix/spinnaker/fiat/roles/UserRolesSyncerSpec.groovy b/fiat-roles/src/test/groovy/com/netflix/spinnaker/fiat/roles/UserRolesSyncerSpec.groovy index 725991afd..f0cd688b1 100644 --- a/fiat-roles/src/test/groovy/com/netflix/spinnaker/fiat/roles/UserRolesSyncerSpec.groovy +++ b/fiat-roles/src/test/groovy/com/netflix/spinnaker/fiat/roles/UserRolesSyncerSpec.groovy @@ -20,6 +20,8 @@ import com.fasterxml.jackson.annotation.JsonInclude import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.appinfo.InstanceInfo.InstanceStatus import com.netflix.discovery.DiscoveryClient +import com.netflix.spectator.api.NoopRegistry +import com.netflix.spectator.api.Registry import com.netflix.spinnaker.fiat.config.ResourceProvidersHealthIndicator import com.netflix.spinnaker.fiat.config.UnrestrictedResourceConfig import com.netflix.spinnaker.fiat.model.UserPermission @@ -47,6 +49,9 @@ class UserRolesSyncerSpec extends Specification { private static final String UNRESTRICTED = UnrestrictedResourceConfig.UNRESTRICTED_USERNAME; + @Shared + Registry registry = new NoopRegistry() + @Shared @AutoCleanup("destroy") EmbeddedRedis embeddedRedis @@ -133,6 +138,7 @@ class UserRolesSyncerSpec extends Specification { @Subject def syncer = new UserRolesSyncer( Optional.ofNullable(null), + registry, lockManager, repo, permissionsResolver, @@ -205,6 +211,7 @@ class UserRolesSyncerSpec extends Specification { def lockManager = Mock(LockManager) def userRolesSyncer = new UserRolesSyncer( Optional.ofNullable(discoveryClient), + registry, lockManager, null, null, From 28db241a67ea1b16d73058b0e93d21ece8fa9ee1 Mon Sep 17 00:00:00 2001 From: Emily Burns Date: Thu, 11 Apr 2019 12:46:32 -0700 Subject: [PATCH 08/10] fix(igor): boot1 doesn't love kebab case (#370) --- .../java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java index 40acf7790..936ea1730 100644 --- a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java +++ b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java @@ -101,7 +101,7 @@ ClouddriverService clouddriverService(ClouddriverApi clouddriverApi) { @Bean @ConditionalOnProperty("services.igor.enabled") - IgorApi igorApi(@Value("${services.igor.base-url}") String igorEndpoint) { + IgorApi igorApi(@Value("${services.igor.baseUrl}") String igorEndpoint) { return new RestAdapter.Builder() .setEndpoint(Endpoints.newFixedEndpoint(igorEndpoint)) .setClient(okClient) From d3fafaffa2c646079564f58911175e86e6394b42 Mon Sep 17 00:00:00 2001 From: Cameron Fieber Date: Fri, 12 Apr 2019 14:23:11 -0700 Subject: [PATCH 09/10] chore(deps): latest spinnaker-dependencies --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 72497bed0..5bfcd050b 100644 --- a/build.gradle +++ b/build.gradle @@ -34,7 +34,7 @@ allprojects { apply plugin: 'io.spring.dependency-management' ext { - spinnakerDependenciesVersion = '2.0.0-rc.21-springBoot2' + spinnakerDependenciesVersion = '2.0.0-rc.24-springBoot2' if (project.hasProperty('spinnakerDependenciesVersion')) { spinnakerDependenciesVersion = project.property('spinnakerDependenciesVersion') } From 03d8217c390f698fe4471bae3fe6f8152865ed51 Mon Sep 17 00:00:00 2001 From: Cameron Fieber Date: Fri, 12 Apr 2019 14:31:04 -0700 Subject: [PATCH 10/10] supply property placeholder for igor --- fiat-web/src/test/resources/fiat-test.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fiat-web/src/test/resources/fiat-test.yml b/fiat-web/src/test/resources/fiat-test.yml index bd8e960ca..db1170b9a 100644 --- a/fiat-web/src/test/resources/fiat-test.yml +++ b/fiat-web/src/test/resources/fiat-test.yml @@ -4,4 +4,5 @@ services: front50: baseUrl: 'http://localhost:8080' igor: - enabled: false \ No newline at end of file + enabled: false + baseUrl: null