From 4096e612cc638f7121e35a74bef1c1fadf936222 Mon Sep 17 00:00:00 2001 From: Apoorv Mahajan Date: Mon, 16 May 2022 11:23:27 -0700 Subject: [PATCH] feat(search): provide a way to turn off cats search provider and kubernetes search provider (#5595) * feat(search): provide a way to turn off cats search provider * test(core): make sure that CatsSearchProviderTest works with spring 2.3 as mentioned here: https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.3-Release-Notes#applicationcontextrunner-disables-bean-overriding-by-default, tests should add: withAllowBeanDefinitionOverriding for backwards consistency * feat(search): provide a way to turn off kubernetes search provider * fix(search): ensure that search result==null is handled properly if any of the search providers return items.results as null, then we see: Ambiguous method overloading for method java.util.ArrayList#addAll. Cannot resolve which method to invoke for [null] due to overlapping prototypes between: [interface java.util.Collection] [interface java.lang.Iterable] [interface java.util.Iterator] Therefore, let's default to an empty [] in such cases. --- clouddriver-core/clouddriver-core.gradle | 1 + .../clouddriver/cache/CacheConfig.groovy | 1 + .../search/NoopSearchProvider.java | 1 + .../cache/CatsSearchProviderTest.java | 77 ++++++++++++++++++ .../clouddriver-kubernetes.gradle | 1 + .../provider/KubernetesSearchProvider.java | 2 + .../KubernetesSearchProviderTest.java | 78 +++++++++++++++++++ .../controllers/SearchController.groovy | 12 ++- .../controllers/SearchControllerSpec.groovy | 36 +++++++++ 9 files changed, 208 insertions(+), 1 deletion(-) create mode 100644 clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/cache/CatsSearchProviderTest.java create mode 100644 clouddriver-kubernetes/src/test/java/com/netflix/spinnaker/clouddriver/kubernetes/caching/view/provider/KubernetesSearchProviderTest.java diff --git a/clouddriver-core/clouddriver-core.gradle b/clouddriver-core/clouddriver-core.gradle index edb4e38e48b..d6c02bd8e58 100644 --- a/clouddriver-core/clouddriver-core.gradle +++ b/clouddriver-core/clouddriver-core.gradle @@ -61,5 +61,6 @@ dependencies { testImplementation "org.junit.jupiter:junit-jupiter-engine" testImplementation "org.junit.jupiter:junit-jupiter-params" testImplementation "org.mockito:mockito-core" + testImplementation "org.springframework.boot:spring-boot-test" testImplementation "org.springframework.boot:spring-boot-starter-test" } diff --git a/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/cache/CacheConfig.groovy b/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/cache/CacheConfig.groovy index c8a5a404cb3..09a73c1aaf2 100644 --- a/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/cache/CacheConfig.groovy +++ b/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/cache/CacheConfig.groovy @@ -103,6 +103,7 @@ class CacheConfig { } @Bean + @ConditionalOnProperty(value = "caching.search.enabled", matchIfMissing = true) SearchProvider catsSearchProvider(CatsInMemorySearchProperties catsInMemorySearchProperties, Cache cacheView, List providers, diff --git a/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/search/NoopSearchProvider.java b/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/search/NoopSearchProvider.java index be5bd74f9ed..da8025446bd 100644 --- a/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/search/NoopSearchProvider.java +++ b/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/search/NoopSearchProvider.java @@ -43,6 +43,7 @@ private static SearchResultSet empty(String query, Integer pageNumger, Integer p .pageNumber(pageNumger) .pageSize(pageSize) .query(query) + .results(List.of()) .build(); } } diff --git a/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/cache/CatsSearchProviderTest.java b/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/cache/CatsSearchProviderTest.java new file mode 100644 index 00000000000..61bd05884c7 --- /dev/null +++ b/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/cache/CatsSearchProviderTest.java @@ -0,0 +1,77 @@ +/* + * Copyright 2021 Salesforce.com, 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.clouddriver.cache; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.netflix.spectator.api.NoopRegistry; +import com.netflix.spectator.api.Registry; +import com.netflix.spinnaker.cats.cache.Cache; +import com.netflix.spinnaker.cats.mem.InMemoryCache; +import org.junit.jupiter.api.Test; +import org.springframework.boot.context.annotation.UserConfigurations; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.context.annotation.Bean; + +public class CatsSearchProviderTest { + private final ApplicationContextRunner runner = + new ApplicationContextRunner() + .withPropertyValues("caching.write-enabled=false", "redis.enabled:false") + .withConfiguration(UserConfigurations.of(CacheConfig.class, TestConfiguration.class)) + .withAllowBeanDefinitionOverriding(true); + + @Test + void testCatsSearchProviderBeanIsPresentByDefault() { + runner.run(ctx -> assertThat(ctx).hasSingleBean(CatsSearchProvider.class)); + } + + @Test + void testCatsSearchProviderBeanIsPresentWhenConfiguredInSuchAWay() { + runner + .withPropertyValues("caching.search.enabled=true") + .run(ctx -> assertThat(ctx).hasSingleBean(CatsSearchProvider.class)); + } + + @Test + void testCatsSearchProviderBeanIsNotPresentWhenConfiguredInSuchAWay() { + runner + .withPropertyValues("caching.search.enabled=false") + .run(ctx -> assertThat(ctx).doesNotHaveBean(CatsSearchProvider.class)); + } + + /** + * test class that supplies the minimum set of beans needed to autowire the CatsSearchProvider + * bean and other required beans in the CacheConfig class + */ + static class TestConfiguration { + @Bean + CatsInMemorySearchProperties catsInMemorySearchProperties() { + return new CatsInMemorySearchProperties(); + } + + @Bean + Cache cache() { + return new InMemoryCache(); + } + + @Bean + Registry registry() { + return new NoopRegistry(); + } + } +} diff --git a/clouddriver-kubernetes/clouddriver-kubernetes.gradle b/clouddriver-kubernetes/clouddriver-kubernetes.gradle index fc93d3d322d..f6409b901ec 100644 --- a/clouddriver-kubernetes/clouddriver-kubernetes.gradle +++ b/clouddriver-kubernetes/clouddriver-kubernetes.gradle @@ -100,6 +100,7 @@ dependencies { testImplementation "org.spockframework:spock-core" testImplementation "org.spockframework:spock-spring" testImplementation "org.springframework:spring-test" + testImplementation "org.springframework.boot:spring-boot-test" integrationImplementation project(":clouddriver-web") diff --git a/clouddriver-kubernetes/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/caching/view/provider/KubernetesSearchProvider.java b/clouddriver-kubernetes/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/caching/view/provider/KubernetesSearchProvider.java index 11e422eee3b..9180f86a870 100644 --- a/clouddriver-kubernetes/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/caching/view/provider/KubernetesSearchProvider.java +++ b/clouddriver-kubernetes/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/caching/view/provider/KubernetesSearchProvider.java @@ -48,9 +48,11 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.stereotype.Component; @Component +@ConditionalOnProperty(value = "kubernetes.search.enabled", matchIfMissing = true) public class KubernetesSearchProvider implements SearchProvider { private static final Logger log = LoggerFactory.getLogger(KubernetesSearchProvider.class); private final KubernetesCacheUtils cacheUtils; diff --git a/clouddriver-kubernetes/src/test/java/com/netflix/spinnaker/clouddriver/kubernetes/caching/view/provider/KubernetesSearchProviderTest.java b/clouddriver-kubernetes/src/test/java/com/netflix/spinnaker/clouddriver/kubernetes/caching/view/provider/KubernetesSearchProviderTest.java new file mode 100644 index 00000000000..60948ceb245 --- /dev/null +++ b/clouddriver-kubernetes/src/test/java/com/netflix/spinnaker/clouddriver/kubernetes/caching/view/provider/KubernetesSearchProviderTest.java @@ -0,0 +1,78 @@ +/* + * Copyright 2021 Salesforce.com, 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.clouddriver.kubernetes.caching.view.provider; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.netflix.spinnaker.clouddriver.kubernetes.description.KubernetesSpinnakerKindMap; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.springframework.boot.context.annotation.UserConfigurations; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.context.annotation.Bean; + +public class KubernetesSearchProviderTest { + private final ApplicationContextRunner runner = + new ApplicationContextRunner() + .withConfiguration( + UserConfigurations.of(KubernetesSearchProvider.class, TestConfiguration.class)); + + @Test + void testKubernetesSearchProviderBeanIsPresentByDefault() { + runner.run(ctx -> assertThat(ctx).hasSingleBean(KubernetesSearchProvider.class)); + } + + @Test + void testKubernetesSearchProviderBeanIsPresentWhenConfiguredInSuchAWay() { + runner + .withPropertyValues("kubernetes.search.enabled=true") + .run(ctx -> assertThat(ctx).hasSingleBean(KubernetesSearchProvider.class)); + } + + @Test + void testKubernetesSearchProviderBeanIsNotPresentWhenConfiguredInSuchAWay() { + runner + .withPropertyValues("kubernetes.search.enabled=false") + .run(ctx -> assertThat(ctx).doesNotHaveBean(KubernetesSearchProvider.class)); + } + + /** test class that supplies beans needed to autowire the KubernetesSearchProvider bean */ + static class TestConfiguration { + @Bean + ObjectMapper getObjectMapper() { + return new ObjectMapper(); + } + + @Bean + KubernetesCacheUtils getKubernetesCacheUtils() { + return mock(KubernetesCacheUtils.class); + } + + @Bean + KubernetesSpinnakerKindMap kubernetesSpinnakerKindMap() { + return new KubernetesSpinnakerKindMap(List.of()); + } + + @Bean + KubernetesAccountResolver kubernetesAccountResolver() { + return mock(KubernetesAccountResolver.class); + } + } +} diff --git a/clouddriver-web/src/main/groovy/com/netflix/spinnaker/clouddriver/controllers/SearchController.groovy b/clouddriver-web/src/main/groovy/com/netflix/spinnaker/clouddriver/controllers/SearchController.groovy index 730443130d6..2a968773d66 100644 --- a/clouddriver-web/src/main/groovy/com/netflix/spinnaker/clouddriver/controllers/SearchController.groovy +++ b/clouddriver-web/src/main/groovy/com/netflix/spinnaker/clouddriver/controllers/SearchController.groovy @@ -87,7 +87,17 @@ class SearchController { } else { int total = results.inject(0) { acc, item -> acc + item.totalMatches } - List> allResults = results.inject([]) { acc, item -> acc.addAll(item.results); acc } + List> allResults = results.inject([]) { acc, item -> + // if any of the search providers return items.results as null, then we see + // Ambiguous method overloading for method java.util.ArrayList#addAll. + // Cannot resolve which method to invoke for [null] due to overlapping prototypes between: + // [interface java.util.Collection] + // [interface java.lang.Iterable] + // [interface java.util.Iterator] + // Therefore, let's default to an empty [] in such cases. + acc.addAll(item.results?: []) + acc + } //TODO-cfieber: this is a temporary workaround to https://github.com/spinnaker/deck/issues/128 [new SearchResultSet( diff --git a/clouddriver-web/src/test/groovy/com/netflix/spinnaker/clouddriver/controllers/SearchControllerSpec.groovy b/clouddriver-web/src/test/groovy/com/netflix/spinnaker/clouddriver/controllers/SearchControllerSpec.groovy index b631aa6dd12..77b5a6b0367 100644 --- a/clouddriver-web/src/test/groovy/com/netflix/spinnaker/clouddriver/controllers/SearchControllerSpec.groovy +++ b/clouddriver-web/src/test/groovy/com/netflix/spinnaker/clouddriver/controllers/SearchControllerSpec.groovy @@ -16,6 +16,10 @@ package com.netflix.spinnaker.clouddriver.controllers +import com.netflix.spinnaker.clouddriver.core.services.Front50Service +import com.netflix.spinnaker.clouddriver.search.ApplicationSearchProvider +import com.netflix.spinnaker.clouddriver.search.NoopSearchProvider +import com.netflix.spinnaker.clouddriver.search.ProjectSearchProvider import com.netflix.spinnaker.clouddriver.search.SearchProvider import com.netflix.spinnaker.clouddriver.search.SearchResultSet import spock.lang.Specification @@ -133,6 +137,38 @@ class SearchControllerSpec extends Specification { searchResultSets == [resultSetA] } + def 'search on type for which there is no search provider'() { + setup: + Front50Service front50Service = Mock(Front50Service) + ApplicationSearchProvider applicationSearchProvider = new ApplicationSearchProvider(front50Service) + ProjectSearchProvider projectSearchProvider = new ProjectSearchProvider(front50Service) + // none of the 3 search providers support the search type + searchController = new SearchController(searchProviders: [applicationSearchProvider, projectSearchProvider, new NoopSearchProvider()]) + def filters = [ + q: "aBC", + type: ['serverGroups'], + page: 1, + pageSize: 10 + ] + + when: + List searchResultSets = getSearchResults(filters) + + then: + 1 * request.getParameterNames() >> enumeration + + searchResultSets == [ + new SearchResultSet( + totalMatches: 0, + pageNumber: 1, + pageSize: 10, + platform: 'aws', + query: 'aBC', + results: [] + ) + ] + } + def "if only one search provider, don't aggregate into an aws result"() { SearchResultSet rsA = Stub(SearchResultSet) searchController = new SearchController(searchProviders: [searchProviderA])