fix(k8s): handle AEC-paused resources properly (#5122)

* fix(k8s): handle AEC-paused resources properly

This is a follow-up quickfix for #4846.
It reads a specific annotation and checks if it has a special value set by Cloud.
This needs to be changed to reply on a more generic and reliable way of k8s resource comparison.

* refactor(k8s): explicit typing

* refactor(k8s): extract local variable

To avoid repetitive creation of the same immutable string.

* chore: add TODO comment

* refactor(k8s): introduce named constant for `manifest-hash` annotation key

* refactor(k8s): use helper to construct the annotation key

* chore: helper function to check sha256 validity

* refactor: move hash-helper function to a dedicated module

* fix(k8s): smarter manifest hash checking

Consider any non-sha256 value as an outdated marker for k8s resources.
This covers the current AEC behavior on the Cloud side.
This also allows users to do manual modifications of k8s resources.

* test: add integration test

* test: fix integration test to restore the valid cluster state

* fix: re-create `Regex` object before every `test` call

Because `Regex` is a stateful object.

* test: use separate module config in integ test

To avoid unwanted modifications of the remote resources used by the other tests.

* chore: typo fix

Co-authored-by: Shumail Mohyuddin <shumailmohyuddin@gmail.com>

---------

Co-authored-by: Shumail Mohyuddin <shumailmohyuddin@gmail.com>

core/src/plugins/kubernetes/api.ts

@@ -377,7 +377,7 @@ export class KubeApi {
   /**
    * Fetch the specified resource from the cluster.
    */
-  async read({ log, namespace, apiVersion, kind, name }: ReadParams) {
+  async read({ log, namespace, apiVersion, kind, name }: ReadParams): Promise<KubernetesResource> {
     log.silly(`Fetching Kubernetes resource ${apiVersion}/${kind}/${name}`)
 
     const typePath = await this.getResourceTypeApiPath({
@@ -390,29 +390,29 @@ export class KubeApi {
     const apiPath = typePath + "/" + name
 
     const res = await this.request({ log, path: apiPath })
-    return res.body
+    return res.body as KubernetesResource
   }
 
-  async readOrNull(params: ReadParams) {
+  async readOrNull(params: ReadParams): Promise<KubernetesResource | null> {
     return await nullIfNotFound(() => this.read(params))
   }
 
   /**
    * Given a manifest, attempt to read the matching resource from the cluster.
    */
-  async readBySpec({ log, namespace, manifest }: ReadBySpecParams) {
+  async readBySpec({ log, namespace, manifest }: ReadBySpecParams): Promise<KubernetesResource> {
     log.silly(`Fetching Kubernetes resource ${manifest.apiVersion}/${manifest.kind}/${manifest.metadata.name}`)
 
     const apiPath = await this.getResourceApiPathFromManifest({ manifest, log, namespace })
 
     const res = await this.request({ log, path: apiPath })
-    return res.body
+    return res.body as KubernetesResource
   }
 
   /**
    * Same as readBySpec() but returns null if the resource is missing.
    */
-  async readBySpecOrNull(params: ReadBySpecParams) {
+  async readBySpecOrNull(params: ReadBySpecParams): Promise<KubernetesResource | null> {
     return await nullIfNotFound(() => this.readBySpec(params))
   }
 

core/src/plugins/kubernetes/helm/status.ts

@@ -24,8 +24,9 @@ import { ActionMode, Resolved } from "../../../actions/types"
 import { deployStateToActionState } from "../../../plugin/handlers/Deploy/get-status"
 import { isTruthy } from "../../../util/util"
 import { ChildProcessError } from "../../../exceptions"
+import { gardenAnnotationKey } from "../../../util/string"
 
-export const gardenCloudAECPauseAnnotation = "garden.io/aec-status"
+export const gardenCloudAECPauseAnnotation = gardenAnnotationKey("aec-status")
 
 const helmStatusMap: { [status: string]: DeployState } = {
   unknown: "unknown",

core/src/plugins/kubernetes/kubectl.ts

@@ -11,14 +11,15 @@ import { ExecParams, PluginTool } from "../../util/ext-tools"
 import { Log } from "../../logger/log-entry"
 import { KubernetesProvider } from "./config"
 import { KubernetesResource } from "./types"
-import { dedent, gardenAnnotationKey } from "../../util/string"
+import { dedent } from "../../util/string"
 import { getResourceKey, hashManifest } from "./util"
 import { PluginToolSpec } from "../../plugin/tools"
 import { PluginContext } from "../../plugin-context"
 import { KUBECTL_RETRY_OPTS, KubeApi, KubernetesError } from "./api"
 import { pathExists } from "fs-extra"
 import { ChildProcessError, ConfigurationError } from "../../exceptions"
 import { requestWithRetry, RetryOpts } from "./retry"
+import { k8sManifestHashAnnotationKey } from "./status/status"
 
 // Corresponds to the default prune whitelist in `kubectl`.
 // See: https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/apply/prune.go#L176-L192
@@ -76,10 +77,10 @@ export async function apply({
     if (!manifest.metadata.annotations) {
       manifest.metadata.annotations = {}
     }
-    if (manifest.metadata.annotations[gardenAnnotationKey("manifest-hash")]) {
-      delete manifest.metadata.annotations[gardenAnnotationKey("manifest-hash")]
+    if (manifest.metadata.annotations[k8sManifestHashAnnotationKey]) {
+      delete manifest.metadata.annotations[k8sManifestHashAnnotationKey]
     }
-    manifest.metadata.annotations[gardenAnnotationKey("manifest-hash")] = await hashManifest(manifest)
+    manifest.metadata.annotations[k8sManifestHashAnnotationKey] = await hashManifest(manifest)
   }
 
   // The `--prune` option for `kubectl apply` currently isn't backwards-compatible, so here, we essentially

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

@@ -20,6 +20,7 @@ import { getForwardablePorts, killPortForwards } from "../port-forward"
 import { getK8sIngresses } from "../status/ingress"
 import {
   getDeployedResource,
+  k8sManifestHashAnnotationKey,
   resolveResourceStatus,
   resolveResourceStatuses,
   ResourceStatus,
@@ -44,6 +45,7 @@ import type { ActionLog } from "../../../logger/log-entry"
 import type { ActionMode, Resolved } from "../../../actions/types"
 import { deployStateToActionState } from "../../../plugin/handlers/Deploy/get-status"
 import { ResolvedDeployAction } from "../../../actions/deploy"
+import { isSha256 } from "../../../util/hashing"
 
 export const kubernetesHandlers: Partial<ModuleActionHandlers<KubernetesModule>> = {
   configure: configureKubernetesModule,
@@ -238,7 +240,7 @@ async function getResourceStatuses({
     return []
   }
 
-  const maybeDeployedResources: [ManifestMetadata, any][] = await Promise.all(
+  const maybeDeployedResources: [ManifestMetadata, KubernetesResource | null][] = await Promise.all(
     manifestMetadata.map(async (m) => {
       return [m, await api.readOrNull({ log, ...m })]
     })
@@ -253,9 +255,18 @@ async function getResourceStatuses({
           metadata: { name: m.name, namespace: m.namespace },
         }
         return { resource: missingResource, state: "missing" } as ResourceStatus
-      } else {
-        return await resolveResourceStatus({ api, namespace, resource, log })
       }
+
+      // TODO: consider removing this quickfix once we have implemented generic manifests/resources comparison
+      // Check if the "garden.io/manifest-hash" annotation is a valid sha256 hash.
+      // If it's not, consider the remote resource as outdated.
+      // AEC feature uses a dummy non-sha256 value to ensure the outdated state.
+      const manifestHash = resource.metadata?.annotations?.[k8sManifestHashAnnotationKey]
+      if (manifestHash && !isSha256(manifestHash)) {
+        return { resource, state: "outdated" } as ResourceStatus
+      }
+
+      return await resolveResourceStatus({ api, namespace, resource, log })
     })
   )
 }

core/src/plugins/kubernetes/status/status.ts

@@ -42,6 +42,8 @@ import { combineStates, DeployState } from "../../../types/service"
 import { isTruthy, sleep } from "../../../util/util"
 import dedent = require("dedent")
 
+export const k8sManifestHashAnnotationKey = gardenAnnotationKey("manifest-hash")
+
 export interface ResourceStatus<T extends BaseResource | KubernetesObject = BaseResource> {
   state: DeployState
   resource: KubernetesServerResource<T>
@@ -460,12 +462,11 @@ export async function compareDeployedResources({
     }
 
     // Discard any last applied config from the input manifest
-    const hashKey = gardenAnnotationKey("manifest-hash")
-    if (manifest.metadata?.annotations?.[hashKey]) {
-      delete manifest.metadata?.annotations?.[hashKey]
+    if (manifest.metadata?.annotations?.[k8sManifestHashAnnotationKey]) {
+      delete manifest.metadata?.annotations?.[k8sManifestHashAnnotationKey]
     }
-    if (manifest.spec?.template?.metadata?.annotations?.[hashKey]) {
-      delete manifest.spec?.template?.metadata?.annotations?.[hashKey]
+    if (manifest.spec?.template?.metadata?.annotations?.[k8sManifestHashAnnotationKey]) {
+      delete manifest.spec?.template?.metadata?.annotations?.[k8sManifestHashAnnotationKey]
     }
 
     if (isWorkloadResource(manifest)) {
@@ -480,7 +481,7 @@ export async function compareDeployedResources({
     // Start by checking for "last applied configuration" annotations and comparing against those.
     // This can be more accurate than comparing against resolved resources.
     if (deployedResource.metadata && deployedResource.metadata.annotations) {
-      const lastAppliedHashed = deployedResource.metadata.annotations[hashKey]
+      const lastAppliedHashed = deployedResource.metadata.annotations[k8sManifestHashAnnotationKey]
 
       // The new manifest matches the last applied manifest
       if (lastAppliedHashed && (await hashManifest(manifest)) === lastAppliedHashed) {

core/src/util/hashing.ts

@@ -0,0 +1,18 @@
+/*
+ * Copyright (C) 2018-2023 Garden Technologies, Inc. <info@garden.io>
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+// String with SHA256 hash
+export function isSha256(str: string): boolean {
+  const sha256Regex = /^[a-f0-9]{64}$/gi
+  return sha256Regex.test(str)
+}
+
+export function isSha1(str: string): boolean {
+  const sha1Regex = new RegExp(/\b([a-f0-9]{40})\b/)
+  return sha1Regex.test(str)
+}

core/src/util/string.ts

@@ -32,6 +32,7 @@ const gardenAnnotationPrefix = "garden.io/"
 export type GardenAnnotationKey =
   | "actionType"
   | "action"
+  | "aec-status"
   | "mode"
   | "generated"
   | "helm-migrated"

core/src/vcs/git.ts

@@ -12,22 +12,23 @@ import { isString } from "lodash"
 import { createReadStream, ensureDir, lstat, pathExists, readlink, realpath, stat, Stats } from "fs-extra"
 import { PassThrough } from "stream"
 import { GetFilesParams, RemoteSourceParams, VcsFile, VcsHandler, VcsHandlerParams, VcsInfo } from "./vcs"
-import { ChildProcessError, ConfigurationError, RuntimeError, isErrnoException } from "../exceptions"
+import { ChildProcessError, ConfigurationError, isErrnoException, RuntimeError } from "../exceptions"
 import { getStatsType, joinWithPosix, matchPath } from "../util/fs"
 import { dedent, deline, splitLast } from "../util/string"
 import { defer, exec } from "../util/util"
 import { Log } from "../logger/log-entry"
 import parseGitConfig from "parse-git-config"
 import { getDefaultProfiler, Profile, Profiler } from "../util/profiling"
 import { STATIC_DIR } from "../constants"
-import split2 = require("split2")
-import execa = require("execa")
 import isGlob from "is-glob"
 import chalk from "chalk"
-import hasha = require("hasha")
 import { pMemoizeDecorator } from "../lib/p-memoize"
 import AsyncLock from "async-lock"
 import PQueue from "p-queue"
+import { isSha1 } from "../util/hashing"
+import split2 = require("split2")
+import execa = require("execa")
+import hasha = require("hasha")
 
 const gitConfigAsyncLock = new AsyncLock()
 
@@ -585,11 +586,6 @@ export class GitHandler extends VcsHandler {
     return files
   }
 
-  private isHashSHA1(hash: string): boolean {
-    const SHA1RegExp = new RegExp(/\b([a-f0-9]{40})\b/)
-    return SHA1RegExp.test(hash)
-  }
-
   private async cloneRemoteSource(
     log: Log,
     repositoryUrl: string,
@@ -600,7 +596,7 @@ export class GitHandler extends VcsHandler {
     await ensureDir(absPath)
     const git = this.gitCli(log, absPath, failOnPrompt)
     // Use `--recursive` to include submodules
-    if (!this.isHashSHA1(hash)) {
+    if (!isSha1(hash)) {
       return git(
         "-c",
         "protocol.file.allow=always",
@@ -673,9 +669,7 @@ export class GitHandler extends VcsHandler {
       await git("remote", "update")
 
       const localCommitId = (await git("rev-parse", "HEAD"))[0]
-      const remoteCommitId = this.isHashSHA1(hash)
-        ? hash
-        : getCommitIdFromRefList(await git("ls-remote", repositoryUrl, hash))
+      const remoteCommitId = isSha1(hash) ? hash : getCommitIdFromRefList(await git("ls-remote", repositoryUrl, hash))
 
       if (localCommitId !== remoteCommitId) {
         gitLog.info(`Fetching from ${url}`)

core/test/data/test-projects/kubernetes-type/module-simple-isolated/garden.yml

@@ -0,0 +1,37 @@
+kind: Module
+type: kubernetes
+name: module-simple-isolated
+description: Simple Kubernetes module with minimum config
+manifests:
+  - apiVersion: apps/v1
+    kind: Deployment
+    metadata:
+      name: busybox-deployment-isolated
+      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
+serviceResource:
+  kind: Deployment
+  name: busybox-deployment-isolated

core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts

@@ -25,7 +25,10 @@ import { KubeApi } from "../../../../../../src/plugins/kubernetes/api"
 import { ActionLog, createActionLog, Log } from "../../../../../../src/logger/log-entry"
 import { KubernetesPluginContext, KubernetesProvider } from "../../../../../../src/plugins/kubernetes/config"
 import { getActionNamespace } from "../../../../../../src/plugins/kubernetes/namespace"
-import { getDeployedResource } from "../../../../../../src/plugins/kubernetes/status/status"
+import {
+  getDeployedResource,
+  k8sManifestHashAnnotationKey,
+} from "../../../../../../src/plugins/kubernetes/status/status"
 import { ModuleConfig } from "../../../../../../src/config/module"
 import { BaseResource, KubernetesResource } from "../../../../../../src/plugins/kubernetes/types"
 import { DeleteDeployTask } from "../../../../../../src/tasks/delete-deploy"
@@ -262,6 +265,38 @@ describe("kubernetes-type handlers", () => {
       expect(status.detail?.state).to.equal("outdated")
     })
 
+    it("should return outdated status when k8s remote resource version is not a valid sha256 hash", async () => {
+      // We use a standalone module here, because we need to modify the state of the remote resource to perform the test.
+      // Using of the `module-simple` will require remote resource redeployment in many other test cases here,
+      // and that will slow down the test execution.
+      const { resolvedAction, deployParams } = await prepareActionDeployParams("module-simple-isolated", {})
+
+      await kubernetesDeploy(deployParams)
+
+      const namespace = await getActionNamespace({
+        ctx,
+        log,
+        action: resolvedAction,
+        provider: ctx.provider,
+        skipCreate: true,
+      })
+
+      const declaredManifests = await readManifests(ctx, resolvedAction, log)
+      expect(declaredManifests.length).to.equal(1)
+
+      const deploymentManifest = declaredManifests[0].manifest
+      const remoteDeploymentResource = await api.readBySpec({ log, namespace, manifest: deploymentManifest })
+      expect(remoteDeploymentResource.metadata.annotations).to.exist
+
+      // Modify the `k8sManifestHashAnnotationKey` annotation to be non-valid sha256 hash
+      remoteDeploymentResource.metadata.annotations![k8sManifestHashAnnotationKey] = "non-sha256-hash"
+      await api.replace({ log, namespace, resource: remoteDeploymentResource })
+
+      const status = await getKubernetesDeployStatus(deployParams)
+      expect(status.state).to.equal("not-ready")
+      expect(status.detail?.state).to.equal("outdated")
+    })
+
     it("should return outdated status when metadata ConfigMap has different mode", async () => {
       const { resolvedAction, deployParams } = await prepareActionDeployParams("module-simple", {})
 
@@ -488,6 +523,7 @@ describe("kubernetes-type handlers", () => {
 
       expect(await getDeployedResource(ctx, ctx.provider, ns1Manifest!, log), "ns1resource").to.exist
       expect(await getDeployedResource(ctx, ctx.provider, ns2Manifest!, log), "ns2resource").to.not.exist
+      expect(await getDeployedResource(ctx, ctx.provider, ns2Resource!, log), "ns2resource").to.not.exist
     })
 
     it("deletes all resources including a metadata ConfigMap describing what was last deployed", async () => {