diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 498595a..a436a48 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,7 +30,7 @@ jobs: go-version-file: go.mod - uses: golangci/golangci-lint-action@v9 with: - version: v2.13.2 + version: v2.15.0 test: runs-on: ubuntu-latest diff --git a/Makefile b/Makefile index 3d43b92..434aa28 100644 --- a/Makefile +++ b/Makefile @@ -257,7 +257,7 @@ KUSTOMIZE_VERSION ?= v5.6.0 CONTROLLER_TOOLS_VERSION ?= v0.17.2 ENVTEST_VERSION ?= $(shell go list -m -f "{{ .Version }}" sigs.k8s.io/controller-runtime | awk -F'[v.]' '{printf "release-%d.%d", $$2, $$3}') ENVTEST_K8S_VERSION ?= $(shell go list -m -f "{{ .Version }}" k8s.io/api | awk -F'[v.]' '{printf "1.%d", $$3}') -GOLANGCI_LINT_VERSION ?= v2.12.2 +GOLANGCI_LINT_VERSION ?= v2.15.0 CHAINSAW_VERSION ?= v0.2.15 GOVULNCHECK_VERSION ?= v1.6.0 diff --git a/api/v1alpha1/cachedimage_types.go b/api/v1alpha1/cachedimage_types.go index 8904926..4d844c0 100644 --- a/api/v1alpha1/cachedimage_types.go +++ b/api/v1alpha1/cachedimage_types.go @@ -118,6 +118,7 @@ type CachedImageStatus struct { // +kubebuilder:printcolumn:name="Set",type=string,JSONPath=`.metadata.labels.drop\.corewire\.io/imageset`,description="Parent CachedImageSet",priority=1 // +kubebuilder:printcolumn:name="Message",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].message`,priority=1 // +kubebuilder:printcolumn:name="Policy",type=string,JSONPath=`.spec.policyRef.name`,priority=1 +// +kubebuilder:validation:XValidation:rule="size(self.metadata.name) <= 63",message="metadata.name must be no more than 63 characters (used as a pod label value)" // CachedImage ensures a single container image is pre-cached on cluster nodes. type CachedImage struct { diff --git a/api/v1alpha1/cachedimageset_types.go b/api/v1alpha1/cachedimageset_types.go index 67e9705..33c7058 100644 --- a/api/v1alpha1/cachedimageset_types.go +++ b/api/v1alpha1/cachedimageset_types.go @@ -100,6 +100,7 @@ type CachedImageSetStatus struct { // +kubebuilder:printcolumn:name="Source",type=string,JSONPath=`.spec.discoveryPolicyRef.name` // +kubebuilder:printcolumn:name="Message",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].message`,priority=1 // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp` +// +kubebuilder:validation:XValidation:rule="size(self.metadata.name) <= 63",message="metadata.name must be no more than 63 characters (used as a label value on child CachedImages)" // CachedImageSet manages a group of images to cache, optionally backed by a DiscoveryPolicy. type CachedImageSet struct { diff --git a/cmd/main.go b/cmd/main.go index ec02941..15218ac 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -195,9 +195,19 @@ func main() { // intervals for our own CRDs where timely reconciliation matters. Cache: cache.Options{ SyncPeriod: ptr.To(10 * time.Minute), // default for all types + // Without scoping, the cache holds every Pod, Secret and full Node in + // the cluster, which OOMs the manager on large clusters. + DefaultTransform: cache.TransformStripManagedFields(), ByObject: map[client.Object]cache.ByObject{ &corev1.Node{}: { SyncPeriod: ptr.To(30 * time.Minute), // nodes rarely change + Transform: stripNodeImages, + }, + &corev1.Pod{}: { + Namespaces: map[string]cache.Config{podNamespace: {}}, + }, + &corev1.Secret{}: { + Namespaces: map[string]cache.Config{podNamespace: {}}, }, &dropv1alpha1.CachedImage{}: { SyncPeriod: ptr.To(5 * time.Minute), // CRDs: tighter resync @@ -277,3 +287,14 @@ func main() { os.Exit(1) } } + +// stripNodeImages drops status.images, by far the largest Node field on nodes +// with many cached images and never read by drop. A per-object transform +// replaces DefaultTransform, so managedFields are stripped here too. +func stripNodeImages(obj any) (any, error) { + if node, ok := obj.(*corev1.Node); ok { + node.Status.Images = nil + node.ManagedFields = nil + } + return obj, nil +} diff --git a/config/crd/bases/drop.corewire.io_cachedimages.yaml b/config/crd/bases/drop.corewire.io_cachedimages.yaml index ecf0eec..925511b 100644 --- a/config/crd/bases/drop.corewire.io_cachedimages.yaml +++ b/config/crd/bases/drop.corewire.io_cachedimages.yaml @@ -320,6 +320,10 @@ spec: type: string type: object type: object + x-kubernetes-validations: + - message: metadata.name must be no more than 63 characters (used as a pod + label value) + rule: size(self.metadata.name) <= 63 served: true storage: true subresources: diff --git a/config/crd/bases/drop.corewire.io_cachedimagesets.yaml b/config/crd/bases/drop.corewire.io_cachedimagesets.yaml index a9d154c..1884b9d 100644 --- a/config/crd/bases/drop.corewire.io_cachedimagesets.yaml +++ b/config/crd/bases/drop.corewire.io_cachedimagesets.yaml @@ -283,6 +283,10 @@ spec: type: string type: object type: object + x-kubernetes-validations: + - message: metadata.name must be no more than 63 characters (used as a label + value on child CachedImages) + rule: size(self.metadata.name) <= 63 served: true storage: true subresources: diff --git a/go.mod b/go.mod index ef86b43..f61f6dd 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ module github.com/corewire/drop go 1.26.0 -toolchain go1.27.1 +toolchain go1.27.3 godebug default=go1.26 @@ -85,7 +85,7 @@ require ( go.yaml.in/yaml/v3 v3.0.5 // indirect golang.org/x/exp v0.0.0-20260908205506-85c1c2202aba // indirect golang.org/x/mod v0.41.0 // indirect - golang.org/x/net v0.59.0 // indirect + golang.org/x/net v0.60.0 // indirect golang.org/x/oauth2 v0.37.0 // indirect golang.org/x/sync v0.23.0 // indirect golang.org/x/sys v0.48.0 // indirect diff --git a/go.sum b/go.sum index 8e8d32b..399275e 100644 --- a/go.sum +++ b/go.sum @@ -197,8 +197,8 @@ golang.org/x/exp v0.0.0-20260908205506-85c1c2202aba h1:Ck8QetSgk912qxWLMCKxd0in+ golang.org/x/exp v0.0.0-20260908205506-85c1c2202aba/go.mod h1:50RgIsmK7OwqzTTeqcSXQW8SswW0o8fRcDxmqGluJ8E= golang.org/x/mod v0.41.0 h1:qJmnOUb4YB+FsEuM3HcWucdZASCPGhsX6uljO6pog0c= golang.org/x/mod v0.41.0/go.mod h1:Ek9pY8RKWXwsWvd3rQiHYtMqkjSUV+s1Rj7j4H5Ur6o= -golang.org/x/net v0.59.0 h1:5zfYln+w5XCxwrnMMJPufRgNoXEaGxl0wo5GqPXyues= -golang.org/x/net v0.59.0/go.mod h1:2DA/G1UfVbCpQPeWTmMPGY7Cs2PkBkwu743bVX5PIVg= +golang.org/x/net v0.60.0 h1:79p50tfZlm0J9YfoDsSi639qSXNGVwEzOPLCxM2FsYU= +golang.org/x/net v0.60.0/go.mod h1:2DA/G1UfVbCpQPeWTmMPGY7Cs2PkBkwu743bVX5PIVg= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.23.0 h1:KameEIfc1IkluZyXWLn39Wd4tURc6GbCiISGiZm2bQk= diff --git a/internal/controller/cachedimage_churn_test.go b/internal/controller/cachedimage_churn_test.go index 1a61c5a..db9768d 100644 --- a/internal/controller/cachedimage_churn_test.go +++ b/internal/controller/cachedimage_churn_test.go @@ -7,6 +7,7 @@ SPDX-License-Identifier: MIT package controller import ( + "strings" "testing" "time" @@ -50,6 +51,20 @@ func TestChildName_DistinguishesSameShortNameAcrossRegistries(t *testing.T) { } } +// Children are cluster-scoped and no longer carry the parent name, so two sets +// caching the same image must still get distinct children. +func TestChildName_DistinctAcrossParents(t *testing.T) { + entry := dropv1alpha1.ImageEntry{Image: "registry.haufe.io/rd/tools", Tag: "stable"} + if childName("set-a", entry) == childName("set-b", entry) { + t.Error("same image in two sets maps to one child name") + } + if got := childName("gitlab-runner-helper-images", dropv1alpha1.ImageEntry{ + Image: "registry.gitlab.com/gitlab-org/gitlab-runner/gitlab-runner-helper", Tag: "x86_64-v19.3.3", + }); !strings.HasPrefix(got, "gitlab-runner-helper-x86-64-v19-3-3-") { + t.Errorf("childName = %q, want gitlab-runner-helper-x86-64-v19-3-3-", got) + } +} + func TestChildName_ValidAndBounded(t *testing.T) { long := "registry.example.com/" + string(make([]byte, 0)) for range 40 { @@ -58,8 +73,8 @@ func TestChildName_ValidAndBounded(t *testing.T) { entry := dropv1alpha1.ImageEntry{Image: long + "image", Tag: "sometag"} got := childName("parent", entry) - if len(got) > 253 { - t.Errorf("childName length = %d, want <= 253", len(got)) + if len(got) > 63 { + t.Errorf("childName length = %d, want <= 63", len(got)) } if got[len(got)-1] == '-' { t.Errorf("childName %q must not end in a hyphen", got) diff --git a/internal/controller/cachedimageset_controller.go b/internal/controller/cachedimageset_controller.go index 9e69af3..f051ca4 100644 --- a/internal/controller/cachedimageset_controller.go +++ b/internal/controller/cachedimageset_controller.go @@ -71,11 +71,20 @@ func (r *CachedImageSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, fmt.Errorf("listing children: %w", err) } - // Build map of existing children by image ref + // Build map of existing children by image ref. Children whose name no longer + // matches childName (e.g. over-long names from older versions) are recreated. existingMap := make(map[string]*dropv1alpha1.CachedImage, len(existingChildren.Items)) for i := range existingChildren.Items { child := &existingChildren.Items[i] ref := buildChildImageRef(child) + entry := dropv1alpha1.ImageEntry{Image: child.Spec.Image, Tag: child.Spec.Tag, Digest: child.Spec.Digest} + if child.Name != childName(imageSet.Name, entry) { + log.Info("deleting child CachedImage with stale name", "name", child.Name, "image", ref) + if err := r.Delete(ctx, child); client.IgnoreNotFound(err) != nil { + return ctrl.Result{}, fmt.Errorf("deleting stale child: %w", err) + } + continue + } existingMap[ref] = child } @@ -340,10 +349,11 @@ func imageName(image string) string { return parts[len(parts)-1] } -// childName builds the name of the child CachedImage for an image entry. -// imageName() keeps only the last path segment, so entries that differ solely by -// registry or org (rd/tools vs dos-devinfra/tools) would otherwise collide on one -// name and the loser could never be created. The ref digest keeps names unique. +// childName builds the name of the child CachedImage for an image entry as +// --. The hash covers the parent name and the full ref, so +// sets caching the same image, or images sharing a short name across registries +// (rd/tools vs dos-devinfra/tools), never collide. The parent is on the +// drop.corewire.io/imageset label. func childName(parentName string, img dropv1alpha1.ImageEntry) string { suffix := img.Tag switch { @@ -353,11 +363,12 @@ func childName(parentName string, img dropv1alpha1.ImageEntry) string { suffix = "latest" } h := fnv.New64a() - _, _ = h.Write([]byte(buildEntryRef(img))) + _, _ = h.Write([]byte(parentName + "/" + buildEntryRef(img))) unique := fmt.Sprintf("%06x", h.Sum64()&0xffffff) - base := sanitizeName(fmt.Sprintf("%s-%s-%s", parentName, imageName(img.Image), suffix)) - if limit := 253 - len(unique) - 1; len(base) > limit { + // CachedImage names are used as pod label values, which are capped at 63. + base := sanitizeName(fmt.Sprintf("%s-%s", imageName(img.Image), suffix)) + if limit := 63 - len(unique) - 1; len(base) > limit { base = base[:limit] } return base + "-" + unique diff --git a/internal/controller/cachedimageset_controller_test.go b/internal/controller/cachedimageset_controller_test.go index 2a33e38..1b6611e 100644 --- a/internal/controller/cachedimageset_controller_test.go +++ b/internal/controller/cachedimageset_controller_test.go @@ -8,6 +8,7 @@ package controller import ( "context" + "strings" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -142,6 +143,63 @@ var _ = Describe("CachedImageSet Controller", func() { Expect(after.Items).To(BeEmpty(), "terminating set recreated its children, so deletion could never finish") }) + + It("rejects CachedImage and CachedImageSet names longer than 63 characters", func() { + longName := strings.Repeat("a", 64) + + ci := &dropv1alpha1.CachedImage{ + ObjectMeta: metav1.ObjectMeta{Name: longName}, + Spec: dropv1alpha1.CachedImageSpec{Image: "docker.io/library/nginx"}, + } + err := k8sClient.Create(ctx, ci) + Expect(errors.IsInvalid(err)).To(BeTrue(), "expected Invalid, got %v", err) + + set := &dropv1alpha1.CachedImageSet{ + ObjectMeta: metav1.ObjectMeta{Name: longName}, + Spec: dropv1alpha1.CachedImageSetSpec{ + Images: []dropv1alpha1.ImageEntry{{Image: "docker.io/library/nginx"}}, + }, + } + err = k8sClient.Create(ctx, set) + Expect(errors.IsInvalid(err)).To(BeTrue(), "expected Invalid, got %v", err) + }) + + It("recreates children whose name does not match childName", func() { + const setName = "test-imageset-stale-child" + entry := dropv1alpha1.ImageEntry{Image: "docker.io/library/nginx", Tag: "1.25"} + + set := &dropv1alpha1.CachedImageSet{ + ObjectMeta: metav1.ObjectMeta{Name: setName}, + Spec: dropv1alpha1.CachedImageSetSpec{Images: []dropv1alpha1.ImageEntry{entry}}, + } + Expect(k8sClient.Create(ctx, set)).To(Succeed()) + DeferCleanup(func() { _ = k8sClient.Delete(ctx, set) }) + + stale := &dropv1alpha1.CachedImage{ + ObjectMeta: metav1.ObjectMeta{ + Name: "stale-child-name", + Labels: map[string]string{labelImageSet: setName}, + }, + Spec: dropv1alpha1.CachedImageSpec{Image: entry.Image, Tag: entry.Tag}, + } + Expect(k8sClient.Create(ctx, stale)).To(Succeed()) + DeferCleanup(func() { _ = k8sClient.Delete(ctx, stale) }) + + controllerReconciler := &CachedImageSetReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + } + _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{Name: setName}, + }) + Expect(err).NotTo(HaveOccurred()) + + children := &dropv1alpha1.CachedImageList{} + Expect(k8sClient.List(ctx, children, client.MatchingLabels{labelImageSet: setName})).To(Succeed()) + Expect(children.Items).To(HaveLen(1)) + Expect(children.Items[0].Name).To(Equal(childName(setName, entry))) + DeferCleanup(func() { _ = k8sClient.Delete(ctx, &children.Items[0]) }) + }) }) Context("When the referenced DiscoveryPolicy is failing", func() { diff --git a/knowledge.yaml b/knowledge.yaml index 66de294..62643d7 100644 --- a/knowledge.yaml +++ b/knowledge.yaml @@ -133,6 +133,7 @@ crds: required: false doc: 'Conditions represent the latest available observations. Condition types: Ready, PullProgress.' markers: + - +kubebuilder:validation:XValidation:rule="size(self.metadata.name) <= 63",message="metadata.name must be no more than 63 characters (used as a pod label value)" - +kubebuilder:printcolumn:name="Policy",type=string,JSONPath=`.spec.policyRef.name`,priority=1 - +kubebuilder:printcolumn:name="Message",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].message`,priority=1 - +kubebuilder:printcolumn:name="Set",type=string,JSONPath=`.metadata.labels.drop\.corewire\.io/imageset`,description="Parent CachedImageSet",priority=1 @@ -222,6 +223,7 @@ crds: required: false doc: Conditions represent the latest available observations. markers: + - +kubebuilder:validation:XValidation:rule="size(self.metadata.name) <= 63",message="metadata.name must be no more than 63 characters (used as a label value on child CachedImages)" - +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp` - +kubebuilder:printcolumn:name="Message",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].message`,priority=1 - +kubebuilder:printcolumn:name="Source",type=string,JSONPath=`.spec.discoveryPolicyRef.name` diff --git a/test/e2e/README.md b/test/e2e/README.md index 8ff67e2..53970d3 100644 --- a/test/e2e/README.md +++ b/test/e2e/README.md @@ -24,6 +24,7 @@ make test-e2e | `cachedimageset/` | CachedImageSet managing child resources | | `cachedimageset-deletion-foreground/` | Foreground-propagation deletion completes, and short-name collisions across registries get distinct children | | `cachedimageset-discovery/` | CachedImageSet backed by a DiscoveryPolicy | +| `cachedimageset-long-names/` | Child names fit the 63-char label limit; over-long CachedImage/CachedImageSet names are rejected | | `discovery/` | DiscoveryPolicy with mock Prometheus | | `discovery-failure/` | DiscoveryPolicy with unreachable Prometheus endpoint | | `discovery-prometheus-basepath/` | DiscoveryPolicy against a Prometheus endpoint with a Mimir-style `/prometheus` base path (via nginx proxy) | diff --git a/test/e2e/cachedimageset-long-names/01-cachedimageset.yaml b/test/e2e/cachedimageset-long-names/01-cachedimageset.yaml new file mode 100644 index 0000000..06389b0 --- /dev/null +++ b/test/e2e/cachedimageset-long-names/01-cachedimageset.yaml @@ -0,0 +1,11 @@ +apiVersion: drop.corewire.io/v1alpha1 +kind: CachedImageSet +metadata: + name: test-set-long-names +spec: + images: + # Image short name + tag exceed the 63-char label limit on their own, so the + # child name must be truncated. The registry is unreachable on purpose: a + # pull failure proves the pull Pod was accepted by the API server. + - image: registry.invalid.local:9999/gitlab-org/gitlab-runner/gitlab-runner-helper-with-a-very-long-name + tag: x86_64-v19.3.3 diff --git a/test/e2e/cachedimageset-long-names/02-assert-child.yaml b/test/e2e/cachedimageset-long-names/02-assert-child.yaml new file mode 100644 index 0000000..881ec29 --- /dev/null +++ b/test/e2e/cachedimageset-long-names/02-assert-child.yaml @@ -0,0 +1,12 @@ +# Degraded is only reachable once a pull Pod was created and failed. If the Pod +# is rejected (label value > 63 chars) the reconcile errors before any status. +apiVersion: drop.corewire.io/v1alpha1 +kind: CachedImage +metadata: + labels: + drop.corewire.io/imageset: test-set-long-names + (length(name) <= `63`): true +status: + phase: Degraded + (conditions[?type == 'Ready']): + - status: "False" diff --git a/test/e2e/cachedimageset-long-names/03-cachedimage-name-too-long.yaml b/test/e2e/cachedimageset-long-names/03-cachedimage-name-too-long.yaml new file mode 100644 index 0000000..b0e561b --- /dev/null +++ b/test/e2e/cachedimageset-long-names/03-cachedimage-name-too-long.yaml @@ -0,0 +1,8 @@ +# 64 characters. +apiVersion: drop.corewire.io/v1alpha1 +kind: CachedImage +metadata: + name: test-cachedimage-name-too-long-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +spec: + image: docker.io/library/busybox + tag: "1.36" diff --git a/test/e2e/cachedimageset-long-names/04-cachedimageset-name-too-long.yaml b/test/e2e/cachedimageset-long-names/04-cachedimageset-name-too-long.yaml new file mode 100644 index 0000000..a57207a --- /dev/null +++ b/test/e2e/cachedimageset-long-names/04-cachedimageset-name-too-long.yaml @@ -0,0 +1,9 @@ +# 64 characters. +apiVersion: drop.corewire.io/v1alpha1 +kind: CachedImageSet +metadata: + name: test-cachedimageset-name-too-long-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +spec: + images: + - image: docker.io/library/busybox + tag: "1.36" diff --git a/test/e2e/cachedimageset-long-names/chainsaw-test.yaml b/test/e2e/cachedimageset-long-names/chainsaw-test.yaml new file mode 100644 index 0000000..54f3dc3 --- /dev/null +++ b/test/e2e/cachedimageset-long-names/chainsaw-test.yaml @@ -0,0 +1,39 @@ +# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: cachedimageset-long-names +spec: + description: | + CachedImage names are used as pull Pod label values, which Kubernetes caps + at 63 characters. Verify that a set with a long image name gets a child + whose pull Pods are accepted, and that the API rejects over-long names. + steps: + - name: Create a CachedImageSet with a long image name + try: + - apply: + file: 01-cachedimageset.yaml + - name: Child name fits and its pull Pod was created + try: + - assert: + timeout: 120s + file: 02-assert-child.yaml + - name: API rejects over-long names + try: + - apply: + file: 03-cachedimage-name-too-long.yaml + expect: + - check: + ($error != null): true + - apply: + file: 04-cachedimageset-name-too-long.yaml + expect: + - check: + ($error != null): true + - name: Cleanup + try: + - delete: + ref: + apiVersion: drop.corewire.io/v1alpha1 + kind: CachedImageSet + name: test-set-long-names