From 8f3d5e59bdaeb35167f40194d94ed2fbd53fa305 Mon Sep 17 00:00:00 2001 From: Eric Zimanyi Date: Wed, 3 Jul 2019 15:53:08 -0400 Subject: [PATCH] refactor(kubernetes): Create KubernetesKindRegistry (#3843) The KubernetesKind class is responsible for both providing information about a given KubernetesKind and for keeping a registry of all registered kinds. The registry of all kinds is stored as a list which is very inefficient to search. In preparation for making this logic more efficient, create a KubernetesKindRegistry that supports registering kind or getting registered kinds. The implementation will be the exact same (inefficient) one that is currently directly in KubernetesKind but we have now defined an API and can write tests against it, and in a future commit replace it with a more efficient implementation. --- .../agent/KubernetesCoreCachingAgent.java | 10 +- .../description/manifest/KubernetesKind.java | 108 +++++--------- .../manifest/KubernetesKindRegistry.java | 103 ++++++++++++++ .../v2/security/KubernetesV2Credentials.java | 2 +- .../KubernetesKindRegistrySpec.groovy | 132 ++++++++++++++++++ 5 files changed, 274 insertions(+), 81 deletions(-) create mode 100644 clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesKindRegistry.java create mode 100644 clouddriver-kubernetes/src/test/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/KubernetesKindRegistrySpec.groovy diff --git a/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/caching/agent/KubernetesCoreCachingAgent.java b/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/caching/agent/KubernetesCoreCachingAgent.java index e2ae22b7990..f811fbb1fd0 100644 --- a/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/caching/agent/KubernetesCoreCachingAgent.java +++ b/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/caching/agent/KubernetesCoreCachingAgent.java @@ -70,11 +70,9 @@ public Collection getProvidedDataTypes() { @Override protected List primaryKinds() { - synchronized (KubernetesKind.getValues()) { - return KubernetesKind.getValues().stream() - .filter(credentials::isValidKind) - .filter(k -> !k.isDynamic()) - .collect(Collectors.toList()); - } + return KubernetesKind.getRegisteredKinds().stream() + .filter(credentials::isValidKind) + .filter(k -> !k.isDynamic()) + .collect(Collectors.toList()); } } diff --git a/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesKind.java b/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesKind.java index d7844b94756..34de646690f 100644 --- a/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesKind.java +++ b/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesKind.java @@ -19,12 +19,8 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonValue; -import java.util.ArrayList; -import java.util.Collections; import java.util.List; -import java.util.Objects; import java.util.Optional; -import java.util.function.Predicate; import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -37,9 +33,7 @@ @EqualsAndHashCode(onlyExplicitlyIncluded = true) @Slf4j public final class KubernetesKind { - @Getter - private static final List values = - Collections.synchronizedList(new ArrayList<>()); + private static final KubernetesKindRegistry kindRegistry = new KubernetesKindRegistry(); public static final KubernetesKind API_SERVICE = createAndRegisterKind( @@ -126,12 +120,12 @@ public final class KubernetesKind { public static final KubernetesKind NONE = createAndRegisterKind("none", KubernetesApiGroup.NONE, null, true, false); - @Nonnull private final String name; + @Getter @Nonnull private final String name; @EqualsAndHashCode.Include @Nonnull private final String lcName; - @Nonnull private final KubernetesApiGroup apiGroup; + @Getter @Nonnull private final KubernetesApiGroup apiGroup; @EqualsAndHashCode.Include @Nullable private final KubernetesApiGroup customApiGroup; - @Nullable private final String alias; + @Getter @Nullable private final String alias; @Getter private final boolean isNamespaced; // generally reserved for workloads, can be read as "does this belong to a spinnaker cluster?" private final boolean hasClusterRelationship; @@ -146,11 +140,9 @@ private static KubernetesKind createAndRegisterKind( @Nullable String alias, boolean isNamespaced, boolean hasClusterRelationship) { - KubernetesKind kind = + return kindRegistry.registerKind( new KubernetesKind( - name, apiGroup, alias, isNamespaced, hasClusterRelationship, false, true); - values.add(kind); - return kind; + name, apiGroup, alias, isNamespaced, hasClusterRelationship, false, true)); } private KubernetesKind( @@ -211,9 +203,11 @@ public static KubernetesKind fromString(@Nonnull final String name) { return fromString(scopedKind.kindName, scopedKind.apiGroup); } + @Nonnull public static KubernetesKind fromString( @Nonnull final String name, @Nullable final KubernetesApiGroup apiGroup) { - return getRegisteredKind(name, apiGroup) + return kindRegistry + .getRegisteredKind(name, apiGroup) .orElseGet( () -> new KubernetesKind( @@ -226,70 +220,30 @@ public static KubernetesKind fromString( false)); } - private static Optional getRegisteredKind( - @Nonnull final String name, @Nullable final KubernetesApiGroup apiGroup) { - if (StringUtils.isEmpty(name)) { - return Optional.of(KubernetesKind.NONE); - } - - if (name.equalsIgnoreCase(KubernetesKind.NONE.toString())) { - throw new IllegalArgumentException("The 'NONE' kind cannot be read."); - } - - Predicate groupMatches = - kind -> { - // Exact match - if (Objects.equals(kind.apiGroup, apiGroup)) { - return true; - } - - // If we have not specified an API group, default to finding a native kind that matches - if (apiGroup == null || apiGroup.isNativeGroup()) { - return kind.apiGroup.isNativeGroup(); - } - - return false; - }; - - // separate from the above chain to avoid concurrent modification of the values list - return values.stream() - .filter( - v -> - v.name.equalsIgnoreCase(name) - || (v.alias != null && v.alias.equalsIgnoreCase(name))) - .filter(groupMatches) - .findAny(); - } - @Nonnull public static KubernetesKind getOrRegisterKind( @Nonnull final String name, final boolean registered, final boolean namespaced, @Nullable final KubernetesApiGroup apiGroup) { - synchronized (values) { - Optional kindOptional = getRegisteredKind(name, apiGroup); - // separate from the above chain to avoid concurrent modification of the values list - return kindOptional.orElseGet( - () -> { - log.info( - "Dynamically registering {}, (namespaced: {}, registered: {})", - name, - namespaced, - registered); - KubernetesKind kind = - new KubernetesKind( - name, - Optional.ofNullable(apiGroup).orElse(KubernetesApiGroup.NONE), - null, - namespaced, - false, - true, - registered); - values.add(kind); - return kind; - }); - } + return kindRegistry.getOrRegisterKind( + name, + apiGroup, + () -> { + log.info( + "Dynamically registering {}, (namespaced: {}, registered: {})", + name, + namespaced, + registered); + return new KubernetesKind( + name, + Optional.ofNullable(apiGroup).orElse(KubernetesApiGroup.NONE), + null, + namespaced, + false, + true, + registered); + }); } @Nonnull @@ -299,10 +253,16 @@ public static KubernetesKind getOrRegisterKind( return getOrRegisterKind(scopedKind.kindName, true, isNamespaced, scopedKind.apiGroup); } - public static List getOrRegisterKinds(List names) { + @Nonnull + public static List getOrRegisterKinds(@Nonnull List names) { return names.stream().map(k -> getOrRegisterKind(k, true)).collect(Collectors.toList()); } + @Nonnull + public static List getRegisteredKinds() { + return kindRegistry.getRegisteredKinds(); + } + @RequiredArgsConstructor private static class ScopedKind { public final String kindName; diff --git a/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesKindRegistry.java b/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesKindRegistry.java new file mode 100644 index 00000000000..ade10c699b4 --- /dev/null +++ b/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesKindRegistry.java @@ -0,0 +1,103 @@ +/* + * Copyright 2019 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.clouddriver.kubernetes.v2.description.manifest; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.function.Predicate; +import java.util.function.Supplier; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import org.apache.commons.lang3.StringUtils; + +public class KubernetesKindRegistry { + private final List values = Collections.synchronizedList(new ArrayList<>()); + + /** Registers a given {@link KubernetesKind} into the registry and returns the kind */ + @Nonnull + public KubernetesKind registerKind(@Nonnull KubernetesKind kind) { + values.add(kind); + return kind; + } + + /** + * Searches the registry for a {@link KubernetesKind} with the supplied name and apiGroup. If a + * kind is found, it is returned. If no kind is found, the provided {@link + * Supplier} is invoked and the resulting kind is registered. + * + *

This method is guaranteed to atomically check and register the kind. + */ + @Nonnull + public synchronized KubernetesKind getOrRegisterKind( + @Nonnull final String name, + @Nullable final KubernetesApiGroup apiGroup, + @Nonnull Supplier supplier) { + return getRegisteredKind(name, apiGroup).orElseGet(() -> registerKind(supplier.get())); + } + + /** + * Searches the registry for a {@link KubernetesKind} with the supplied name and apiGroup. Returns + * an {@link Optional} containing the kind, or an empty {@link Optional} if no + * kind is found. + * + *

Kinds whose API groups are different but are both is a native API groups (see {@link + * KubernetesApiGroup#isNativeGroup()}) are considered to match. + */ + @Nonnull + public Optional getRegisteredKind( + @Nonnull final String name, @Nullable final KubernetesApiGroup apiGroup) { + if (StringUtils.isEmpty(name)) { + return Optional.of(KubernetesKind.NONE); + } + + if (name.equalsIgnoreCase(KubernetesKind.NONE.toString())) { + throw new IllegalArgumentException("The 'NONE' kind cannot be read."); + } + + Predicate groupMatches = + kind -> { + // Exact match + if (Objects.equals(kind.getApiGroup(), apiGroup)) { + return true; + } + + // If we have not specified an API group, default to finding a native kind that matches + if (apiGroup == null || apiGroup.isNativeGroup()) { + return kind.getApiGroup().isNativeGroup(); + } + + return false; + }; + + return values.stream() + .filter( + v -> + v.getName().equalsIgnoreCase(name) + || (v.getAlias() != null && v.getAlias().equalsIgnoreCase(name))) + .filter(groupMatches) + .findAny(); + } + + /** Returns a list of all registered kinds */ + @Nonnull + public List getRegisteredKinds() { + return values; + } +} diff --git a/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2Credentials.java b/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2Credentials.java index 010cfa34aba..4a3874bf3cb 100644 --- a/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2Credentials.java +++ b/clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2Credentials.java @@ -334,7 +334,7 @@ private void determineOmitKinds() { // otherwise, checking all namespaces for all kinds is too expensive in large clusters (imagine // a cluster with 100s of namespaces). String checkNamespace = namespaces.get(0); - List allKinds = KubernetesKind.getValues(); + List allKinds = KubernetesKind.getRegisteredKinds(); log.info( "Checking permissions on configured kinds for account {}... {}", accountName, allKinds); diff --git a/clouddriver-kubernetes/src/test/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/KubernetesKindRegistrySpec.groovy b/clouddriver-kubernetes/src/test/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/KubernetesKindRegistrySpec.groovy new file mode 100644 index 00000000000..923e37fd778 --- /dev/null +++ b/clouddriver-kubernetes/src/test/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/KubernetesKindRegistrySpec.groovy @@ -0,0 +1,132 @@ +/* + * Copyright 2019 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.clouddriver.kubernetes.v2.description + +import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesApiGroup +import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind +import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKindRegistry +import spock.lang.Specification +import spock.lang.Subject +import spock.lang.Unroll + +class KubernetesKindRegistrySpec extends Specification { + static final KubernetesApiGroup CUSTOM_API_GROUP = KubernetesApiGroup.fromString("test") + static final KubernetesKind CUSTOM_KIND = KubernetesKind.fromString("customKind", CUSTOM_API_GROUP) + + @Unroll + void "kinds from core API groups are returned if any core API group is input"() { + given: + @Subject KubernetesKindRegistry kindRegistry = new KubernetesKindRegistry() + kindRegistry.registerKind(KubernetesKind.REPLICA_SET) + + when: + def kind = kindRegistry.getRegisteredKind(name, apiGroup) + + then: + result == kind.orElse(null) + + where: + name | apiGroup | result + "replicaSet" | null | KubernetesKind.REPLICA_SET + "replicaSet" | KubernetesApiGroup.APPS | KubernetesKind.REPLICA_SET + "replicaSet" | KubernetesApiGroup.EXTENSIONS | KubernetesKind.REPLICA_SET + "replicaSet" | CUSTOM_API_GROUP | null + "rs" | null | KubernetesKind.REPLICA_SET + "rs" | KubernetesApiGroup.APPS | KubernetesKind.REPLICA_SET + "replicaSet" | KubernetesApiGroup.EXTENSIONS | KubernetesKind.REPLICA_SET + "replicaSet" | CUSTOM_API_GROUP | null + + } + + @Unroll + void "getRegisteredKind returns kinds that have been registered"() { + given: + @Subject KubernetesKindRegistry kindRegistry = new KubernetesKindRegistry() + KubernetesKind result + + when: + result = kindRegistry.getRegisteredKind("customKind", CUSTOM_API_GROUP).orElse(null) + + then: + result == null + + when: + kindRegistry.registerKind(CUSTOM_KIND) + result = kindRegistry.getRegisteredKind("customKind", CUSTOM_API_GROUP).orElse(null) + + then: + result == CUSTOM_KIND + } + + @Unroll + void "getOrRegisterKind registers kinds that are not in the registry"() { + given: + @Subject KubernetesKindRegistry kindRegistry = new KubernetesKindRegistry() + KubernetesKind result + + when: + result = kindRegistry.getOrRegisterKind("customKind", CUSTOM_API_GROUP, {CUSTOM_KIND}) + + then: + result == CUSTOM_KIND + + when: + result = kindRegistry.getRegisteredKind("customKind", CUSTOM_API_GROUP).orElse(null) + + then: + result == CUSTOM_KIND + } + + @Unroll + void "getOrRegisterKind does not call the supplier for a kind that is already registered"() { + given: + @Subject KubernetesKindRegistry kindRegistry = new KubernetesKindRegistry() + kindRegistry.registerKind(CUSTOM_KIND) + + when: + def result = kindRegistry.getOrRegisterKind("customKind", CUSTOM_API_GROUP, { + throw new Exception("Should not have called supplier") + }) + + then: + result == CUSTOM_KIND + noExceptionThrown() + } + + @Unroll + void "getRegisteredKinds returns all kinds that are registered"() { + given: + @Subject KubernetesKindRegistry kindRegistry = new KubernetesKindRegistry() + Collection kinds + + when: + kinds = kindRegistry.getRegisteredKinds() + + then: + kinds.isEmpty() + + when: + kindRegistry.registerKind(KubernetesKind.REPLICA_SET) + kindRegistry.registerKind(CUSTOM_KIND) + kinds = kindRegistry.getRegisteredKinds() + + then: + kinds.size() == 2 + kinds.contains(KubernetesKind.REPLICA_SET) + kinds.contains(CUSTOM_KIND) + } +}