Skip to content

Commit

Permalink
refactor(kubernetes): Fix some unsafe casts in credential creation (#…
Browse files Browse the repository at this point in the history
…4023)

* refactor(kubernetes): Split CredentialFactory to V1 and V2

A lot of the unecessary casts that we do when creating credentials
stem from the fact that we have a single CredentialFactory with
different methods for creating V1 and V2 credentials.

Instead, make a generic interface KubernetesCredentialFactory that
pulls out the common logic to default methods.  (These default methods
should probably both eventually live somewhere else, but I think
having them on the interface is a resonable solution for now.)

Then give both the V1 and V2 credentials an inner factory class that
implements that interface and allows us to create credentials in a
type-safe way. It also logically makes more sense, as the factory for
credentials lives with the credentials it's creating.

* refactor(kubernetes): Add some generic bounds to the account code

Add some parametrized types to eliminate a bunch of unsafe casts.

* refactor(kubernetes): Push getSpinnakerKindMap to V1/V2

The getSpinnakerKindMap function on the base
KubernetesNamedAccountCredentials class is needed to support
sending the kind map to the UI. It forks using an if block
depending on which implementation we're using, which means
we should push the logic down to the actual implementations.

One solution would have been to keep the kind map at the top
level and just implement an "add custom resources" in the
V2 provider class (that is a no-op in V1). I didn't do that
because:
(1) It's strange that there's logic in this class anyway, which
is just supposed to hold some metadata and delegate down to
the actual credentials.
(2) Ultimately, it probably makes sense for the V2 provider to
implement its own account-specific KubernetesSpinnakerKindMap
instead of building this map on the fly. Pushing this down to
the V1/V2 implementations allows us to do this later.

* fix(kubernetes): Add missing import statements
  • Loading branch information
ezimanyi committed Sep 13, 2019
1 parent d179f8d commit 843ddbe
Show file tree
Hide file tree
Showing 26 changed files with 285 additions and 235 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@

package com.netflix.spinnaker.clouddriver.kubernetes.caching;

import com.netflix.spinnaker.clouddriver.kubernetes.security.KubernetesCredentials;
import com.netflix.spinnaker.clouddriver.kubernetes.security.KubernetesNamedAccountCredentials;
import java.util.Collection;

