Make platforms a feature of rules that is enforced, rather than looke…

…d up at pod admission time

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

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
 
 # ====================================================================================

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:

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: []
 

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

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=

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"`

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"`
+}

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
 		}
 	}

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) {