Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(k8s): correctly resolve manifests when
build is set (#4846)
* refactor: extract named function for manifest post-processing * refactor: split manifest reading and post-processing * fix(k8s): correctly resolve manifests when `build` is set * fix(k8s): quickfix to avoid resetting `outdated` state to `ready` * test: fix tests * refactor(test): move helper to the upper-level scope It will be used by other test contexts. * refactor(test): introduce one more convenience helper * refactor(test): introduce and rename some local vars * refactor(test): use helpers to avoid code duplication * chore: re-arranged code Keep all helpers close to each other for simpler navigation. * chore(test): rename some tests and variables To avoid usage of the old glossary. * 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 * refactor(test): convert lambdas to functions * test: ensure all resources are deleted by `deleteKubernetesDeploy` A metadata `ConfigMap` describing what was last deployed must be deleted too. * 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. * refactor(k8s): helper to compose k8s deploy status * refactor(k8s): return deploy status immediately on "missing" state * refactor: unwrap unnecessary else-block * chore: variable scoping * refactor: extract helper to check if deploy is outdated * refactor: move input args check into `getForwardablePorts` * chore: remove unnecessary code and comments Local mode checks were moved to `getForwardablePorts` in #5022. * 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. * chore: remove unused local var * refactor: use SRP in status calculation Split individual resource status retrieval and overall state calculation. * refactor: simplified code Minimized mutability of the local vars. More straightforward and linear processing flow. * test: fix test setup to cover the original bug * chore: post-merge corrections * refactor: remove unnecessary function call The value has already been calculated as an immutable value. * chore: typos and wording Got rid of old-fashioned term "service". Replaced it with "deploy". --------- Co-authored-by: Jon Edvald <[email protected]>
- Loading branch information
1 parent
230ed41
commit 6c737a9
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,13 +8,13 @@ | |
|
|
||
| import { join, resolve } from "path" | ||
| import { pathExists, readFile } from "fs-extra" | ||
| 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, 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, 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,54 +56,65 @@ export async function getManifests({ | |
| log, | ||
| action, | ||
| defaultNamespace, | ||
| }: { | ||
| ctx: PluginContext | ||
| api: KubeApi | ||
| log: Log | ||
| action: Resolved<KubernetesDeployAction | KubernetesPodRunAction | KubernetesPodTestAction> | ||
| defaultNamespace: string | ||
| }): Promise<KubernetesResource[]> { | ||
| // Local function to set some default values and Garden-specific annotations. | ||
| async function postProcessManifest({ manifest, declaration }: DeclaredManifest): Promise<DeclaredManifest> { | ||
| // 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 | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 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 = 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) | ||
|
|
||
| return postProcessedManifests.map((m) => m.manifest) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -156,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<KubernetesDeployAction>, | ||
| defaultNamespace: string, | ||
| declaredManifests: DeclaredManifest[] | ||
| ): KubernetesResource<V1ConfigMap> { | ||
| 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<V1ConfigMap>): 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}` }) | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong. 
vvagaytsev
Author
Collaborator
|
||
| } | ||
| } | ||
|
|
||
| return output | ||
| } | ||
|
|
||
| /** | ||
| * Read the manifests from the module config, as well as any referenced files in the config. | ||
| */ | ||
| export async function readManifests( | ||
| ctx: PluginContext, | ||
| action: Resolved<KubernetesDeployAction | KubernetesPodRunAction | KubernetesPodTestAction>, | ||
| log: Log | ||
| ): Promise<DeclaredManifest[]> { | ||
| const manifestPath = action.getBuildPath() | ||
|
|
||
| const inlineManifests = readInlineManifests(action) | ||
| const fileManifests = await readFileManifests(ctx, action, log, manifestPath) | ||
|
|
||
Oops, something went wrong.
@vvagaytsev if we can't read the metadata, shouldn't we assume that everything is outdated, instead of assuming everything is up-to-date?
If we only log the error, we assume everything is up-to-date instead of verifying every single resource in
getKubernetesDeployStatus. I think it would be better to throw an error, and catch it where we call the function and do the appropriate thing (e.g. assume that the action is outdated)