From ed87cfdac27a0d3a6634c278f56626f828564fc5 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Mon, 25 Sep 2023 17:06:07 +0200 Subject: [PATCH] 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 --------- Co-authored-by: Shumail Mohyuddin --- core/src/plugins/kubernetes/api.ts | 12 +++--- core/src/plugins/kubernetes/helm/status.ts | 3 +- core/src/plugins/kubernetes/kubectl.ts | 9 +++-- .../kubernetes/kubernetes-type/handlers.ts | 17 +++++++-- core/src/plugins/kubernetes/status/status.ts | 13 ++++--- core/src/util/hashing.ts | 18 +++++++++ core/src/util/string.ts | 1 + core/src/vcs/git.ts | 20 ++++------ .../module-simple-isolated/garden.yml | 37 ++++++++++++++++++ .../kubernetes/kubernetes-type/handlers.ts | 38 ++++++++++++++++++- 10 files changed, 134 insertions(+), 34 deletions(-) create mode 100644 core/src/util/hashing.ts create mode 100644 core/test/data/test-projects/kubernetes-type/module-simple-isolated/garden.yml diff --git a/core/src/plugins/kubernetes/api.ts b/core/src/plugins/kubernetes/api.ts index a41bb61286..cff49e9a93 100644 --- a/core/src/plugins/kubernetes/api.ts +++ b/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 { 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 { 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 { 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 { return await nullIfNotFound(() => this.readBySpec(params)) } diff --git a/core/src/plugins/kubernetes/helm/status.ts b/core/src/plugins/kubernetes/helm/status.ts index 4cd72b57d1..edf3e83cdc 100644 --- a/core/src/plugins/kubernetes/helm/status.ts +++ b/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", diff --git a/core/src/plugins/kubernetes/kubectl.ts b/core/src/plugins/kubernetes/kubectl.ts index be0ec0f2f0..00d81885d5 100644 --- a/core/src/plugins/kubernetes/kubectl.ts +++ b/core/src/plugins/kubernetes/kubectl.ts @@ -11,7 +11,7 @@ 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" @@ -19,6 +19,7 @@ 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 diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index c0077af48e..6ead60a49f 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/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> = { 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 }) }) ) } diff --git a/core/src/plugins/kubernetes/status/status.ts b/core/src/plugins/kubernetes/status/status.ts index c85c515f58..86b16a3797 100644 --- a/core/src/plugins/kubernetes/status/status.ts +++ b/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 { state: DeployState resource: KubernetesServerResource @@ -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) { diff --git a/core/src/util/hashing.ts b/core/src/util/hashing.ts new file mode 100644 index 0000000000..e6209d08c2 --- /dev/null +++ b/core/src/util/hashing.ts @@ -0,0 +1,18 @@ +/* + * Copyright (C) 2018-2023 Garden Technologies, Inc. + * + * 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) +} diff --git a/core/src/util/string.ts b/core/src/util/string.ts index 320004892c..cec881a313 100644 --- a/core/src/util/string.ts +++ b/core/src/util/string.ts @@ -32,6 +32,7 @@ const gardenAnnotationPrefix = "garden.io/" export type GardenAnnotationKey = | "actionType" | "action" + | "aec-status" | "mode" | "generated" | "helm-migrated" diff --git a/core/src/vcs/git.ts b/core/src/vcs/git.ts index b46a5e0eee..bc0f8af099 100644 --- a/core/src/vcs/git.ts +++ b/core/src/vcs/git.ts @@ -12,7 +12,7 @@ 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" @@ -20,14 +20,15 @@ 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}`) diff --git a/core/test/data/test-projects/kubernetes-type/module-simple-isolated/garden.yml b/core/test/data/test-projects/kubernetes-type/module-simple-isolated/garden.yml new file mode 100644 index 0000000000..f84d86f340 --- /dev/null +++ b/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 diff --git a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts index 3574d65e41..853c6ecea1 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/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 () => {