Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(manifests/helmfile): add helmfile templating engine #986

Merged
Merged
7 changes: 7 additions & 0 deletions Dockerfile.slim
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ LABEL maintainer="sig-platform@spinnaker.io"
ENV KUSTOMIZE_VERSION=3.8.6
ENV KUSTOMIZE4_VERSION=4.5.5
ENV PACKER_VERSION=1.8.1
ENV HELMFILE_VERSION=0.153.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that 0.154.0 has now appeared. I'm OK to go ahead with 0.153.1 and update in a follow-up PR.

Copy link
Contributor

@mazzy89 mazzy89 Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR, 0.155.0 has come out. Once this will be merged we are going to bump the version of helmfile.



ARG TARGETARCH
Expand Down Expand Up @@ -42,6 +43,12 @@ RUN mkdir kustomize && \
mv ./kustomize/kustomize /usr/local/bin/kustomize4 && \
rm -rf ./kustomize

RUN mkdir helmfile && \
curl -s -L https://github.com/helmfile/helmfile/releases/download/v${HELMFILE_VERSION}/helmfile_${HELMFILE_VERSION}_linux_${TARGETARCH}.tar.gz|\
tar xvz -C helmfile/ && \
mv ./helmfile/helmfile /usr/local/bin/helmfile && \
rm -rf ./helmfile

RUN addgroup -S -g 10111 spinnaker
RUN adduser -S -G spinnaker -u 10111 spinnaker
COPY rosco-web/build/install/rosco /opt/rosco
Expand Down
7 changes: 7 additions & 0 deletions Dockerfile.ubuntu
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ LABEL maintainer="sig-platform@spinnaker.io"
ENV KUSTOMIZE_VERSION=3.8.6
ENV KUSTOMIZE4_VERSION=4.5.5
ENV PACKER_VERSION=1.8.1
ENV HELMFILE_VERSION=0.153.1

ARG TARGETARCH

Expand Down Expand Up @@ -41,6 +42,12 @@ RUN mkdir kustomize && \
mv ./kustomize/kustomize /usr/local/bin/kustomize4 && \
rm -rf ./kustomize

RUN mkdir helmfile && \
curl -s -L https://github.com/helmfile/helmfile/releases/download/v${HELMFILE_VERSION}/helmfile_${HELMFILE_VERSION}_linux_${TARGETARCH}.tar.gz|\
tar xvz -C helmfile/ && \
mv ./helmfile/helmfile /usr/local/bin/helmfile && \
rm -rf ./helmfile