public interface KubernetesCachingAgentDispatcher {
Collection<KubernetesCachingAgent> buildAllCachingAgents(
KubernetesNamedAccountCredentials credentials);
public interface KubernetesCachingAgentDispatcher<C extends KubernetesCredentials> {
Collection<KubernetesCachingAgent<C>> buildAllCachingAgents(
KubernetesNamedAccountCredentials<C> credentials);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* 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.security;

import com.netflix.spinnaker.clouddriver.kubernetes.config.KubernetesConfigurationProperties;
import com.netflix.spinnaker.kork.configserver.ConfigFileService;
import org.apache.commons.lang3.StringUtils;

public interface KubernetesCredentialFactory<C extends KubernetesCredentials> {
C build(KubernetesConfigurationProperties.ManagedAccount managedAccount);

default void validateAccount(KubernetesConfigurationProperties.ManagedAccount managedAccount) {
if (StringUtils.isEmpty(managedAccount.getName())) {
throw new IllegalArgumentException("Account name for Kubernetes provider missing.");
}

if (!managedAccount.getOmitNamespaces().isEmpty()
&& !managedAccount.getNamespaces().isEmpty()) {
throw new IllegalArgumentException(
"At most one of 'namespaces' and 'omitNamespaces' can be specified");
}

if (!managedAccount.getOmitKinds().isEmpty() && !managedAccount.getKinds().isEmpty()) {
throw new IllegalArgumentException("At most one of 'kinds' and 'omitKinds' can be specified");
}
}

default String getKubeconfigFile(
ConfigFileService configFileService,
KubernetesConfigurationProperties.ManagedAccount managedAccount) {
if (StringUtils.isNotEmpty(managedAccount.getKubeconfigFile())) {
return configFileService.getLocalPath(managedAccount.getKubeconfigFile());
}

if (StringUtils.isNotEmpty(managedAccount.getKubeconfigContents())) {
return configFileService.getLocalPathForContents(
managedAccount.getKubeconfigContents(), managedAccount.getName());
}

return System.getProperty("user.home") + "/.kube/config";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@
package com.netflix.spinnaker.clouddriver.kubernetes.security;

import java.util.List;
import java.util.Map;

public interface KubernetesCredentials {
List<String> getDeclaredNamespaces();

Map<String, String> getSpinnakerKindMap();
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,31 +18,18 @@

import static lombok.EqualsAndHashCode.Include;

import com.netflix.spectator.api.Registry;
import com.netflix.spinnaker.clouddriver.kubernetes.KubernetesCloudProvider;
import com.netflix.spinnaker.clouddriver.kubernetes.config.KubernetesConfigurationProperties;
import com.netflix.spinnaker.clouddriver.kubernetes.v1.security.KubernetesV1Credentials;
import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.AccountResourcePropertyRegistry;
import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.KubernetesSpinnakerKindMap;
import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKindRegistry;
import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesManifest;
import com.netflix.spinnaker.clouddriver.kubernetes.v2.op.job.KubectlJobExecutor;
import com.netflix.spinnaker.clouddriver.kubernetes.v2.security.KubernetesV2Credentials;
import com.netflix.spinnaker.clouddriver.names.NamerRegistry;
import com.netflix.spinnaker.clouddriver.kubernetes.config.KubernetesConfigurationProperties.ManagedAccount;
import com.netflix.spinnaker.clouddriver.security.AccountCredentials;
import com.netflix.spinnaker.clouddriver.security.AccountCredentialsRepository;
import com.netflix.spinnaker.clouddriver.security.ProviderVersion;
import com.netflix.spinnaker.fiat.model.resources.Permissions;
import com.netflix.spinnaker.kork.configserver.ConfigFileService;
import java.util.*;
import javax.annotation.ParametersAreNonnullByDefault;
import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import org.apache.commons.lang3.StringUtils;
import org.springframework.stereotype.Component;

@Getter
@EqualsAndHashCode(onlyExplicitlyIncluded = true)
@ParametersAreNonnullByDefault
public class KubernetesNamedAccountCredentials<C extends KubernetesCredentials>
implements AccountCredentials<C> {
private final String cloudProvider = "kubernetes";
Expand All @@ -67,12 +54,8 @@ public class KubernetesNamedAccountCredentials<C extends KubernetesCredentials>

@Include private final Long cacheIntervalSeconds;

private final KubernetesSpinnakerKindMap kubernetesSpinnakerKindMap;

public KubernetesNamedAccountCredentials(
KubernetesConfigurationProperties.ManagedAccount managedAccount,
KubernetesSpinnakerKindMap kubernetesSpinnakerKindMap,
CredentialFactory factory) {
ManagedAccount managedAccount, KubernetesCredentialFactory<C> credentialFactory) {
this.name = managedAccount.getName();
this.providerVersion = managedAccount.getProviderVersion();
this.environment =
Expand All @@ -84,7 +67,6 @@ public KubernetesNamedAccountCredentials(
.orElse(managedAccount.getProviderVersion().toString());
this.cacheThreads = managedAccount.getCacheThreads();
this.cacheIntervalSeconds = managedAccount.getCacheIntervalSeconds();
this.kubernetesSpinnakerKindMap = kubernetesSpinnakerKindMap;

Permissions permissions = managedAccount.getPermissions().build();
if (permissions.isRestricted()) {
Expand All @@ -95,120 +77,14 @@ public KubernetesNamedAccountCredentials(
this.requiredGroupMembership =
Collections.unmodifiableList(managedAccount.getRequiredGroupMembership());
}

switch (managedAccount.getProviderVersion()) {
case v1:
this.credentials = (C) factory.buildV1Credentials(managedAccount);
break;
case v2:
this.credentials = (C) factory.buildV2Credentials(managedAccount);
break;
default:
throw new IllegalArgumentException(
"Unknown provider type: " + managedAccount.getProviderVersion());
}
this.credentials = credentialFactory.build(managedAccount);
}

public List<String> getNamespaces() {
return credentials.getDeclaredNamespaces();
}

public Map<String, String> getSpinnakerKindMap() {
if (kubernetesSpinnakerKindMap == null) {
return Collections.emptyMap();
}
Map<String, String> kindMap =
new HashMap<>(kubernetesSpinnakerKindMap.kubernetesToSpinnakerKindStringMap());
C creds = getCredentials();
if (creds instanceof KubernetesV2Credentials) {
((KubernetesV2Credentials) creds)
.getCustomResources()
.forEach(
customResource ->
kindMap.put(
customResource.getKubernetesKind(), customResource.getSpinnakerKind()));
}
return kindMap;
}

@Component
@RequiredArgsConstructor
public static class CredentialFactory {
private final String userAgent;
private final Registry spectatorRegistry;
private final NamerRegistry namerRegistry;
private final AccountCredentialsRepository accountCredentialsRepository;
private final KubectlJobExecutor jobExecutor;
private final ConfigFileService configFileService;
private final AccountResourcePropertyRegistry.Factory resourcePropertyRegistryFactory;
private final KubernetesKindRegistry.Factory kindRegistryFactory;

KubernetesV1Credentials buildV1Credentials(
KubernetesConfigurationProperties.ManagedAccount managedAccount) {
validateAccount(managedAccount);
return new KubernetesV1Credentials(
managedAccount.getName(),
getKubeconfigFile(managedAccount),
managedAccount.getContext(),
managedAccount.getCluster(),
managedAccount.getUser(),
userAgent,
managedAccount.isServiceAccount(),
managedAccount.isConfigureImagePullSecrets(),
managedAccount.getNamespaces(),
managedAccount.getOmitNamespaces(),
managedAccount.getDockerRegistries(),
spectatorRegistry,
accountCredentialsRepository);
}

KubernetesV2Credentials buildV2Credentials(
KubernetesConfigurationProperties.ManagedAccount managedAccount) {
validateAccount(managedAccount);
NamerRegistry.lookup()
.withProvider(KubernetesCloudProvider.ID)
.withAccount(managedAccount.getName())
.setNamer(
KubernetesManifest.class,
namerRegistry.getNamingStrategy(managedAccount.getNamingStrategy()));
return new KubernetesV2Credentials(
spectatorRegistry,
jobExecutor,
managedAccount,
resourcePropertyRegistryFactory,
kindRegistryFactory.create(),
getKubeconfigFile(managedAccount));
}

private void validateAccount(KubernetesConfigurationProperties.ManagedAccount managedAccount) {
if (StringUtils.isEmpty(managedAccount.getName())) {
throw new IllegalArgumentException("Account name for Kubernetes provider missing.");
}

if (!managedAccount.getOmitNamespaces().isEmpty()
&& !managedAccount.getNamespaces().isEmpty()) {
throw new IllegalArgumentException(
"At most one of 'namespaces' and 'omitNamespaces' can be specified");
}

if (!managedAccount.getOmitKinds().isEmpty() && !managedAccount.getKinds().isEmpty()) {
throw new IllegalArgumentException(
"At most one of 'kinds' and 'omitKinds' can be specified");
}
}

private String getKubeconfigFile(
KubernetesConfigurationProperties.ManagedAccount managedAccount) {
if (StringUtils.isNotEmpty(managedAccount.getKubeconfigFile())) {
return configFileService.getLocalPath(managedAccount.getKubeconfigFile());
}

if (StringUtils.isNotEmpty(managedAccount.getKubeconfigContents())) {
return configFileService.getLocalPathForContents(
managedAccount.getKubeconfigContents(), managedAccount.getName());
}

return System.getProperty("user.home") + "/.kube/config";
}
return credentials.getSpinnakerKindMap();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.netflix.spinnaker.clouddriver.kubernetes.config.KubernetesConfigurationProperties;
import com.netflix.spinnaker.clouddriver.kubernetes.security.KubernetesNamedAccountCredentials;
import com.netflix.spinnaker.clouddriver.kubernetes.v1.provider.agent.KubernetesV1CachingAgentDispatcher;
import com.netflix.spinnaker.clouddriver.kubernetes.v1.security.KubernetesV1Credentials;
import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.KubernetesSpinnakerKindMap;
import com.netflix.spinnaker.clouddriver.security.AccountCredentials;
import com.netflix.spinnaker.clouddriver.security.AccountCredentialsRepository;
Expand All @@ -44,7 +45,7 @@ public class KubernetesV1ProviderSynchronizable implements CredentialsInitialize
private AccountCredentialsRepository accountCredentialsRepository;
private KubernetesV1CachingAgentDispatcher kubernetesV1CachingAgentDispatcher;
private KubernetesConfigurationProperties kubernetesConfigurationProperties;
private KubernetesNamedAccountCredentials.CredentialFactory credentialFactory;
private KubernetesV1Credentials.Factory credentialFactory;
private KubernetesSpinnakerKindMap kubernetesSpinnakerKindMap;
private CatsModule catsModule;

Expand All @@ -53,7 +54,7 @@ public KubernetesV1ProviderSynchronizable(
AccountCredentialsRepository accountCredentialsRepository,
KubernetesV1CachingAgentDispatcher kubernetesV1CachingAgentDispatcher,
KubernetesConfigurationProperties kubernetesConfigurationProperties,
KubernetesNamedAccountCredentials.CredentialFactory credentialFactory,
KubernetesV1Credentials.Factory credentialFactory,
KubernetesSpinnakerKindMap kubernetesSpinnakerKindMap,
CatsModule catsModule) {
this.kubernetesV1Provider = kubernetesV1Provider;
Expand Down Expand Up @@ -98,15 +99,14 @@ private Set<String> synchronizeAccountCredentials() {
List<String> changedAccounts = new ArrayList<>();
Set<String> newAndChangedAccounts = new HashSet<>();

deletedAccounts.stream().forEach(accountCredentialsRepository::delete);
deletedAccounts.forEach(accountCredentialsRepository::delete);

kubernetesConfigurationProperties.getAccounts().stream()
.filter(a -> ProviderVersion.v1.equals(a.getProviderVersion()))
.forEach(
managedAccount -> {
KubernetesNamedAccountCredentials credentials =
new KubernetesNamedAccountCredentials(
managedAccount, kubernetesSpinnakerKindMap, credentialFactory);
new KubernetesNamedAccountCredentials<>(managedAccount, credentialFactory);

AccountCredentials existingCredentials =
accountCredentialsRepository.getOne(managedAccount.getName());
Expand Down
Loading

0 comments on commit 843ddbe

Please sign in to comment.