Skip to content

Commit

Permalink
fix(kubernetes): Don't register kinds on deserialization (#3840)
Browse files Browse the repository at this point in the history 
Currently every time we deserialize a KubernetesKind we attempt
to register that kind. In addition to affecting performance, this
is not what we actually want. We should not be registering every kind
encountered by clouddriver (including mis-spelled kinds on manifests,
etc.) into the static list of kinds.

If the kind we're deserializing *is* registered, we do want to return
that kind as the registered kind will have more information than we
have available from just the string, such as if the kind is namespaced
or if the kind has a cluster relationship. But if it's not registered
we don't want to register this kind (and in fact we have no additional
information to provide when registering the kind as just the string
doesn't tell us the other attributes anyway).

Clean up the public interface of this class so it's more clear when
we're registering kinds and when we're just fetching a kind. Namely:
(1) getOrRegisterKind will try to find an existing registered kind;
if none is present, it will register a new kind with the information
provided in the function call
(2) fromString will try to find an existing registered kind; if none
is present, it will return a new instance of KubernetesKind but will
*not* register that kind

There's still some follow-up work to clean up this class and also make
looking up registered kinds more efficient, but this seemed like a
reasonable start to make a PR.
  • Loading branch information
@ezimanyi
ezimanyi authored Jul 3, 2019
1 parent 7a60959 commit 3ec1701
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 73 deletions.
2 changes: 1 addition & 1 deletion ...netflix/spinnaker/clouddriver/kubernetes/v2/description/KubernetesResourceProperties.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public static KubernetesResourceProperties fromCustomResource(

KubernetesHandler handler =
CustomKubernetesHandlerFactory.create(
KubernetesKind.fromString(
KubernetesKind.getOrRegisterKind(
customResource.getKubernetesKind(), customResource.isNamespaced()),
KubernetesSpinnakerKindMap.SpinnakerKind.fromString(customResource.getSpinnakerKind()),
customResource.isVersioned(),
Expand Down
181 changes: 111 additions & 70 deletions .../com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesKind.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import javax.annotation.Nullable;
import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.StringUtils;

Expand All @@ -41,78 +42,79 @@ public final class KubernetesKind {
Collections.synchronizedList(new ArrayList<>());

public static final KubernetesKind API_SERVICE =
new KubernetesKind(
createAndRegisterKind(
"apiService", KubernetesApiGroup.APIREGISTRATION_K8S_IO, null, false, false);
public static final KubernetesKind CLUSTER_ROLE =
new KubernetesKind(
createAndRegisterKind(
"clusterRole", KubernetesApiGroup.RBAC_AUTHORIZATION_K8S_IO, null, false, false);
public static final KubernetesKind CLUSTER_ROLE_BINDING =
new KubernetesKind(
createAndRegisterKind(
"clusterRoleBinding", KubernetesApiGroup.RBAC_AUTHORIZATION_K8S_IO, null, false, false);
public static final KubernetesKind CONFIG_MAP =
new KubernetesKind("configMap", KubernetesApiGroup.CORE, "cm", true, false);
createAndRegisterKind("configMap", KubernetesApiGroup.CORE, "cm", true, false);
public static final KubernetesKind CONTROLLER_REVISION =
new KubernetesKind("controllerRevision", KubernetesApiGroup.APPS, null, true, false);
createAndRegisterKind("controllerRevision", KubernetesApiGroup.APPS, null, true, false);
public static final KubernetesKind CUSTOM_RESOURCE_DEFINITION =
new KubernetesKind(
createAndRegisterKind(
"customResourceDefinition", KubernetesApiGroup.EXTENSIONS, "crd", false, false);
public static final KubernetesKind CRON_JOB =
new KubernetesKind("cronJob", KubernetesApiGroup.BATCH, null, true, false);
createAndRegisterKind("cronJob", KubernetesApiGroup.BATCH, null, true, false);
public static final KubernetesKind DAEMON_SET =
new KubernetesKind("daemonSet", KubernetesApiGroup.APPS, "ds", true, true);
createAndRegisterKind("daemonSet", KubernetesApiGroup.APPS, "ds", true, true);
public static final KubernetesKind DEPLOYMENT =
new KubernetesKind("deployment", KubernetesApiGroup.APPS, "deploy", true, true);
createAndRegisterKind("deployment", KubernetesApiGroup.APPS, "deploy", true, true);
public static final KubernetesKind EVENT =
new KubernetesKind("event", KubernetesApiGroup.CORE, null, true, false);
createAndRegisterKind("event", KubernetesApiGroup.CORE, null, true, false);
public static final KubernetesKind HORIZONTAL_POD_AUTOSCALER =
new KubernetesKind(
createAndRegisterKind(
"horizontalpodautoscaler", KubernetesApiGroup.AUTOSCALING, "hpa", true, false);
public static final KubernetesKind INGRESS =
new KubernetesKind("ingress", KubernetesApiGroup.EXTENSIONS, "ing", true, true);
createAndRegisterKind("ingress", KubernetesApiGroup.EXTENSIONS, "ing", true, true);
public static final KubernetesKind JOB =
new KubernetesKind("job", KubernetesApiGroup.BATCH, null, true, false);
createAndRegisterKind("job", KubernetesApiGroup.BATCH, null, true, false);
public static final KubernetesKind MUTATING_WEBHOOK_CONFIGURATION =
new KubernetesKind(
createAndRegisterKind(
"mutatingWebhookConfiguration",
KubernetesApiGroup.ADMISSIONREGISTRATION_K8S_IO,
null,
false,
false);
public static final KubernetesKind NAMESPACE =
new KubernetesKind("namespace", KubernetesApiGroup.CORE, "ns", false, false);
createAndRegisterKind("namespace", KubernetesApiGroup.CORE, "ns", false, false);
public static final KubernetesKind NETWORK_POLICY =
new KubernetesKind("networkPolicy", KubernetesApiGroup.EXTENSIONS, "netpol", true, true);
createAndRegisterKind("networkPolicy", KubernetesApiGroup.EXTENSIONS, "netpol", true, true);
public static final KubernetesKind PERSISTENT_VOLUME =
new KubernetesKind("persistentVolume", KubernetesApiGroup.CORE, "pv", false, false);
createAndRegisterKind("persistentVolume", KubernetesApiGroup.CORE, "pv", false, false);
public static final KubernetesKind PERSISTENT_VOLUME_CLAIM =
new KubernetesKind("persistentVolumeClaim", KubernetesApiGroup.CORE, "pvc", true, false);
createAndRegisterKind("persistentVolumeClaim", KubernetesApiGroup.CORE, "pvc", true, false);
public static final KubernetesKind POD =
new KubernetesKind("pod", KubernetesApiGroup.CORE, "po", true, false);
createAndRegisterKind("pod", KubernetesApiGroup.CORE, "po", true, false);
public static final KubernetesKind POD_PRESET =
new KubernetesKind("podPreset", KubernetesApiGroup.SETTINGS_K8S_IO, null, true, false);
createAndRegisterKind("podPreset", KubernetesApiGroup.SETTINGS_K8S_IO, null, true, false);
public static final KubernetesKind POD_SECURITY_POLICY =
new KubernetesKind("podSecurityPolicy", KubernetesApiGroup.EXTENSIONS, null, false, false);
createAndRegisterKind("podSecurityPolicy", KubernetesApiGroup.EXTENSIONS, null, false, false);
public static final KubernetesKind POD_DISRUPTION_BUDGET =
new KubernetesKind("podDisruptionBudget", KubernetesApiGroup.POLICY, null, true, false);
createAndRegisterKind("podDisruptionBudget", KubernetesApiGroup.POLICY, null, true, false);
public static final KubernetesKind REPLICA_SET =
new KubernetesKind("replicaSet", KubernetesApiGroup.APPS, "rs", true, true);
createAndRegisterKind("replicaSet", KubernetesApiGroup.APPS, "rs", true, true);
public static final KubernetesKind ROLE =
new KubernetesKind("role", KubernetesApiGroup.RBAC_AUTHORIZATION_K8S_IO, null, true, false);
createAndRegisterKind(
"role", KubernetesApiGroup.RBAC_AUTHORIZATION_K8S_IO, null, true, false);
public static final KubernetesKind ROLE_BINDING =
new KubernetesKind(
createAndRegisterKind(
"roleBinding", KubernetesApiGroup.RBAC_AUTHORIZATION_K8S_IO, null, true, false);
public static final KubernetesKind SECRET =
new KubernetesKind("secret", KubernetesApiGroup.CORE, null, true, false);
createAndRegisterKind("secret", KubernetesApiGroup.CORE, null, true, false);
public static final KubernetesKind SERVICE =
new KubernetesKind("service", KubernetesApiGroup.CORE, "svc", true, true);
createAndRegisterKind("service", KubernetesApiGroup.CORE, "svc", true, true);
public static final KubernetesKind SERVICE_ACCOUNT =
new KubernetesKind("serviceAccount", KubernetesApiGroup.CORE, "sa", true, false);
createAndRegisterKind("serviceAccount", KubernetesApiGroup.CORE, "sa", true, false);
public static final KubernetesKind STATEFUL_SET =
new KubernetesKind("statefulSet", KubernetesApiGroup.APPS, null, true, true);
createAndRegisterKind("statefulSet", KubernetesApiGroup.APPS, null, true, true);
public static final KubernetesKind STORAGE_CLASS =
new KubernetesKind("storageClass", KubernetesApiGroup.STORAGE_K8S_IO, "sc", false, false);
createAndRegisterKind("storageClass", KubernetesApiGroup.STORAGE_K8S_IO, "sc", false, false);
public static final KubernetesKind VALIDATING_WEBHOOK_CONFIGURATION =
new KubernetesKind(
createAndRegisterKind(
"validatingWebhookConfiguration",
KubernetesApiGroup.ADMISSIONREGISTRATION_K8S_IO,
null,
Expand All @@ -122,7 +124,7 @@ public final class KubernetesKind {
// special kind that should never be assigned to a manifest, used only to represent objects whose
// kind is not in spinnaker's registry
public static final KubernetesKind NONE =
new KubernetesKind("none", KubernetesApiGroup.NONE, null, true, false);
createAndRegisterKind("none", KubernetesApiGroup.NONE, null, true, false);

@Nonnull private final String name;
@EqualsAndHashCode.Include @Nonnull private final String lcName;
Expand All @@ -138,13 +140,17 @@ public final class KubernetesKind {
// was this kind added by a user in their clouddriver.yml?
@Getter private final boolean isRegistered;

private KubernetesKind(
private static KubernetesKind createAndRegisterKind(
@Nonnull String name,
@Nonnull KubernetesApiGroup apiGroup,
@Nullable String alias,
boolean isNamespaced,
boolean hasClusterRelationship) {
this(name, apiGroup, alias, isNamespaced, hasClusterRelationship, false, true);
KubernetesKind kind =
new KubernetesKind(
name, apiGroup, alias, isNamespaced, hasClusterRelationship, false, true);
values.add(kind);
return kind;
}

private KubernetesKind(
Expand All @@ -168,7 +174,6 @@ private KubernetesKind(
this.hasClusterRelationship = hasClusterRelationship;
this.isDynamic = isDynamic;
this.isRegistered = isRegistered;
values.add(this);
}

public boolean hasClusterRelationship() {
Expand All @@ -184,35 +189,47 @@ public String toString() {
return name + "." + apiGroup.toString();
}

@JsonCreator
@Nonnull
public static KubernetesKind fromString(@Nonnull String name) {
return fromString(name, true);
}

@Nonnull
public static KubernetesKind fromString(@Nonnull String name, boolean namespaced) {
private static ScopedKind parseQualifiedKind(@Nonnull String qualifiedKind) {
KubernetesApiGroup apiGroup;
String kindName;
String[] parts = StringUtils.split(name, ".", 2);
String[] parts = StringUtils.split(qualifiedKind, ".", 2);
if (parts.length == 2) {
kindName = parts[0];
apiGroup = KubernetesApiGroup.fromString(parts[1]);
} else {
kindName = name;
kindName = qualifiedKind;
apiGroup = null;
}
return KubernetesKind.getOrRegisterKind(kindName, true, namespaced, apiGroup);
return new ScopedKind(kindName, apiGroup);
}

@JsonCreator
@Nonnull
public static KubernetesKind getOrRegisterKind(
@Nonnull final String name,
final boolean registered,
final boolean namespaced,
@Nullable final KubernetesApiGroup apiGroup) {
public static KubernetesKind fromString(@Nonnull final String name) {
ScopedKind scopedKind = parseQualifiedKind(name);
return fromString(scopedKind.kindName, scopedKind.apiGroup);
}

public static KubernetesKind fromString(
@Nonnull final String name, @Nullable final KubernetesApiGroup apiGroup) {
return getRegisteredKind(name, apiGroup)
.orElseGet(
() ->
new KubernetesKind(
name,
Optional.ofNullable(apiGroup).orElse(KubernetesApiGroup.NONE),
null,
true,
false,
true,
false));
}

private static Optional<KubernetesKind> getRegisteredKind(
@Nonnull final String name, @Nullable final KubernetesApiGroup apiGroup) {
if (StringUtils.isEmpty(name)) {
return KubernetesKind.NONE;
return Optional.of(KubernetesKind.NONE);
}

if (name.equalsIgnoreCase(KubernetesKind.NONE.toString())) {
Expand All @@ -234,16 +251,24 @@ public static KubernetesKind getOrRegisterKind(
return false;
};

synchronized (values) {
Optional<KubernetesKind> kindOptional =
values.stream()
.filter(
v ->
v.name.equalsIgnoreCase(name)
|| (v.alias != null && v.alias.equalsIgnoreCase(name)))
.filter(groupMatches)
.findAny();
// 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<KubernetesKind> kindOptional = getRegisteredKind(name, apiGroup);
// separate from the above chain to avoid concurrent modification of the values list
return kindOptional.orElseGet(
() -> {
Expand All @@ -252,19 +277,35 @@ public static KubernetesKind getOrRegisterKind(
name,
namespaced,
registered);
return new KubernetesKind(
name,
Optional.ofNullable(apiGroup).orElse(KubernetesApiGroup.NONE),
null,
namespaced,
false,
true,
registered);
KubernetesKind kind =
new KubernetesKind(
name,
Optional.ofNullable(apiGroup).orElse(KubernetesApiGroup.NONE),
null,
namespaced,
false,
true,
registered);
values.add(kind);
return kind;
});
}
}

public static List<KubernetesKind> registeredStringList(List<String> names) {
return names.stream().map(KubernetesKind::fromString).collect(Collectors.toList());
@Nonnull
public static KubernetesKind getOrRegisterKind(
@Nonnull final String qualifiedName, boolean isNamespaced) {
ScopedKind scopedKind = parseQualifiedKind(qualifiedName);
return getOrRegisterKind(scopedKind.kindName, true, isNamespaced, scopedKind.apiGroup);
}

public static List<KubernetesKind> getOrRegisterKinds(List<String> names) {
return names.stream().map(k -> getOrRegisterKind(k, true)).collect(Collectors.toList());
}

@RequiredArgsConstructor
private static class ScopedKind {
public final String kindName;
public final KubernetesApiGroup apiGroup;
}
}
2 changes: 1 addition & 1 deletion .../netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesManifest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public KubernetesKind getKind() {
} else {
kubernetesApiGroup = null;
}
return KubernetesKind.getOrRegisterKind(kindName, true, true, kubernetesApiGroup);
return KubernetesKind.fromString(kindName, kubernetesApiGroup);
}

@JsonIgnore
Expand Down
2 changes: 1 addition & 1 deletion ...ovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2Credentials.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public KubernetesV2Credentials(
this.accountName = managedAccount.getName();
this.namespaces = managedAccount.getNamespaces();
this.omitNamespaces = managedAccount.getOmitNamespaces();
this.kinds = KubernetesKind.registeredStringList(managedAccount.getKinds());
this.kinds = KubernetesKind.getOrRegisterKinds(managedAccount.getKinds());
this.omitKinds =
managedAccount.getOmitKinds().stream()
.map(KubernetesKind::fromString)
Expand Down

0 comments on commit 3ec1701

Please sign in to comment.