From 8d1de09fdc697a13532028a9777cdceecf65e419 Mon Sep 17 00:00:00 2001 From: Cameron McAvoy Date: Wed, 31 Jan 2024 17:21:36 -0600 Subject: [PATCH] Make platforms a feature of rules that is enforced, rather than looked up at pod admission time --- CHANGELOG.md | 4 ++ Makefile | 2 +- .../harbor-container-webhook/Chart.yaml | 4 +- .../harbor-container-webhook/values.yaml | 3 + go.mod | 6 +- go.sum | 11 ++++ internal/config/config.go | 5 ++ internal/webhook/manifest.go | 23 ++++++++ internal/webhook/mutate.go | 39 ++++--------- internal/webhook/transformer.go | 56 ++++++++++++++++--- 10 files changed, 113 insertions(+), 40 deletions(-) create mode 100644 internal/webhook/manifest.go diff --git a/CHANGELOG.md b/CHANGELOG.md index c613507..9f7e8f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.7.0] - 2024-01-31 +### Changed +- Instead of querying for the node architecture and os when inspecting pods, which rarely worked, use `platforms` on the config to determine which platforms should be required when checking upstream. + ## [0.6.3] - 2024-01-26 ### Fixed - Fixed rewrite_success prometheus metric counting every rule invocation, instead of only rewrites diff --git a/Makefile b/Makefile index 76c56d1..798c9fd 100644 --- a/Makefile +++ b/Makefile @@ -127,7 +127,7 @@ docker.build: docker.buildx.setup ## Build the docker image docker.buildx.setup: @$(INFO) docker buildx setup - @docker buildx ls 2>/dev/null | grep -vq $(DOCKER_BUILDX_BUILDER) || docker buildx create --name $(DOCKER_BUILDX_BUILDER) --driver docker-container --driver-opt network=host --bootstrap --use + @docker buildx ls 2>/dev/null | grep -vq $(DOCKER_BUILDX_BUILDER) || docker buildx create --name $(DOCKER_BUILDX_BUILDER) --driver docker-container --driver-opt network=host --use @$(OK) docker buildx setup # ==================================================================================== diff --git a/deploy/charts/harbor-container-webhook/Chart.yaml b/deploy/charts/harbor-container-webhook/Chart.yaml index d64ff31..f7d6599 100644 --- a/deploy/charts/harbor-container-webhook/Chart.yaml +++ b/deploy/charts/harbor-container-webhook/Chart.yaml @@ -2,8 +2,8 @@ apiVersion: v2 name: harbor-container-webhook description: Webhook to configure pods with harbor proxy cache projects type: application -version: 0.6.3 -appVersion: "0.6.3" +version: 0.7.0 +appVersion: "0.7.0" kubeVersion: ">= 1.16.0-0" home: https://github.com/IndeedEng-alpha/harbor-container-webhook maintainers: diff --git a/deploy/charts/harbor-container-webhook/values.yaml b/deploy/charts/harbor-container-webhook/values.yaml index 6b9a2b9..9c98c9c 100644 --- a/deploy/charts/harbor-container-webhook/values.yaml +++ b/deploy/charts/harbor-container-webhook/values.yaml @@ -93,6 +93,9 @@ rules: [] # - '^docker.io/(library/)?ubuntu:.*$' # replace: 'harbor.example.com/ubuntu-proxy' # checkUpstream: true # tests if the manifest for the rewritten image exists +# platforms: # defaults to linux/amd64, only used if checkUpstream is set +# - linux/amd64 +# - linux/arm64 extraRules: [] diff --git a/go.mod b/go.mod index d392771..dd7d72f 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,10 @@ module github.com/indeedeng-alpha/harbor-container-webhook go 1.19 require ( + github.com/containerd/containerd v1.7.13 github.com/containers/image/v5 v5.29.0 github.com/google/go-containerregistry v0.17.0 + github.com/opencontainers/image-spec v1.1.0-rc5 github.com/prometheus/client_golang v1.17.0 github.com/stretchr/testify v1.8.4 gomodules.xyz/jsonpatch/v2 v2.4.0 @@ -18,6 +20,7 @@ require ( require ( github.com/beorn7/perks v1.0.1 // indirect github.com/cespare/xxhash/v2 v2.2.0 // indirect + github.com/containerd/log v0.1.0 // indirect github.com/containerd/stargz-snapshotter/estargz v0.15.1 // indirect github.com/containers/storage v1.51.0 // indirect github.com/davecgh/go-spew v1.1.1 // indirect @@ -51,7 +54,6 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/opencontainers/go-digest v1.0.0 // indirect - github.com/opencontainers/image-spec v1.1.0-rc5 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/client_model v0.5.0 // indirect @@ -71,6 +73,8 @@ require ( golang.org/x/text v0.14.0 // indirect golang.org/x/time v0.5.0 // indirect google.golang.org/appengine v1.6.8 // indirect + google.golang.org/genproto/googleapis/rpc v0.0.0-20230920204549-e6e6cdab5c13 // indirect + google.golang.org/grpc v1.58.3 // indirect google.golang.org/protobuf v1.31.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/go.sum b/go.sum index f5b99e0..934974e 100644 --- a/go.sum +++ b/go.sum @@ -2,6 +2,13 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44= github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= +github.com/containerd/containerd v1.7.0 h1:G/ZQr3gMZs6ZT0qPUZ15znx5QSdQdASW11nXTLTM2Pg= +github.com/containerd/containerd v1.7.0/go.mod h1:QfR7Efgb/6X2BDpTPJRvPTYDE9rsF0FsXX9J8sIs/sc= +github.com/containerd/containerd v1.7.11/go.mod h1:5UluHxHTX2rdvYuZ5OJTC5m/KJNs0Zs9wVoJm9zf5ZE= +github.com/containerd/containerd v1.7.13 h1:wPYKIeGMN8vaggSKuV1X0wZulpMz4CrgEsZdaCyB6Is= +github.com/containerd/containerd v1.7.13/go.mod h1:zT3up6yTRfEUa6+GsITYIJNgSVL9NQ4x4h1RPzk0Wu4= +github.com/containerd/log v0.1.0 h1:TCJt7ioM2cr/tfR8GPbGf9/VRAX8D2B4PjzCpfX540I= +github.com/containerd/log v0.1.0/go.mod h1:VRRf09a7mHDIRezVKTRCrOq78v577GXq3bSa3EhrzVo= github.com/containerd/stargz-snapshotter/estargz v0.15.1 h1:eXJjw9RbkLFgioVaTG+G/ZW/0kEe2oEKCdS/ZxIyoCU= github.com/containerd/stargz-snapshotter/estargz v0.15.1/go.mod h1:gr2RNwukQ/S9Nv33Lt6UC7xEx58C+LHRdoqbEKjz1Kk= github.com/containers/image/v5 v5.29.0 h1:9+nhS/ZM7c4Kuzu5tJ0NMpxrgoryOJ2HAYTgG8Ny7j4= @@ -199,6 +206,10 @@ gomodules.xyz/jsonpatch/v2 v2.4.0 h1:Ci3iUJyx9UeRx7CeFN8ARgGbkESwJK+KB9lLcWxY/Zw gomodules.xyz/jsonpatch/v2 v2.4.0/go.mod h1:AH3dM2RI6uoBZxn3LVrfvJ3E0/9dG4cSrbuBJT4moAY= google.golang.org/appengine v1.6.8 h1:IhEN5q69dyKagZPYMSdIjS2HqprW324FRQZJcGqPAsM= google.golang.org/appengine v1.6.8/go.mod h1:1jJ3jBArFh5pcgW8gCtRJnepW8FzD1V44FJffLiz/Ds= +google.golang.org/genproto/googleapis/rpc v0.0.0-20230920204549-e6e6cdab5c13 h1:N3bU/SQDCDyD6R528GJ/PwW9KjYcJA3dgyH+MovAkIM= +google.golang.org/genproto/googleapis/rpc v0.0.0-20230920204549-e6e6cdab5c13/go.mod h1:KSqppvjFjtoCI+KGd4PELB0qLNxdJHRGqRI09mB6pQA= +google.golang.org/grpc v1.58.3 h1:BjnpXut1btbtgN/6sp+brB2Kbm2LjNXnidYujAVbSoQ= +google.golang.org/grpc v1.58.3/go.mod h1:tgX3ZQDlNJGU96V6yHh1T/JeoBQ2TXdr43YbYSsCJk0= google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw= google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc= google.golang.org/protobuf v1.31.0 h1:g0LDEJHgrBl9N9r17Ru3sqWhkIx2NB67okBHPwC7hs8= diff --git a/internal/config/config.go b/internal/config/config.go index f87e259..6fd3c9c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,6 +20,9 @@ func LoadConfiguration(path string) (*Configuration, error) { ns := detectNamespace() for i := range conf.Rules { conf.Rules[i].Namespace = ns + if len(conf.Rules[i].Platforms) == 0 { + conf.Rules[i].Platforms = []string{"linux/amd64"} + } } return conf, nil } @@ -76,6 +79,8 @@ type ProxyRule struct { // If the webhook lacks permissions to fetch the image manifest or the registry is down, the image // will not be rewritten. Experimental. CheckUpstream bool `yaml:"checkUpstream"` + // List of the required platforms to check for if CheckUpstream is set. Defaults to "linux/amd64" if unset. + Platforms []string `yaml:"platforms"` // AuthSecretName is a reference to an image pull secret (must be .dockerconfigjson type) which // will be used to authenticate if `checkUpstream` is set. Unused if not specified or `checkUpstream` is false. AuthSecretName string `yaml:"authSecretName"` diff --git a/internal/webhook/manifest.go b/internal/webhook/manifest.go new file mode 100644 index 0000000..930d043 --- /dev/null +++ b/internal/webhook/manifest.go @@ -0,0 +1,23 @@ +package webhook + +// slimManifest is a partial representation of the oci manifest to access the mediaType +type slimManifest struct { + MediaType string `json:"mediaType"` +} + +type platform struct { + Architecture string `json:"architecture"` + OS string `json:"os"` +} + +// indexManifest is a partial representation of the sub manifest present in a manifest list +type indexManifest struct { + MediaType string `json:"mediaType"` + Platform platform `json:"platform"` +} + +// slimManifestList is a partial representation of the oci manifest list to access the supported architectures +type slimManifestList struct { + MediaType string `json:"mediaType"` + Manifests []indexManifest `json:"manifests"` +} diff --git a/internal/webhook/mutate.go b/internal/webhook/mutate.go index e854767..dc17401 100644 --- a/internal/webhook/mutate.go +++ b/internal/webhook/mutate.go @@ -5,14 +5,9 @@ import ( "encoding/json" "fmt" "net/http" - "runtime" - - v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/prometheus/client_golang/prometheus" - "gomodules.xyz/jsonpatch/v2" - corev1 "k8s.io/api/core/v1" ctrl "sigs.k8s.io/controller-runtime" @@ -57,21 +52,11 @@ func (p *PodContainerProxier) Handle(ctx context.Context, req admission.Request) return admission.Errored(http.StatusBadRequest, err) } - platformArch := runtime.GOARCH - os := runtime.GOOS - nodeName := pod.Spec.NodeName - if nodeName != "" { - platformArch, os, err = p.lookupNodeArchAndOS(ctx, p.Client, nodeName) - if err != nil { - logger.Info(fmt.Sprintf("unable to lookup node for pod %q, defaulting pod to webhook runtime OS and architecture: %s", string(pod.UID), err.Error())) - } - } - - initContainers, updatedInit, err := p.updateContainers(ctx, pod.Spec.InitContainers, platformArch, os, "init") + initContainers, updatedInit, err := p.updateContainers(ctx, pod.Spec.InitContainers, "init") if err != nil { return admission.Errored(http.StatusInternalServerError, err) } - containers, updated, err := p.updateContainers(ctx, pod.Spec.Containers, platformArch, os, "normal") + containers, updated, err := p.updateContainers(ctx, pod.Spec.Containers, "normal") if err != nil { return admission.Errored(http.StatusInternalServerError, err) } @@ -85,12 +70,6 @@ func (p *PodContainerProxier) Handle(ctx context.Context, req admission.Request) if err != nil { return admission.Errored(http.StatusInternalServerError, err) } - if p.Verbose { - patch, err := jsonpatch.CreatePatch(req.Object.Raw, marshaledPod) - if err == nil { // errors will be surfaced in return below - logger.Info(fmt.Sprintf("patch for %s: %v", string(pod.UID), patch)) - } - } return admission.PatchResponseFromRaw(req.Object.Raw, marshaledPod) } @@ -99,15 +78,16 @@ func (p *PodContainerProxier) lookupNodeArchAndOS(ctx context.Context, restClien if err = restClient.Get(ctx, client.ObjectKey{Name: nodeName}, &node); err != nil { return "", "", fmt.Errorf("failed to lookup node %s: %w", nodeName, err) } + logger.Info(fmt.Sprintf("node %v", node)) return node.Status.NodeInfo.Architecture, node.Status.NodeInfo.OperatingSystem, nil } -func (p *PodContainerProxier) updateContainers(ctx context.Context, containers []corev1.Container, platform, os, kind string) ([]corev1.Container, bool, error) { +func (p *PodContainerProxier) updateContainers(ctx context.Context, containers []corev1.Container, kind string) ([]corev1.Container, bool, error) { containersReplacement := make([]corev1.Container, 0, len(containers)) updated := false for i := range containers { container := containers[i] - imageRef, err := p.rewriteImage(ctx, container.Image, platform, os) + imageRef, err := p.rewriteImage(ctx, container.Image) if err != nil { return []corev1.Container{}, false, err } @@ -124,20 +104,21 @@ func (p *PodContainerProxier) updateContainers(ctx context.Context, containers [ return containersReplacement, updated, nil } -func (p *PodContainerProxier) rewriteImage(ctx context.Context, imageRef, platform, os string) (string, error) { +func (p *PodContainerProxier) rewriteImage(ctx context.Context, imageRef string) (string, error) { for _, transformer := range p.Transformers { updatedRef, err := transformer.RewriteImage(imageRef) if err != nil { return "", fmt.Errorf("transformer %q failed to update imageRef %q: %w", transformer.Name(), imageRef, err) } if updatedRef != imageRef { - if found, err := transformer.CheckUpstream(ctx, updatedRef, &v1.Platform{Architecture: platform, OS: os}); err != nil { - logger.Info(fmt.Sprintf("skipping rewriting %q to %q, could not fetch image manifest: %s", imageRef, updatedRef, err.Error())) + if found, err := transformer.CheckUpstream(ctx, updatedRef); err != nil { + logger.Info(fmt.Sprintf("transformer %q skipping rewriting %q to %q, could not fetch image manifest: %s", transformer.Name(), imageRef, updatedRef, err.Error())) continue } else if !found { - logger.Info(fmt.Sprintf("skipping rewriting %q to %q, registry reported image not found.", imageRef, updatedRef)) + logger.Info(fmt.Sprintf("transformer %q skipping rewriting %q to %q, registry reported image not found.", transformer.Name(), imageRef, updatedRef)) continue } + logger.Info(fmt.Sprintf("transformer %q rewriting %q to %q", transformer.Name(), imageRef, updatedRef)) return updatedRef, nil } } diff --git a/internal/webhook/transformer.go b/internal/webhook/transformer.go index 0f4d207..a3e2784 100644 --- a/internal/webhook/transformer.go +++ b/internal/webhook/transformer.go @@ -8,12 +8,15 @@ import ( "strings" "time" + "github.com/containerd/containerd/images" + "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/crane" - v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/indeedeng-alpha/harbor-container-webhook/internal/config" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/prometheus/client_golang/prometheus" corev1 "k8s.io/api/core/v1" @@ -72,7 +75,7 @@ type ContainerTransformer interface { // CheckUpstream ensures that the docker image reference exists in the upstream registry // and returns if the image exists, or an error if the registry can't be contacted. - CheckUpstream(ctx context.Context, imageRef string, platform *v1.Platform) (bool, error) + CheckUpstream(ctx context.Context, imageRef string) (bool, error) } func MakeTransformers(rules []config.ProxyRule, client client.Client) ([]ContainerTransformer, error) { @@ -129,7 +132,7 @@ func (t *ruleTransformer) Name() string { return t.rule.Name } -func (t *ruleTransformer) CheckUpstream(ctx context.Context, imageRef string, platform *v1.Platform) (bool, error) { +func (t *ruleTransformer) CheckUpstream(ctx context.Context, imageRef string) (bool, error) { if !t.rule.CheckUpstream { return true, nil } @@ -142,13 +145,52 @@ func (t *ruleTransformer) CheckUpstream(ctx context.Context, imageRef string, pl } options = append(options, crane.WithAuth(auth)) } - options = append(options, crane.WithPlatform(platform), crane.WithContext(ctx)) - if _, err := crane.Manifest(imageRef, options...); err != nil { + // we don't pass in the platform to crane to retrieve the full manifest list for multi-arch + options = append(options, crane.WithContext(ctx)) + manifestBytes, err := crane.Manifest(imageRef, options...) + if err != nil { upstreamErrors.WithLabelValues(t.metricName).Inc() return false, err } - upstream.WithLabelValues(t.metricName).Inc() - return true, nil + + // try and parse the manifest to decode the MediaType to determine if it's a manifest or manifest list + manifest := slimManifest{} + if err := json.Unmarshal(manifestBytes, &manifest); err != nil { + upstreamErrors.WithLabelValues(t.metricName).Inc() + return false, fmt.Errorf("failed to parse manifest %s payload=%s: %w", imageRef, string(manifestBytes), err) + } + + switch manifest.MediaType { + case images.MediaTypeDockerSchema2ManifestList, ocispec.MediaTypeImageIndex: + manifestList := slimManifestList{} + if err := json.Unmarshal(manifestBytes, &manifestList); err != nil { + upstreamErrors.WithLabelValues(t.metricName).Inc() + return false, fmt.Errorf("failed to parse manifest list %s, payload=%s: %w", imageRef, string(manifestBytes), err) + } + matches := 0 + for _, rulePlatform := range t.rule.Platforms { + for _, subManifest := range manifestList.Manifests { + subPlatform := subManifest.Platform.OS + "/" + subManifest.Platform.Architecture + if subPlatform == rulePlatform { + matches++ + break + } + } + } + if matches == len(t.rule.Platforms) { + upstream.WithLabelValues(t.metricName).Inc() + return true, nil + } + + return false, nil + case images.MediaTypeDockerSchema1Manifest, images.MediaTypeDockerSchema2Manifest, ocispec.MediaTypeImageManifest: + upstream.WithLabelValues(t.metricName).Inc() + return true, nil + default: + logger.Info(fmt.Sprintf("unknown manifest media type: %s, rule=%s,imageRef=%s", manifest.MediaType, t.rule.Name, imageRef)) + upstream.WithLabelValues(t.metricName).Inc() + return true, nil + } } func (t *ruleTransformer) auth(ctx context.Context) (authn.Authenticator, error) {