From 0d35d845a9419fa6e1019210ffc60f434752fb60 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Tue, 5 Sep 2023 11:42:59 +0200 Subject: [PATCH 01/30] refactor: extract named function for manifest post-processing --- .../kubernetes/kubernetes-type/common.ts | 67 ++++++++++--------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/common.ts b/core/src/plugins/kubernetes/kubernetes-type/common.ts index 9ab6f15855..d2a58b273c 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/common.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/common.ts @@ -64,40 +64,43 @@ export async function getManifests({ defaultNamespace: string readFromSrcDir?: boolean }): Promise { - const declaredManifests: DeclaredManifest[] = await Promise.all( - (await readManifests(ctx, action, log, readFromSrcDir)).map(async ({ manifest, declaration }) => { - // Ensure a namespace is set, if not already set, and if required by the resource type - if (!manifest.metadata?.namespace) { - if (!manifest.metadata) { - // TODO: Type system complains that name is missing - ;(manifest as any).metadata = {} - } - - const info = await api.getApiResourceInfo(log, manifest.apiVersion, manifest.kind) - - if (info?.namespaced) { - manifest.metadata.namespace = defaultNamespace - } + // Local function to set some default values and Garden-specific annotations. + async function postProcessManifest({ manifest, declaration }: DeclaredManifest): Promise { + // Ensure a namespace is set, if not already set, and if required by the resource type + if (!manifest.metadata?.namespace) { + if (!manifest.metadata) { + // TODO: Type system complains that name is missing + ;(manifest as any).metadata = {} } - /** - * Set Garden annotations. - * - * For namespace resources, we use the namespace's name as the annotation value, to ensure that namespace resources - * with different names aren't considered by Garden to be the same resource. - * - * This is relevant e.g. in the context of a shared dev cluster, where several users might create their own - * copies of a namespace resource (each named e.g. "${username}-some-namespace") through deploying a `kubernetes` - * module that includes a namespace resource in its manifests. - */ - const annotationValue = - manifest.kind === "Namespace" ? gardenNamespaceAnnotationValue(manifest.metadata.name) : action.name - set(manifest, ["metadata", "annotations", gardenAnnotationKey("service")], annotationValue) - set(manifest, ["metadata", "annotations", gardenAnnotationKey("mode")], action.mode()) - set(manifest, ["metadata", "labels", gardenAnnotationKey("service")], annotationValue) - - return { manifest, declaration } - }) + const info = await api.getApiResourceInfo(log, manifest.apiVersion, manifest.kind) + + if (info?.namespaced) { + manifest.metadata.namespace = defaultNamespace + } + } + + /** + * Set Garden annotations. + * + * For namespace resources, we use the namespace's name as the annotation value, to ensure that namespace resources + * with different names aren't considered by Garden to be the same resource. + * + * This is relevant e.g. in the context of a shared dev cluster, where several users might create their own + * copies of a namespace resource (each named e.g. "${username}-some-namespace") through deploying a `kubernetes` + * module that includes a namespace resource in its manifests. + */ + const annotationValue = + manifest.kind === "Namespace" ? gardenNamespaceAnnotationValue(manifest.metadata.name) : action.name + set(manifest, ["metadata", "annotations", gardenAnnotationKey("service")], annotationValue) + set(manifest, ["metadata", "annotations", gardenAnnotationKey("mode")], action.mode()) + set(manifest, ["metadata", "labels", gardenAnnotationKey("service")], annotationValue) + + return { manifest, declaration } + } + + const declaredManifests: DeclaredManifest[] = await Promise.all( + (await readManifests(ctx, action, log, readFromSrcDir)).map(postProcessManifest) ) validateDeclaredManifests(declaredManifests) From 9430dcc6d5415d002e8dc7048d3ad387be90f6e6 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Tue, 5 Sep 2023 11:43:41 +0200 Subject: [PATCH 02/30] refactor: split manifest reading and post-processing --- core/src/plugins/kubernetes/kubernetes-type/common.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/common.ts b/core/src/plugins/kubernetes/kubernetes-type/common.ts index d2a58b273c..dfddaed346 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/common.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/common.ts @@ -99,13 +99,12 @@ export async function getManifests({ return { manifest, declaration } } - const declaredManifests: DeclaredManifest[] = await Promise.all( - (await readManifests(ctx, action, log, readFromSrcDir)).map(postProcessManifest) - ) + const declaredManifests = await readManifests(ctx, action, log, readFromSrcDir) + const postProcessedManifests: DeclaredManifest[] = await Promise.all(declaredManifests.map(postProcessManifest)) - validateDeclaredManifests(declaredManifests) + validateDeclaredManifests(postProcessedManifests) - return declaredManifests.map((m) => m.manifest) + return postProcessedManifests.map((m) => m.manifest) } /** From 649eddcc5dea8e6904d5fe75310262322017caed Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Mon, 17 Jul 2023 15:20:14 +0200 Subject: [PATCH 03/30] fix(k8s): correctly resolve manifests when `build` is set --- .../kubernetes/kubernetes-type/common.ts | 103 +++++++-- .../kubernetes/kubernetes-type/handlers.ts | 114 +++++++--- core/src/plugins/kubernetes/status/status.ts | 94 ++++---- .../kubernetes/kubernetes-type/common.ts | 24 +- .../kubernetes/kubernetes-type/handlers.ts | 212 +++++++++++++++++- 5 files changed, 435 insertions(+), 112 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/common.ts b/core/src/plugins/kubernetes/kubernetes-type/common.ts index dfddaed346..d8e2bc5ec2 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/common.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/common.ts @@ -8,13 +8,13 @@ import { join, resolve } from "path" import { pathExists, readFile } from "fs-extra" -import { flatten, set } from "lodash" +import { flatten, keyBy, set } from "lodash" import { loadAll } from "js-yaml" import { KubernetesModule } from "./module-config" import { KubernetesResource } from "../types" import { KubeApi } from "../api" -import { dedent, gardenAnnotationKey, naturalList } from "../../../util/string" +import { dedent, gardenAnnotationKey, naturalList, stableStringify } from "../../../util/string" import { Log } from "../../../logger/log-entry" import { PluginContext } from "../../../plugin-context" import { ConfigurationError, GardenError, PluginError } from "../../../exceptions" @@ -23,9 +23,10 @@ import { HelmModule } from "../helm/module-config" import { KubernetesDeployAction } from "./config" import { CommonRunParams } from "../../../plugin/handlers/Run/run" import { runAndCopy } from "../run" -import { getResourceContainer, getResourcePodSpec, getTargetResource, makePodName } from "../util" -import { Resolved } from "../../../actions/types" +import { getResourceContainer, getResourceKey, getResourcePodSpec, getTargetResource, makePodName } from "../util" +import { ActionMode, Resolved } from "../../../actions/types" import { KubernetesPodRunAction, KubernetesPodTestAction } from "./kubernetes-pod" +import { V1ConfigMap } from "@kubernetes/client-node" import { glob } from "glob" import isGlob from "is-glob" import pFilter from "p-filter" @@ -55,14 +56,12 @@ export async function getManifests({ log, action, defaultNamespace, - readFromSrcDir = false, }: { ctx: PluginContext api: KubeApi log: Log action: Resolved defaultNamespace: string - readFromSrcDir?: boolean }): Promise { // Local function to set some default values and Garden-specific annotations. async function postProcessManifest({ manifest, declaration }: DeclaredManifest): Promise { @@ -99,7 +98,18 @@ export async function getManifests({ return { manifest, declaration } } - const declaredManifests = await readManifests(ctx, action, log, readFromSrcDir) + const declaredManifests = await readManifests(ctx, action, log) + + if (action.kind === "Deploy") { + // Add metadata ConfigMap to aid quick status check + const metadataManifest = getMetadataManifest(action, defaultNamespace, declaredManifests) + const declaredMetadataManifest: DeclaredManifest = { + declaration: { type: "inline", index: declaredManifests.length }, + manifest: metadataManifest, + } + declaredManifests.push(declaredMetadataManifest) + } + const postProcessedManifests: DeclaredManifest[] = await Promise.all(declaredManifests.map(postProcessManifest)) validateDeclaredManifests(postProcessedManifests) @@ -158,24 +168,81 @@ export function validateDeclaredManifests(declaredManifests: DeclaredManifest[]) } } +export interface ManifestMetadata { + key: string + apiVersion: string + kind: string + name: string + namespace: string +} + +export interface ParsedMetadataManifestData { + resolvedVersion: string + mode: ActionMode + manifestMetadata: { [key: string]: ManifestMetadata } +} + +export function getMetadataManifest( + action: Resolved, + defaultNamespace: string, + declaredManifests: DeclaredManifest[] +): KubernetesResource { + const manifestMetadata: ManifestMetadata[] = declaredManifests.map((declaredManifest) => { + const m = declaredManifest.manifest + return { + key: getResourceKey(m), + apiVersion: m.apiVersion, + kind: m.kind, + name: m.metadata.name, + namespace: m.metadata.namespace || defaultNamespace, + } + }) + + return { + apiVersion: "v1", + kind: "ConfigMap", + metadata: { + name: `garden-meta-${action.kind.toLowerCase()}-${action.name}`, + }, + data: { + resolvedVersion: action.versionString(), + mode: action.mode(), + manifestMetadata: stableStringify(keyBy(manifestMetadata, "key")), + }, + } +} + +export function parseMetadataResource(log: Log, resource: KubernetesResource): ParsedMetadataManifestData { + // TODO: validate schema here + const output: ParsedMetadataManifestData = { + resolvedVersion: resource.data?.resolvedVersion || "", + mode: (resource.data?.mode || "default") as ActionMode, + manifestMetadata: {}, + } + + const manifestMetadata = resource.data?.manifestMetadata + + if (manifestMetadata) { + try { + // TODO: validate by schema + output.manifestMetadata = JSON.parse(manifestMetadata) + } catch (error) { + log.debug({ msg: `Failed querying for remote resources: ${error.message}`, error }) + } + } + + return output +} + /** * Read the manifests from the module config, as well as any referenced files in the config. - * - * @param module The kubernetes module to read manifests for. - * @param readFromSrcDir Whether or not to read the manifests from the module build dir or from the module source dir. - * In general we want to read from the build dir to ensure that manifests added via the `build.dependencies[].copy` - * field will be included. However, in some cases, e.g. when getting the service status, we can't be certain that - * the build has been staged and we therefore read the manifests from the source. - * - * TODO: Remove this once we're checking for kubernetes module service statuses with version hashes. */ async function readManifests( ctx: PluginContext, action: Resolved, - log: Log, - readFromSrcDir = false + log: Log ): Promise { - const manifestPath = readFromSrcDir ? action.basePath() : action.getBuildPath() + const manifestPath = action.getBuildPath() const inlineManifests = readInlineManifests(action) const fileManifests = await readFileManifests(ctx, action, log, manifestPath) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index 649bb436a4..705eb622b5 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -8,7 +8,7 @@ import { isEmpty, omit, partition, uniq } from "lodash" import type { ModuleActionHandlers } from "../../../plugin/plugin" -import { ServiceStatus } from "../../../types/service" +import { DeployState, ForwardablePort, ServiceStatus } from "../../../types/service" import { gardenAnnotationKey } from "../../../util/string" import { KubeApi } from "../api" import type { KubernetesPluginContext } from "../config" @@ -18,16 +18,28 @@ import { streamK8sLogs } from "../logs" import { getActionNamespace, getActionNamespaceStatus } from "../namespace" import { getForwardablePorts, killPortForwards } from "../port-forward" import { getK8sIngresses } from "../status/ingress" -import { compareDeployedResources, waitForResources } from "../status/status" +import { + getDeployedResource, + resolveResourceStatus, + resolveResourceStatuses, + ResourceStatus, + waitForResources, +} from "../status/status" import type { BaseResource, KubernetesResource, KubernetesServerResource, SyncableResource } from "../types" -import { convertServiceResource, gardenNamespaceAnnotationValue, getManifests } from "./common" +import { + convertServiceResource, + gardenNamespaceAnnotationValue, + getManifests, + getMetadataManifest, + parseMetadataResource, +} from "./common" import { configureKubernetesModule, KubernetesModule } from "./module-config" import { configureLocalMode, startServiceInLocalMode } from "../local-mode" import type { ExecBuildConfig } from "../../exec/build" import type { KubernetesActionConfig, KubernetesDeployAction, KubernetesDeployActionConfig } from "./config" import type { DeployActionHandler } from "../../../plugin/action-types" import type { ActionLog } from "../../../logger/log-entry" -import type { Resolved } from "../../../actions/types" +import type { ActionMode, Resolved } from "../../../actions/types" import { deployStateToActionState } from "../../../plugin/handlers/Deploy/get-status" export const kubernetesHandlers: Partial> = { @@ -168,41 +180,77 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne provider, skipCreate: true, }) - const namespace = namespaceStatus.namespaceName + const defaultNamespace = namespaceStatus.namespaceName const api = await KubeApi.factory(log, ctx, k8sCtx.provider) - // FIXME: We're currently reading the manifests from the module source dir (instead of build dir) - // because the build may not have been staged. - // This means that manifests added via the `build.dependencies[].copy` field will not be included. - const manifests = await getManifests({ ctx, api, log, action, defaultNamespace: namespace, readFromSrcDir: true }) - const prepareResult = await configureSpecialModesForManifests({ - ctx: k8sCtx, - log, - action, - manifests, - }) - const preparedManifests = prepareResult.manifests + // Note: This is analogous to how we version check Helm charts, i.e. we don't check every resource individually. + // Users can always force deploy, much like with Helm Deploys. + const metadataManifest = getMetadataManifest(action, defaultNamespace, []) - let { - state, - remoteResources, - mode: deployedMode, - } = await compareDeployedResources({ - ctx: k8sCtx, - api, - namespace, - manifests: preparedManifests, - log, - }) + let deployedMode: ActionMode = "default" + let state: DeployState = "ready" + let remoteResources: KubernetesResource[] = [] + let forwardablePorts: ForwardablePort[] | undefined - const forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode }) + const remoteMetadataResource = await getDeployedResource(ctx, provider, metadataManifest, log) - if (state === "ready") { - // Local mode always takes precedence over sync mode - if (mode === "local" && spec.localMode && deployedMode !== "local") { + if (!remoteMetadataResource) { + state = "missing" + } else { + const deployedMetadata = parseMetadataResource(log, remoteMetadataResource) + deployedMode = deployedMetadata.mode + + if (deployedMetadata.resolvedVersion !== action.versionString()) { + state = "outdated" + } else if (mode === "local" && spec.localMode && deployedMode !== "local") { state = "outdated" } else if (mode === "sync" && spec.sync?.paths && deployedMode !== "sync") { state = "outdated" + } else if (mode === "default" && deployedMode !== mode) { + state = "outdated" + } + + const manifestMetadata = Object.values(deployedMetadata.manifestMetadata) + + if (manifestMetadata.length > 0) { + try { + const maybeDeployedResources = await Promise.all( + manifestMetadata.map(async (m) => { + return [m, await api.readOrNull({ log, ...m })] + }) + ) + + const statuses: ResourceStatus[] = await Promise.all( + maybeDeployedResources.map(async ([m, resource]) => { + if (!resource) { + return { + state: "missing" as const, + resource: { + apiVersion: m.apiVersion, + kind: m.kind, + metadata: { name: m.name, namespace: m.namespace }, + }, + } + } + remoteResources.push(resource) + return resolveResourceStatus({ api, namespace: defaultNamespace, resource, log }) + }) + ) + + state = resolveResourceStatuses(log, statuses) + } catch (error) { + log.debug({ msg: `Failed querying for remote resources: ${error.message}`, error }) + state = "unknown" + } + } + + // Note: Local mode has its own port-forwarding configuration + if (deployedMode !== "local" && remoteResources && remoteResources.length > 0) { + try { + forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode }) + } catch (error) { + log.debug({ msg: `Unable to extract forwardable ports: ${error.message}`, error }) + } } } @@ -214,7 +262,7 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne version: state === "ready" ? action.versionString() : undefined, detail: { remoteResources }, mode: deployedMode, - ingresses: getK8sIngresses(remoteResources, provider), + ingresses: getK8sIngresses(remoteResources || [], provider), }, // TODO-0.13.1 outputs: {}, @@ -397,7 +445,7 @@ export const deleteKubernetesDeploy: DeployActionHandler<"delete", KubernetesDep } return { - state: "ready", + state: "not-ready", detail: status, outputs: {}, } diff --git a/core/src/plugins/kubernetes/status/status.ts b/core/src/plugins/kubernetes/status/status.ts index ec132bc945..d90602b8e0 100644 --- a/core/src/plugins/kubernetes/status/status.ts +++ b/core/src/plugins/kubernetes/status/status.ts @@ -11,31 +11,36 @@ import { DeploymentError, GardenErrorParams } from "../../../exceptions" import { PluginContext } from "../../../plugin-context" import { KubeApi, KubernetesError } from "../api" import { getAppNamespace } from "../namespace" -import { KubernetesResource, KubernetesServerResource, BaseResource, KubernetesWorkload } from "../types" -import { zip, isArray, isPlainObject, pickBy, mapValues, flatten, cloneDeep, omit, isEqual, keyBy } from "lodash" -import { KubernetesProvider, KubernetesPluginContext } from "../config" +import { + BaseResource, + KubernetesResource, + KubernetesServerResource, + KubernetesWorkload, + SyncableResource, +} from "../types" +import { cloneDeep, flatten, isArray, isEqual, isPlainObject, keyBy, mapValues, omit, pickBy } from "lodash" +import { KubernetesPluginContext, KubernetesProvider } from "../config" import { isSubset } from "../../../util/is-subset" import { Log } from "../../../logger/log-entry" import { - V1ReplicationController, - V1ReplicaSet, - V1Pod, - V1PersistentVolumeClaim, - V1Service, - V1Container, KubernetesObject, + V1Container, V1Job, + V1PersistentVolumeClaim, + V1Pod, + V1ReplicaSet, + V1ReplicationController, + V1Service, } from "@kubernetes/client-node" -import dedent = require("dedent") import { getPods, getResourceKey, hashManifest } from "../util" import { checkWorkloadStatus } from "./workload" import { checkWorkloadPodStatus } from "./pod" import { deline, gardenAnnotationKey, stableStringify } from "../../../util/string" -import { SyncableResource } from "../types" import { ActionMode } from "../../../actions/types" import { deepMap } from "../../../util/objects" -import { DeployState, combineStates } from "../../../types/service" +import { combineStates, DeployState } from "../../../types/service" import { isTruthy, sleep } from "../../../util/util" +import dedent = require("dedent") export interface ResourceStatus { state: DeployState @@ -184,18 +189,14 @@ export async function checkResourceStatus({ log: Log waitForJobs?: boolean }) { - const handler = objHandlers[manifest.kind] - if (manifest.metadata?.namespace) { namespace = manifest.metadata.namespace } let resource: KubernetesServerResource - let resourceVersion: number | undefined try { resource = await api.readBySpec({ namespace, manifest, log }) - resourceVersion = parseInt(resource.metadata.resourceVersion!, 10) } catch (err) { if (!(err instanceof KubernetesError)) { throw err @@ -207,15 +208,41 @@ export async function checkResourceStatus({ } } - let status: ResourceStatus + return resolveResourceStatus({ api, namespace, resource, log, waitForJobs }) +} + +export async function resolveResourceStatus( + params: Omit +): Promise { + const handler = objHandlers[params.resource.kind] + if (handler) { - status = await handler({ api, namespace, resource, log, resourceVersion, waitForJobs }) + const resourceVersion = parseInt(params.resource.metadata.resourceVersion!, 10) + return handler({ ...params, resourceVersion }) } else { // if there is no explicit handler to check the status, we assume there's no rollout phase to wait for - status = { state: "ready", resource: manifest } + return { state: "ready", resource: params.resource } } +} + +export function resolveResourceStatuses(log: Log, statuses: ResourceStatus[]) { + const deployedStates = statuses.map((s) => s.state) + const state = combineStates(deployedStates) - return status + if (state !== "ready") { + const descriptions = statuses + .filter((s) => s.state !== "ready") + .map((s) => `${getResourceKey(s.resource)}: "${s.state}"`) + .join("\n") + + log.silly( + dedent` + Resource(s) with non-ready status found in the cluster: + + ${descriptions}` + "\n" + ) + } + return state } interface WaitParams { @@ -380,10 +407,7 @@ export async function compareDeployedResources({ manifests = flatten(manifests.map((r: any) => (r.apiVersion === "v1" && r.kind === "List" ? r.items : [r]))) // Check if any resources are missing from the cluster. - const maybeDeployedObjects = await Promise.all( - manifests.map((resource) => getDeployedResource(ctx, ctx.provider, resource, log)) - ) - const deployedResources = maybeDeployedObjects.filter((o) => o !== null) + const deployedResources = await getDeployedResources({ ctx, log, manifests }) const manifestsMap = keyBy(manifests, (m) => getResourceKey(m)) const manifestKeys = Object.keys(manifestsMap) const deployedMap = keyBy(deployedResources, (m) => getResourceKey(m)) @@ -415,24 +439,13 @@ export async function compareDeployedResources({ log.debug(`Getting currently deployed resource statuses...`) const deployedObjectStatuses: ResourceStatus[] = await Promise.all( - deployedResources.map(async (resource) => checkResourceStatus({ api, namespace, manifest: resource, log })) + deployedResources.map(async (resource) => resolveResourceStatus({ api, namespace, resource, log })) ) - const deployedStates = deployedObjectStatuses.map((s) => s.state) - if (deployedStates.find((s) => s !== "ready")) { - const descriptions = zip(deployedResources, deployedStates) - .filter(([_, s]) => s !== "ready") - .map(([o, s]) => `${logDescription(o!)}: "${s}"`) - .join("\n") - - log.silly( - dedent` - Resource(s) with non-ready status found in the cluster: + const resolvedState = resolveResourceStatuses(log, deployedObjectStatuses) - ${descriptions}` + "\n" - ) - - result.state = combineStates(deployedStates) + if (resolvedState !== "ready") { + result.state = resolvedState return result } @@ -608,9 +621,6 @@ export async function getDeployedResource } } -/** - * Fetches matching deployed resources from the cluster for the provided array of manifests. - */ export async function getDeployedResources({ ctx, manifests, 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 2b8526c2f2..3abef5c6b9 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/common.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/common.ts @@ -64,7 +64,7 @@ describe("getManifests", () => { }) await expectError( - () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace, readFromSrcDir: false }), + () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace }), (err) => { expect(err.message).to.equal(dedent` Duplicate manifest definition: Service named silly-demo is declared more than once: @@ -84,7 +84,7 @@ describe("getManifests", () => { }) await expectError( - () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace, readFromSrcDir: false }), + () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace }), (err) => { expect(err.message).to.equal(dedent` Duplicate manifest definition: Service named silly-demo is declared more than once: @@ -107,7 +107,7 @@ describe("getManifests", () => { }) await expectError( - () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace, readFromSrcDir: false }), + () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace }), (err) => { expect(err.message).to.equal(dedent` Duplicate manifest definition: Service named silly-demo is declared more than once: @@ -130,7 +130,7 @@ describe("getManifests", () => { }) await expectError( - () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace, readFromSrcDir: false }), + () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace }), (err) => { expect(err.message).to.equal(dedent` Duplicate manifest definition: Service named silly-demo is declared more than once: @@ -176,7 +176,7 @@ describe("getManifests", () => { action["_config"].spec.kustomize!.extraArgs = ["--output", "foo"] await expectError( - () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace, readFromSrcDir: false }), + () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace }), (err) => expect(err.message).to.include(expectedErr) ) }) @@ -185,7 +185,7 @@ describe("getManifests", () => { action["_config"].spec.kustomize!.extraArgs = ["-o", "foo"] await expectError( - () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace, readFromSrcDir: false }), + () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace }), (err) => expect(err.message).to.include(expectedErr) ) }) @@ -194,7 +194,7 @@ describe("getManifests", () => { action["_config"].spec.kustomize!.extraArgs = ["-h"] await expectError( - () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace, readFromSrcDir: false }), + () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace }), (err) => expect(err.message).to.include(expectedErr) ) }) @@ -203,7 +203,7 @@ describe("getManifests", () => { action["_config"].spec.kustomize!.extraArgs = ["--help"] await expectError( - () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace, readFromSrcDir: false }), + () => getManifests({ ctx, api, action, log: garden.log, defaultNamespace }), (err) => { expect(err.message).to.include(expectedErr) } @@ -211,14 +211,14 @@ describe("getManifests", () => { }) it("runs kustomize build in the given path", async () => { - const result = await getManifests({ ctx, api, action, log: garden.log, defaultNamespace, readFromSrcDir: true }) + const result = await getManifests({ ctx, api, action, log: garden.log, defaultNamespace }) const kinds = result.map((r) => r.kind) expect(kinds).to.have.members(["ConfigMap", "Service", "Deployment"]) }) it("adds extraArgs if specified to the build command", async () => { action["_config"].spec.kustomize!.extraArgs = ["--reorder", "none"] - const result = await getManifests({ ctx, api, action, log: garden.log, defaultNamespace, readFromSrcDir: true }) + const result = await getManifests({ ctx, api, action, log: garden.log, defaultNamespace }) const kinds = result.map((r) => r.kind) expect(kinds).to.eql(["Deployment", "Service", "ConfigMap"]) }) @@ -253,7 +253,6 @@ describe("getManifests", () => { action: resolvedAction, log: garden.log, defaultNamespace, - readFromSrcDir: true, }) expect(manifests).to.exist const names = manifests.map((m) => ({ kind: m.kind, name: m.metadata?.name })) @@ -280,7 +279,6 @@ describe("getManifests", () => { action: resolvedAction, log: garden.log, defaultNamespace, - readFromSrcDir: true, }) expect(manifests).to.exist const names = manifests.map((m) => ({ kind: m.kind, name: m.metadata?.name })) @@ -306,7 +304,6 @@ describe("getManifests", () => { action: resolvedAction, log: garden.log, defaultNamespace, - readFromSrcDir: true, }), { contains: `Invalid manifest file path(s) declared in ${action.longDescription()}`, @@ -334,7 +331,6 @@ describe("getManifests", () => { action: resolvedAction, log: garden.log, defaultNamespace, - readFromSrcDir: true, }), { contains: `Invalid manifest file path(s) declared in ${action.longDescription()}`, 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 fdf28f1969..3d0b9b8564 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -15,7 +15,11 @@ import tmp from "tmp-promise" import { TestGarden } from "../../../../../helpers" import { getKubernetesTestGarden } from "./common" import { DeployTask } from "../../../../../../src/tasks/deploy" -import { getManifests } from "../../../../../../src/plugins/kubernetes/kubernetes-type/common" +import { + getManifests, + getMetadataManifest, + parseMetadataResource, +} from "../../../../../../src/plugins/kubernetes/kubernetes-type/common" import { KubeApi } from "../../../../../../src/plugins/kubernetes/api" import { ActionLog, createActionLog, Log } from "../../../../../../src/logger/log-entry" import { KubernetesPluginContext, KubernetesProvider } from "../../../../../../src/plugins/kubernetes/config" @@ -127,7 +131,143 @@ describe("kubernetes-type handlers", () => { } }) - describe("getServiceStatus", () => { + describe("getKubernetesDeployStatus", () => { + it("returns ready status and correct detail for a ready deployment", async () => { + const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) + const action = await garden.resolveAction({ + action: graph.getDeploy("module-simple"), + log: garden.log, + graph, + }) + const deployParams = { + ctx, + log: actionLog, + action, + force: false, + } + + await kubernetesDeploy(deployParams) + + const status = await getKubernetesDeployStatus(deployParams) + expect(status.state).to.equal("ready") + + expect(status.detail?.mode).to.equal("default") + expect(status.detail?.forwardablePorts).to.eql([ + { + name: undefined, + protocol: "TCP", + targetName: "Deployment/busybox-deployment", + targetPort: 80, + }, + ]) + + const remoteResources = status.detail?.detail?.remoteResources + expect(remoteResources).to.exist + expect(remoteResources.length).to.equal(1) + expect(remoteResources[0].kind).to.equal("Deployment") + }) + + it("should return missing status when metadata ConfigMap is missing", async () => { + const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) + const action = await garden.resolveAction({ + action: graph.getDeploy("module-simple"), + log: garden.log, + graph, + }) + const deployParams = { + ctx, + log: actionLog, + action, + force: false, + } + + await kubernetesDeploy(deployParams) + + const namespace = await getActionNamespace({ + ctx, + log, + action, + provider: ctx.provider, + skipCreate: true, + }) + + const metadataManifest = getMetadataManifest(action, namespace, []) + + await api.deleteBySpec({ log, namespace, manifest: metadataManifest }) + + const status = await getKubernetesDeployStatus(deployParams) + expect(status.state).to.equal("not-ready") + expect(status.detail?.state).to.equal("missing") + }) + + it("should return outdated status when metadata ConfigMap has different version", async () => { + const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) + const action = await garden.resolveAction({ + action: graph.getDeploy("module-simple"), + log: garden.log, + graph, + }) + const deployParams = { + ctx, + log: actionLog, + action, + force: false, + } + + await kubernetesDeploy(deployParams) + + const namespace = await getActionNamespace({ + ctx, + log, + action, + provider: ctx.provider, + skipCreate: true, + }) + + const metadataManifest = getMetadataManifest(action, namespace, []) + metadataManifest.data!.resolvedVersion = "v-foo" + + await api.replace({ log, namespace, resource: metadataManifest }) + + 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 graph = await garden.getConfigGraph({ log: garden.log, emit: false }) + const action = await garden.resolveAction({ + action: graph.getDeploy("module-simple"), + log: garden.log, + graph, + }) + const deployParams = { + ctx, + log: actionLog, + action, + force: false, + } + + await kubernetesDeploy(deployParams) + + const namespace = await getActionNamespace({ + ctx, + log, + action, + provider: ctx.provider, + skipCreate: true, + }) + + const metadataManifest = getMetadataManifest(action, namespace, []) + metadataManifest.data!.mode = "sync" + + await api.replace({ log, namespace, resource: metadataManifest }) + + const status = await getKubernetesDeployStatus(deployParams) + expect(status.state).to.equal("not-ready") + expect(status.detail?.state).to.equal("outdated") + }) + it("should return not-ready status for a manifest with a missing resource type", async () => { const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) const action = await garden.resolveAction({ @@ -183,11 +323,30 @@ describe("kubernetes-type handlers", () => { log, action: resolvedAction, defaultNamespace: namespace, - readFromSrcDir: true, }) return { deployParams, manifests } } + it("gets the correct manifests when `build` is set", async () => { + const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) + const action = graph.getDeploy("with-build-action") + const deployParams = { + ctx, + log: actionLog, + action: await garden.resolveAction({ action, log: garden.log, graph }), + force: false, + } + + const status = await kubernetesDeploy(deployParams) + expect(status.state).to.eql("ready") + + const remoteResources = status.detail?.detail?.remoteResources + expect(remoteResources).to.exist + expect(remoteResources.length).to.equal(1) + expect(remoteResources[0].kind).to.equal("Deployment") + expect(remoteResources[0].metadata?.name).to.equal("busybox-deployment") + }) + it("should successfully deploy when serviceResource doesn't have a containerModule", async () => { const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) const action = graph.getDeploy("module-simple") @@ -207,6 +366,48 @@ describe("kubernetes-type handlers", () => { expect(namespaceStatus!.namespaceName).to.eql("kubernetes-type-test-default") }) + it("creates a metadata ConfigMap describing what was last deployed", async () => { + const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) + const action = graph.getDeploy("module-simple") + const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) + const deployParams = { + ctx, + log: actionLog, + action: resolvedAction, + force: false, + } + const status = await kubernetesDeploy(deployParams) + expect(status.state).to.eql("ready") + + const namespace = await getActionNamespace({ + ctx, + log, + action: resolvedAction, + provider: ctx.provider, + skipCreate: true, + }) + + const emptyManifest = getMetadataManifest(resolvedAction, namespace, []) + + const metadataResource = await api.readBySpec({ log, namespace, manifest: emptyManifest }) + + expect(metadataResource).to.exist + + const parsedMetadata = parseMetadataResource(log, metadataResource) + + expect(parsedMetadata.resolvedVersion).to.equal(resolvedAction.versionString()) + expect(parsedMetadata.mode).to.equal("default") + expect(parsedMetadata.manifestMetadata).to.eql({ + "Deployment/busybox-deployment": { + apiVersion: "apps/v1", + key: "Deployment/busybox-deployment", + kind: "Deployment", + name: "busybox-deployment", + namespace: "kubernetes-type-test-default", + }, + }) + }) + it("should toggle sync mode", async () => { const syncData = await getTestData("with-source-module", { sync: ["deploy.with-source-module"], @@ -322,8 +523,9 @@ describe("kubernetes-type handlers", () => { it("should successfully deploy List manifest kinds", async () => { const configMapList = await getTestData("config-map-list", {}) - // this should be 3, and not 1, as we transform *List objects to separate manifests - expect(configMapList.manifests.length).to.be.equal(3) + // this should be 4, and not 2 (including the metadata manifest), + // as we transform *List objects to separate manifests + expect(configMapList.manifests.length).to.be.equal(4) // test successful deploy await kubernetesDeploy(configMapList.deployParams) From 48abf35d1bf0ff6d4b4866bca45384303f2dd09e Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Tue, 18 Jul 2023 14:05:05 +0200 Subject: [PATCH 04/30] fix(k8s): quickfix to avoid resetting `outdated` state to `ready` --- core/src/plugins/kubernetes/kubernetes-type/handlers.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index 705eb622b5..d982e6e74b 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -237,7 +237,9 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne }) ) - state = resolveResourceStatuses(log, statuses) + if (state !== "outdated") { + state = resolveResourceStatuses(log, statuses) + } } catch (error) { log.debug({ msg: `Failed querying for remote resources: ${error.message}`, error }) state = "unknown" From bc762e9c950c099f288c319b31615a85b13f8992 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Mon, 14 Aug 2023 13:38:29 +0200 Subject: [PATCH 05/30] test: fix tests --- .../with-build-action/deployment-action.yaml | 29 -------- .../with-build-action/with-build.garden.yml | 2 +- .../kubernetes/kubernetes-type/common.ts | 68 +++++++++---------- 3 files changed, 33 insertions(+), 66 deletions(-) delete mode 100644 core/test/data/test-projects/kubernetes-type/with-build-action/deployment-action.yaml 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 deleted file mode 100644 index a65e8c5176..0000000000 --- a/core/test/data/test-projects/kubernetes-type/with-build-action/deployment-action.yaml +++ /dev/null @@ -1,29 +0,0 @@ -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 f754084cf6..dd5eeee6ff 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: [ "deployment-action.yaml" ] + files: [ "*.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 3abef5c6b9..8e141462fd 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/common.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/common.ts @@ -213,14 +213,16 @@ describe("getManifests", () => { it("runs kustomize build in the given path", async () => { const result = await getManifests({ ctx, api, action, log: garden.log, defaultNamespace }) const kinds = result.map((r) => r.kind) - expect(kinds).to.have.members(["ConfigMap", "Service", "Deployment"]) + // the last ConfigMap stands for internal metadata ConfigMap + expect(kinds).to.have.members(["ConfigMap", "Service", "Deployment", "ConfigMap"]) }) it("adds extraArgs if specified to the build command", async () => { action["_config"].spec.kustomize!.extraArgs = ["--reorder", "none"] const result = await getManifests({ ctx, api, action, log: garden.log, defaultNamespace }) const kinds = result.map((r) => r.kind) - expect(kinds).to.eql(["Deployment", "Service", "ConfigMap"]) + // the last ConfigMap stands for internal metadata ConfigMap + expect(kinds).to.eql(["Deployment", "Service", "ConfigMap", "ConfigMap"]) }) }) @@ -237,52 +239,50 @@ describe("getManifests", () => { }) it("should support regular files paths", async () => { - const resolvedAction = await garden.resolveAction({ + const executedAction = await garden.executeAction({ 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 getManifests({ - ctx, - api, - action: resolvedAction, - log: garden.log, - defaultNamespace, - }) + // Pre-check to ensure that the test project has a correct default glob file pattern. + expect(executedAction.getSpec().files).to.eql(["*.yaml"]) + + const manifests = await getManifests({ ctx, api, action: executedAction, log: garden.log, defaultNamespace }) 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" }]) + // Now `getManifests` also returns a ConfigMap with internal metadata + expect(names).to.eql([ + { kind: "Deployment", name: "busybox-deployment" }, + { + kind: "ConfigMap", + name: "garden-meta-deploy-with-build-action", + }, + ]) }) 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({ + // Append a valid filename that results to the default glob pattern '*.yaml'. + action["_config"]["spec"]["files"].push("deployment.yaml") + const executedAction = 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 getManifests({ - ctx, - api, - action: resolvedAction, - log: garden.log, - defaultNamespace, - }) + // Pre-check to ensure that the list of files in the test project config is correct. + expect(executedAction.getSpec().files).to.eql(["*.yaml", "deployment.yaml"]) + + const manifests = await getManifests({ ctx, api, action: executedAction, log: garden.log, defaultNamespace }) 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" }]) + // Now `getManifests` also returns a ConfigMap with internal metadata + expect(names).to.eql([ + { kind: "Deployment", name: "busybox-deployment" }, + { + kind: "ConfigMap", + name: "garden-meta-deploy-with-build-action", + }, + ]) }) it("should throw on missing regular path", async () => { @@ -294,8 +294,6 @@ describe("getManifests", () => { 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( () => getManifests({ @@ -321,8 +319,6 @@ describe("getManifests", () => { 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( () => getManifests({ From 53dcb31374193815ba88734118d405aceea258bc Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Wed, 6 Sep 2023 11:53:21 +0200 Subject: [PATCH 06/30] refactor(test): move helper to the upper-level scope It will be used by other test contexts. --- .../kubernetes/kubernetes-type/handlers.ts | 60 +++++++++---------- 1 file changed, 30 insertions(+), 30 deletions(-) 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 3d0b9b8564..5fd32555fb 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -295,38 +295,38 @@ describe("kubernetes-type handlers", () => { }) }) - describe("kubernetesDeploy", () => { - async function getTestData(actionName: string, mode: ActionModeMap) { - const graph = await garden.getConfigGraph({ - log: garden.log, - emit: false, - actionModes: mode, - }) - const action = graph.getDeploy(actionName) - const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) - const deployParams = { - ctx, - log: actionLog, - action: resolvedAction, - force: false, - } - const namespace = await getActionNamespace({ - ctx, - log, - action: resolvedAction, - provider: ctx.provider, - skipCreate: true, - }) - const manifests = await getManifests({ - ctx, - api, - log, - action: resolvedAction, - defaultNamespace: namespace, - }) - return { deployParams, manifests } + async function getTestData(actionName: string, mode: ActionModeMap) { + const graph = await garden.getConfigGraph({ + log: garden.log, + emit: false, + actionModes: mode, + }) + const action = graph.getDeploy(actionName) + const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) + const deployParams = { + ctx, + log: actionLog, + action: resolvedAction, + force: false, } + const namespace = await getActionNamespace({ + ctx, + log, + action: resolvedAction, + provider: ctx.provider, + skipCreate: true, + }) + const manifests = await getManifests({ + ctx, + api, + log, + action: resolvedAction, + defaultNamespace: namespace, + }) + return { deployParams, manifests } + } + describe("kubernetesDeploy", () => { it("gets the correct manifests when `build` is set", async () => { const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) const action = graph.getDeploy("with-build-action") From 2f192f2df3a47f95c0b016a61675345c4900040e Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Wed, 6 Sep 2023 13:35:46 +0200 Subject: [PATCH 07/30] refactor(test): introduce one more convenience helper --- .../kubernetes/kubernetes-type/handlers.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) 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 5fd32555fb..0651484495 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -295,7 +295,7 @@ describe("kubernetes-type handlers", () => { }) }) - async function getTestData(actionName: string, mode: ActionModeMap) { + async function prepareActionDeployParams(actionName: string, mode: ActionModeMap) { const graph = await garden.getConfigGraph({ log: garden.log, emit: false, @@ -309,6 +309,11 @@ describe("kubernetes-type handlers", () => { action: resolvedAction, force: false, } + return { action, resolvedAction, deployParams } + } + + async function prepareActionDeployParamsWithManifests(actionName: string, mode: ActionModeMap) { + const { action, resolvedAction, deployParams } = await prepareActionDeployParams(actionName, mode) const namespace = await getActionNamespace({ ctx, log, @@ -323,7 +328,7 @@ describe("kubernetes-type handlers", () => { action: resolvedAction, defaultNamespace: namespace, }) - return { deployParams, manifests } + return { action, resolvedAction, deployParams, manifests } } describe("kubernetesDeploy", () => { @@ -409,10 +414,10 @@ describe("kubernetes-type handlers", () => { }) it("should toggle sync mode", async () => { - const syncData = await getTestData("with-source-module", { + const syncData = await prepareActionDeployParamsWithManifests("with-source-module", { sync: ["deploy.with-source-module"], }) - const defaultData = await getTestData("with-source-module", { + const defaultData = await prepareActionDeployParamsWithManifests("with-source-module", { default: ["deploy.with-source-module"], }) @@ -434,10 +439,10 @@ describe("kubernetes-type handlers", () => { }) it("should handle local mode", async () => { - const localData = await getTestData("with-source-module", { + const localData = await prepareActionDeployParamsWithManifests("with-source-module", { local: ["deploy.with-source-module"], }) - const defaultData = await getTestData("with-source-module", { + const defaultData = await prepareActionDeployParamsWithManifests("with-source-module", { default: ["deploy.with-source-module"], }) @@ -521,7 +526,7 @@ describe("kubernetes-type handlers", () => { }) it("should successfully deploy List manifest kinds", async () => { - const configMapList = await getTestData("config-map-list", {}) + const configMapList = await prepareActionDeployParamsWithManifests("config-map-list", {}) // this should be 4, and not 2 (including the metadata manifest), // as we transform *List objects to separate manifests From 00e33bfc2b64b4d827c4cb29e11166fa5bb1457b Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Wed, 6 Sep 2023 13:38:56 +0200 Subject: [PATCH 08/30] refactor(test): introduce and rename some local vars --- .../kubernetes/kubernetes-type/handlers.ts | 40 ++++++++++--------- 1 file changed, 21 insertions(+), 19 deletions(-) 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 0651484495..7ae4cf0b6c 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -134,7 +134,7 @@ describe("kubernetes-type handlers", () => { describe("getKubernetesDeployStatus", () => { it("returns ready status and correct detail for a ready deployment", async () => { const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const action = await garden.resolveAction({ + const resolvedAction = await garden.resolveAction({ action: graph.getDeploy("module-simple"), log: garden.log, graph, @@ -142,7 +142,7 @@ describe("kubernetes-type handlers", () => { const deployParams = { ctx, log: actionLog, - action, + action: resolvedAction, force: false, } @@ -169,7 +169,7 @@ describe("kubernetes-type handlers", () => { it("should return missing status when metadata ConfigMap is missing", async () => { const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const action = await garden.resolveAction({ + const resolvedAction = await garden.resolveAction({ action: graph.getDeploy("module-simple"), log: garden.log, graph, @@ -177,7 +177,7 @@ describe("kubernetes-type handlers", () => { const deployParams = { ctx, log: actionLog, - action, + action: resolvedAction, force: false, } @@ -186,12 +186,12 @@ describe("kubernetes-type handlers", () => { const namespace = await getActionNamespace({ ctx, log, - action, + action: resolvedAction, provider: ctx.provider, skipCreate: true, }) - const metadataManifest = getMetadataManifest(action, namespace, []) + const metadataManifest = getMetadataManifest(resolvedAction, namespace, []) await api.deleteBySpec({ log, namespace, manifest: metadataManifest }) @@ -202,7 +202,7 @@ describe("kubernetes-type handlers", () => { it("should return outdated status when metadata ConfigMap has different version", async () => { const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const action = await garden.resolveAction({ + const resolvedAction = await garden.resolveAction({ action: graph.getDeploy("module-simple"), log: garden.log, graph, @@ -210,7 +210,7 @@ describe("kubernetes-type handlers", () => { const deployParams = { ctx, log: actionLog, - action, + action: resolvedAction, force: false, } @@ -219,12 +219,12 @@ describe("kubernetes-type handlers", () => { const namespace = await getActionNamespace({ ctx, log, - action, + action: resolvedAction, provider: ctx.provider, skipCreate: true, }) - const metadataManifest = getMetadataManifest(action, namespace, []) + const metadataManifest = getMetadataManifest(resolvedAction, namespace, []) metadataManifest.data!.resolvedVersion = "v-foo" await api.replace({ log, namespace, resource: metadataManifest }) @@ -236,7 +236,7 @@ describe("kubernetes-type handlers", () => { it("should return outdated status when metadata ConfigMap has different mode", async () => { const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const action = await garden.resolveAction({ + const resolvedAction = await garden.resolveAction({ action: graph.getDeploy("module-simple"), log: garden.log, graph, @@ -244,7 +244,7 @@ describe("kubernetes-type handlers", () => { const deployParams = { ctx, log: actionLog, - action, + action: resolvedAction, force: false, } @@ -253,12 +253,12 @@ describe("kubernetes-type handlers", () => { const namespace = await getActionNamespace({ ctx, log, - action, + action: resolvedAction, provider: ctx.provider, skipCreate: true, }) - const metadataManifest = getMetadataManifest(action, namespace, []) + const metadataManifest = getMetadataManifest(resolvedAction, namespace, []) metadataManifest.data!.mode = "sync" await api.replace({ log, namespace, resource: metadataManifest }) @@ -270,7 +270,7 @@ describe("kubernetes-type handlers", () => { it("should return not-ready status for a manifest with a missing resource type", async () => { const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const action = await garden.resolveAction({ + const resolvedAction = await garden.resolveAction({ action: graph.getDeploy("module-simple"), log: garden.log, graph, @@ -278,10 +278,10 @@ describe("kubernetes-type handlers", () => { const deployParams = { ctx, log: actionLog, - action, + action: resolvedAction, force: false, } - action["_config"].spec.manifests = [ + resolvedAction["_config"].spec.manifests = [ { apiVersion: "foo.bar/baz", kind: "Whatever", @@ -335,10 +335,11 @@ describe("kubernetes-type handlers", () => { it("gets the correct manifests when `build` is set", async () => { const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) const action = graph.getDeploy("with-build-action") + const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) const deployParams = { ctx, log: actionLog, - action: await garden.resolveAction({ action, log: garden.log, graph }), + action: resolvedAction, force: false, } @@ -355,10 +356,11 @@ describe("kubernetes-type handlers", () => { it("should successfully deploy when serviceResource doesn't have a containerModule", async () => { const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) const action = graph.getDeploy("module-simple") + const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) const deployParams = { ctx, log: actionLog, - action: await garden.resolveAction({ action, log: garden.log, graph }), + action: resolvedAction, force: false, } From 120874b1d88f3a3a3f383cb8a24fcc6be7bd7a36 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Wed, 6 Sep 2023 13:48:13 +0200 Subject: [PATCH 09/30] refactor(test): use helpers to avoid code duplication --- .../kubernetes/kubernetes-type/handlers.ts | 96 ++----------------- 1 file changed, 9 insertions(+), 87 deletions(-) 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 7ae4cf0b6c..229f33f2c3 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -133,18 +133,7 @@ describe("kubernetes-type handlers", () => { describe("getKubernetesDeployStatus", () => { it("returns ready status and correct detail for a ready deployment", async () => { - const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const resolvedAction = await garden.resolveAction({ - action: graph.getDeploy("module-simple"), - log: garden.log, - graph, - }) - const deployParams = { - ctx, - log: actionLog, - action: resolvedAction, - force: false, - } + const { deployParams } = await prepareActionDeployParams("module-simple", {}) await kubernetesDeploy(deployParams) @@ -168,18 +157,7 @@ describe("kubernetes-type handlers", () => { }) it("should return missing status when metadata ConfigMap is missing", async () => { - const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const resolvedAction = await garden.resolveAction({ - action: graph.getDeploy("module-simple"), - log: garden.log, - graph, - }) - const deployParams = { - ctx, - log: actionLog, - action: resolvedAction, - force: false, - } + const { resolvedAction, deployParams } = await prepareActionDeployParams("module-simple", {}) await kubernetesDeploy(deployParams) @@ -201,18 +179,7 @@ describe("kubernetes-type handlers", () => { }) it("should return outdated status when metadata ConfigMap has different version", async () => { - const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const resolvedAction = await garden.resolveAction({ - action: graph.getDeploy("module-simple"), - log: garden.log, - graph, - }) - const deployParams = { - ctx, - log: actionLog, - action: resolvedAction, - force: false, - } + const { resolvedAction, deployParams } = await prepareActionDeployParams("module-simple", {}) await kubernetesDeploy(deployParams) @@ -235,18 +202,7 @@ describe("kubernetes-type handlers", () => { }) it("should return outdated status when metadata ConfigMap has different mode", async () => { - const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const resolvedAction = await garden.resolveAction({ - action: graph.getDeploy("module-simple"), - log: garden.log, - graph, - }) - const deployParams = { - ctx, - log: actionLog, - action: resolvedAction, - force: false, - } + const { resolvedAction, deployParams } = await prepareActionDeployParams("module-simple", {}) await kubernetesDeploy(deployParams) @@ -269,18 +225,7 @@ describe("kubernetes-type handlers", () => { }) it("should return not-ready status for a manifest with a missing resource type", async () => { - const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const resolvedAction = await garden.resolveAction({ - action: graph.getDeploy("module-simple"), - log: garden.log, - graph, - }) - const deployParams = { - ctx, - log: actionLog, - action: resolvedAction, - force: false, - } + const { resolvedAction, deployParams } = await prepareActionDeployParams("module-simple", {}) resolvedAction["_config"].spec.manifests = [ { apiVersion: "foo.bar/baz", @@ -333,15 +278,7 @@ describe("kubernetes-type handlers", () => { describe("kubernetesDeploy", () => { it("gets the correct manifests when `build` is set", async () => { - const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const action = graph.getDeploy("with-build-action") - const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) - const deployParams = { - ctx, - log: actionLog, - action: resolvedAction, - force: false, - } + const { resolvedAction, deployParams } = await prepareActionDeployParams("with-build-action", {}) const status = await kubernetesDeploy(deployParams) expect(status.state).to.eql("ready") @@ -354,15 +291,7 @@ describe("kubernetes-type handlers", () => { }) it("should successfully deploy when serviceResource doesn't have a containerModule", async () => { - const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const action = graph.getDeploy("module-simple") - const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) - const deployParams = { - ctx, - log: actionLog, - action: resolvedAction, - force: false, - } + const { deployParams } = await prepareActionDeployParams("module-simple", {}) // Here, we're not going through a router, so we listen for the `namespaceStatus` event directly. let namespaceStatus: NamespaceStatus | null = null @@ -374,15 +303,8 @@ describe("kubernetes-type handlers", () => { }) it("creates a metadata ConfigMap describing what was last deployed", async () => { - const graph = await garden.getConfigGraph({ log: garden.log, emit: false }) - const action = graph.getDeploy("module-simple") - const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) - const deployParams = { - ctx, - log: actionLog, - action: resolvedAction, - force: false, - } + const { resolvedAction, deployParams } = await prepareActionDeployParams("module-simple", {}) + const status = await kubernetesDeploy(deployParams) expect(status.state).to.eql("ready") From b6f146f162e84e55c5352da46c8e9652abdc98d2 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Wed, 6 Sep 2023 13:52:12 +0200 Subject: [PATCH 10/30] chore: re-arranged code Keep all helpers close to each other for simpler navigation. --- .../kubernetes/kubernetes-type/handlers.ts | 72 +++++++++---------- 1 file changed, 36 insertions(+), 36 deletions(-) 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 229f33f2c3..a58dcf7cc3 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -75,6 +75,42 @@ describe("kubernetes-type handlers", () => { return maybeDeployedObjects.filter((o) => o !== null) } + async function prepareActionDeployParams(actionName: string, mode: ActionModeMap) { + const graph = await garden.getConfigGraph({ + log: garden.log, + emit: false, + actionModes: mode, + }) + const action = graph.getDeploy(actionName) + const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) + const deployParams = { + ctx, + log: actionLog, + action: resolvedAction, + force: false, + } + return { action, resolvedAction, deployParams } + } + + async function prepareActionDeployParamsWithManifests(actionName: string, mode: ActionModeMap) { + const { action, resolvedAction, deployParams } = await prepareActionDeployParams(actionName, mode) + const namespace = await getActionNamespace({ + ctx, + log, + action: resolvedAction, + provider: ctx.provider, + skipCreate: true, + }) + const manifests = await getManifests({ + ctx, + api, + log, + action: resolvedAction, + defaultNamespace: namespace, + }) + return { action, resolvedAction, deployParams, manifests } + } + before(async () => { garden = await getKubernetesTestGarden() moduleConfigBackup = await garden.getRawModuleConfigs() @@ -240,42 +276,6 @@ describe("kubernetes-type handlers", () => { }) }) - async function prepareActionDeployParams(actionName: string, mode: ActionModeMap) { - const graph = await garden.getConfigGraph({ - log: garden.log, - emit: false, - actionModes: mode, - }) - const action = graph.getDeploy(actionName) - const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) - const deployParams = { - ctx, - log: actionLog, - action: resolvedAction, - force: false, - } - return { action, resolvedAction, deployParams } - } - - async function prepareActionDeployParamsWithManifests(actionName: string, mode: ActionModeMap) { - const { action, resolvedAction, deployParams } = await prepareActionDeployParams(actionName, mode) - const namespace = await getActionNamespace({ - ctx, - log, - action: resolvedAction, - provider: ctx.provider, - skipCreate: true, - }) - const manifests = await getManifests({ - ctx, - api, - log, - action: resolvedAction, - defaultNamespace: namespace, - }) - return { action, resolvedAction, deployParams, manifests } - } - describe("kubernetesDeploy", () => { it("gets the correct manifests when `build` is set", async () => { const { resolvedAction, deployParams } = await prepareActionDeployParams("with-build-action", {}) From 016fc435d7709afefd1b1ffa996057e9d26d13e5 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Tue, 5 Sep 2023 15:25:19 +0200 Subject: [PATCH 11/30] chore(test): rename some tests and variables To avoid usage of the old glossary. --- .../src/plugins/kubernetes/kubernetes-type/handlers.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 a58dcf7cc3..23ad0a8cce 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -461,14 +461,14 @@ describe("kubernetes-type handlers", () => { }) }) - describe("deleteService", () => { + describe("deleteKubernetesDeploy", () => { it("should only delete namespace resources having the current name in the manifests", async () => { // First, we verify that the namespaces created in the preceding test case are still there. expect(await getDeployedResource(ctx, ctx.provider, ns1Manifest!, log), "ns1resource").to.exist expect(await getDeployedResource(ctx, ctx.provider, ns2Manifest!, log), "ns2resource").to.exist const graph = await garden.getConfigGraph({ log, emit: false }) - const deleteServiceTask = new DeleteDeployTask({ + const deleteDeployTask = new DeleteDeployTask({ garden, graph, log, @@ -477,7 +477,7 @@ describe("kubernetes-type handlers", () => { }) // This should only delete kubernetes-type-ns-2. - await garden.processTasks({ tasks: [deleteServiceTask], throwOnError: true }) + await garden.processTasks({ tasks: [deleteDeployTask], throwOnError: true }) expect(await getDeployedResource(ctx, ctx.provider, ns1Manifest!, log), "ns1resource").to.exist expect(await getDeployedResource(ctx, ctx.provider, ns2Manifest!, log), "ns2resource").to.not.exist From 15b28a2c51346a5913d2117a6fb0ab769101679a Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Tue, 5 Sep 2023 16:19:48 +0200 Subject: [PATCH 12/30] refactor(test): helper function to deploy in namespaces * DRY to avoid repetition and too complex local state * Less shared global state between different test contexts * Avoid dependencies between tests and reliance on the execution order * Ability to run individual tests locally --- .../kubernetes/kubernetes-type/handlers.ts | 90 +++++++++---------- 1 file changed, 45 insertions(+), 45 deletions(-) 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 23ad0a8cce..aeecfd6930 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -56,10 +56,6 @@ describe("kubernetes-type handlers", () => { */ let moduleConfigBackup: ModuleConfig[] let nsModuleConfig: ModuleConfig - let ns1Manifest: KubernetesResource | undefined - let ns1Resource: KubernetesResource | null - let ns2Manifest: KubernetesResource | undefined - let ns2Resource: KubernetesResource | null const withNamespace = (moduleConfig: ModuleConfig, nsName: string): ModuleConfig => { const cloned = cloneDeep(moduleConfig) @@ -111,6 +107,28 @@ describe("kubernetes-type handlers", () => { return { action, resolvedAction, deployParams, manifests } } + async function deployInNamespace({ nsName, deployName }: { nsName: string; deployName: string }) { + garden.setModuleConfigs([withNamespace(nsModuleConfig, nsName)]) + const graph = await garden.getConfigGraph({ log, emit: false }) + const action = graph.getDeploy(deployName) + const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) + const defaultNamespace = await getActionNamespace({ ctx, log, action: resolvedAction, provider: ctx.provider }) + const manifests = await getManifests({ ctx, api, log, action: resolvedAction, defaultNamespace }) + const manifest = manifests.find((resource) => resource.kind === "Namespace") + + const deployTask = new DeployTask({ + garden, + graph, + log, + action, + force: true, + forceBuild: false, + }) + await garden.processTasks({ tasks: [deployTask], throwOnError: true }) + const resource = await getDeployedResource(ctx, ctx.provider, manifest!, log) + return { manifest, resource, graph, action, resolvedAction } + } + before(async () => { garden = await getKubernetesTestGarden() moduleConfigBackup = await garden.getRawModuleConfigs() @@ -391,24 +409,10 @@ describe("kubernetes-type handlers", () => { }) it("should not delete previously deployed namespace resources", async () => { - garden.setModuleConfigs([withNamespace(nsModuleConfig, "kubernetes-type-ns-1")]) - let graph = await garden.getConfigGraph({ log, emit: false }) - let action = graph.getDeploy("namespace-resource") - const resolvedAction = await garden.resolveAction({ action, log: garden.log, graph }) - const defaultNamespace = await getActionNamespace({ ctx, log, action: resolvedAction, provider: ctx.provider }) - let manifests = await getManifests({ ctx, api, log, action: resolvedAction, defaultNamespace }) - ns1Manifest = manifests.find((resource) => resource.kind === "Namespace") - - const deployTask = new DeployTask({ - garden, - graph, - log, - action, - force: true, - forceBuild: false, + const { manifest: ns1Manifest, resource: ns1Resource } = await deployInNamespace({ + nsName: "kubernetes-type-ns-1", + deployName: "namespace-resource", }) - const results = await garden.processTasks({ tasks: [deployTask], throwOnError: true }) - ns1Resource = await getDeployedResource(ctx, ctx.provider, ns1Manifest!, log) expect(ns1Manifest, "ns1Manifest").to.exist expect(ns1Manifest!.metadata.name).to.match(/ns-1/) @@ -417,27 +421,10 @@ describe("kubernetes-type handlers", () => { // this module. // This should result in a new namespace with a new name being deployed. - garden.setModuleConfigs([withNamespace(nsModuleConfig, "kubernetes-type-ns-2")]) - graph = await garden.getConfigGraph({ log, emit: false }) - action = graph.getDeploy("namespace-resource") - manifests = await getManifests({ - ctx, - api, - log, - action: await garden.resolveAction({ action, log: garden.log, graph }), - defaultNamespace, - }) - ns2Manifest = manifests.find((resource) => resource.kind === "Namespace") - const deployTask2 = new DeployTask({ - garden, - graph, - log, - action, - force: true, - forceBuild: true, + const { manifest: ns2Manifest, resource: ns2Resource } = await deployInNamespace({ + nsName: "kubernetes-type-ns-2", + deployName: "namespace-resource", }) - await garden.processTasks({ tasks: [deployTask2], throwOnError: true }) - ns2Resource = await getDeployedResource(ctx, ctx.provider, ns2Manifest!, log) expect(ns2Manifest, "ns2Manifest").to.exist expect(ns2Manifest!.metadata.name).to.match(/ns-2/) @@ -463,16 +450,29 @@ describe("kubernetes-type handlers", () => { describe("deleteKubernetesDeploy", () => { it("should only delete namespace resources having the current name in the manifests", async () => { - // First, we verify that the namespaces created in the preceding test case are still there. + const { manifest: ns1Manifest, resource: ns1Resource } = await deployInNamespace({ + nsName: "kubernetes-type-ns-1", + deployName: "namespace-resource", + }) + // Verify the presence of the namespace expect(await getDeployedResource(ctx, ctx.provider, ns1Manifest!, log), "ns1resource").to.exist + + const { + graph: ns2Graph, + manifest: ns2Manifest, + resource: ns2Resource, + } = await deployInNamespace({ + nsName: "kubernetes-type-ns-2", + deployName: "namespace-resource", + }) + // Verify the presence of the namespace expect(await getDeployedResource(ctx, ctx.provider, ns2Manifest!, log), "ns2resource").to.exist - const graph = await garden.getConfigGraph({ log, emit: false }) const deleteDeployTask = new DeleteDeployTask({ garden, - graph, + graph: ns2Graph, log, - action: graph.getDeploy("namespace-resource"), + action: ns2Graph.getDeploy("namespace-resource"), force: false, }) From f3ec73855453316ac561735d06dcb043f5608611 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Tue, 5 Sep 2023 16:21:57 +0200 Subject: [PATCH 13/30] refactor(test): convert lambdas to functions --- .../integ/src/plugins/kubernetes/kubernetes-type/handlers.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 aeecfd6930..4480f7fddd 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -57,14 +57,14 @@ describe("kubernetes-type handlers", () => { let moduleConfigBackup: ModuleConfig[] let nsModuleConfig: ModuleConfig - const withNamespace = (moduleConfig: ModuleConfig, nsName: string): ModuleConfig => { + function withNamespace(moduleConfig: ModuleConfig, nsName: string): ModuleConfig { const cloned = cloneDeep(moduleConfig) cloned.spec.manifests[0].metadata.name = nsName cloned.spec.manifests[0].metadata.labels.name = nsName return cloned } - const findDeployedResources = async (manifests: KubernetesResource[], logCtx: Log) => { + async function findDeployedResources(manifests: KubernetesResource[], logCtx: Log) { const maybeDeployedObjects = await Promise.all( manifests.map((resource) => getDeployedResource(ctx, ctx.provider, resource, logCtx)) ) From 485e05ba20f2817767c8f36deaf0bd451699f8d5 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Wed, 6 Sep 2023 15:21:13 +0200 Subject: [PATCH 14/30] test: ensure all resources are deleted by `deleteKubernetesDeploy` A metadata `ConfigMap` describing what was last deployed must be deleted too. --- .../kubernetes/kubernetes-type/handlers.ts | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) 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 4480f7fddd..3331301d35 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -29,8 +29,9 @@ import { ModuleConfig } from "../../../../../../src/config/module" import { BaseResource, KubernetesResource } from "../../../../../../src/plugins/kubernetes/types" import { DeleteDeployTask } from "../../../../../../src/tasks/delete-deploy" import { - kubernetesDeploy, + deleteKubernetesDeploy, getKubernetesDeployStatus, + kubernetesDeploy, } from "../../../../../../src/plugins/kubernetes/kubernetes-type/handlers" import { buildHelmModules } from "../helm/common" import { gardenAnnotationKey } from "../../../../../../src/util/string" @@ -482,5 +483,31 @@ 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 }) + + it("deletes all resources including a metadata ConfigMap describing what was last deployed", async () => { + const { resolvedAction, deployParams, manifests } = await prepareActionDeployParamsWithManifests( + "module-simple", + {} + ) + + const status = await kubernetesDeploy(deployParams) + expect(status.state).to.eql("ready") + + const namespace = await getActionNamespace({ + ctx, + log, + action: resolvedAction, + provider: ctx.provider, + skipCreate: true, + }) + + const deleteDeployParams = { ctx, action: resolvedAction, log: actionLog } + await deleteKubernetesDeploy(deleteDeployParams) + + for (const manifest of manifests) { + const metadataResource = await api.readBySpecOrNull({ log, namespace, manifest }) + expect(metadataResource).to.be.null + } + }) }) }) From dc3012d374f3ccc4a44175cc656d958839270657 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Wed, 6 Sep 2023 16:09:55 +0200 Subject: [PATCH 15/30] test: restore initial module config state after each test To avoid unexpected pollution of the Garden instance's state. Multiple test can define temporary custom module config. Such config changes should not affect the other tests. --- .../integ/src/plugins/kubernetes/kubernetes-type/handlers.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 3331301d35..c47ba52bdf 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -179,13 +179,16 @@ describe("kubernetes-type handlers", () => { }) after(async () => { - garden.setModuleConfigs(moduleConfigBackup) await tmpDir.cleanup() if (garden) { garden.close() } }) + afterEach(() => { + garden.setModuleConfigs(moduleConfigBackup) + }) + describe("getKubernetesDeployStatus", () => { it("returns ready status and correct detail for a ready deployment", async () => { const { deployParams } = await prepareActionDeployParams("module-simple", {}) From 0b82205989cf168dd9d1caaf050308e231eb2be8 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Wed, 6 Sep 2023 13:38:56 +0200 Subject: [PATCH 16/30] refactor(k8s): helper to compose k8s deploy status --- .../kubernetes/kubernetes-type/handlers.ts | 55 ++++++++++++++----- 1 file changed, 40 insertions(+), 15 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index d982e6e74b..7376d1b1d5 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -11,7 +11,7 @@ import type { ModuleActionHandlers } from "../../../plugin/plugin" import { DeployState, ForwardablePort, ServiceStatus } from "../../../types/service" import { gardenAnnotationKey } from "../../../util/string" import { KubeApi } from "../api" -import type { KubernetesPluginContext } from "../config" +import type {KubernetesPluginContext, KubernetesProvider} from "../config" import { configureSyncMode, convertKubernetesModuleDevModeSpec } from "../sync" import { apply, deleteObjectsBySelector } from "../kubectl" import { streamK8sLogs } from "../logs" @@ -166,6 +166,36 @@ interface KubernetesStatusDetail { export type KubernetesServiceStatus = ServiceStatus +function composeKubernetesDeployStatus({ + action, + deployedMode, + state, + remoteResources, + forwardablePorts, + provider, +}: { + action: KubernetesDeployAction + deployedMode: ActionMode + state: DeployState + remoteResources: KubernetesResource[] + forwardablePorts: ForwardablePort[] + provider: KubernetesProvider +}) { + return { + state: deployStateToActionState(state), + detail: { + forwardablePorts, + state, + version: state === "ready" ? action.versionString() : undefined, + detail: { remoteResources }, + mode: deployedMode, + ingresses: getK8sIngresses(remoteResources, provider), + }, + // TODO-0.13.1 + outputs: {}, + } +} + export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", KubernetesDeployAction> = async (params) => { const { ctx, action, log } = params const spec = action.getSpec() @@ -190,7 +220,7 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne let deployedMode: ActionMode = "default" let state: DeployState = "ready" let remoteResources: KubernetesResource[] = [] - let forwardablePorts: ForwardablePort[] | undefined + let forwardablePorts: ForwardablePort[] = [] const remoteMetadataResource = await getDeployedResource(ctx, provider, metadataManifest, log) @@ -256,19 +286,14 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne } } - return { - state: deployStateToActionState(state), - detail: { - forwardablePorts, - state, - version: state === "ready" ? action.versionString() : undefined, - detail: { remoteResources }, - mode: deployedMode, - ingresses: getK8sIngresses(remoteResources || [], provider), - }, - // TODO-0.13.1 - outputs: {}, - } + return composeKubernetesDeployStatus({ + action, + deployedMode, + state, + remoteResources, + forwardablePorts, + provider, + }) } export const kubernetesDeploy: DeployActionHandler<"deploy", KubernetesDeployAction> = async (params) => { From a3d44f5c65d688c1889323bd8282414b46009944 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Thu, 7 Sep 2023 10:22:27 +0200 Subject: [PATCH 17/30] refactor(k8s): return deploy status immediately on "missing" state --- core/src/plugins/kubernetes/kubernetes-type/handlers.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index 7376d1b1d5..fd28ae63ec 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -225,7 +225,13 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne const remoteMetadataResource = await getDeployedResource(ctx, provider, metadataManifest, log) if (!remoteMetadataResource) { - state = "missing" + return composeKubernetesDeployStatus({ + action, + deployedMode: "default", + state: "missing", + remoteResources: [], + forwardablePorts: [], + }) } else { const deployedMetadata = parseMetadataResource(log, remoteMetadataResource) deployedMode = deployedMetadata.mode From 0f603967e4c913bea21e8bc02899dd3976806ed7 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Thu, 7 Sep 2023 10:28:40 +0200 Subject: [PATCH 18/30] refactor: unwrap unnecessary else-block --- .../kubernetes/kubernetes-type/handlers.ts | 104 +++++++++--------- 1 file changed, 52 insertions(+), 52 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index fd28ae63ec..c8a96c9a21 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -232,63 +232,63 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne remoteResources: [], forwardablePorts: [], }) - } else { - const deployedMetadata = parseMetadataResource(log, remoteMetadataResource) - deployedMode = deployedMetadata.mode - - if (deployedMetadata.resolvedVersion !== action.versionString()) { - state = "outdated" - } else if (mode === "local" && spec.localMode && deployedMode !== "local") { - state = "outdated" - } else if (mode === "sync" && spec.sync?.paths && deployedMode !== "sync") { - state = "outdated" - } else if (mode === "default" && deployedMode !== mode) { - state = "outdated" - } + } + + const deployedMetadata = parseMetadataResource(log, remoteMetadataResource) + deployedMode = deployedMetadata.mode + + if (deployedMetadata.resolvedVersion !== action.versionString()) { + state = "outdated" + } else if (mode === "local" && spec.localMode && deployedMode !== "local") { + state = "outdated" + } else if (mode === "sync" && spec.sync?.paths && deployedMode !== "sync") { + state = "outdated" + } else if (mode === "default" && deployedMode !== mode) { + state = "outdated" + } + + const manifestMetadata = Object.values(deployedMetadata.manifestMetadata) - const manifestMetadata = Object.values(deployedMetadata.manifestMetadata) - - if (manifestMetadata.length > 0) { - try { - const maybeDeployedResources = await Promise.all( - manifestMetadata.map(async (m) => { - return [m, await api.readOrNull({ log, ...m })] - }) - ) - - const statuses: ResourceStatus[] = await Promise.all( - maybeDeployedResources.map(async ([m, resource]) => { - if (!resource) { - return { - state: "missing" as const, - resource: { - apiVersion: m.apiVersion, - kind: m.kind, - metadata: { name: m.name, namespace: m.namespace }, - }, - } + if (manifestMetadata.length > 0) { + try { + const maybeDeployedResources = await Promise.all( + manifestMetadata.map(async (m) => { + return [m, await api.readOrNull({ log, ...m })] + }) + ) + + const statuses: ResourceStatus[] = await Promise.all( + maybeDeployedResources.map(async ([m, resource]) => { + if (!resource) { + return { + state: "missing" as const, + resource: { + apiVersion: m.apiVersion, + kind: m.kind, + metadata: { name: m.name, namespace: m.namespace }, + }, } - remoteResources.push(resource) - return resolveResourceStatus({ api, namespace: defaultNamespace, resource, log }) - }) - ) - - if (state !== "outdated") { - state = resolveResourceStatuses(log, statuses) - } - } catch (error) { - log.debug({ msg: `Failed querying for remote resources: ${error.message}`, error }) - state = "unknown" + } + remoteResources.push(resource) + return resolveResourceStatus({ api, namespace: defaultNamespace, resource, log }) + }) + ) + + if (state !== "outdated") { + state = resolveResourceStatuses(log, statuses) } + } catch (error) { + log.debug({ msg: `Failed querying for remote resources: ${error.message}`, error }) + state = "unknown" } + } - // Note: Local mode has its own port-forwarding configuration - if (deployedMode !== "local" && remoteResources && remoteResources.length > 0) { - try { - forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode }) - } catch (error) { - log.debug({ msg: `Unable to extract forwardable ports: ${error.message}`, error }) - } + // Note: Local mode has its own port-forwarding configuration + if (deployedMode !== "local" && remoteResources && remoteResources.length > 0) { + try { + forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode }) + } catch (error) { + log.debug({ msg: `Unable to extract forwardable ports: ${error.message}`, error }) } } From 7f898d5f84af798b1b29e8803a67b526a53a0e03 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Thu, 7 Sep 2023 10:31:20 +0200 Subject: [PATCH 19/30] chore: variable scoping --- .../plugins/kubernetes/kubernetes-type/handlers.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index c8a96c9a21..492843fe88 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -216,12 +216,6 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne // Note: This is analogous to how we version check Helm charts, i.e. we don't check every resource individually. // Users can always force deploy, much like with Helm Deploys. const metadataManifest = getMetadataManifest(action, defaultNamespace, []) - - let deployedMode: ActionMode = "default" - let state: DeployState = "ready" - let remoteResources: KubernetesResource[] = [] - let forwardablePorts: ForwardablePort[] = [] - const remoteMetadataResource = await getDeployedResource(ctx, provider, metadataManifest, log) if (!remoteMetadataResource) { @@ -235,7 +229,10 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne } const deployedMetadata = parseMetadataResource(log, remoteMetadataResource) - deployedMode = deployedMetadata.mode + const deployedMode = deployedMetadata.mode + let remoteResources: KubernetesResource[] = [] + let forwardablePorts: ForwardablePort[] = [] + let state: DeployState = "ready" if (deployedMetadata.resolvedVersion !== action.versionString()) { state = "outdated" From da6a0b8fb6399de53bdaadadf902795c69b1cb4c Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Thu, 7 Sep 2023 10:45:50 +0200 Subject: [PATCH 20/30] refactor: extract helper to check if deploy is outdated --- .../kubernetes/kubernetes-type/handlers.ts | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index 492843fe88..da862f22e6 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -31,6 +31,7 @@ import { gardenNamespaceAnnotationValue, getManifests, getMetadataManifest, + ParsedMetadataManifestData, parseMetadataResource, } from "./common" import { configureKubernetesModule, KubernetesModule } from "./module-config" @@ -41,6 +42,7 @@ import type { DeployActionHandler } from "../../../plugin/action-types" 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" export const kubernetesHandlers: Partial> = { configure: configureKubernetesModule, @@ -196,9 +198,32 @@ function composeKubernetesDeployStatus({ } } +function isOutdated({ + action, + deployedMetadata, +}: { + action: ResolvedDeployAction + deployedMetadata: ParsedMetadataManifestData +}): boolean { + const spec = action.getSpec() + const actionMode = action.mode() + const deployedMode = deployedMetadata.mode + + if (deployedMetadata.resolvedVersion !== action.versionString()) { + return true + } else if (actionMode === "local" && spec.localMode && deployedMode !== "local") { + return true + } else if (actionMode === "sync" && spec.sync?.paths && deployedMode !== "sync") { + return true + } else if (actionMode === "default" && deployedMode !== actionMode) { + return true + } + return false +} + export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", KubernetesDeployAction> = async (params) => { const { ctx, action, log } = params - const spec = action.getSpec() + const mode = action.mode() const k8sCtx = ctx @@ -234,13 +259,7 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne let forwardablePorts: ForwardablePort[] = [] let state: DeployState = "ready" - if (deployedMetadata.resolvedVersion !== action.versionString()) { - state = "outdated" - } else if (mode === "local" && spec.localMode && deployedMode !== "local") { - state = "outdated" - } else if (mode === "sync" && spec.sync?.paths && deployedMode !== "sync") { - state = "outdated" - } else if (mode === "default" && deployedMode !== mode) { + if (isOutdated({ action, deployedMetadata })) { state = "outdated" } From 6b60f03a0866633a232fc5ff45ba3347c56847e6 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Thu, 7 Sep 2023 10:54:51 +0200 Subject: [PATCH 21/30] refactor: move input args check into `getForwardablePorts` --- core/src/plugins/kubernetes/kubernetes-type/handlers.ts | 2 +- core/src/plugins/kubernetes/port-forward.ts | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index da862f22e6..10bbb6b729 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -300,7 +300,7 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne } // Note: Local mode has its own port-forwarding configuration - if (deployedMode !== "local" && remoteResources && remoteResources.length > 0) { + if (deployedMode !== "local") { try { forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode }) } catch (error) { diff --git a/core/src/plugins/kubernetes/port-forward.ts b/core/src/plugins/kubernetes/port-forward.ts index 1821fab2fe..ed7bc7a7ab 100644 --- a/core/src/plugins/kubernetes/port-forward.ts +++ b/core/src/plugins/kubernetes/port-forward.ts @@ -228,6 +228,10 @@ export function getForwardablePorts({ parentAction: Resolved | undefined mode: ActionMode }): ForwardablePort[] { + if (resources.length === 0) { + return [] + } + const spec = parentAction?.getSpec() // Note: Local mode has its own port-forwarding configuration From 3ef9725b18412bfdec05c42c46b5830824a53e82 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Thu, 7 Sep 2023 10:57:58 +0200 Subject: [PATCH 22/30] chore: remove unnecessary code and comments Local mode checks were moved to `getForwardablePorts` in #5022. --- core/src/plugins/kubernetes/helm/deployment.ts | 1 - .../plugins/kubernetes/kubernetes-type/handlers.ts | 11 ++++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/core/src/plugins/kubernetes/helm/deployment.ts b/core/src/plugins/kubernetes/helm/deployment.ts index 990821f7ee..413c5756f7 100644 --- a/core/src/plugins/kubernetes/helm/deployment.ts +++ b/core/src/plugins/kubernetes/helm/deployment.ts @@ -148,7 +148,6 @@ export const helmDeploy: DeployActionHandler<"deploy", HelmDeployAction> = async timeoutSec: timeout, }) - // Local mode has its own port-forwarding configuration const forwardablePorts = getForwardablePorts({ resources: manifests, parentAction: action, mode }) // Make sure port forwards work after redeployment diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index 10bbb6b729..207ff6e57c 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -299,13 +299,10 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne } } - // Note: Local mode has its own port-forwarding configuration - if (deployedMode !== "local") { - try { - forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode }) - } catch (error) { - log.debug({ msg: `Unable to extract forwardable ports: ${error.message}`, error }) - } + try { + forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode }) + } catch (error) { + log.debug({ msg: `Unable to extract forwardable ports: ${error.message}`, error }) } return composeKubernetesDeployStatus({ From f702c7f13ee5babb16a441951d501b56b65df3eb Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Thu, 7 Sep 2023 11:02:16 +0200 Subject: [PATCH 23/30] chore: remove unnecessary try/catch Function `getForwardablePorts` does not call any potentially unsafe operations. It's not wrapped into try/catch in the other code places. --- core/src/plugins/kubernetes/kubernetes-type/handlers.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index 207ff6e57c..5dd7f8824d 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -256,7 +256,6 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne const deployedMetadata = parseMetadataResource(log, remoteMetadataResource) const deployedMode = deployedMetadata.mode let remoteResources: KubernetesResource[] = [] - let forwardablePorts: ForwardablePort[] = [] let state: DeployState = "ready" if (isOutdated({ action, deployedMetadata })) { @@ -299,11 +298,7 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne } } - try { - forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode }) - } catch (error) { - log.debug({ msg: `Unable to extract forwardable ports: ${error.message}`, error }) - } + const forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode }) return composeKubernetesDeployStatus({ action, From 8bab6c2c1e7c542075d0e05bdb487774604a6867 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Thu, 7 Sep 2023 11:09:33 +0200 Subject: [PATCH 24/30] chore: remove unused local var --- core/src/plugins/kubernetes/kubernetes-type/handlers.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index 5dd7f8824d..e90005441e 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -223,9 +223,6 @@ function isOutdated({ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", KubernetesDeployAction> = async (params) => { const { ctx, action, log } = params - - const mode = action.mode() - const k8sCtx = ctx const provider = k8sCtx.provider const namespaceStatus = await getActionNamespaceStatus({ From 297f5cdbccde10db6d2b6287ef732595cbb91565 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Thu, 7 Sep 2023 12:16:42 +0200 Subject: [PATCH 25/30] refactor: use SRP in status calculation Split individual resource status retrieval and overall state calculation. --- .../kubernetes/kubernetes-type/handlers.ts | 89 ++++++++++++------- 1 file changed, 57 insertions(+), 32 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index e90005441e..4c6d45b0b0 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -31,6 +31,7 @@ import { gardenNamespaceAnnotationValue, getManifests, getMetadataManifest, + ManifestMetadata, ParsedMetadataManifestData, parseMetadataResource, } from "./common" @@ -221,6 +222,44 @@ function isOutdated({ return false } +async function getResourceStatuses({ + deployedMetadata, + namespace, + api, + log, +}: { + deployedMetadata: ParsedMetadataManifestData + namespace: string + api: KubeApi + log: ActionLog +}): Promise { + const manifestMetadata = Object.values(deployedMetadata.manifestMetadata) + if (manifestMetadata.length === 0) { + return [] + } + + const maybeDeployedResources: [ManifestMetadata, any][] = await Promise.all( + manifestMetadata.map(async (m) => { + return [m, await api.readOrNull({ log, ...m })] + }) + ) + + return Promise.all( + maybeDeployedResources.map(async ([m, resource]) => { + if (!resource) { + const missingResource: KubernetesResource = { + apiVersion: m.apiVersion, + kind: m.kind, + metadata: { name: m.name, namespace: m.namespace }, + } + return { resource: missingResource, state: "missing" } as ResourceStatus + } else { + return await resolveResourceStatus({ api, namespace, resource, log }) + } + }) + ) +} + export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", KubernetesDeployAction> = async (params) => { const { ctx, action, log } = params const k8sCtx = ctx @@ -252,47 +291,33 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne const deployedMetadata = parseMetadataResource(log, remoteMetadataResource) const deployedMode = deployedMetadata.mode - let remoteResources: KubernetesResource[] = [] + const remoteResources: KubernetesResource[] = [] let state: DeployState = "ready" if (isOutdated({ action, deployedMetadata })) { state = "outdated" } - const manifestMetadata = Object.values(deployedMetadata.manifestMetadata) - - if (manifestMetadata.length > 0) { - try { - const maybeDeployedResources = await Promise.all( - manifestMetadata.map(async (m) => { - return [m, await api.readOrNull({ log, ...m })] - }) - ) - - const statuses: ResourceStatus[] = await Promise.all( - maybeDeployedResources.map(async ([m, resource]) => { - if (!resource) { - return { - state: "missing" as const, - resource: { - apiVersion: m.apiVersion, - kind: m.kind, - metadata: { name: m.name, namespace: m.namespace }, - }, - } - } - remoteResources.push(resource) - return resolveResourceStatus({ api, namespace: defaultNamespace, resource, log }) - }) - ) + try { + const resourceStatuses = await getResourceStatuses({ + deployedMetadata, + namespace: defaultNamespace, + api, + log, + }) - if (state !== "outdated") { - state = resolveResourceStatuses(log, statuses) + resourceStatuses.forEach((rs) => { + if (rs.state !== "missing") { + remoteResources.push(rs.resource) } - } catch (error) { - log.debug({ msg: `Failed querying for remote resources: ${error.message}`, error }) - state = "unknown" + }) + + if (state !== "outdated") { + state = resolveResourceStatuses(log, resourceStatuses) } + } catch (error) { + log.debug({ msg: `Failed querying for remote resources: ${error.message}`, error }) + state = "unknown" } const forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode }) From 3a1b913db3b2800ef6039cba505bc0508b98a085 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Thu, 7 Sep 2023 13:04:57 +0200 Subject: [PATCH 26/30] refactor: simplified code Minimized mutability of the local vars. More straightforward and linear processing flow. --- .../kubernetes/kubernetes-type/handlers.ts | 59 +++++++++++-------- 1 file changed, 33 insertions(+), 26 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts index 4c6d45b0b0..e27305e9ee 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -11,7 +11,7 @@ import type { ModuleActionHandlers } from "../../../plugin/plugin" import { DeployState, ForwardablePort, ServiceStatus } from "../../../types/service" import { gardenAnnotationKey } from "../../../util/string" import { KubeApi } from "../api" -import type {KubernetesPluginContext, KubernetesProvider} from "../config" +import type { KubernetesPluginContext, KubernetesProvider } from "../config" import { configureSyncMode, convertKubernetesModuleDevModeSpec } from "../sync" import { apply, deleteObjectsBySelector } from "../kubectl" import { streamK8sLogs } from "../logs" @@ -286,17 +286,12 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne state: "missing", remoteResources: [], forwardablePorts: [], + provider, }) } const deployedMetadata = parseMetadataResource(log, remoteMetadataResource) const deployedMode = deployedMetadata.mode - const remoteResources: KubernetesResource[] = [] - let state: DeployState = "ready" - - if (isOutdated({ action, deployedMetadata })) { - state = "outdated" - } try { const resourceStatuses = await getResourceStatuses({ @@ -306,30 +301,42 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne log, }) - resourceStatuses.forEach((rs) => { - if (rs.state !== "missing") { - remoteResources.push(rs.resource) - } + const remoteResources: KubernetesResource[] = resourceStatuses + .filter((rs) => rs.state !== "missing") + .map((rs) => rs.resource) + + const forwardablePorts = getForwardablePorts({ + resources: remoteResources, + parentAction: action, + mode: deployedMode, }) - if (state !== "outdated") { - state = resolveResourceStatuses(log, resourceStatuses) - } + const state: DeployState = isOutdated({ + action, + deployedMetadata, + }) + ? "outdated" + : resolveResourceStatuses(log, resourceStatuses) + + return composeKubernetesDeployStatus({ + action, + deployedMode, + state, + remoteResources, + forwardablePorts, + provider, + }) } catch (error) { log.debug({ msg: `Failed querying for remote resources: ${error.message}`, error }) - state = "unknown" + return composeKubernetesDeployStatus({ + action, + deployedMode, + state: "unknown", + remoteResources: [], + forwardablePorts: [], + provider, + }) } - - const forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode }) - - return composeKubernetesDeployStatus({ - action, - deployedMode, - state, - remoteResources, - forwardablePorts, - provider, - }) } export const kubernetesDeploy: DeployActionHandler<"deploy", KubernetesDeployAction> = async (params) => { From 08b7a376b5f0b3b1b74cfd8e7463e90488786790 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Thu, 7 Sep 2023 14:51:57 +0200 Subject: [PATCH 27/30] test: fix test setup to cover the original bug --- core/src/plugins/kubernetes/kubernetes-type/common.ts | 2 +- .../integ/src/plugins/kubernetes/kubernetes-type/handlers.ts | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/common.ts b/core/src/plugins/kubernetes/kubernetes-type/common.ts index d8e2bc5ec2..d7e948fe8d 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/common.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/common.ts @@ -237,7 +237,7 @@ export function parseMetadataResource(log: Log, resource: KubernetesResource, log: Log 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 c47ba52bdf..3574d65e41 100644 --- a/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts +++ b/core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts @@ -19,6 +19,7 @@ import { getManifests, getMetadataManifest, parseMetadataResource, + readManifests, } from "../../../../../../src/plugins/kubernetes/kubernetes-type/common" import { KubeApi } from "../../../../../../src/plugins/kubernetes/api" import { ActionLog, createActionLog, Log } from "../../../../../../src/logger/log-entry" @@ -249,7 +250,9 @@ describe("kubernetes-type handlers", () => { skipCreate: true, }) - const metadataManifest = getMetadataManifest(resolvedAction, namespace, []) + const declaredManifests = await readManifests(ctx, resolvedAction, log) + // It's critical to pass in the existing manifests here, otherwise the code will not check their statuses + const metadataManifest = getMetadataManifest(resolvedAction, namespace, declaredManifests) metadataManifest.data!.resolvedVersion = "v-foo" await api.replace({ log, namespace, resource: metadataManifest }) From 30f14356f16dd9000a16badba651b6ddb0293e2d Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Tue, 19 Sep 2023 12:01:16 +0200 Subject: [PATCH 28/30] chore: post-merge corrections --- core/src/plugins/kubernetes/kubernetes-type/common.ts | 2 +- core/src/plugins/kubernetes/kubernetes-type/handlers.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/plugins/kubernetes/kubernetes-type/common.ts b/core/src/plugins/kubernetes/kubernetes-type/common.ts index d7e948fe8d..a668925665 100644 --- a/core/src/plugins/kubernetes/kubernetes-type/common.ts +++ b/core/src/plugins/kubernetes/kubernetes-type/common.ts @@ -227,7 +227,7 @@ export function parseMetadataResource(log: Log, resource: KubernetesResource Date: Wed, 20 Sep 2023 14:07:42 +0200 Subject: [PATCH 29/30] refactor: remove unnecessary function call The value has already been calculated as an immutable value. --- core/src/plugins/kubernetes/status/status.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/plugins/kubernetes/status/status.ts b/core/src/plugins/kubernetes/status/status.ts index d90602b8e0..c85c515f58 100644 --- a/core/src/plugins/kubernetes/status/status.ts +++ b/core/src/plugins/kubernetes/status/status.ts @@ -480,7 +480,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[gardenAnnotationKey("manifest-hash")] + const lastAppliedHashed = deployedResource.metadata.annotations[hashKey] // The new manifest matches the last applied manifest if (lastAppliedHashed && (await hashManifest(manifest)) === lastAppliedHashed) { From 07c5427b117d88586838edd0329908c9981cdb33 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Wed, 20 Sep 2023 16:25:02 +0200 Subject: [PATCH 30/30] chore: typos and wording Got rid of old-fashioned term "service". Replaced it with "deploy". --- core/src/plugins/kubernetes/helm/deployment.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/plugins/kubernetes/helm/deployment.ts b/core/src/plugins/kubernetes/helm/deployment.ts index 413c5756f7..4f40f69c21 100644 --- a/core/src/plugins/kubernetes/helm/deployment.ts +++ b/core/src/plugins/kubernetes/helm/deployment.ts @@ -76,7 +76,7 @@ export const helmDeploy: DeployActionHandler<"deploy", HelmDeployAction> = async await helm({ ctx: k8sCtx, namespace, log, args: [...upgradeArgs], emitLogEvents: true }) // If ctx.cloudApi is defined, the user is logged in and they might be trying to deploy to an environment - // that could have been paused by by Garden Cloud's AEC functionality. We therefore make sure to clean up any + // that could have been paused by Garden Cloud's AEC functionality. We therefore make sure to clean up any // dangling annotations created by Garden Cloud. if (ctx.cloudApi) { try { @@ -92,7 +92,7 @@ export const helmDeploy: DeployActionHandler<"deploy", HelmDeployAction> = async }) ) } catch (error) { - const errorMsg = `Failed to remove Garden Cloud AEC annotations for service: ${action.name}.` + const errorMsg = `Failed to remove Garden Cloud AEC annotations for deploy: ${action.name}.` log.warn(errorMsg) log.debug({ error: toGardenError(error) }) }