From f18f96f5ffe0e32a9a677c2c27aea994bc0f862b 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 | 5 + .../kubernetes/container/build/buildkit.ts | 35 ++++- .../kubernetes/container/build/common.ts | 130 +++++++++++++++++- core/src/plugins/kubernetes/jib-container.ts | 2 - core/src/plugins/kubernetes/types.ts | 2 + .../kubernetes/container/build/common.ts | 22 +++ 7 files changed, 196 insertions(+), 10 deletions(-) diff --git a/core/src/plugins/kubernetes/api.ts b/core/src/plugins/kubernetes/api.ts index b99c22a931..0ae55099bb 100644 --- a/core/src/plugins/kubernetes/api.ts +++ b/core/src/plugins/kubernetes/api.ts @@ -32,6 +32,7 @@ import { Log, NetworkingV1Api, ApiextensionsV1Api, + V1ServiceAccount, } from "@kubernetes/client-node" import AsyncLock = require("async-lock") import request = require("request-promise") @@ -130,6 +131,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 28f5a19dc2..854b670a5b 100644 --- a/core/src/plugins/kubernetes/config.ts +++ b/core/src/plugins/kubernetes/config.ts @@ -215,6 +215,7 @@ export interface KubernetesConfig extends BaseProviderConfig { nodeSelector?: StringMap tolerations?: V1Toleration[] annotations?: StringMap + serviceAccountAnnotations?: StringMap } clusterDocker?: { enableBuildKit?: boolean @@ -229,6 +230,7 @@ export interface KubernetesConfig extends BaseProviderConfig { nodeSelector?: StringMap tolerations?: V1Toleration[] annotations?: StringMap + serviceAccountAnnotations?: StringMap util?: { tolerations?: V1Toleration[] annotations?: StringMap @@ -649,6 +651,9 @@ 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."), diff --git a/core/src/plugins/kubernetes/container/build/buildkit.ts b/core/src/plugins/kubernetes/container/build/buildkit.ts index e7e7c57b46..31537ee454 100644 --- a/core/src/plugins/kubernetes/container/build/buildkit.ts +++ b/core/src/plugins/kubernetes/container/build/buildkit.ts @@ -27,6 +27,9 @@ import { utilRsyncPort, ensureBuilderSecret, builderToleration, + inClusterBuilderServiceAccount, + ensureServiceAccount, + cycleDeployment, } from "./common" import { getNamespaceStatus } from "../../namespace" import { LogLevel } from "../../../../logger/logger" @@ -179,6 +182,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.placeholder() @@ -193,15 +204,28 @@ export async function ensureBuildkit({ const imagePullSecrets = await prepareSecrets({ api, namespace, secrets: provider.config.imagePullSecrets, log }) // Check status of the buildkit deployment - const manifest = getBuildkitDeployment(provider, authSecret.metadata.name, imagePullSecrets) - const status = await compareDeployedResources(ctx as KubernetesPluginContext, api, namespace, [manifest], deployLog) + const deployment = getBuildkitDeployment(provider, authSecret.metadata.name, imagePullSecrets) + + const status = await compareDeployedResources( + ctx as KubernetesPluginContext, + api, + namespace, + [deployment], + 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 buildkit daemon @@ -209,14 +233,14 @@ export async function ensureBuildkit({ chalk.gray(`-> Deploying ${buildkitDeploymentName} daemon in ${namespace} namespace (was ${status.state})`) ) - await api.upsert({ kind: "Deployment", namespace, log: deployLog, obj: manifest }) + await api.upsert({ kind: "Deployment", namespace, log: deployLog, obj: deployment }) await waitForResources({ namespace, ctx, provider, serviceName: "garden-buildkit", - resources: [manifest], + resources: [deployment], log: deployLog, timeoutSec: 600, }) @@ -377,6 +401,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 b15c8c358a..08db11ee21 100644 --- a/core/src/plugins/kubernetes/container/build/common.ts +++ b/core/src/plugins/kubernetes/container/build/common.ts @@ -37,13 +37,15 @@ import { gardenEnv } from "../../../../constants" import { ensureMutagenSync, flushMutagenSync, getKubectlExecDestination, terminateMutagenSync } from "../../mutagen" import { randomString } from "../../../../util/string" import { V1Container, V1Service } from "@kubernetes/client-node" -import { cloneDeep, isEmpty } from "lodash" +import _, { cloneDeep, isEmpty } from "lodash" import { compareDeployedResources, waitForResources } from "../../status/status" -import { KubernetesDeployment, KubernetesResource } from "../../types" +import { KubernetesDeployment, KubernetesResource, KubernetesServiceAccount } from "../../types" import { stringifyResources } from "../util" +import { StringMap } from "../../../../config/common" const inClusterRegistryPort = 5000 +export const inClusterBuilderServiceAccount = "garden-in-cluster-builder" export const sharedBuildSyncDeploymentName = "garden-build-sync" export const utilContainerName = "util" export const utilRsyncPort = 8730 @@ -306,6 +308,59 @@ export async function getUtilDaemonPodRunner({ }) } +export async function ensureServiceAccount({ + ctx, + log, + api, + namespace, + annotations, +}: { + ctx: PluginContext + log: LogEntry + api: KubeApi + namespace: string + annotations?: StringMap +}): Promise { + return deployLock.acquire(namespace, async () => { + const deployLog = log.placeholder() + + const serviceAccount = getBuilderServiceAccountSpec(annotations) + + const status = await compareDeployedResources( + ctx as KubernetesPluginContext, + api, + namespace, + [serviceAccount], + deployLog + ) + + // 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: deployLog, 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. @@ -323,6 +378,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.placeholder() @@ -345,12 +408,18 @@ export async function ensureUtilDeployment({ 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 @@ -377,6 +446,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: LogEntry +}) { + 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 function getSocatContainer(provider: KubernetesProvider) { const registryHostname = getInClusterRegistryHostname(provider.config) @@ -450,6 +559,20 @@ function isLocalHostname(hostname: string) { return hostname === "localhost" || hostname.startsWith("127.") } +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, @@ -549,6 +672,7 @@ export function getUtilManifests( annotations: kanikoAnnotations, }, spec: { + serviceAccountName: inClusterBuilderServiceAccount, containers: [getUtilContainer(authSecretName, provider)], imagePullSecrets, volumes: [ diff --git a/core/src/plugins/kubernetes/jib-container.ts b/core/src/plugins/kubernetes/jib-container.ts index dd63d329d2..364a6f09d9 100644 --- a/core/src/plugins/kubernetes/jib-container.ts +++ b/core/src/plugins/kubernetes/jib-container.ts @@ -107,7 +107,6 @@ async function buildAndPushViaRemote(params: BuildModuleParams) { // Make sure the sync target is up if (buildMode === "kaniko") { - // Make sure the garden-util deployment is up await ensureUtilDeployment({ ctx, provider, @@ -117,7 +116,6 @@ async function buildAndPushViaRemote(params: BuildModuleParams) { }) 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 e5355a117e..0bf16501ec 100644 --- a/core/src/plugins/kubernetes/types.ts +++ b/core/src/plugins/kubernetes/types.ts @@ -10,6 +10,7 @@ import { KubernetesObject, V1DaemonSet, V1Deployment, + V1ServiceAccount, V1ObjectMeta, V1ReplicaSet, V1StatefulSet, @@ -76,6 +77,7 @@ export type KubernetesDeployment = KubernetesResource export type KubernetesReplicaSet = KubernetesResource export type KubernetesStatefulSet = KubernetesResource export type KubernetesPod = 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 5a3c73907b..694d5bbdbd 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" import { expect } from "chai" @@ -38,6 +40,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: { @@ -64,6 +85,7 @@ describe("common build", () => { template: { metadata: { labels: { app: "garden-util" }, annotations: undefined }, spec: { + serviceAccountName: inClusterBuilderServiceAccount, containers: [ { name: "util",