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 c9efb47 did not have effect.
The Deploy action `with-build-action` has never been called with `readManifests`.

core/src/plugins/kubernetes/kubernetes-type/common.ts

@@ -7,15 +7,15 @@
  */
 
 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"
 
 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()

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

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" ]

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<KubernetesDeployAction>
   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<KubernetesDeployAction>
 
-  beforeEach(async () => {
-    graph = await garden.getConfigGraph({ log: garden.log, emit: false })
-    action = await garden.resolveAction<KubernetesDeployAction>({
-      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<KubernetesDeployAction>({
+        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<KubernetesDeployAction>({
+        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<KubernetesDeployAction>({
+        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<KubernetesDeployAction>({
+        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<KubernetesDeployAction>({
+        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}'`,
+      })
+    })
+  })
 })