From 16b875659b79f603616fb2881b208047b41efee8 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Fri, 25 Nov 2022 18:53:55 +0100 Subject: [PATCH] feat(k8s): add service account and irsa support for in-cluster-builder This commit introduces a new service account, and makes the annotations configurable in kaniko and buildkit in-cluster-builders. This change enables the use of IRSA for in-cluter-building which makes it more secure. Fixes #2931 --- core/src/plugins/kubernetes/api.ts | 10 ++ core/src/plugins/kubernetes/config.ts | 28 +++- .../kubernetes/container/build/buildkit.ts | 20 ++- .../kubernetes/container/build/common.ts | 127 +++++++++++++++++- core/src/plugins/kubernetes/jib-container.ts | 2 - core/src/plugins/kubernetes/types.ts | 2 + .../kubernetes/container/build/common.ts | 22 +++ 7 files changed, 205 insertions(+), 6 deletions(-) diff --git a/core/src/plugins/kubernetes/api.ts b/core/src/plugins/kubernetes/api.ts index 4396ee5239..4d605df9b4 100644 --- a/core/src/plugins/kubernetes/api.ts +++ b/core/src/plugins/kubernetes/api.ts @@ -31,6 +31,7 @@ import { V1Deployment, V1Secret, V1Service, + V1ServiceAccount, ServerConfiguration, createConfiguration, } from "@kubernetes/client-node" @@ -129,6 +130,15 @@ const crudMap = { delete: "deleteNamespacedService", patch: "patchNamespacedService", }, + ServiceAccount: { + cls: new V1ServiceAccount(), + group: "core", + read: "readNamespacedServiceAccount", + create: "createNamespacedServiceAccount", + replace: "replaceNamespacedServiceAccount", + delete: "deleteNamespacedServiceAccount", + patch: "patchNamespacedServiceAccount", + }, } type CrudMap = typeof crudMap diff --git a/core/src/plugins/kubernetes/config.ts b/core/src/plugins/kubernetes/config.ts index a880891c76..fc86e1336b 100644 --- a/core/src/plugins/kubernetes/config.ts +++ b/core/src/plugins/kubernetes/config.ts @@ -132,6 +132,7 @@ export interface KubernetesConfig extends BaseProviderConfig { nodeSelector?: StringMap tolerations?: V1Toleration[] annotations?: StringMap + serviceAccountAnnotations?: StringMap } jib?: { pushViaCluster?: boolean @@ -143,6 +144,7 @@ export interface KubernetesConfig extends BaseProviderConfig { nodeSelector?: StringMap tolerations?: V1Toleration[] annotations?: StringMap + serviceAccountAnnotations?: StringMap util?: { tolerations?: V1Toleration[] annotations?: StringMap @@ -517,9 +519,29 @@ export const kubernetesConfigBase = () => annotations: annotationsSchema().description( "Specify annotations to apply to both the Pod and Deployment resources associated with cluster-buildkit. Annotations may have an effect on the behaviour of certain components, for example autoscalers." ), + serviceAccountAnnotations: annotationsSchema().description( + "Specify annotations to apply to the Kubernetes service account used by cluster-buildkit. This can be useful to set up IRSA with in-cluster building." + ), }) .default(() => ({})) .description("Configuration options for the `cluster-buildkit` build mode."), + clusterDocker: joi + .object() + .keys({ + enableBuildKit: joi + .boolean() + .default(false) + .description( + deline` + Enable [BuildKit](https://github.com/moby/buildkit) support. This should in most cases work well and be + more performant, but we're opting to keep it optional until it's enabled by default in Docker. + ` + ) + .meta({ deprecated: true }), + }) + .default(() => ({})) + .description("Configuration options for the `cluster-docker` build mode.") + .meta({ deprecated: "The cluster-docker build mode has been deprecated." }), jib: joi .object() .keys({ @@ -529,8 +551,12 @@ export const kubernetesConfigBase = () => .description( "In some cases you may need to push images built with Jib to the remote registry via Kubernetes cluster, e.g. if you don't have connectivity or access from where Garden is being run. In that case, set this flag to true, but do note that the build will take considerably take longer to complete! Only applies when using in-cluster building." ), + annotations: annotationsSchema().description( + "Specify annotations to apply to both the Pod and Deployment resources associated with cluster-buildkit. Annotations may have an effect on the behaviour of certain components, for example autoscalers." + ), }) - .description("Setting related to Jib image builds."), + .default(() => ({})) + .description("Configuration options for the `cluster-buildkit` build mode."), kaniko: joi .object() .keys({ diff --git a/core/src/plugins/kubernetes/container/build/buildkit.ts b/core/src/plugins/kubernetes/container/build/buildkit.ts index 4b68d2fc7d..99d8f65e54 100644 --- a/core/src/plugins/kubernetes/container/build/buildkit.ts +++ b/core/src/plugins/kubernetes/container/build/buildkit.ts @@ -30,6 +30,9 @@ import { getUtilContainer, ensureBuilderSecret, builderToleration, + inClusterBuilderServiceAccount, + ensureServiceAccount, + cycleDeployment, } from "./common.js" import { getNamespaceStatus } from "../../namespace.js" import { sleep } from "../../../../util/util.js" @@ -187,6 +190,14 @@ export async function ensureBuildkit({ api: KubeApi namespace: string }) { + const serviceAccountChanged = await ensureServiceAccount({ + ctx, + log, + api, + namespace, + annotations: provider.config.clusterBuildkit?.serviceAccountAnnotations, + }) + return deployLock.acquire(namespace, async () => { const deployLog = log.createLog() @@ -210,12 +221,18 @@ export async function ensureBuildkit({ log: deployLog, }) + // if the service account changed, all pods part of the deployment must be restarted + // so that they receive new credentials (e.g. for IRSA) + if (status.remoteResources.length && serviceAccountChanged) { + await cycleDeployment({ ctx, provider, deployment: manifest, api, namespace, deployLog }) + } + if (status.state === "ready") { // Need to wait a little to ensure the secret is updated in the deployment if (secretUpdated) { await sleep(1000) } - return { authSecret, updated: false } + return { authSecret, updated: serviceAccountChanged } } // Deploy the buildkit daemon @@ -397,6 +414,7 @@ export function getBuildkitDeployment( annotations: provider.config.clusterBuildkit?.annotations, }, spec: { + serviceAccountName: inClusterBuilderServiceAccount, containers: [ { name: buildkitContainerName, diff --git a/core/src/plugins/kubernetes/container/build/common.ts b/core/src/plugins/kubernetes/container/build/common.ts index ebd523a812..919e7189bf 100644 --- a/core/src/plugins/kubernetes/container/build/common.ts +++ b/core/src/plugins/kubernetes/container/build/common.ts @@ -22,7 +22,7 @@ import { randomString } from "../../../../util/string.js" import type { V1Container, V1Service } from "@kubernetes/client-node" import { cloneDeep, isEmpty } from "lodash-es" import { compareDeployedResources, waitForResources } from "../../status/status.js" -import type { KubernetesDeployment, KubernetesResource } from "../../types.js" +import type { KubernetesDeployment, KubernetesResource, KubernetesServiceAccount } from "../../types.js" import type { BuildActionHandler, BuildActionResults } from "../../../../plugin/action-types.js" import { k8sGetContainerBuildActionOutputs } from "../handlers.js" import type { Resolved } from "../../../../actions/types.js" @@ -31,7 +31,10 @@ import { getKubectlExecDestination } from "../../sync.js" import { getRunningDeploymentPod } from "../../util.js" import { buildSyncVolumeName, dockerAuthSecretKey, k8sUtilImageName, rsyncPortName } from "../../constants.js" import { styles } from "../../../../logger/styles.js" +import type { StringMap } from "../../../../config/common.js" +export const inClusterBuilderServiceAccount = "garden-in-cluster-builder" +export const sharedBuildSyncDeploymentName = "garden-build-sync" export const utilContainerName = "util" export const utilRsyncPort = 8730 export const utilDeploymentName = "garden-util" @@ -262,6 +265,57 @@ export function skopeoManifestUnknown(errMsg: string | null | undefined): boolea ) } +export async function ensureServiceAccount({ + ctx, + log, + api, + namespace, + annotations, +}: { + ctx: PluginContext + log: Log + api: KubeApi + namespace: string + annotations?: StringMap +}): Promise { + return deployLock.acquire(namespace, async () => { + const serviceAccount = getBuilderServiceAccountSpec(annotations) + + const status = await compareDeployedResources({ + ctx: ctx as KubernetesPluginContext, + api, + namespace, + manifests: [serviceAccount], + log, + }) + + // NOTE: This is here to make sure that we remove annotations in case they are removed in the garden config. + // `compareDeployedResources` as of today only checks wether the manifest is a subset of the deployed manifest. + // The manifest is still a subset of the deployed manifest, if an annotation has been removed. But we want the + // annotation to be actually removed. + // NOTE(steffen): I tried to change the behaviour of `compareDeployedResources` to return "outdated" when the + // annotations have changed. But a lot of code actually depends on the behaviour of it with missing annotations. + const annotationsNeedUpdate = + status.remoteResources.length > 0 && !isEqualAnnotations(serviceAccount, status.remoteResources[0]) + + const needUpsert = status.state !== "ready" || annotationsNeedUpdate + + if (needUpsert) { + await api.upsert({ kind: "ServiceAccount", namespace, log, obj: serviceAccount }) + return true + } + + return false + }) +} + +function isEqualAnnotations(r1: KubernetesResource, r2: KubernetesResource): boolean { + // normalize annotations before comparison + const a1 = r1.metadata.annotations !== undefined ? r1.metadata.annotations : {} + const a2 = r2.metadata.annotations !== undefined ? r2.metadata.annotations : {} + return JSON.stringify(a1) === JSON.stringify(a2) +} + /** * Ensures that a garden-util deployment exists in the specified namespace. * Returns the docker auth secret that's generated and mounted in the deployment. @@ -279,6 +333,14 @@ export async function ensureUtilDeployment({ api: KubeApi namespace: string }) { + const serviceAccountChanged = await ensureServiceAccount({ + ctx, + log, + api, + namespace, + annotations: provider.config.kaniko?.serviceAccountAnnotations, + }) + return deployLock.acquire(namespace, async () => { const deployLog = log.createLog() @@ -301,12 +363,18 @@ export async function ensureUtilDeployment({ log: deployLog, }) + // if the service account changed, all pods part of the deployment must be restarted + // so that they receive new credentials (e.g. for IRSA) + if (status.remoteResources.length && serviceAccountChanged) { + await cycleDeployment({ ctx, provider, deployment, api, namespace, deployLog }) + } + if (status.state === "ready") { // Need to wait a little to ensure the secret is updated in the deployment if (secretUpdated) { await sleep(1000) } - return { authSecret, updated: false } + return { authSecret, updated: serviceAccountChanged } } // Deploy the service @@ -333,6 +401,46 @@ export async function ensureUtilDeployment({ }) } +export async function cycleDeployment({ + ctx, + provider, + deployment, + api, + namespace, + deployLog, +}: { + ctx: PluginContext + provider: KubernetesProvider + deployment: KubernetesDeployment + api: KubeApi + namespace: string + deployLog: Log +}) { + const originalReplicas = deployment.spec.replicas + + deployment.spec.replicas = 0 + await api.upsert({ kind: "Deployment", namespace, log: deployLog, obj: deployment }) + await waitForResources({ + namespace, + ctx, + provider, + resources: [deployment], + log: deployLog, + timeoutSec: 600, + }) + + deployment.spec.replicas = originalReplicas + await api.upsert({ kind: "Deployment", namespace, log: deployLog, obj: deployment }) + await waitForResources({ + namespace, + ctx, + provider, + resources: [deployment], + log: deployLog, + timeoutSec: 600, + }) +} + export async function getManifestInspectArgs(remoteId: string, deploymentRegistry: ContainerRegistryConfig) { const dockerArgs = ["manifest", "inspect", remoteId] const { hostname } = deploymentRegistry @@ -386,6 +494,20 @@ export async function ensureBuilderSecret({ return { authSecret, updated } } +export function getBuilderServiceAccountSpec(annotations?: StringMap) { + const serviceAccount: KubernetesServiceAccount = { + apiVersion: "v1", + kind: "ServiceAccount", + metadata: { + name: inClusterBuilderServiceAccount, + // ensure we clear old annotations if config flags are removed + annotations: annotations || {}, + }, + } + + return serviceAccount +} + export function getUtilContainer(authSecretName: string, provider: KubernetesProvider): V1Container { return { name: utilContainerName, @@ -491,6 +613,7 @@ export function getUtilManifests( annotations: kanikoAnnotations, }, spec: { + serviceAccountName: inClusterBuilderServiceAccount, containers: [utilContainer], imagePullSecrets, volumes: [ diff --git a/core/src/plugins/kubernetes/jib-container.ts b/core/src/plugins/kubernetes/jib-container.ts index bf0015d2a5..567b22b6cc 100644 --- a/core/src/plugins/kubernetes/jib-container.ts +++ b/core/src/plugins/kubernetes/jib-container.ts @@ -110,7 +110,6 @@ async function buildAndPushViaRemote(params: BuildActionParams<"build", Containe // Make sure the sync target is up if (buildMode === "kaniko") { - // Make sure the garden-util deployment is up await ensureUtilDeployment({ ctx, provider, @@ -120,7 +119,6 @@ async function buildAndPushViaRemote(params: BuildActionParams<"build", Containe }) deploymentName = utilDeploymentName } else if (buildMode === "cluster-buildkit") { - // Make sure the buildkit deployment is up await ensureBuildkit({ ctx, provider, diff --git a/core/src/plugins/kubernetes/types.ts b/core/src/plugins/kubernetes/types.ts index 4fe6453e45..5507a956ce 100644 --- a/core/src/plugins/kubernetes/types.ts +++ b/core/src/plugins/kubernetes/types.ts @@ -10,6 +10,7 @@ import type { KubernetesObject, V1DaemonSet, V1Deployment, + V1ServiceAccount, V1ObjectMeta, V1ReplicaSet, V1StatefulSet, @@ -92,6 +93,7 @@ export type KubernetesReplicaSet = KubernetesResource export type KubernetesStatefulSet = KubernetesResource export type KubernetesPod = KubernetesResource export type KubernetesService = KubernetesResource +export type KubernetesServiceAccount = KubernetesResource export type KubernetesWorkload = KubernetesResource export type KubernetesIngress = KubernetesResource diff --git a/core/test/unit/src/plugins/kubernetes/container/build/common.ts b/core/test/unit/src/plugins/kubernetes/container/build/common.ts index e495ba69dd..c0a0aaa3e1 100644 --- a/core/test/unit/src/plugins/kubernetes/container/build/common.ts +++ b/core/test/unit/src/plugins/kubernetes/container/build/common.ts @@ -7,7 +7,9 @@ */ import { + getBuilderServiceAccountSpec, getUtilManifests, + inClusterBuilderServiceAccount, skopeoManifestUnknown, } from "../../../../../../../src/plugins/kubernetes/container/build/common.js" import { expect } from "chai" @@ -46,6 +48,25 @@ describe("common build", () => { }) }) + describe("getBuilderServiceAccountSpec", () => { + it("should return the manifest", () => { + const result = getBuilderServiceAccountSpec({ "some-annotation": "annotation-value" }) + expect(result).eql({ + apiVersion: "v1", + kind: "ServiceAccount", + metadata: { + name: inClusterBuilderServiceAccount, + annotations: { "some-annotations": "annotation-value" }, + }, + }) + }) + + it("should return empty annotations when no annotations are provided", () => { + const result = getBuilderServiceAccountSpec() + expect(result.metadata.annotations).eql({}) + }) + }) + describe("getUtilManifests", () => { const _provider: DeepPartial = { config: { @@ -75,6 +96,7 @@ describe("common build", () => { template: { metadata: { labels: { app: "garden-util" }, annotations: undefined }, spec: { + serviceAccountName: inClusterBuilderServiceAccount, containers: [ { name: "util",