RUN adduser --system --uid 10111 --group spinnaker
COPY rosco-web/build/install/rosco /opt/rosco
COPY rosco-web/config /opt/rosco
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public enum TemplateRenderer {
HELM3,
KUSTOMIZE,
KUSTOMIZE4,
HELMFILE,
CF;

@JsonCreator
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright 2020 Grab Holdings, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Copyright 2020 Grab Holdings, Inc.
* Copyright 2023 Grab Holdings, 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.rosco.manifests.config;

import lombok.Data;
import org.springframework.boot.context.properties.ConfigurationProperties;

@ConfigurationProperties("helmfile")
@Data
public class RoscoHelmfileConfigurationProperties {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix Rosco is unnecessary

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other files have the Rosco prefix. we have just used the same conventions as the other files.

private String ExecutablePath = "helmfile";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private String ExecutablePath = "helmfile";
private String executablePath = "helmfile";

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.netflix.spinnaker.rosco.manifests.helmfile;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

License header, here and elsewhere.


import com.netflix.spinnaker.kork.artifacts.model.Artifact;
import com.netflix.spinnaker.rosco.manifests.BakeManifestRequest;
import java.util.List;
import lombok.Data;
import lombok.EqualsAndHashCode;

@Data
@EqualsAndHashCode(callSuper = true)
public class HelmfileBakeManifestRequest extends BakeManifestRequest {
private String helmfileFilePath;
private String environment;
private String namespace;

List<Artifact> inputArtifacts;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add javadoc for each member, but especially this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boolean includeCRDs;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package com.netflix.spinnaker.rosco.manifests.helmfile;

import static com.netflix.spinnaker.rosco.manifests.BakeManifestRequest.TemplateRenderer;

import com.google.common.collect.ImmutableSet;
import com.netflix.spinnaker.kork.artifacts.model.Artifact;
import com.netflix.spinnaker.rosco.jobs.BakeRecipe;
import com.netflix.spinnaker.rosco.jobs.JobExecutor;
import com.netflix.spinnaker.rosco.manifests.BakeManifestEnvironment;
import com.netflix.spinnaker.rosco.manifests.BakeManifestService;
import java.io.IOException;
import java.util.Base64;
import org.springframework.stereotype.Component;

@Component
public class HelmfileBakeManifestService extends BakeManifestService<HelmfileBakeManifestRequest> {
private final HelmfileTemplateUtils helmfileTemplateUtils;
private static final ImmutableSet<String> supportedTemplates =
ImmutableSet.of(TemplateRenderer.HELMFILE.toString());

public HelmfileBakeManifestService(
HelmfileTemplateUtils helmTemplateUtils, JobExecutor jobExecutor) {
super(jobExecutor);
this.helmfileTemplateUtils = helmTemplateUtils;
}

@Override
public Class<HelmfileBakeManifestRequest> requestType() {
return HelmfileBakeManifestRequest.class;
}

@Override
public boolean handles(String type) {
return supportedTemplates.contains(type);
}

public Artifact bake(HelmfileBakeManifestRequest helmfileBakeManifestRequest) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so close to HelmBakeManifestService. How can we share the code?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put this code within the new package suggested here com.netflix.spinnaker.rosco.manifests.utils.helm. what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code this is a function a bit hard to reduce in an OOP. I would need to create a separate class (abstract) to share only thing function. happy to hear here any proposals.

try (BakeManifestEnvironment env = BakeManifestEnvironment.create()) {
BakeRecipe recipe = helmfileTemplateUtils.buildBakeRecipe(env, helmfileBakeManifestRequest);

String bakeResult = helmfileTemplateUtils.removeTestsDirectoryTemplates(doBake(recipe));
return Artifact.builder()
.type("embedded/base64")
.name(helmfileBakeManifestRequest.getOutputArtifactName())
.reference(Base64.getEncoder().encodeToString(bakeResult.getBytes()))
.build();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
package com.netflix.spinnaker.rosco.manifests.helmfile;

import com.netflix.spinnaker.kork.artifacts.model.Artifact;
import com.netflix.spinnaker.kork.exceptions.SpinnakerException;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
import com.netflix.spinnaker.rosco.jobs.BakeRecipe;
import com.netflix.spinnaker.rosco.manifests.ArtifactDownloader;
import com.netflix.spinnaker.rosco.manifests.BakeManifestEnvironment;
import com.netflix.spinnaker.rosco.manifests.config.RoscoHelmConfigurationProperties;
import com.netflix.spinnaker.rosco.manifests.config.RoscoHelmfileConfigurationProperties;
import java.io.IOException;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.UUID;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Component;

@Component
@Slf4j
public class HelmfileTemplateUtils {
private static final String MANIFEST_SEPARATOR = "---\n";
private static final Pattern REGEX_TESTS_MANIFESTS =
Pattern.compile("# Source: .*/templates/tests/.*");

private final ArtifactDownloader artifactDownloader;
private final RoscoHelmfileConfigurationProperties helmfileConfigurationProperties;
private final RoscoHelmConfigurationProperties helmConfigurationProperties =
new RoscoHelmConfigurationProperties();

public HelmfileTemplateUtils(
ArtifactDownloader artifactDownloader,
RoscoHelmfileConfigurationProperties helmfileConfigurationProperties) {
this.artifactDownloader = artifactDownloader;
this.helmfileConfigurationProperties = helmfileConfigurationProperties;
}

public BakeRecipe buildBakeRecipe(
BakeManifestEnvironment env, HelmfileBakeManifestRequest request) throws IOException {
BakeRecipe result = new BakeRecipe();
result.setName(request.getOutputName());

Path helmfileFilePath;

List<Path> valuePaths = new ArrayList<>();
List<Artifact> inputArtifacts = request.getInputArtifacts();
if (inputArtifacts == null || inputArtifacts.isEmpty()) {
throw new IllegalArgumentException("At least one input artifact must be provided to bake");
}

log.info("helmfileFilePath: '{}'", request.getHelmfileFilePath());
Artifact helmfileTemplateArtifact = inputArtifacts.get(0);
String artifactType = Optional.ofNullable(helmfileTemplateArtifact.getType()).orElse("");
if ("git/repo".equals(artifactType)) {
env.downloadArtifactTarballAndExtract(artifactDownloader, helmfileTemplateArtifact);

// If there's no helmfile path specified, assume it lives in the root of
// the git/repo artifact.
helmfileFilePath =
env.resolvePath(Optional.ofNullable(request.getHelmfileFilePath()).orElse(""));
} else {
try {
helmfileFilePath = downloadArtifactToTmpFile(env, helmfileTemplateArtifact);
} catch (SpinnakerHttpException e) {
throw new SpinnakerHttpException(fetchFailureMessage("template", e), e);
} catch (IOException | SpinnakerException e) {
throw new IllegalStateException(fetchFailureMessage("template", e), e);
}
}

log.info("path to helmfile: {}", helmfileFilePath);

try {
// not a stream to keep exception handling cleaner
for (Artifact valueArtifact : inputArtifacts.subList(1, inputArtifacts.size())) {
valuePaths.add(downloadArtifactToTmpFile(env, valueArtifact));
}
} catch (SpinnakerHttpException e) {
throw new SpinnakerHttpException(fetchFailureMessage("values file", e), e);
} catch (IOException | SpinnakerException e) {
throw new IllegalStateException(fetchFailureMessage("values file", e), e);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From here up, this basically dupilcates HelmTemplateUtils.buildBakeRecipe. How can we share the code?

Copy link
Contributor

@mazzy89 mazzy89 Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. The HelmTemplateUtils is spread of duplications. What about to create a common utils class for both helm and helmfile baker? Not sure under which package to place though. Perhaps I would have to create a new package com.netflix.spinnaker.rosco.manifests.utils.helm. what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbyron-sf I've tried to make the HelmTemplateUtils and HelmfileTemplateUtils dryer reusing where I could the code. The only thing missing to reduce is https://github.com/thameezb/rosco/blob/7e1f5160aaa7459b22009484ea56c8b6dcd8aea9/rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/helmfile/HelmfileTemplateUtils.java#L55-L97 but I cannot find a good way to reduce everything when there are in the middle POJOs. do you have any better ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also reduce another bit the code in this commit 256756c


List<String> command = new ArrayList<>();
String executable = helmfileConfigurationProperties.getExecutablePath();

command.add(executable);
command.add("template");
command.add("--file");
command.add(helmfileFilePath.toString());

command.add("--helm-binary");
command.add(getHelm3ExecutablePath());

String environment = request.getEnvironment();
if (environment != null && !environment.isEmpty()) {
command.add("--environment");
command.add(environment);
}

String namespace = request.getNamespace();
if (namespace != null && !namespace.isEmpty()) {
command.add("--namespace");
command.add(namespace);
}

if (request.isIncludeCRDs()) {
command.add("--include-crds");
}

Map<String, Object> overrides = request.getOverrides();
if (!overrides.isEmpty()) {
List<String> overrideList = new ArrayList<>();
for (Map.Entry<String, Object> entry : overrides.entrySet()) {
overrideList.add(entry.getKey() + "=" + entry.getValue().toString());
}
command.add("--set");
command.add(String.join(",", overrideList));
}

if (!valuePaths.isEmpty()) {
command.add("--values");
command.add(valuePaths.stream().map(Path::toString).collect(Collectors.joining(",")));
}

result.setCommand(command);

return result;
}

private String fetchFailureMessage(String description, Exception e) {
return "Failed to fetch helmfile " + description + ": " + e.getMessage();
}

public String removeTestsDirectoryTemplates(String inputString) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's figure out a way to reduce the duplication / share the code in HelmTemplateUtils.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made a proposal for the this problem in here #986 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this method under an abstract class.

return Arrays.stream(inputString.split(MANIFEST_SEPARATOR))
.filter(manifest -> !REGEX_TESTS_MANIFESTS.matcher(manifest).find())
.collect(Collectors.joining(MANIFEST_SEPARATOR));
}

private Path downloadArtifactToTmpFile(BakeManifestEnvironment env, Artifact artifact)
throws IOException {
String fileName = UUID.randomUUID().toString();
Path targetPath = env.resolvePath(fileName);
artifactDownloader.downloadArtifactToFile(artifact, targetPath);
return targetPath;
}

private String getHelm3ExecutablePath() {
return helmConfigurationProperties.getV3ExecutablePath();
}
}
Loading