From 1b511dc64aa057d71e68c1c86a7375c1541b94c8 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Tue, 1 Aug 2023 13:23:03 +0200 Subject: [PATCH] fix(k8s): regression in globs in k8s manifest files (#4903) * fix(k8s): throw on missing k8s manifests files The regression was introduced in #4516. Initial implementation of glob patterns in files paths could result into empty list of files. This fixes the behaviour by fail-fast existence checks for manifest files. * test: add missing tests for `readManifest` Test coverage for `spec.files` of `Deploy` action of `kubernetes` type. * Add own Deployment manifest file, because builds are not executed in the test. * Add unit tests to document and cover the expected behaviour. Initial changes from c9efb473d4f352ac13f12a7f8ada2a2b28445cb3 did not have effect. The Deploy action `with-build-action` has never been called with `readManifests`. --- .../kubernetes/kubernetes-type/common.ts | 54 +++++++- .../with-build-action/deployment-action.yaml | 29 +++++ .../with-build-action/with-build.garden.yml | 2 +- .../kubernetes/kubernetes-type/common.ts | 115 +++++++++++++++--- 4 files changed, 178 insertions(+), 22 deletions(-) create mode 100644 core/test/data/test-projects/kubernetes-type/with-build-action/deployment-action.yaml diff --git a/core/src/plugins/kubernetes/kubernetes-type/common.ts b/core/src/plugins/kubernetes/kubernetes-type/common.ts index e7d4e45b09..d7dcd4e928 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/common.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/common.ts @@ -7,7 +7,7 @@ */ import { resolve } from "path" -import { readFile } from "fs-extra" +import { pathExists, readFile } from "fs-extra" import Bluebird from "bluebird" import { flatten, set } from "lodash" import { loadAll } from "js-yaml" @@ -15,7 +15,7 @@ import { loadAll } from "js-yaml" import { KubernetesModule } from "./module-config" import { KubernetesResource } from "../types" import { KubeApi } from "../api" -import { gardenAnnotationKey } from "../../../util/string" +import { gardenAnnotationKey, naturalList } from "../../../util/string" import { Log } from "../../../logger/log-entry" import { PluginContext } from "../../../plugin-context" import { ConfigurationError, PluginError } from "../../../exceptions" @@ -24,10 +24,11 @@ import { HelmModule } from "../helm/module-config" import { KubernetesDeployAction } from "./config" import { CommonRunParams } from "../../../plugin/handlers/Run/run" import { runAndCopy } from "../run" -import { getTargetResource, getResourcePodSpec, getResourceContainer, makePodName } from "../util" +import { getResourceContainer, getResourcePodSpec, getTargetResource, makePodName } from "../util" import { Resolved } from "../../../actions/types" import { KubernetesPodRunAction, KubernetesPodTestAction } from "./kubernetes-pod" import { glob } from "glob" +import isGlob from "is-glob" /** * Reads the manifests and makes sure each has a namespace set (when applicable) and adds annotations. @@ -128,11 +129,54 @@ export async function readManifests( const manifestPath = readFromSrcDir ? action.basePath() : action.getBuildPath() const spec = action.getSpec() + const specFiles = spec.files - const files = await glob(spec.files, { cwd: manifestPath }) + const regularPaths = specFiles.filter((f) => !isGlob(f)) + const missingPaths = await Bluebird.filter(regularPaths, async (regularPath) => { + const resolvedPath = resolve(manifestPath, regularPath) + return !(await pathExists(resolvedPath)) + }) + if (missingPaths.length) { + throw new ConfigurationError({ + message: `Invalid manifest file path(s) in ${action.kind} action '${ + action.name + }'. Cannot find manifest file(s) at ${naturalList(missingPaths)} in ${manifestPath} directory.`, + detail: { + action: { + kind: action.kind, + name: action.name, + type: action.type, + spec: { + files: specFiles, + }, + }, + missingPaths, + }, + }) + } + + const resolvedFiles = await glob(specFiles, { cwd: manifestPath }) + if (specFiles.length > 0 && resolvedFiles.length === 0) { + throw new ConfigurationError({ + message: `Invalid manifest file path(s) in ${action.kind} action '${ + action.name + }'. Cannot find any manifest files for paths ${naturalList(specFiles)} in ${manifestPath} directory.`, + detail: { + action: { + kind: action.kind, + name: action.name, + type: action.type, + spec: { + files: specFiles, + }, + }, + resolvedFiles, + }, + }) + } const fileManifests = flatten( - await Bluebird.map(files, async (path) => { + await Bluebird.map(resolvedFiles, async (path) => { const absPath = resolve(manifestPath, path) log.debug(`Reading manifest for ${action.longDescription()} from path ${absPath}`) const str = (await readFile(absPath)).toString() diff --git a/core/test/data/test-projects/kubernetes-type/with-build-action/deployment-action.yaml b/core/test/data/test-projects/kubernetes-type/with-build-action/deployment-action.yaml new file mode 100644 index 0000000000..a65e8c5176 --- /dev/null +++ b/core/test/data/test-projects/kubernetes-type/with-build-action/deployment-action.yaml @@ -0,0 +1,29 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: busybox-deployment + labels: + app: busybox +spec: + replicas: 1 + selector: + matchLabels: + app: busybox + template: + metadata: + labels: + app: busybox + spec: + containers: + - name: busybox + image: busybox:1.31.1 + args: [sh, -c, "while :; do sleep 2073600; done"] + env: + - name: FOO + value: banana + - name: BAR + value: "" + - name: BAZ + value: null + ports: + - containerPort: 80 diff --git a/core/test/data/test-projects/kubernetes-type/with-build-action/with-build.garden.yml b/core/test/data/test-projects/kubernetes-type/with-build-action/with-build.garden.yml index ef98aff3ce..f754084cf6 100644 --- a/core/test/data/test-projects/kubernetes-type/with-build-action/with-build.garden.yml +++ b/core/test/data/test-projects/kubernetes-type/with-build-action/with-build.garden.yml @@ -3,4 +3,4 @@ type: kubernetes name: with-build-action build: exec-build spec: - files: ["*.yaml"] + files: [ "deployment-action.yaml" ] diff --git a/core/test/integ/src/plugins/kubernetes/kubernetes-type/common.ts b/core/test/integ/src/plugins/kubernetes/kubernetes-type/common.ts index 6278118c8c..fb04628630 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/common.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/common.ts @@ -11,7 +11,7 @@ import { cloneDeep } from "lodash" import { ConfigGraph } from "../../../../../../src/graph/config-graph" import { PluginContext } from "../../../../../../src/plugin-context" import { readManifests } from "../../../../../../src/plugins/kubernetes/kubernetes-type/common" -import { TestGarden, makeTestGarden, getExampleDir, expectError, getDataDir } from "../../../../../helpers" +import { expectError, getDataDir, getExampleDir, makeTestGarden, TestGarden } from "../../../../../helpers" import { KubernetesDeployAction } from "../../../../../../src/plugins/kubernetes/kubernetes-type/config" import { Resolved } from "../../../../../../src/actions/types" @@ -33,27 +33,28 @@ export async function getKubernetesTestGarden() { describe("readManifests", () => { let garden: TestGarden let ctx: PluginContext - let action: Resolved let graph: ConfigGraph - const exampleDir = getExampleDir("kustomize") + context("kustomize", () => { + const exampleDir = getExampleDir("kustomize") - before(async () => { - garden = await makeTestGarden(exampleDir) - const provider = await garden.resolveProvider(garden.log, "local-kubernetes") - ctx = await garden.getPluginContext({ provider, templateContext: undefined, events: undefined }) - }) + let action: Resolved - beforeEach(async () => { - graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - action = await garden.resolveAction({ - action: cloneDeep(graph.getDeploy("hello-world")), - log: garden.log, - graph, + before(async () => { + garden = await makeTestGarden(exampleDir) + const provider = await garden.resolveProvider(garden.log, "local-kubernetes") + ctx = await garden.getPluginContext({ provider, templateContext: undefined, events: undefined }) + }) + + beforeEach(async () => { + graph = await garden.getConfigGraph({ log: garden.log, emit: false }) + action = await garden.resolveAction({ + action: cloneDeep(graph.getDeploy("hello-world")), + log: garden.log, + graph, + }) }) - }) - context("kustomize", () => { const expectedErr = "kustomize.extraArgs must not include any of -o, --output, -h, --help" it("throws if --output is set in extraArgs", async () => { @@ -105,4 +106,86 @@ describe("readManifests", () => { expect(kinds).to.eql(["Deployment", "Service", "ConfigMap"]) }) }) + + context("kubernetes manifest files resolution", () => { + before(async () => { + garden = await getKubernetesTestGarden() + const provider = await garden.resolveProvider(garden.log, "local-kubernetes") + ctx = await garden.getPluginContext({ provider, templateContext: undefined, events: undefined }) + }) + + beforeEach(async () => { + graph = await garden.getConfigGraph({ log: garden.log, emit: false }) + }) + + it("should support regular files paths", async () => { + const resolvedAction = await garden.resolveAction({ + action: cloneDeep(graph.getDeploy("with-build-action")), + log: garden.log, + graph, + }) + // Pre-check to ensure that the test filenames in the test data are correct. + expect(resolvedAction.getSpec().files).to.eql(["deployment-action.yaml"]) + + // We use readFromSrcDir = true here because we just resolve but do not execute any actions. + // It means that the build directory will not be created. + const manifests = await readManifests(ctx, resolvedAction, garden.log, true) + expect(manifests).to.exist + const names = manifests.map((m) => ({ kind: m.kind, name: m.metadata?.name })) + expect(names).to.eql([{ kind: "Deployment", name: "busybox-deployment" }]) + }) + + it("should support both regular paths and glob patterns with deduplication", async () => { + const action = cloneDeep(graph.getDeploy("with-build-action")) + // Append a valid glob pattern that results to a non-empty list of files. + action["_config"]["spec"]["files"].push("*.yaml") + const resolvedAction = await garden.resolveAction({ + action, + log: garden.log, + graph, + }) + // Pre-check to ensure that the test filenames in the test data are correct. + expect(resolvedAction.getSpec().files).to.eql(["deployment-action.yaml", "*.yaml"]) + + // We use readFromSrcDir = true here because we just resolve but do not execute any actions. + // It means that the build directory will not be created. + const manifests = await readManifests(ctx, resolvedAction, garden.log, true) + expect(manifests).to.exist + const names = manifests.map((m) => ({ kind: m.kind, name: m.metadata?.name })) + expect(names).to.eql([{ kind: "Deployment", name: "busybox-deployment" }]) + }) + + it("should throw on missing regular path", async () => { + const action = cloneDeep(graph.getDeploy("with-build-action")) + action["_config"]["spec"]["files"].push("missing-file.yaml") + const resolvedAction = await garden.resolveAction({ + action, + log: garden.log, + graph, + }) + + // We use readFromSrcDir = true here because we just resolve but do not execute any actions. + // It means that the build directory will not be created. + await expectError(() => readManifests(ctx, resolvedAction, garden.log, true), { + contains: `Invalid manifest file path(s) in ${action.kind} action '${action.name}'`, + }) + }) + + it("should throw when no files found from glob pattens", async () => { + const action = cloneDeep(graph.getDeploy("with-build-action")) + // Rewrite the whole files array to have a glob pattern that results to an empty list of files. + action["_config"]["spec"]["files"] = ["./**/manifests/*.yaml"] + const resolvedAction = await garden.resolveAction({ + action, + log: garden.log, + graph, + }) + + // We use readFromSrcDir = true here because we just resolve but do not execute any actions. + // It means that the build directory will not be created. + await expectError(() => readManifests(ctx, resolvedAction, garden.log, true), { + contains: `Invalid manifest file path(s) in ${action.kind} action '${action.name}'`, + }) + }) + }) })