From e30ab0ba5c39e22237d7d73c0e0d442588ef7e62 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Fri, 17 May 2024 17:56:33 +0200 Subject: [PATCH] fix: `garden publish` command to respect `publishId` (#6052) * refactor: rename local vars for more clarity * chore: log image tags while publishing * chore: (tidying) re-arrange the code To see more similarities between 2 handlers * fix: read image tag from existing parsed image The parsed image comes from a helper function. That helpers function uses `splitLast` instead of `splitFirst` with `:`-delimiter to get the tag name. That looks correct. * chore: rename some local vars for more clarity * chore: optimized imports * refactor: inline local var * refactor: extract helper function to reduce code duplication The only diff between `getContainerBuildActionOutputs` and `k8sGetContainerBuildActionOutputs` was in the deploymentImageId calculation that depended on the presence of deployment registry config. * chore: remove outdated comment * refactor: rename local vars for more clarity * refactor: rename functions args for more clarity * chore: add clarifying comment * refactor: unwrap else-conditions * chore: always tag container images Align the behaviour with the `k8sPublishContainerBuild` which does not compare local and remote images. The repeated tagging is idemponent and should not be too costly. * refactor: inline function It was used only in one place. * chore: amend var names, logs and comments for better clarity * chore: log Docker tags while local build * refactor: declare semantically different image ids separately Each in its own variable. * fix: use correct Docker image ID in `publish` command * fix: use local image name for deployment registries Publish ID is used only by `publish` command, and it should not be used for internal deployment registries and while Build-actions resolution. * improvement: add image details to outputs of `publish` command * chore: delete outdated comment --- core/src/plugins/container/build.ts | 33 +------ core/src/plugins/container/helpers.ts | 89 +++++++++++++------ core/src/plugins/container/publish.ts | 34 +++---- .../plugins/kubernetes/commands/pull-image.ts | 8 +- .../kubernetes/container/build/local.ts | 8 +- .../plugins/kubernetes/container/handlers.ts | 28 +----- .../plugins/kubernetes/container/publish.ts | 36 ++++---- 7 files changed, 108 insertions(+), 128 deletions(-) diff --git a/core/src/plugins/container/build.ts b/core/src/plugins/container/build.ts index 74d41b2d56..720a02079c 100644 --- a/core/src/plugins/container/build.ts +++ b/core/src/plugins/container/build.ts @@ -16,7 +16,6 @@ import { defaultDockerfileName } from "./config.js" import { joinWithPosix } from "../../util/fs.js" import type { Resolved } from "../../actions/types.js" import dedent from "dedent" -import { splitFirst } from "../../util/string.js" import { CONTAINER_BUILD_CONCURRENCY_LIMIT_CLOUD_BUILDER, CONTAINER_BUILD_CONCURRENCY_LIMIT_LOCAL, @@ -210,37 +209,7 @@ async function buildContainerInCloudBuilder(params: { } export function getContainerBuildActionOutputs(action: Resolved): ContainerBuildOutputs { - const buildName = action.name - const localId = action.getSpec("localId") - const explicitImage = action.getSpec("publishId") - let imageId = localId - if (explicitImage) { - // override imageId if publishId is set - const imageTag = splitFirst(explicitImage, ":")[1] - const parsedImage = containerHelpers.parseImageId(explicitImage) - const tag = imageTag || action.versionString() - imageId = containerHelpers.unparseImageId({ ...parsedImage, tag }) - } - const version = action.moduleVersion() - - const localImageName = containerHelpers.getLocalImageName(buildName, localId) - const localImageId = containerHelpers.getLocalImageId(buildName, localId, version) - - // Note: The deployment image name/ID outputs are overridden by the kubernetes provider, these defaults are - // generally not used. - const deploymentImageName = containerHelpers.getDeploymentImageName(buildName, imageId, undefined) - const deploymentImageId = containerHelpers.getBuildDeploymentImageId(buildName, imageId, version, undefined) - - return { - localImageName, - localImageId, - deploymentImageName, - deploymentImageId, - "local-image-name": localImageName, - "local-image-id": localImageId, - "deployment-image-name": deploymentImageName, - "deployment-image-id": deploymentImageId, - } + return containerHelpers.getBuildActionOutputs(action, undefined) } export function getDockerBuildFlags( diff --git a/core/src/plugins/container/helpers.ts b/core/src/plugins/container/helpers.ts index da8552e879..86776fe29d 100644 --- a/core/src/plugins/container/helpers.ts +++ b/core/src/plugins/container/helpers.ts @@ -8,8 +8,6 @@ import { join, posix } from "path" import fsExtra from "fs-extra" - -const { readFile, pathExists, lstat } = fsExtra import semver from "semver" import type { CommandEntry } from "docker-file-parser" import { parse } from "docker-file-parser" @@ -17,15 +15,15 @@ import isGlob from "is-glob" import { ConfigurationError, GardenError, RuntimeError } from "../../exceptions.js" import type { SpawnOutput } from "../../util/util.js" import { spawn } from "../../util/util.js" -import type { ContainerRegistryConfig, ContainerModuleConfig } from "./moduleConfig.js" -import { defaultTag as _defaultTag, defaultImageNamespace } from "./moduleConfig.js" +import type { ContainerBuildOutputs, ContainerModuleConfig, ContainerRegistryConfig } from "./moduleConfig.js" +import { defaultImageNamespace, defaultTag as _defaultTag } from "./moduleConfig.js" import type { Writable } from "stream" -import { flatten, uniq, fromPairs, reduce } from "lodash-es" +import { flatten, fromPairs, reduce, uniq } from "lodash-es" import type { ActionLog, Log } from "../../logger/log-entry.js" import isUrl from "is-url" import titleize from "titleize" -import { deline, stripQuotes, splitLast, splitFirst } from "../../util/string.js" +import { deline, splitFirst, splitLast, stripQuotes } from "../../util/string.js" import type { PluginContext } from "../../plugin-context.js" import type { ModuleVersion } from "../../vcs/vcs.js" import type { ContainerBuildAction } from "./config.js" @@ -36,6 +34,8 @@ import pMemoize from "../../lib/p-memoize.js" import { styles } from "../../logger/styles.js" import type { ContainerProviderConfig } from "./container.js" +const { readFile, pathExists, lstat } = fsExtra + interface DockerVersion { client?: string server?: string @@ -60,9 +60,9 @@ const helpers = { * Returns the image ID used locally, when building and deploying to local environments * (when we don't need to push to remote registries). */ - getLocalImageId(buildName: string, explicitImage: string | undefined, version: ModuleVersion): string { + getLocalImageId(buildName: string, explicitImageId: string | undefined, version: ModuleVersion): string { const { versionString } = version - const name = helpers.getLocalImageName(buildName, explicitImage) + const name = helpers.getLocalImageName(buildName, explicitImageId) const parsedImage = helpers.parseImageId(name) return helpers.unparseImageId({ ...parsedImage, tag: versionString }) }, @@ -71,9 +71,9 @@ const helpers = { * Returns the image name used locally (without tag/version), when building and deploying to local environments * (when we don't need to push to remote registries). */ - getLocalImageName(buildName: string, explicitImage: string | undefined): string { - if (explicitImage) { - const parsedImage = helpers.parseImageId(explicitImage) + getLocalImageName(buildName: string, explicitImageId: string | undefined): string { + if (explicitImageId) { + const parsedImage = helpers.parseImageId(explicitImageId) return helpers.unparseImageId({ ...parsedImage, tag: undefined }) } else { return buildName @@ -122,18 +122,17 @@ const helpers = { */ getDeploymentImageName( buildName: string, - explicitImage: string | undefined, + explicitImageId: string | undefined, registryConfig: ContainerRegistryConfig | undefined ) { - const localName = explicitImage || buildName - const parsedId = helpers.parseImageId(localName) - const withoutVersion = helpers.unparseImageId({ - ...parsedId, - tag: undefined, - }) + const localImageName = explicitImageId || buildName + const parsedImageId = helpers.parseImageId(localImageName) if (!registryConfig) { - return withoutVersion + return helpers.unparseImageId({ + ...parsedImageId, + tag: undefined, + }) } const host = registryConfig.port ? `${registryConfig.hostname}:${registryConfig.port}` : registryConfig.hostname @@ -141,14 +140,15 @@ const helpers = { return helpers.unparseImageId({ host, namespace: registryConfig.namespace, - repository: parsedId.repository, + repository: parsedImageId.repository, tag: undefined, }) }, /** - * Returns the image ID to be used when pushing to deployment registries. This always has the version - * set as the tag. + * Returns the image ID to be used when pushing to deployment registries. + * This always has the version set as the tag. + * Do not confuse this with the publishing image ID used by the `garden publish` command. */ getBuildDeploymentImageId( buildName: string, @@ -171,15 +171,48 @@ const helpers = { // Requiring this parameter to avoid accidentally missing it registryConfig: ContainerRegistryConfig | undefined ): string { + // The `dockerfile` configuration always takes precedence over the `image`. if (helpers.moduleHasDockerfile(moduleConfig, version)) { return helpers.getBuildDeploymentImageId(moduleConfig.name, moduleConfig.spec.image, version, registryConfig) - } else if (moduleConfig.spec.image) { - // Otherwise, return the configured image ID. + } + + // Return the configured image ID if no Dockerfile is defined in the config. + if (moduleConfig.spec.image) { return moduleConfig.spec.image - } else { - throw new ConfigurationError({ - message: `Module ${moduleConfig.name} neither specifies image nor can a Dockerfile be found in the module directory.`, - }) + } + + throw new ConfigurationError({ + message: `Module ${moduleConfig.name} neither specifies image nor can a Dockerfile be found in the module directory.`, + }) + }, + + /** + * Serves build action outputs in container and kubernetes plugins. + */ + getBuildActionOutputs( + action: Resolved, + // Requiring this parameter to avoid accidentally missing it + registryConfig: ContainerRegistryConfig | undefined + ): ContainerBuildOutputs { + const localId = action.getSpec("localId") + const version = action.moduleVersion() + const buildName = action.name + + const localImageName = containerHelpers.getLocalImageName(buildName, localId) + const localImageId = containerHelpers.getLocalImageId(buildName, localId, version) + + const deploymentImageName = containerHelpers.getDeploymentImageName(buildName, localId, registryConfig) + const deploymentImageId = containerHelpers.getBuildDeploymentImageId(buildName, localId, version, registryConfig) + + return { + localImageName, + localImageId, + deploymentImageName, + deploymentImageId, + "local-image-name": localImageName, + "local-image-id": localImageId, + "deployment-image-name": deploymentImageName, + "deployment-image-id": deploymentImageId, } }, diff --git a/core/src/plugins/container/publish.ts b/core/src/plugins/container/publish.ts index ce931166a2..a1cafc7ab6 100644 --- a/core/src/plugins/container/publish.ts +++ b/core/src/plugins/container/publish.ts @@ -9,6 +9,7 @@ import type { ContainerBuildAction } from "./moduleConfig.js" import { containerHelpers } from "./helpers.js" import type { BuildActionHandler } from "../../plugin/action-types.js" +import { naturalList } from "../../util/string.js" export const publishContainerBuild: BuildActionHandler<"publish", ContainerBuildAction> = async ({ ctx, @@ -16,27 +17,28 @@ export const publishContainerBuild: BuildActionHandler<"publish", ContainerBuild log, tagOverride, }) => { - const localId = action.getOutput("localImageId") - const remoteId = containerHelpers.getPublicImageId(action, tagOverride) + const localImageId = action.getOutput("localImageId") + const remoteImageId = containerHelpers.getPublicImageId(action, tagOverride) - if (localId !== remoteId) { - log.info({ msg: `Tagging image with ${localId} and ${remoteId}` }) - await containerHelpers.dockerCli({ - cwd: action.getBuildPath(), - args: ["tag", localId, remoteId], - log, - ctx, - }) - } + const taggedImages = [localImageId, remoteImageId] + log.info({ msg: `Tagging images ${naturalList(taggedImages)}` }) + await containerHelpers.dockerCli({ + cwd: action.getBuildPath(), + args: ["tag", ...taggedImages], + log, + ctx, + }) - log.info({ msg: `Publishing image ${remoteId}...` }) + log.info({ msg: `Publishing image ${remoteImageId}...` }) // TODO: stream output to log if at debug log level - await containerHelpers.dockerCli({ cwd: action.getBuildPath(), args: ["push", remoteId], log, ctx }) + await containerHelpers.dockerCli({ cwd: action.getBuildPath(), args: ["push", remoteImageId], log, ctx }) return { state: "ready", - detail: { published: true, message: `Published ${remoteId}` }, - // TODO-0.13.1 - outputs: {}, + detail: { published: true, message: `Published ${remoteImageId}` }, + outputs: { + localImageId, + remoteImageId, + }, } } diff --git a/core/src/plugins/kubernetes/commands/pull-image.ts b/core/src/plugins/kubernetes/commands/pull-image.ts index b201ce9e63..bde88eb6d6 100644 --- a/core/src/plugins/kubernetes/commands/pull-image.ts +++ b/core/src/plugins/kubernetes/commands/pull-image.ts @@ -9,12 +9,11 @@ import fs from "fs" import tmp from "tmp-promise" import type { KubernetesPluginContext } from "../config.js" -import { PluginError, ParameterError, GardenError } from "../../../exceptions.js" +import { GardenError, ParameterError, PluginError, RuntimeError } from "../../../exceptions.js" import type { PluginCommand } from "../../../plugin/command.js" import { KubeApi } from "../api.js" import type { Log } from "../../../logger/log-entry.js" import { containerHelpers } from "../../container/helpers.js" -import { RuntimeError } from "../../../exceptions.js" import { PodRunner } from "../run.js" import { dockerAuthSecretKey, getK8sUtilImageName, systemDockerAuthSecretName } from "../constants.js" import { getAppNamespace, getSystemNamespace } from "../namespace.js" @@ -91,10 +90,7 @@ interface PullParams { } export async function pullBuild(params: PullParams) { - await pullFromExternalRegistry(params) -} - -async function pullFromExternalRegistry({ ctx, log, localId, remoteId }: PullParams) { + const { ctx, log, localId, remoteId }: PullParams = params const api = await KubeApi.factory(log, ctx, ctx.provider) const buildMode = ctx.provider.config.buildMode diff --git a/core/src/plugins/kubernetes/container/build/local.ts b/core/src/plugins/kubernetes/container/build/local.ts index f1c35c6cfa..d12896970c 100644 --- a/core/src/plugins/kubernetes/container/build/local.ts +++ b/core/src/plugins/kubernetes/container/build/local.ts @@ -18,6 +18,7 @@ import type { ContainerBuildAction } from "../../../container/moduleConfig.js" import type { BuildActionParams } from "../../../../plugin/action-types.js" import { k8sGetContainerBuildActionOutputs } from "../handlers.js" import { cloudBuilder } from "../../../container/cloudbuilder.js" +import { naturalList } from "../../../../util/string.js" export const getLocalBuildStatus: BuildStatusHandler = async (params) => { const { ctx, action, log } = params @@ -92,10 +93,13 @@ export const localBuild: BuildHandler = async (params) => { // Cloud Builder already pushes the image. if (!builtByCloudBuilder) { - log.info({ msg: `→ Pushing image ${remoteId} to remote...` }) - const buildPath = action.getBuildPath() + const taggedImages = [localId, remoteId] + + log.info({ msg: `→ Tagging images ${naturalList(taggedImages)}` }) await containerHelpers.dockerCli({ cwd: buildPath, args: ["tag", localId, remoteId], log, ctx }) + + log.info({ msg: `→ Pushing image ${remoteId} to remote...` }) await containerHelpers.dockerCli({ cwd: buildPath, args: ["push", remoteId], log, ctx }) } diff --git a/core/src/plugins/kubernetes/container/handlers.ts b/core/src/plugins/kubernetes/container/handlers.ts index f09da6aa43..2126f16cd8 100644 --- a/core/src/plugins/kubernetes/container/handlers.ts +++ b/core/src/plugins/kubernetes/container/handlers.ts @@ -19,9 +19,7 @@ import type { import type { GetModuleOutputsParams } from "../../../plugin/handlers/Module/get-outputs.js" import { containerHelpers } from "../../container/helpers.js" import { getContainerModuleOutputs } from "../../container/container.js" -import { getContainerBuildActionOutputs } from "../../container/build.js" import type { Resolved } from "../../../actions/types.js" -import { splitFirst } from "../../../util/string.js" async function configure(params: ConfigureModuleParams) { const { moduleConfig } = await params.base!(params) @@ -61,31 +59,7 @@ export function k8sGetContainerBuildActionOutputs({ provider: KubernetesProvider action: Resolved }): ContainerBuildOutputs { - const localId = action.getSpec("localId") - const outputs = getContainerBuildActionOutputs(action) - const explicitImage = action.getSpec("publishId") - let imageId = localId - if (explicitImage) { - // override imageId if publishId is set - const imageTag = splitFirst(explicitImage, ":")[1] - const parsedImage = containerHelpers.parseImageId(explicitImage) - const tag = imageTag || action.versionString() - imageId = containerHelpers.unparseImageId({ ...parsedImage, tag }) - } - - outputs.deploymentImageName = outputs["deployment-image-name"] = containerHelpers.getDeploymentImageName( - action.name, - imageId, - provider.config.deploymentRegistry - ) - outputs.deploymentImageId = outputs["deployment-image-id"] = containerHelpers.getBuildDeploymentImageId( - action.name, - imageId, - action.moduleVersion(), - provider.config.deploymentRegistry - ) - - return outputs + return containerHelpers.getBuildActionOutputs(action, provider.config.deploymentRegistry) } function validateConfig(params: ConfigureModuleParams) { diff --git a/core/src/plugins/kubernetes/container/publish.ts b/core/src/plugins/kubernetes/container/publish.ts index 5a0d861e15..31ff97810c 100644 --- a/core/src/plugins/kubernetes/container/publish.ts +++ b/core/src/plugins/kubernetes/container/publish.ts @@ -11,43 +11,45 @@ import type { KubernetesPluginContext } from "../config.js" import { pullBuild } from "../commands/pull-image.js" import type { BuildActionHandler } from "../../../plugin/action-types.js" import { containerHelpers } from "../../container/helpers.js" +import { naturalList } from "../../../util/string.js" export const k8sPublishContainerBuild: BuildActionHandler<"publish", ContainerBuildAction> = async (params) => { const { ctx, action, log, tagOverride } = params const k8sCtx = ctx as KubernetesPluginContext const provider = k8sCtx.provider - const localId = action.getOutput("localImageId") - // NOTE: this may contain a custom deploymentRegistry, from the kubernetes provider config - // We cannot combine this publish method with the container plugin's publish method, because it won't have the context - const remoteId = action.getOutput("deploymentImageId") + const localImageId = action.getOutput("localImageId") if (provider.config.buildMode !== "local-docker") { - // First pull from the remote registry, then resume standard publish flow. + // NOTE: this may contain a custom deploymentRegistry, from the kubernetes provider config + const deploymentRegistryImageId = action.getOutput("deploymentImageId") + + // First pull from the deployment registry, then resume standard publish flow. // This does mean we require a local docker as a go-between, but the upside is that we can rely on the user's // standard authentication setup, instead of having to re-implement or account for all the different ways the // user might be authenticating with their registries. // We also generally prefer this because the remote cluster very likely doesn't (and shouldn't) have // privileges to push to production registries. - log.info(`Pulling from remote registry...`) - await pullBuild({ ctx: k8sCtx, action, log, localId, remoteId }) + log.info(`Pulling from deployment registry...`) + await pullBuild({ ctx: k8sCtx, action, log, localId: localImageId, remoteId: deploymentRegistryImageId }) } - // optionally use the tag instead of the garden version, this requires that we tag the image locally - // before publishing to the remote registry - - const remotePublishId = tagOverride ? `${action.getOutput("deploymentImageName")}:${tagOverride}` : remoteId + const remoteImageId = containerHelpers.getPublicImageId(action, tagOverride) - await containerHelpers.dockerCli({ cwd: action.getBuildPath(), args: ["tag", localId, remotePublishId], log, ctx }) + const taggedImages = [localImageId, remoteImageId] + log.info({ msg: `Tagging images ${naturalList(taggedImages)}` }) + await containerHelpers.dockerCli({ cwd: action.getBuildPath(), args: ["tag", ...taggedImages], log, ctx }) - log.info({ msg: `Publishing image ${remotePublishId}...` }) + log.info({ msg: `Publishing image ${remoteImageId}...` }) // TODO: stream output to log if at debug log level - await containerHelpers.dockerCli({ cwd: action.getBuildPath(), args: ["push", remotePublishId], log, ctx }) + await containerHelpers.dockerCli({ cwd: action.getBuildPath(), args: ["push", remoteImageId], log, ctx }) return { state: "ready", - detail: { published: true, message: `Published ${remotePublishId}` }, - // TODO-0.13.1 - outputs: {}, + detail: { published: true, message: `Published ${remoteImageId}` }, + outputs: { + localImageId, + remoteImageId, + }, } }