Skip to content

Commit

Permalink
feat(kubernetes): Skip applying labels to spec.template.metadata if s…
Browse files Browse the repository at this point in the history
…kipSpecTemplateLabels is true

Moved the OperationDescription from clouddriver-api to kork-moniker and updated all the relevant references.
This will allow us to pass Manifest Description to the Namer class.

Setting skipSpecTemplateLabels to true, clouddriver will not apply the app.kubernetes.io/* labels to the manifests' spec.template.metadata.labels.

---------

Co-authored-by: Kunal Sharma <[email protected]>
  • Loading branch information
Kunal Sharma and Kunal Sharma committed Jul 25, 2024
1 parent a5fc30d commit 9e12a38
Show file tree
Hide file tree
Showing 28 changed files with 132 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package com.netflix.spinnaker.clouddriver.orchestration;

import com.netflix.spinnaker.kork.annotations.Beta;
import com.netflix.spinnaker.orchestration.OperationDescription;
import java.util.Map;
import javax.annotation.Nullable;

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

package com.netflix.spinnaker.clouddriver.aws.deploy.description

import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription
import com.netflix.spinnaker.orchestration.OperationDescription

class AllowLaunchDescription extends AbstractAmazonCredentialsDescription {
String targetAccount
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package com.netflix.spinnaker.clouddriver.azure.resources.common
import com.fasterxml.jackson.databind.DeserializationFeature
import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.clouddriver.azure.security.AzureCredentials
import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription
import com.netflix.spinnaker.orchestration.OperationDescription

class AzureResourceOpsDescription implements OperationDescription {
static ObjectMapper mapper = new ObjectMapper().configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@
import com.netflix.spinnaker.clouddriver.cloudrun.security.CloudrunNamedAccountCredentials;
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation;
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperations;
import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription;
import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsConverter;
import com.netflix.spinnaker.orchestration.OperationDescription;
import java.util.Map;
import org.springframework.stereotype.Component;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ package com.netflix.spinnaker.clouddriver.core

import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperationConverter
import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription
import com.netflix.spinnaker.orchestration.OperationDescription
import groovy.util.logging.Slf4j
import org.springframework.stereotype.Component

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

package com.netflix.spinnaker.clouddriver.deploy;

import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription;
import com.netflix.spinnaker.orchestration.OperationDescription;
import org.springframework.validation.Errors;

public class DefaultDescriptionAuthorizer implements DescriptionAuthorizer {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

package com.netflix.spinnaker.clouddriver.deploy;

import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription;
import com.netflix.spinnaker.clouddriver.orchestration.events.OperationEvent;
import com.netflix.spinnaker.orchestration.OperationDescription;
import java.util.Collection;
import java.util.Collections;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package com.netflix.spinnaker.clouddriver.deploy;

import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription;
import com.netflix.spinnaker.orchestration.OperationDescription;
import org.springframework.validation.Errors;

/** Authorizes atomic operation description objects. */
Expand All @@ -13,4 +13,4 @@ default boolean supports(Object description) {

/** @param description - The atomic operation description object. */
void authorize(OperationDescription description, Errors errors);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.netflix.spinnaker.clouddriver.security.AllowedAccountsValidator;
import com.netflix.spinnaker.kork.exceptions.SystemException;
import com.netflix.spinnaker.kork.web.exceptions.ExceptionMessageDecorator;
import com.netflix.spinnaker.orchestration.OperationDescription;
import com.netflix.spinnaker.security.AuthenticatedRequest;
import java.lang.reflect.Method;
import java.util.ArrayList;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package com.netflix.spinnaker.clouddriver.orchestration
import com.netflix.spinnaker.clouddriver.core.CloudProvider
import com.netflix.spinnaker.clouddriver.deploy.DescriptionValidator
import com.netflix.spinnaker.clouddriver.deploy.ValidationErrors
import com.netflix.spinnaker.orchestration.OperationDescription
import org.springframework.beans.factory.NoSuchBeanDefinitionException
import org.springframework.context.annotation.AnnotationConfigApplicationContext
import org.springframework.context.annotation.Bean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import com.netflix.spinnaker.clouddriver.deploy.DefaultDescriptionAuthorizer
import com.netflix.spinnaker.clouddriver.saga.persistence.SagaRepository
import com.netflix.spinnaker.clouddriver.security.AccountCredentialsRepository
import com.netflix.spinnaker.kork.web.exceptions.ExceptionMessageDecorator
import com.netflix.spinnaker.orchestration.OperationDescription
import org.springframework.context.annotation.AnnotationConfigApplicationContext
import org.springframework.context.annotation.Bean
import org.springframework.context.annotation.Configuration
Expand Down
1 change: 1 addition & 0 deletions clouddriver-elasticsearch/clouddriver-elasticsearch.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ dependencies {
implementation "com.netflix.frigga:frigga"
implementation "io.searchbox:jest:6.3.1"
implementation "io.spinnaker.kork:kork-exceptions"
implementation "io.spinnaker.kork:kork-moniker"
implementation "io.spinnaker.kork:kork-retrofit"
implementation "io.spinnaker.kork:kork-security"
implementation "com.squareup.retrofit:retrofit"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
package com.netflix.spinnaker.clouddriver.elasticsearch.descriptions;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription;
import com.netflix.spinnaker.clouddriver.security.resources.NonCredentialed;
import com.netflix.spinnaker.orchestration.OperationDescription;
import java.util.List;

public class DeleteEntityTagsDescription implements NonCredentialed, OperationDescription {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import com.netflix.spinnaker.clouddriver.google.provider.view.GoogleClusterProvi
import com.netflix.spinnaker.clouddriver.google.security.GoogleNamedAccountCredentials
import com.netflix.spinnaker.clouddriver.orchestration.*
import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsConverter
import com.netflix.spinnaker.orchestration.OperationDescription
import com.fasterxml.jackson.databind.ObjectMapper
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.stereotype.Component
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public class KubernetesDeployManifestDescription extends KubernetesAtomicOperati
private List<String> services;
private Strategy strategy;
private KubernetesSelectorList labelSelectors = new KubernetesSelectorList();
private boolean skipSpecTemplateLabels = false;

/**
* If false, and using (non-empty) label selectors, fail if a deploy manifest operation doesn't
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,22 @@ private static void storeLabel(Map<String, String> labels, String key, String va
}

public static void labelManifest(
String managedBySuffix, KubernetesManifest manifest, Moniker moniker) {
String managedBySuffix,
KubernetesManifest manifest,
Moniker moniker,
Boolean skipSpecTemplateLabels) {
Map<String, String> labels = manifest.getLabels();
storeLabels(managedBySuffix, labels, moniker);

manifest.getSpecTemplateLabels().ifPresent(l -> storeLabels(managedBySuffix, l, moniker));
// Deployment fails for some Kubernetes resources (e.g. Karpenter NodePool) when
// the app.kubernetes.io/* labels are applied to the manifest's
// .spec.template.metadata.labels. If skipSpecTemplateLabels is
// set to true in the manifest description, Spinnaker won't apply
// the Kubernetes and Moniker labels
// to the .spec.template.metadata.labels of the manifest.
if (!skipSpecTemplateLabels) {
manifest.getSpecTemplateLabels().ifPresent(l -> storeLabels(managedBySuffix, l, moniker));
}
}

public static void storeLabels(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@

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

import com.netflix.spinnaker.clouddriver.kubernetes.description.manifest.KubernetesDeployManifestDescription;
import com.netflix.spinnaker.clouddriver.kubernetes.description.manifest.KubernetesManifest;
import com.netflix.spinnaker.clouddriver.kubernetes.description.manifest.KubernetesManifestAnnotater;
import com.netflix.spinnaker.clouddriver.kubernetes.description.manifest.KubernetesManifestLabeler;
import com.netflix.spinnaker.clouddriver.names.NamingStrategy;
import com.netflix.spinnaker.moniker.Moniker;
import com.netflix.spinnaker.orchestration.OperationDescription;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.stereotype.Component;
Expand Down Expand Up @@ -50,9 +52,46 @@ public String getName() {

@Override
public void applyMoniker(KubernetesManifest obj, Moniker moniker) {
applyMoniker(obj, moniker, null);
}

/**
* Applies the given Moniker to the specified KubernetesManifest. If the provided
* OperationDescription is an instance of KubernetesDeployManifestDescription, the method will
* annotate and label the manifest. If
* KubernetesDeployManifestDescription.isSkipSpecTemplateLabels() is true, skip applying the
* Kubernetes and Moniker labels to the manifest's spec.template.metadata.labels. If the
* OperationDescription is null, or the
* KubernetesDeployManifestDescription.isSkipSpecTemplateLabels() is false, apply the Kubernetes
* and Moniker labels to the manifest's spec.template.metadata.labels
*
* @param obj the KubernetesManifest to which the moniker will be applied
* @param moniker the moniker to apply
* @param description a description expected to be of type KubernetesDeployManifestDescription
* that provides context for the operation.
*/
@Override
public void applyMoniker(
KubernetesManifest obj, Moniker moniker, OperationDescription description) {
// The OperationDescription passed to this method must
// always have the dynamic type of KubernetesDeployManifestDescription.
// If not, fail the operation.
if (description != null && !(description instanceof KubernetesDeployManifestDescription)) {
throw new IllegalArgumentException(
String.format(
"OperationDescription passed to Namer.applyMoniker() must be a KubernetesDeployManifestDescription for the KubernetesDeployManifestOperation. Provided description: %s",
description.getClass().getName()));
}
KubernetesManifestAnnotater.annotateManifest(obj, moniker);
if (applyAppLabels) {
KubernetesManifestLabeler.labelManifest(managedBySuffix, obj, moniker);
KubernetesDeployManifestDescription kubernetesDeployManifestDescription =
(KubernetesDeployManifestDescription) description;
boolean skipSpecTemplateLabels =
(kubernetesDeployManifestDescription != null)
? kubernetesDeployManifestDescription.isSkipSpecTemplateLabels()
: false;
KubernetesManifestLabeler.labelManifest(
managedBySuffix, obj, moniker, skipSpecTemplateLabels);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public OperationResult operate(List<OperationResult> _unused) {
}
}

credentials.getNamer().applyMoniker(manifest, moniker);
credentials.getNamer().applyMoniker(manifest, moniker, description);
manifest.setName(artifact.getReference());

return new ManifestArtifactHolder(manifest, artifact, strategy);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,60 @@ void failsWhenServiceSelectorOverlapsWithTargetLabels() {
assertThatThrownBy(() -> deploy(description)).isInstanceOf(IllegalArgumentException.class);
}

@Test
void appliesSpecTemplateLabelsWhenSkipSpecTemplateLabelsFalse() {
// When skipSpecTemplateLabels is false, defaults to the standard flow,
// where applyMoniker() applies the Kubernetes and Moniker labels to both the
// manifest's metadata.labels and the spec.template.metadata.labels.
KubernetesDeployManifestDescription description =
baseDeployDescription("deploy/replicaset-no-namespace.yml")
.setSkipSpecTemplateLabels(false);
OperationResult result = deploy(description);

KubernetesManifest manifest = Iterables.getOnlyElement(result.getManifests());
KubernetesCredentials credentials = description.getCredentials().getCredentials();

// Verifying that the getNamer() method is called only once
// (credentials.getNamer().applyMoniker()).
verify(credentials, times(1)).getNamer();
// Assert that the Kubernetes labels are also applied to the spec template labels. The test
// manifest only has "app: nginx" within it's spec.template.metadata.labels.
assertThat(manifest.getSpecTemplateLabels()).isPresent();
manifest
.getSpecTemplateLabels()
.ifPresent(
l ->
assertThat(l)
.contains(
entry("app", "nginx"), entry("app.kubernetes.io/managed-by", "spinnaker")));
// Verify that the Kubernetes and Moniker labels are applied to the metadata labels.
assertThat(manifest.getLabels()).contains(entry("app.kubernetes.io/managed-by", "spinnaker"));
}

@Test
void doesNotApplySpecTemplateLabelsWhenSkipSpecTemplateLabelsTrue() {
// When skipSpecTemplateLabels is true, applyMoniker() skips applying
// the Kubernetes and Moniker labels to the manifest's spec.template.metadata.labels.
KubernetesDeployManifestDescription description =
baseDeployDescription("deploy/replicaset-no-namespace.yml").setSkipSpecTemplateLabels(true);
OperationResult result = deploy(description);

KubernetesManifest manifest = Iterables.getOnlyElement(result.getManifests());
KubernetesCredentials credentials = description.getCredentials().getCredentials();

// Verifying that the getNamer() method is called only once
// (credentials.getNamer().applyMoniker()).
verify(credentials, times(1)).getNamer();
// Assert that the spec template labels only has the "app: nginx" from the manifest and that no
// other labels were applied.
assertThat(manifest.getSpecTemplateLabels()).isPresent();
manifest
.getSpecTemplateLabels()
.ifPresent(l -> assertThat(l).containsExactly(entry("app", "nginx")));
// Verify that the Kubernetes and Moniker labels are still applied to the metadata labels.
assertThat(manifest.getLabels()).contains(entry("app.kubernetes.io/managed-by", "spinnaker"));
}

@Test
void deploysWithArtifactBindingDisabled() {
KubernetesDeployManifestDescription description =
Expand Down
1 change: 1 addition & 0 deletions clouddriver-lambda/clouddriver-lambda.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ dependencies {
implementation "com.netflix.awsobjectmapper:awsobjectmapper"
implementation "io.spinnaker.kork:kork-artifacts"
implementation "io.spinnaker.kork:kork-core"
implementation "io.spinnaker.kork:kork-moniker"
implementation "org.apache.commons:commons-lang3"
implementation "org.apache.httpcomponents:httpclient"
implementation "org.apache.httpcomponents:httpcore"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ package com.netflix.spinnaker.clouddriver.oracle.deploy.converter
import com.netflix.spinnaker.clouddriver.oracle.OracleOperation
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperations
import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription
import com.netflix.spinnaker.orchestration.OperationDescription
import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport
import org.springframework.stereotype.Component

Expand Down
1 change: 1 addition & 0 deletions clouddriver-security/clouddriver-security.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ dependencies {
implementation "io.spinnaker.fiat:fiat-api:$fiatVersion"
implementation "io.spinnaker.fiat:fiat-core:$fiatVersion"
implementation "io.spinnaker.kork:kork-core"
implementation "io.spinnaker.kork:kork-moniker"
implementation "org.codehaus.groovy:groovy"
implementation "org.slf4j:jcl-over-slf4j"
implementation "org.springframework.boot:spring-boot-starter-web"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

package com.netflix.spinnaker.clouddriver.security.resources;

import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription;
import com.netflix.spinnaker.clouddriver.security.config.SecurityConfig;
import com.netflix.spinnaker.orchestration.OperationDescription;

/** Denotes an operation description operates on a specific account. */
public interface AccountNameable extends OperationDescription {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

package com.netflix.spinnaker.clouddriver.security.resources;

import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription;
import com.netflix.spinnaker.orchestration.OperationDescription;
import java.util.Collection;

/** Denotes an operation description operates on one or more specific application resources. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

package com.netflix.spinnaker.clouddriver.security.resources;

import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription;
import com.netflix.spinnaker.orchestration.OperationDescription;

/**
* Marker interface indicating that a description does not have account-level credentials specified.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import com.netflix.spinnaker.clouddriver.elasticsearch.converters.UpsertEntityTa
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperationConverter
import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperations
import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription
import com.netflix.spinnaker.orchestration.OperationDescription
import org.springframework.stereotype.Component
import spock.lang.Specification

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
package com.netflix.spinnaker.clouddriver.yandex.deploy;

import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation;
import com.netflix.spinnaker.clouddriver.orchestration.OperationDescription;
import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport;
import com.netflix.spinnaker.orchestration.OperationDescription;
import java.util.Map;
import javax.annotation.Nullable;

Expand Down

0 comments on commit 9e12a38

Please sign in to comment.