From abbbefafaf33c059e9ccfbb4bff84c33681c0e11 Mon Sep 17 00:00:00 2001 From: Matheus Pimenta Date: Sun, 4 Oct 2026 16:02:33 +0100 Subject: [PATCH] Introduce namespace management in ArtifactGenerator Signed-off-by: Matheus Pimenta --- api/v1beta1/artifactgenerator_types.go | 128 +++- api/v1beta1/artifactgenerator_types_test.go | 68 ++ api/v1beta1/zz_generated.deepcopy.go | 35 +- cmd/main.go | 2 +- ...tensions.fluxcd.io_artifactgenerators.yaml | 93 ++- config/rbac/role.yaml | 12 + docs/README.md | 2 +- docs/spec/v1beta1/artifactgenerators.md | 154 +++- .../artifactgenerator_controller.go | 189 ++++- .../artifactgenerator_controller_test.go | 96 +++ .../controller/artifactgenerator_drift.go | 36 +- .../artifactgenerator_drift_test.go | 8 +- .../controller/artifactgenerator_finalize.go | 102 ++- .../artifactgenerator_finalize_test.go | 57 ++ .../artifactgenerator_inventory_test.go | 51 ++ .../controller/artifactgenerator_namespace.go | 269 +++++++ .../artifactgenerator_namespace_test.go | 708 +++++++++++++++++- .../artifactgenerator_pathpattern.go | 39 +- .../artifactgenerator_pathpattern_test.go | 133 ++++ .../artifactgenerator_validation.go | 26 +- .../artifactgenerator_validation_test.go | 88 +++ 21 files changed, 2122 insertions(+), 174 deletions(-) create mode 100644 internal/controller/artifactgenerator_inventory_test.go create mode 100644 internal/controller/artifactgenerator_namespace.go diff --git a/api/v1beta1/artifactgenerator_types.go b/api/v1beta1/artifactgenerator_types.go index faaaedf8..55f6e5a8 100644 --- a/api/v1beta1/artifactgenerator_types.go +++ b/api/v1beta1/artifactgenerator_types.go @@ -41,6 +41,10 @@ const ( ExtractStrategy = "Extract" EnabledValue = "enabled" DisabledValue = "disabled" + NamespaceStrategyUnmanaged = "Unmanaged" + NamespaceStrategyManaged = "Managed" + NamespaceKind = "Namespace" + NamespaceAdoptedReason = "NamespaceAdopted" ) // CommonMetadata defines the common labels and annotations. @@ -55,7 +59,8 @@ type CommonMetadata struct { } // ArtifactGeneratorSpec defines the desired state of ArtifactGenerator. -// +kubebuilder:validation:XValidation:rule="has(self.pathPattern) && size(self.pathPattern) > 0 || self.artifacts.all(a, a.name.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$'))",message="artifact names must be valid Kubernetes object names when pathPattern is not set" +// +kubebuilder:validation:XValidation:rule="has(self.pathPattern) && size(self.pathPattern) > 0 || self.artifacts.all(a, a.name.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$') && a.name.size() <= 253)",message="artifact names must be valid Kubernetes object names when pathPattern is not set" +// +kubebuilder:validation:XValidation:rule="has(self.pathPattern) && size(self.pathPattern) > 0 || self.artifacts.all(a, !has(a.__namespace__) || (a.__namespace__.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?$') && a.__namespace__.size() <= 63))",message="artifact namespaces must be valid Kubernetes namespaces when pathPattern is not set" type ArtifactGeneratorSpec struct { // CommonMetadata specifies the common labels and annotations that are // applied to all resources. Any existing label or annotation will be @@ -71,15 +76,16 @@ type ArtifactGeneratorSpec struct { Sources []SourceReference `json:"sources"` // ServiceAccountName is the name of the ServiceAccount used to reconcile - // the generated ExternalArtifacts that target a namespace other than the - // ArtifactGenerator namespace. The ServiceAccount must exist in the - // ArtifactGenerator namespace. When specified, the controller impersonates - // this ServiceAccount for those ExternalArtifacts, and its RBAC bindings - // determine the namespaces in which they can be created, updated and - // deleted. ExternalArtifacts in the ArtifactGenerator namespace are always - // reconciled with the controller credentials. - // When not specified, the controller uses its own credentials, or the - // default ServiceAccount configured by the cluster administrator. + // the generated ExternalArtifacts and the managed Namespaces. The + // ServiceAccount must exist in the ArtifactGenerator namespace. When + // specified, the controller impersonates this ServiceAccount for all + // generated ExternalArtifacts, including those in the ArtifactGenerator + // namespace, and its RBAC bindings determine the namespaces in which they + // can be created, updated and deleted. + // When not specified, the controller uses its own credentials for + // ExternalArtifacts in the ArtifactGenerator namespace, and the default + // ServiceAccount configured by the cluster administrator (when set) for + // artifacts targeting another namespace. // +kubebuilder:validation:Pattern="^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=63 @@ -94,6 +100,11 @@ type ArtifactGeneratorSpec struct { // +optional PathPattern string `json:"pathPattern,omitempty"` + // Namespaces defines how the controller manages the namespaces + // targeted by the generated artifacts. + // +optional + Namespaces *Namespaces `json:"namespaces,omitempty"` + // OutputArtifacts is a list of output artifacts to be generated. // +kubebuilder:validation:MinItems=1 // +kubebuilder:validation:MaxItems=1000 @@ -101,6 +112,26 @@ type ArtifactGeneratorSpec struct { OutputArtifacts []OutputArtifact `json:"artifacts"` } +// Namespaces defines how the controller manages the namespaces targeted by +// the generated artifacts. +type Namespaces struct { + // Strategy specifies the namespace management strategy. + // 'Unmanaged' leaves the target namespaces untouched, they must exist and + // are not modified by the controller. + // 'Managed' makes the controller the manager of the target namespaces: it + // creates them when missing and applies the common metadata to them. + // When .spec.namespaces is omitted, namespaces are unmanaged. + // +kubebuilder:validation:Enum=Unmanaged;Managed + // +required + Strategy string `json:"strategy"` + + // Prune specifies whether the controller deletes managed namespaces that + // are no longer targeted by any generated artifact, or when the + // ArtifactGenerator is deleted. Defaults to true. + // +optional + Prune *bool `json:"prune,omitempty"` +} + // SourceReference contains the reference to a Flux source-controller resource. type SourceReference struct { // Alias of the source within the ArtifactGenerator context. @@ -137,18 +168,20 @@ type SourceReference struct { type OutputArtifact struct { // Name is the name of the generated artifact. // When pathPattern is set, this field may use capture placeholders such as "{app}". - // +kubebuilder:validation:MinLength=1 - // +kubebuilder:validation:MaxLength=253 + // The maximum length accommodates capture placeholders; the effective + // limits are enforced by the CEL validation when pathPattern is not set. + // +kubebuilder:validation:MaxLength=1024 // +required Name string `json:"name"` // Namespace is the namespace of the generated artifact. // If not provided, defaults to the same namespace as the ArtifactGenerator. + // When pathPattern is set, this field may use capture placeholders such as "{app}". // When set to a different namespace, the controller reconciles the artifact // with the credentials of .spec.serviceAccountName or the controller default. - // +kubebuilder:validation:Pattern="^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" - // +kubebuilder:validation:MinLength=1 - // +kubebuilder:validation:MaxLength=63 + // The maximum length accommodates capture placeholders; the effective + // limits are enforced by the CEL validation when pathPattern is not set. + // +kubebuilder:validation:MaxLength=1024 // +optional Namespace string `json:"namespace,omitempty"` @@ -236,9 +269,10 @@ type ArtifactGeneratorStatus struct { // +optional Conditions []metav1.Condition `json:"conditions,omitempty"` - // Inventory contains the list of generated ExternalArtifact references. + // Inventory contains the list of objects managed by the ArtifactGenerator, + // such as the generated ExternalArtifacts and the managed Namespaces. // +optional - Inventory []ExternalArtifactReference `json:"inventory,omitempty"` + Inventory []InventoryEntry `json:"inventory,omitempty"` // ObservedSourcesDigest is a hash representing the current state of // all the sources referenced by the ArtifactGenerator. @@ -246,24 +280,32 @@ type ArtifactGeneratorStatus struct { ObservedSourcesDigest string `json:"observedSourcesDigest,omitempty"` } -// ExternalArtifactReference contains the reference to a -// generated ExternalArtifact along with its digest. -type ExternalArtifactReference struct { - // Name of the referent artifact. +// InventoryEntry contains a reference to an object managed by the +// ArtifactGenerator, such as a generated ExternalArtifact or a managed +// Namespace. +type InventoryEntry struct { + // Kind is the kind of the referent object. + // +kubebuilder:validation:Enum=ExternalArtifact;Namespace + // +optional + Kind string `json:"kind,omitempty"` + + // Name of the referent object. // +required Name string `json:"name"` - // Namespace of the referent artifact. - // +required - Namespace string `json:"namespace"` + // Namespace of the referent object. Empty for cluster-scoped objects. + // +optional + Namespace string `json:"namespace,omitempty"` - // Digest of the referent artifact. + // Digest of the referent object. For generated artifacts this is the + // artifact content digest; for managed namespaces this is a digest of the + // metadata applied by the controller. // +required Digest string `json:"digest"` // Filename is the name of the artifact file. - // +required - Filename string `json:"filename"` + // +optional + Filename string `json:"filename,omitempty"` } // GetConditions returns the status conditions of the object. @@ -303,10 +345,29 @@ func (in *ArtifactGenerator) GetArtifactNamespace(outputArtifact *OutputArtifact return in.Namespace } +// ManagesNamespaces returns true when the controller manages the target +// namespaces of the generated artifacts. +func (in *ArtifactGenerator) ManagesNamespaces() bool { + return in.Spec.Namespaces != nil && in.Spec.Namespaces.Strategy == NamespaceStrategyManaged +} + +// NamespacePrune returns whether the controller prunes managed namespaces +// that are no longer targeted by any generated artifact, or when the +// ArtifactGenerator is deleted. It defaults to true. +func (in *ArtifactGenerator) NamespacePrune() bool { + if in.Spec.Namespaces == nil || in.Spec.Namespaces.Prune == nil { + return true + } + return *in.Spec.Namespaces.Prune +} + // HasArtifactInInventory returns true if the artifact with the given -// kind, name, namespace, and digest exists in the inventory. +// name, namespace, and digest exists in the inventory. func (in *ArtifactGenerator) HasArtifactInInventory(name, namespace, digest string) bool { for _, ref := range in.Status.Inventory { + if ref.Kind == NamespaceKind { + continue + } if ref.Name == name && ref.Namespace == namespace && ref.Digest == digest { return true } @@ -314,6 +375,17 @@ func (in *ArtifactGenerator) HasArtifactInInventory(name, namespace, digest stri return false } +// HasNamespaceInInventory returns true if the namespace with the given +// name and metadata digest exists in the inventory. +func (in *ArtifactGenerator) HasNamespaceInInventory(name, digest string) bool { + for _, ref := range in.Status.Inventory { + if ref.Kind == NamespaceKind && ref.Name == name && ref.Digest == digest { + return true + } + } + return false +} + // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:shortName=ag,categories=all;fluxcd;fluxcd-sources diff --git a/api/v1beta1/artifactgenerator_types_test.go b/api/v1beta1/artifactgenerator_types_test.go index 434e8f2e..3d86638c 100644 --- a/api/v1beta1/artifactgenerator_types_test.go +++ b/api/v1beta1/artifactgenerator_types_test.go @@ -34,3 +34,71 @@ func TestArtifactGeneratorGetArtifactNamespace(t *testing.T) { t.Errorf("GetArtifactNamespace() = %q, want %q", got, "target-ns") } } + +func TestArtifactGeneratorNamespaces(t *testing.T) { + obj := &v1beta1.ArtifactGenerator{} + + if obj.ManagesNamespaces() { + t.Error("ManagesNamespaces() = true, want false when namespaces is not set") + } + if !obj.NamespacePrune() { + t.Error("NamespacePrune() = false, want true when namespaces is not set") + } + + obj.Spec.Namespaces = &v1beta1.Namespaces{Strategy: v1beta1.NamespaceStrategyUnmanaged} + if obj.ManagesNamespaces() { + t.Error("ManagesNamespaces() = true, want false for Unmanaged") + } + if !obj.NamespacePrune() { + t.Error("NamespacePrune() = false, want true when prune is not set") + } + + prune := false + obj.Spec.Namespaces = &v1beta1.Namespaces{ + Strategy: v1beta1.NamespaceStrategyManaged, + Prune: &prune, + } + if !obj.ManagesNamespaces() { + t.Error("ManagesNamespaces() = false, want true for Managed") + } + if obj.NamespacePrune() { + t.Error("NamespacePrune() = true, want false when prune is false") + } +} + +func TestArtifactGeneratorInventoryHelpers(t *testing.T) { + obj := &v1beta1.ArtifactGenerator{ + Status: v1beta1.ArtifactGeneratorStatus{ + Inventory: []v1beta1.InventoryEntry{ + { + Kind: "ExternalArtifact", + Name: "app", + Namespace: "tenant", + Digest: "sha256:abc", + Filename: "app.tar.gz", + }, + { + Kind: v1beta1.NamespaceKind, + Name: "tenant", + Digest: "sha256:def", + }, + }, + }, + } + + if !obj.HasArtifactInInventory("app", "tenant", "sha256:abc") { + t.Error("HasArtifactInInventory() = false, want true") + } + if obj.HasArtifactInInventory("tenant", "", "") { + t.Error("HasArtifactInInventory() matched a namespace entry") + } + if !obj.HasNamespaceInInventory("tenant", "sha256:def") { + t.Error("HasNamespaceInInventory() = false, want true") + } + if obj.HasNamespaceInInventory("tenant", "sha256:other") { + t.Error("HasNamespaceInInventory() = true for a different digest") + } + if obj.HasNamespaceInInventory("app", "sha256:def") { + t.Error("HasNamespaceInInventory() = true for an artifact entry") + } +} diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index d595dc17..2f07f462 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -97,6 +97,11 @@ func (in *ArtifactGeneratorSpec) DeepCopyInto(out *ArtifactGeneratorSpec) { *out = make([]SourceReference, len(*in)) copy(*out, *in) } + if in.Namespaces != nil { + in, out := &in.Namespaces, &out.Namespaces + *out = new(Namespaces) + (*in).DeepCopyInto(*out) + } if in.OutputArtifacts != nil { in, out := &in.OutputArtifacts, &out.OutputArtifacts *out = make([]OutputArtifact, len(*in)) @@ -129,7 +134,7 @@ func (in *ArtifactGeneratorStatus) DeepCopyInto(out *ArtifactGeneratorStatus) { } if in.Inventory != nil { in, out := &in.Inventory, &out.Inventory - *out = make([]ExternalArtifactReference, len(*in)) + *out = make([]InventoryEntry, len(*in)) copy(*out, *in) } } @@ -194,16 +199,36 @@ func (in *CopyOperation) DeepCopy() *CopyOperation { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ExternalArtifactReference) DeepCopyInto(out *ExternalArtifactReference) { +func (in *InventoryEntry) DeepCopyInto(out *InventoryEntry) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new InventoryEntry. +func (in *InventoryEntry) DeepCopy() *InventoryEntry { + if in == nil { + return nil + } + out := new(InventoryEntry) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Namespaces) DeepCopyInto(out *Namespaces) { *out = *in + if in.Prune != nil { + in, out := &in.Prune, &out.Prune + *out = new(bool) + **out = **in + } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ExternalArtifactReference. -func (in *ExternalArtifactReference) DeepCopy() *ExternalArtifactReference { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Namespaces. +func (in *Namespaces) DeepCopy() *Namespaces { if in == nil { return nil } - out := new(ExternalArtifactReference) + out := new(Namespaces) in.DeepCopyInto(out) return out } diff --git a/cmd/main.go b/cmd/main.go index bb801155..01141726 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -107,7 +107,7 @@ func main() { flag.DurationVar(&requeueDependency, "requeue-dependency", 5*time.Second, "The interval at which failing dependencies are reevaluated.") flag.StringVar(&defaultServiceAccount, "default-service-account", "", - "The default service account used for impersonation.") + "The default service account used to reconcile cross-namespace artifacts and managed namespaces.") aclOptions.BindFlags(flag.CommandLine) artifactOptions.BindFlags(flag.CommandLine) diff --git a/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml b/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml index 67dc28b4..0ea6f07a 100644 --- a/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml +++ b/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml @@ -132,18 +132,20 @@ spec: description: |- Name is the name of the generated artifact. When pathPattern is set, this field may use capture placeholders such as "{app}". - maxLength: 253 - minLength: 1 + The maximum length accommodates capture placeholders; the effective + limits are enforced by the CEL validation when pathPattern is not set. + maxLength: 1024 type: string namespace: description: |- Namespace is the namespace of the generated artifact. If not provided, defaults to the same namespace as the ArtifactGenerator. + When pathPattern is set, this field may use capture placeholders such as "{app}". When set to a different namespace, the controller reconciles the artifact with the credentials of .spec.serviceAccountName or the controller default. - maxLength: 63 - minLength: 1 - pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + The maximum length accommodates capture placeholders; the effective + limits are enforced by the CEL validation when pathPattern is not set. + maxLength: 1024 type: string originRevision: description: |- @@ -188,6 +190,32 @@ spec: description: Labels to be added to the object's metadata. type: object type: object + namespaces: + description: |- + Namespaces defines how the controller manages the namespaces + targeted by the generated artifacts. + properties: + prune: + description: |- + Prune specifies whether the controller deletes managed namespaces that + are no longer targeted by any generated artifact, or when the + ArtifactGenerator is deleted. Defaults to true. + type: boolean + strategy: + description: |- + Strategy specifies the namespace management strategy. + 'Unmanaged' leaves the target namespaces untouched, they must exist and + are not modified by the controller. + 'Managed' makes the controller the manager of the target namespaces: it + creates them when missing and applies the common metadata to them. + When .spec.namespaces is omitted, namespaces are unmanaged. + enum: + - Unmanaged + - Managed + type: string + required: + - strategy + type: object pathPattern: description: |- PathPattern specifies a directory traversal pattern to match within the sources. @@ -199,15 +227,16 @@ spec: serviceAccountName: description: |- ServiceAccountName is the name of the ServiceAccount used to reconcile - the generated ExternalArtifacts that target a namespace other than the - ArtifactGenerator namespace. The ServiceAccount must exist in the - ArtifactGenerator namespace. When specified, the controller impersonates - this ServiceAccount for those ExternalArtifacts, and its RBAC bindings - determine the namespaces in which they can be created, updated and - deleted. ExternalArtifacts in the ArtifactGenerator namespace are always - reconciled with the controller credentials. - When not specified, the controller uses its own credentials, or the - default ServiceAccount configured by the cluster administrator. + the generated ExternalArtifacts and the managed Namespaces. The + ServiceAccount must exist in the ArtifactGenerator namespace. When + specified, the controller impersonates this ServiceAccount for all + generated ExternalArtifacts, including those in the ArtifactGenerator + namespace, and its RBAC bindings determine the namespaces in which they + can be created, updated and deleted. + When not specified, the controller uses its own credentials for + ExternalArtifacts in the ArtifactGenerator namespace, and the default + ServiceAccount configured by the cluster administrator (when set) for + artifacts targeting another namespace. maxLength: 63 minLength: 1 pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ @@ -267,7 +296,13 @@ spec: - message: artifact names must be valid Kubernetes object names when pathPattern is not set rule: has(self.pathPattern) && size(self.pathPattern) > 0 || self.artifacts.all(a, - a.name.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$')) + a.name.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$') + && a.name.size() <= 253) + - message: artifact namespaces must be valid Kubernetes namespaces when + pathPattern is not set + rule: has(self.pathPattern) && size(self.pathPattern) > 0 || self.artifacts.all(a, + !has(a.__namespace__) || (a.__namespace__.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?$') + && a.__namespace__.size() <= 63)) status: description: ArtifactGeneratorStatus defines the observed state of ArtifactGenerator. properties: @@ -329,30 +364,40 @@ spec: type: object type: array inventory: - description: Inventory contains the list of generated ExternalArtifact - references. + description: |- + Inventory contains the list of objects managed by the ArtifactGenerator, + such as the generated ExternalArtifacts and the managed Namespaces. items: description: |- - ExternalArtifactReference contains the reference to a - generated ExternalArtifact along with its digest. + InventoryEntry contains a reference to an object managed by the + ArtifactGenerator, such as a generated ExternalArtifact or a managed + Namespace. properties: digest: - description: Digest of the referent artifact. + description: |- + Digest of the referent object. For generated artifacts this is the + artifact content digest; for managed namespaces this is a digest of the + metadata applied by the controller. type: string filename: description: Filename is the name of the artifact file. type: string + kind: + description: Kind is the kind of the referent object. + enum: + - ExternalArtifact + - Namespace + type: string name: - description: Name of the referent artifact. + description: Name of the referent object. type: string namespace: - description: Namespace of the referent artifact. + description: Namespace of the referent object. Empty for cluster-scoped + objects. type: string required: - digest - - filename - name - - namespace type: object type: array lastHandledReconcileAt: diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 1bbd6e71..e6f1cbeb 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -4,6 +4,18 @@ kind: ClusterRole metadata: name: manager-role rules: +- apiGroups: + - "" + resources: + - namespaces + verbs: + - create + - delete + - get + - list + - patch + - update + - watch - apiGroups: - source.extensions.fluxcd.io resources: diff --git a/docs/README.md b/docs/README.md index 5b61fc18..a2e935d4 100644 --- a/docs/README.md +++ b/docs/README.md @@ -14,7 +14,7 @@ with advanced source composition and decomposition patterns. | `--artifact-retention-records` | int | The maximum number of artifacts to be kept in storage after a garbage collection. (default 2) | | `--artifact-retention-ttl` | duration | The duration of time that artifacts from previous reconciliations will be kept in storage before being garbage collected. (default 1m0s) | | `--concurrent` | int | The number of concurrent reconciles per controller. (default 10) | -| `--default-service-account` | string | The default service account used for impersonation when `.spec.serviceAccountName` is not specified. | +| `--default-service-account` | string | The default service account used to reconcile cross-namespace artifacts and managed namespaces when `.spec.serviceAccountName` is not specified. | | `--enable-leader-election` | boolean | Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager. | | `--events-addr` | string | The address of the events receiver. | | `--health-addr` | string | The address the health endpoint binds to. (default ":9440") | diff --git a/docs/spec/v1beta1/artifactgenerators.md b/docs/spec/v1beta1/artifactgenerators.md index 6f3d1ec4..aa0f0943 100644 --- a/docs/spec/v1beta1/artifactgenerators.md +++ b/docs/spec/v1beta1/artifactgenerators.md @@ -285,8 +285,8 @@ Each artifact must specify: - `name` (required): The name of the generated ExternalArtifact resource. It must be unique in the context of the ArtifactGenerator and must conform to Kubernetes resource naming conventions. Supports capture placeholders if `pathPattern` is used. - `namespace` (optional): The namespace where the generated ExternalArtifact is created. - If not specified, it defaults to the ArtifactGenerator namespace. See - [Cross-namespace Artifacts](#cross-namespace-artifacts). + If not specified, it defaults to the ArtifactGenerator namespace. Supports capture placeholders + if `pathPattern` is used. See [Cross-namespace Artifacts](#cross-namespace-artifacts). - `copy` (required): A list of copy operations to perform from sources to the artifact. - `revision` (optional): A specific source revision to use in the format `@alias`. If not specified, the revision is automatically computed as `latest@` based on the artifact content. @@ -295,6 +295,10 @@ Each artifact must specify: the original source revision of the artifact (e.g. the monorepo commit SHA) without affecting the artifact revision itself. +When `pathPattern` is not set, `name` and `namespace` are validated as a Kubernetes object name +and namespace respectively. When `pathPattern` is set, these fields are treated as templates and +the rendered values are validated instead. + ```yaml spec: artifacts: @@ -450,17 +454,17 @@ ExternalArtifact in a different namespace. This is useful for multi-tenant clust where the sources and the ArtifactGenerator run in a shared namespace, while the generated artifacts are consumed by tenants in their own namespaces. -The controller uses the ServiceAccount credentials only for artifacts whose -`.namespace` is set to a namespace different from the ArtifactGenerator namespace. -For artifacts in the ArtifactGenerator namespace (the default when `.namespace` is not -set), the controller always uses its own credentials, even when a ServiceAccount is -configured. This keeps the behavior of existing ArtifactGenerators unchanged. +When `.spec.serviceAccountName` is set, the controller impersonates that +ServiceAccount for every generated ExternalArtifact, including the ones created +in the ArtifactGenerator namespace. The ServiceAccount must exist in the +ArtifactGenerator namespace, and its RBAC bindings determine which namespaces it +can access. -For artifacts targeting another namespace, the controller impersonates the ServiceAccount -configured in `.spec.serviceAccountName`. The ServiceAccount must exist in the -ArtifactGenerator namespace, and its RBAC bindings determine which namespaces it can -access. When `.spec.serviceAccountName` is not specified, the controller uses its own -credentials. +When `.spec.serviceAccountName` is not specified, the controller uses its own +credentials for artifacts in the ArtifactGenerator namespace (the default when +`.namespace` is not set). For artifacts targeting another namespace, it uses the +default ServiceAccount configured by the cluster administrator (see below), or +its own credentials when no default is configured. For example, the following generator creates an ExternalArtifact in the `tenant-app` namespace, using the `tenant-artifacts` ServiceAccount: @@ -487,9 +491,108 @@ spec: **Note** that on multi-tenant clusters, platform admins should configure a default ServiceAccount for impersonation by starting the controller with the -`--default-service-account=` flag. It is used whenever `.spec.serviceAccountName` -is not specified, and, like `.spec.serviceAccountName`, it only applies to artifacts -targeting a namespace different from the ArtifactGenerator namespace. +`--default-service-account=` flag. It is used whenever +`.spec.serviceAccountName` is not specified, and only applies to artifacts +targeting a namespace different from the ArtifactGenerator namespace and to the +managed namespaces those artifacts target. Artifacts in the ArtifactGenerator +namespace keep using the controller credentials unless `.spec.serviceAccountName` +is set, so enabling the default does not change how in-namespace artifacts are +reconciled. Set `.spec.serviceAccountName` when an ArtifactGenerator must be +reconciled entirely with a specific ServiceAccount. + +When `pathPattern` is set, `.spec.artifacts[].namespace` may use capture placeholders, +so each matched directory can be published to a namespace derived from the captured +values. For example, the following generator decomposes a monorepo into one +ExternalArtifact per tenant namespace: + +```yaml +apiVersion: source.extensions.fluxcd.io/v1beta1 +kind: ArtifactGenerator +metadata: + name: tenants + namespace: flux-system +spec: + serviceAccountName: tenant-artifacts + sources: + - alias: repo + kind: GitRepository + name: my-monorepo + pathPattern: "@repo/tenants/{tenant}/apps/{app}" + artifacts: + - name: "{app}" + namespace: "{tenant}" + copy: + - from: "@repo/tenants/{tenant}/apps/{app}/**" + to: "@artifact/" +``` + +The captured directory names are lowercased before being used as the artifact +name and namespace, so a directory named `Tenant-A` is published to the +`tenant-a` namespace. The rendered namespace must be a valid Kubernetes +namespace; otherwise the reconciliation fails with a terminal error. + +### Namespace Management + +By default, the controller does not manage the namespaces targeted by the +generated artifacts: they must already exist and are left untouched. The +`.spec.namespaces` field can be used to make the controller the manager +of those namespaces: + +- `.spec.namespaces.strategy` (required when `.spec.namespaces` is set): + `Unmanaged` or `Managed`. When `.spec.namespaces` is omitted, the target + namespaces are unmanaged. +- `.spec.namespaces.prune` (optional): whether the controller deletes + managed namespaces that are no longer targeted by any generated artifact, or + when the ArtifactGenerator is deleted. Defaults to `true`. + +When set to `Managed`, the controller creates the namespaces that do not exist +and applies `.spec.commonMetadata` to all of them, along with the +`app.kubernetes.io/managed-by` and `source.extensions.fluxcd.io/generator` +labels. Namespaces that already exist are adopted, and namespaces managed by +another ArtifactGenerator are taken over, in both cases a warning event is +emitted. Combined with `pathPattern`, this allows namespaces to be created and +removed dynamically as directories are added or removed from the source. + +```yaml +apiVersion: source.extensions.fluxcd.io/v1beta1 +kind: ArtifactGenerator +metadata: + name: tenants + namespace: flux-system +spec: + namespaces: + strategy: Managed + prune: true + commonMetadata: + labels: + app.kubernetes.io/part-of: tenants + sources: + - alias: repo + kind: GitRepository + name: my-monorepo + pathPattern: "@repo/tenants/{tenant}" + artifacts: + - name: "{tenant}" + namespace: "{tenant}" + copy: + - from: "@repo/tenants/{tenant}/**" + to: "@artifact/" +``` + +**Note** that pruning deletes the entire namespace and everything in it, and +cannot be undone. Managed namespaces are tracked in `.status.inventory`, so set +`.spec.namespaces.prune` to `false` if the namespaces may contain +resources that must be preserved. If a delete request is rejected by the API +server, the namespace is kept in the inventory and the deletion is retried on +the next reconciliation. + +Namespace management uses the same credentials as artifact generation. When +`.spec.serviceAccountName` is set, the controller impersonates it. Otherwise, +when `--default-service-account` is set, the controller impersonates the default +for the namespaces targeted by cross-namespace artifacts; otherwise it uses its +own credentials. Creating and deleting namespaces requires cluster-scoped RBAC, +so the ServiceAccount must be bound to a `ClusterRole` granting the `namespaces` +verbs. ## Working with ArtifactGenerators @@ -600,9 +703,19 @@ When the ArtifactGenerator is stalled, the controller sets the following conditi ### Inventory -The controller reports the list of generated ExternalArtifacts in the`.status.inventory` -field of the ArtifactGenerator. The inventory is used by the controller to keep track -of the artifacts in storage and to perform garbage collection of orphaned artifacts. +The controller reports the list of objects managed by the ArtifactGenerator in +the `.status.inventory` field. The inventory is used by the controller to keep +track of the generated ExternalArtifacts and the managed namespaces, and to +perform garbage collection of the ones that are no longer targeted. + +Each entry has a `kind`: + +- Generated artifacts have `kind: ExternalArtifact`, along with the `name`, + `namespace`, `digest` and `filename` of the referent. The `digest` is the + content digest of the artifact. +- Managed namespaces have `kind: Namespace`, with the namespace `name` and a + `digest` computed from the metadata applied by the controller. The + `namespace` and `filename` fields are empty as namespaces are cluster-scoped. ## ArtifactGenerator Events @@ -614,11 +727,12 @@ Events are emitted for the following scenarios: - ArtifactGenerator reconciliation completion (success or failure). - ExternalArtifacts creation, update, or deletion. +- Namespaces creation, update, adoption, takeover, or deletion. - Source fetch failures or access issues. - Build failures (e.g. invalid glob patterns, missing files). - Storage operations (e.g. garbage collection, integrity validation failures). -- Drift detection (e.g. manual changes to generated ExternalArtifacts). -- Ownership conflicts (e.g. an ExternalArtifact generated by another ArtifactGenerator is taken over). +- Drift detection (e.g. manual changes to generated ExternalArtifacts or managed namespaces). +- Ownership conflicts (e.g. an ExternalArtifact or namespace managed by another ArtifactGenerator is taken over). All events are also logged to the controller's standard output and contain the ArtifactGenerator name and namespace. diff --git a/internal/controller/artifactgenerator_controller.go b/internal/controller/artifactgenerator_controller.go index 2c19f635..12f902a1 100644 --- a/internal/controller/artifactgenerator_controller.go +++ b/internal/controller/artifactgenerator_controller.go @@ -71,6 +71,7 @@ type ArtifactGeneratorReconciler struct { // +kubebuilder:rbac:groups=source.extensions.fluxcd.io,resources=artifactgenerators/status,verbs=get;update;patchStatus // +kubebuilder:rbac:groups=source.extensions.fluxcd.io,resources=artifactgenerators/finalizers,verbs=update // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=*,verbs=get;list;watch;create;update;patchStatus;delete +// +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch;create;update;patch;delete // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -93,11 +94,10 @@ func (r *ArtifactGeneratorReconciler) Reconcile(ctx context.Context, req ctrl.Re } }() - // Build the impersonation client only when at least one ExternalArtifact - // targets a namespace other than the ArtifactGenerator namespace. Output - // artifacts in the ArtifactGenerator namespace are always reconciled with - // the controller client, so enabling impersonation does not change the - // behavior of existing ArtifactGenerators. + // Build the impersonation client only when there is work that requires it: + // an explicit .spec.serviceAccountName, an ExternalArtifact targeting a + // namespace other than the ArtifactGenerator namespace, or a managed + // namespace tracked in the inventory. var impersonated client.Client if r.needsImpersonation(obj) { var err error @@ -140,15 +140,36 @@ func (r *ArtifactGeneratorReconciler) Reconcile(ctx context.Context, req ctrl.Re return r.reconcile(ctx, obj, patcher, impersonated) } -// needsImpersonation returns true when the controller must impersonate the -// configured ServiceAccount to reconcile at least one ExternalArtifact, i.e. -// when a ServiceAccount is configured and an output artifact or an inventory -// reference targets a namespace other than the ArtifactGenerator namespace. +// needsImpersonation returns true when the controller must impersonate a +// ServiceAccount to reconcile the ArtifactGenerator. An explicit +// .spec.serviceAccountName is authoritative for every generated +// ExternalArtifact, including those in the ArtifactGenerator namespace, so it +// always requires impersonation. The default ServiceAccount is only used for +// ExternalArtifacts targeting another namespace and for managed namespaces, +// so it requires impersonation only when there is such work. func (r *ArtifactGeneratorReconciler) needsImpersonation(obj *swapi.ArtifactGenerator) bool { if r.DefaultServiceAccount == "" && obj.Spec.ServiceAccountName == "" { return false } + // An explicit ServiceAccount applies to every generated ExternalArtifact, + // including those in the ArtifactGenerator namespace. + if obj.Spec.ServiceAccountName != "" { + return true + } + + // Namespace management is cluster-scoped and only exercised for the + // namespaces tracked in the inventory or targeted by an output artifact + // outside the ArtifactGenerator namespace, so only impersonate when there + // is such work. + if obj.ManagesNamespaces() { + for _, ref := range obj.Status.Inventory { + if ref.Kind == swapi.NamespaceKind { + return true + } + } + } + for i := range obj.Spec.OutputArtifacts { if obj.GetArtifactNamespace(&obj.Spec.OutputArtifacts[i]) != obj.Namespace { return true @@ -156,6 +177,9 @@ func (r *ArtifactGeneratorReconciler) needsImpersonation(obj *swapi.ArtifactGene } for _, ref := range obj.Status.Inventory { + if ref.Kind == swapi.NamespaceKind { + continue + } if ref.Namespace != obj.Namespace { return true } @@ -181,11 +205,12 @@ func (r *ArtifactGeneratorReconciler) newImpersonatedClient(ctx context.Context, // clientForNamespace returns the client that must be used to reconcile // ExternalArtifacts in the given namespace. The impersonated client is used -// only when the target namespace differs from the ArtifactGenerator namespace; -// otherwise the controller client is returned. +// when an explicit .spec.serviceAccountName is set, or when the target +// namespace differs from the ArtifactGenerator namespace; otherwise the +// controller client is returned. func (r *ArtifactGeneratorReconciler) clientForNamespace(obj *swapi.ArtifactGenerator, impersonated client.Client, namespace string) client.Client { - if impersonated != nil && namespace != obj.Namespace { + if impersonated != nil && (obj.Spec.ServiceAccountName != "" || namespace != obj.Namespace) { return impersonated } return r.Client @@ -233,7 +258,18 @@ func (r *ArtifactGeneratorReconciler) reconcile(ctx context.Context, // integrity verification, the reconciliation is complete and we can exit early. hasDrifted, reason := r.detectDrift(ctx, obj, observedSourcesDigest, impersonated) if !hasDrifted { - msg := fmt.Sprintf("No drift detected, %d artifact(s) up to date", len(obj.Status.Inventory)) + artifactCount, namespaceCount := 0, 0 + for _, ref := range obj.Status.Inventory { + if ref.Kind == swapi.NamespaceKind { + namespaceCount++ + } else { + artifactCount++ + } + } + msg := fmt.Sprintf("No drift detected, %d artifact(s) up to date", artifactCount) + if namespaceCount > 0 { + msg = fmt.Sprintf("%s and %d namespace(s) up to date", msg, namespaceCount) + } log.Info(msg) r.Event(obj, eventv1.EventTypeTrace, gotkmeta.ReadyCondition, msg) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil @@ -289,8 +325,51 @@ func (r *ArtifactGeneratorReconciler) reconcile(ctx context.Context, return ctrl.Result{}, err } + // Reconcile the namespaces managed by the ArtifactGenerator before the + // ExternalArtifacts that target them. Only namespaces explicitly targeted + // by .spec.artifacts[].namespace are managed, the ArtifactGenerator + // namespace is never managed implicitly through the default. + var nsRefs []swapi.InventoryEntry + if obj.ManagesNamespaces() { + var nsConflicts []namespaceConflict + nsRefs, nsConflicts, err = r.reconcileNamespaces(ctx, obj, reqs, impersonated) + if err != nil { + msg := fmt.Sprintf("namespace reconciliation failed: %s", err.Error()) + gotkconditions.MarkFalse(obj, + gotkmeta.ReadyCondition, + gotkmeta.ReconciliationFailedReason, + "%s", msg) + r.Event(obj, corev1.EventTypeWarning, gotkmeta.ReconciliationFailedReason, msg) + return ctrl.Result{}, err + } + // Summarize the adopted and taken over namespaces once per + // reconciliation, mirroring the ExternalArtifact ownership conflicts. + if len(nsConflicts) > 0 { + adopted, taken := 0, 0 + for _, c := range nsConflicts { + if c.Adopted { + adopted++ + } else { + taken++ + } + } + if adopted > 0 { + log.Error(fmt.Errorf("adopted %d unmanaged namespace(s)", adopted), + "adopting namespaces not managed by any ArtifactGenerator") + r.Event(obj, corev1.EventTypeWarning, swapi.NamespaceAdoptedReason, + fmt.Sprintf("adopted %d unmanaged namespace(s)", adopted)) + } + if taken > 0 { + log.Error(fmt.Errorf("ownership conflict detected for %d Namespace(s)", taken), + "taking over namespaces from other ArtifactGenerators") + r.Event(obj, corev1.EventTypeWarning, swapi.OwnershipConflictReason, + fmt.Sprintf("ownership conflict detected for %d Namespace(s)", taken)) + } + } + } + // Prepare a slice to hold the references to the created ExternalArtifact objects. - eaRefs := make([]swapi.ExternalArtifactReference, 0, len(reqs)) + eaRefs := make([]swapi.InventoryEntry, 0, len(reqs)) // Collect ExternalArtifacts whose ownership is being taken over from // another ArtifactGenerator to emit a single error log and a single @@ -344,9 +423,22 @@ func (r *ArtifactGeneratorReconciler) reconcile(ctx context.Context, fmt.Sprintf("ownership conflict detected for %d ExternalArtifact(s)", len(ownershipConflicts))) } - // Garbage collect orphaned ExternalArtifacts and their associated artifacts in gotkstroage. - if orphans := r.findOrphanedReferences(obj.Status.Inventory, eaRefs); len(orphans) > 0 { - r.finalizeExternalArtifacts(ctx, obj, orphans, impersonated) + // Merge the artifact and namespace references into the inventory. + inventory := append(eaRefs, nsRefs...) + + // Garbage collect orphaned ExternalArtifacts and managed namespaces. + // Objects whose deletion was not confirmed are kept in the inventory so + // the next reconciliation retries their deletion. + if orphans := r.findOrphanedReferences(obj.Status.Inventory, inventory); len(orphans) > 0 { + survivors := r.finalizeReferences(ctx, obj, orphans, impersonated) + if len(survivors) > 0 { + inventory = append(inventory, survivors...) + obj.Status.Inventory = inventory + msg := fmt.Sprintf("failed to prune %d object(s), retrying", len(survivors)) + gotkconditions.MarkFalse(obj, gotkmeta.ReadyCondition, gotkmeta.PruneFailedReason, "%s", msg) + r.Event(obj, corev1.EventTypeWarning, gotkmeta.PruneFailedReason, msg) + return ctrl.Result{}, fmt.Errorf("failed to prune %d object(s)", len(survivors)) + } } // Garbage collect old artifacts in storage according to the retention policy. @@ -361,28 +453,39 @@ func (r *ArtifactGeneratorReconciler) reconcile(ctx context.Context, } // Update the status with to reflect the successful reconciliation. - obj.Status.Inventory = eaRefs + obj.Status.Inventory = inventory obj.Status.ObservedSourcesDigest = observedSourcesDigest msg := fmt.Sprintf("reconciliation succeeded, generated %d artifact(s)", len(eaRefs)) + if len(nsRefs) > 0 { + msg = fmt.Sprintf("%s and manages %d namespace(s)", msg, len(nsRefs)) + } gotkconditions.MarkTrue(obj, gotkmeta.ReadyCondition, gotkmeta.SucceededReason, "%s", msg) r.Event(obj, eventv1.EventTypeTrace, gotkmeta.ReadyCondition, msg) - r.notify(oldObj, obj, eaRefs) + r.notify(oldObj, obj, inventory) return ctrl.Result{RequeueAfter: gotkjitter.JitteredIntervalDuration(obj.GetRequeueAfter())}, nil } // notify emits notification related to the result of reconciliation. It will only send events if -// there is a least one external artifact update -func (r *ArtifactGeneratorReconciler) notify(oldObj, newObj *swapi.ArtifactGenerator, eaRefs []swapi.ExternalArtifactReference) { +// there is a least one external artifact or namespace update +func (r *ArtifactGeneratorReconciler) notify(oldObj, newObj *swapi.ArtifactGenerator, refs []swapi.InventoryEntry) { eaChanged := make([]string, 0) + nsChanged := make([]string, 0) - for _, eaRef := range eaRefs { - if !oldObj.HasArtifactInInventory(eaRef.Name, eaRef.Namespace, eaRef.Digest) { - eaChanged = append(eaChanged, fmt.Sprintf("%s/%s (%s)", eaRef.Namespace, eaRef.Name, eaRef.Digest)) + for _, ref := range refs { + switch ref.Kind { + case swapi.NamespaceKind: + if !oldObj.HasNamespaceInInventory(ref.Name, ref.Digest) { + nsChanged = append(nsChanged, fmt.Sprintf("%s (%s)", ref.Name, ref.Digest)) + } + default: + if !oldObj.HasArtifactInInventory(ref.Name, ref.Namespace, ref.Digest) { + eaChanged = append(eaChanged, fmt.Sprintf("%s/%s (%s)", ref.Namespace, ref.Name, ref.Digest)) + } } } @@ -390,6 +493,10 @@ func (r *ArtifactGeneratorReconciler) notify(oldObj, newObj *swapi.ArtifactGener msg := fmt.Sprintf("external artifacts reconciled: %s", strings.Join(eaChanged, "\n")) r.Event(newObj, corev1.EventTypeNormal, gotkmeta.ReadyCondition, msg) } + if len(nsChanged) > 0 { + msg := fmt.Sprintf("namespaces reconciled: %s", strings.Join(nsChanged, "\n")) + r.Event(newObj, corev1.EventTypeNormal, gotkmeta.ReadyCondition, msg) + } } // observeSources retrieves the current state of sources, @@ -538,7 +645,7 @@ func (r *ArtifactGeneratorReconciler) reconcileExternalArtifact(ctx context.Cont outputArtifact *swapi.OutputArtifact, artifact *gotkmeta.Artifact, dynamicLabels map[string]string, - impersonated client.Client) (*swapi.ExternalArtifactReference, *ownershipConflict, error) { + impersonated client.Client) (*swapi.InventoryEntry, *ownershipConflict, error) { log := ctrl.LoggerFrom(ctx) // Select the client based on the target namespace. Output artifacts in the @@ -635,7 +742,8 @@ func (r *ArtifactGeneratorReconciler) reconcileExternalArtifact(ctx context.Cont r.Event(obj, eventv1.EventTypeTrace, gotkmeta.ReadyCondition, msg) } - return &swapi.ExternalArtifactReference{ + return &swapi.InventoryEntry{ + Kind: sourcev1.ExternalArtifactKind, Name: ea.Name, Namespace: ea.Namespace, Digest: artifact.Digest, @@ -695,24 +803,22 @@ func (r *ArtifactGeneratorReconciler) detectOwnershipConflict(ctx context.Contex return conflict, nil } -// findOrphanedReferences identifies ExternalArtifact references -// in the inventory that are not present in the current references, -// indicating they should be garbage collected. +// findOrphanedReferences identifies references in the inventory that are not +// present in the current references, indicating they should be garbage +// collected. func (r *ArtifactGeneratorReconciler) findOrphanedReferences( - inventory []swapi.ExternalArtifactReference, - currentRefs []swapi.ExternalArtifactReference) []swapi.ExternalArtifactReference { + inventory []swapi.InventoryEntry, + currentRefs []swapi.InventoryEntry) []swapi.InventoryEntry { // Create map of current references for O(1) lookup currentSet := make(map[string]struct{}) for _, ref := range currentRefs { - key := fmt.Sprintf("%s/%s/%s", sourcev1.ExternalArtifactKind, ref.Namespace, ref.Name) - currentSet[key] = struct{}{} + currentSet[inventoryKey(ref)] = struct{}{} } // Find inventory items not in current set - var orphaned []swapi.ExternalArtifactReference + var orphaned []swapi.InventoryEntry for _, ref := range inventory { - key := fmt.Sprintf("%s/%s/%s", sourcev1.ExternalArtifactKind, ref.Namespace, ref.Name) - if _, exists := currentSet[key]; !exists { + if _, exists := currentSet[inventoryKey(ref)]; !exists { orphaned = append(orphaned, ref) } } @@ -720,6 +826,17 @@ func (r *ArtifactGeneratorReconciler) findOrphanedReferences( return orphaned } +// inventoryKey returns a unique key for an inventory entry. Entries written +// before the type and kind fields were introduced are treated as +// ExternalArtifacts. +func inventoryKey(ref swapi.InventoryEntry) string { + kind := ref.Kind + if kind == "" { + kind = sourcev1.ExternalArtifactKind + } + return fmt.Sprintf("%s/%s/%s", kind, ref.Namespace, ref.Name) +} + // setArtifactRevisions sets the revision and origin revision // metadata on the artifact based on the output artifact // configuration and available remote sources. diff --git a/internal/controller/artifactgenerator_controller_test.go b/internal/controller/artifactgenerator_controller_test.go index 3c5fe231..4b0dad92 100644 --- a/internal/controller/artifactgenerator_controller_test.go +++ b/internal/controller/artifactgenerator_controller_test.go @@ -1095,3 +1095,99 @@ func TestArtifactGeneratorReconciler_PathPattern(t *testing.T) { g.Expect(eaProd.Status.Artifact.Revision).To(Equal(oldProdRevision)) }) } + +func TestArtifactGeneratorReconciler_needsImpersonation(t *testing.T) { + tests := []struct { + name string + defaultServiceAccount string + serviceAccountName string + namespaces *swapi.Namespaces + artifacts []swapi.OutputArtifact + inventory []swapi.InventoryEntry + want bool + }{ + { + name: "no service account configured", + artifacts: []swapi.OutputArtifact{{Name: "app", Namespace: "other"}}, + want: false, + }, + { + name: "explicit service account, in-namespace artifact", + serviceAccountName: "sa", + artifacts: []swapi.OutputArtifact{{Name: "app"}}, + want: true, + }, + { + name: "explicit service account, cross-namespace artifact", + serviceAccountName: "sa", + artifacts: []swapi.OutputArtifact{{Name: "app", Namespace: "other"}}, + want: true, + }, + { + name: "explicit service account, managed namespaces without external targets", + serviceAccountName: "sa", + namespaces: &swapi.Namespaces{Strategy: swapi.NamespaceStrategyManaged}, + artifacts: []swapi.OutputArtifact{{Name: "app"}}, + want: true, + }, + { + name: "default service account, in-namespace artifact", + defaultServiceAccount: "default-sa", + artifacts: []swapi.OutputArtifact{{Name: "app"}}, + want: false, + }, + { + name: "default service account, cross-namespace artifact", + defaultServiceAccount: "default-sa", + artifacts: []swapi.OutputArtifact{{Name: "app", Namespace: "other"}}, + want: true, + }, + { + name: "default service account, managed namespaces without external targets", + defaultServiceAccount: "default-sa", + namespaces: &swapi.Namespaces{Strategy: swapi.NamespaceStrategyManaged}, + artifacts: []swapi.OutputArtifact{{Name: "app"}}, + want: false, + }, + { + name: "default service account, managed namespaces tracked in inventory", + defaultServiceAccount: "default-sa", + namespaces: &swapi.Namespaces{Strategy: swapi.NamespaceStrategyManaged}, + artifacts: []swapi.OutputArtifact{{Name: "app"}}, + inventory: []swapi.InventoryEntry{{Kind: swapi.NamespaceKind, Name: "other"}}, + want: true, + }, + { + name: "default service account, unmanaged strategy with namespace inventory", + defaultServiceAccount: "default-sa", + namespaces: &swapi.Namespaces{Strategy: swapi.NamespaceStrategyUnmanaged}, + artifacts: []swapi.OutputArtifact{{Name: "app"}}, + inventory: []swapi.InventoryEntry{{Kind: swapi.NamespaceKind, Name: "other"}}, + want: false, + }, + { + name: "default service account, cross-namespace artifact tracked in inventory", + defaultServiceAccount: "default-sa", + artifacts: []swapi.OutputArtifact{{Name: "app"}}, + inventory: []swapi.InventoryEntry{{Name: "app", Namespace: "other"}}, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + r := &ArtifactGeneratorReconciler{DefaultServiceAccount: tt.defaultServiceAccount} + obj := &swapi.ArtifactGenerator{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "flux-system"}, + Spec: swapi.ArtifactGeneratorSpec{ + ServiceAccountName: tt.serviceAccountName, + Namespaces: tt.namespaces, + OutputArtifacts: tt.artifacts, + }, + Status: swapi.ArtifactGeneratorStatus{Inventory: tt.inventory}, + } + g.Expect(r.needsImpersonation(obj)).To(Equal(tt.want)) + }) + } +} diff --git a/internal/controller/artifactgenerator_drift.go b/internal/controller/artifactgenerator_drift.go index 8710fb7e..7c7ebffd 100644 --- a/internal/controller/artifactgenerator_drift.go +++ b/internal/controller/artifactgenerator_drift.go @@ -42,6 +42,8 @@ import ( // - "ArtifactCorrupted" - artifact exists in storage but fails integrity verification // - "ExternalArtifactsNotFound" - failed to query in-cluster external artifacts // - "ExternalArtifactsChanged" - in-cluster external artifacts differ from inventory +// - "NamespacesNotFound" - failed to query managed namespaces +// - "NamespacesChanged" - managed namespaces differ from inventory // - "NoDriftDetected" - no drift detected and the storage is up to date func (r *ArtifactGeneratorReconciler) detectDrift(ctx context.Context, obj *swapi.ArtifactGenerator, @@ -69,14 +71,25 @@ func (r *ArtifactGeneratorReconciler) detectDrift(ctx context.Context, return true, "SourcesChanged" } - if len(obj.Status.Inventory) != len(obj.Spec.OutputArtifacts) && obj.Spec.PathPattern == "" { + // Count only the artifact entries, as the inventory also tracks + // managed namespaces. + artifactCount := 0 + for _, ref := range obj.Status.Inventory { + if ref.Kind != swapi.NamespaceKind { + artifactCount++ + } + } + if artifactCount != len(obj.Spec.OutputArtifacts) && obj.Spec.PathPattern == "" { log.Info("Drift detected, number of output artifacts has changed", - "old", len(obj.Status.Inventory), + "old", artifactCount, "new", len(obj.Spec.OutputArtifacts)) return true, "ArtifactsChanged" } for _, eaRef := range obj.Status.Inventory { + if eaRef.Kind == swapi.NamespaceKind { + continue + } storagePath := gotkstorage.ArtifactPath(sourcev1.ExternalArtifactKind, eaRef.Namespace, eaRef.Name, eaRef.Filename) artifact := gotkmeta.Artifact{ Digest: eaRef.Digest, @@ -109,6 +122,16 @@ func (r *ArtifactGeneratorReconciler) detectDrift(ctx context.Context, return true, "ExternalArtifactsChanged" } + nsDrift, err := r.detectNamespacesDrift(ctx, obj, impersonated) + if err != nil { + log.Error(err, "Failed to verify managed namespaces for drift") + return true, "NamespacesNotFound" + } + if nsDrift { + log.Info("Drift detected, managed namespaces have changed") + return true, "NamespacesChanged" + } + return false, "NoDriftDetected" } @@ -118,12 +141,17 @@ func (r *ArtifactGeneratorReconciler) detectExternalArtifactsDrift(ctx context.C obj *swapi.ArtifactGenerator, impersonated client.Client) (bool, error) { - // Group the inventory references by namespace, as the generated + // Group the artifact inventory references by namespace, as the generated // ExternalArtifacts may live in namespaces other than the one of // the ArtifactGenerator. namespaces := make(map[string]struct{}) + artifactCount := 0 for _, ref := range obj.Status.Inventory { + if ref.Kind == swapi.NamespaceKind { + continue + } namespaces[ref.Namespace] = struct{}{} + artifactCount++ } // Check if the number of ExternalArtifacts in the cluster matches the inventory. @@ -150,7 +178,7 @@ func (r *ArtifactGeneratorReconciler) detectExternalArtifactsDrift(ctx context.C } } - if total != len(obj.Status.Inventory) { + if total != artifactCount { return true, nil } diff --git a/internal/controller/artifactgenerator_drift_test.go b/internal/controller/artifactgenerator_drift_test.go index 862cf054..516fbee2 100644 --- a/internal/controller/artifactgenerator_drift_test.go +++ b/internal/controller/artifactgenerator_drift_test.go @@ -113,7 +113,7 @@ func TestArtifactGeneratorReconciler_DetectDrift(t *testing.T) { }, }, ObservedSourcesDigest: "test123", - Inventory: []swapi.ExternalArtifactReference{ + Inventory: []swapi.InventoryEntry{ { Namespace: ns.Name, Name: "test-artifact", @@ -223,7 +223,7 @@ func TestArtifactGeneratorReconciler_DetectDrift(t *testing.T) { }, }, ObservedSourcesDigest: "test123", - Inventory: []swapi.ExternalArtifactReference{ + Inventory: []swapi.InventoryEntry{ { Namespace: ns.Name, Name: "artifact-1", @@ -259,7 +259,7 @@ func TestArtifactGeneratorReconciler_DetectDrift(t *testing.T) { }, }, ObservedSourcesDigest: "test123", - Inventory: []swapi.ExternalArtifactReference{ + Inventory: []swapi.InventoryEntry{ { Namespace: ns.Name, Name: "missing-artifact", @@ -307,7 +307,7 @@ func TestArtifactGeneratorReconciler_DetectDrift(t *testing.T) { }, }, ObservedSourcesDigest: "test123", - Inventory: []swapi.ExternalArtifactReference{ + Inventory: []swapi.InventoryEntry{ { Namespace: ns.Name, Name: "test-artifact", diff --git a/internal/controller/artifactgenerator_finalize.go b/internal/controller/artifactgenerator_finalize.go index cbf1b3ec..4396a22f 100644 --- a/internal/controller/artifactgenerator_finalize.go +++ b/internal/controller/artifactgenerator_finalize.go @@ -18,9 +18,11 @@ package controller import ( "context" + "errors" "fmt" "time" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" @@ -41,8 +43,17 @@ func (r *ArtifactGeneratorReconciler) finalize(ctx context.Context, impersonated client.Client) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx) - // Delete ExternalArtifacts found in the inventory. - r.finalizeExternalArtifacts(ctx, obj, obj.Status.Inventory, impersonated) + // Delete the objects found in the inventory. Objects whose deletion is + // not confirmed are kept in the inventory so the finalizer is retried on + // the next reconciliation. + survivors := r.finalizeReferences(ctx, obj, obj.Status.Inventory, impersonated) + if len(survivors) > 0 { + obj.Status.Inventory = survivors + msg := fmt.Sprintf("failed to prune %d object(s), retrying", len(survivors)) + gotkconditions.MarkFalse(obj, gotkmeta.ReadyCondition, gotkmeta.PruneFailedReason, "%s", msg) + r.Event(obj, corev1.EventTypeWarning, gotkmeta.PruneFailedReason, msg) + return ctrl.Result{}, fmt.Errorf("failed to prune %d object(s)", len(survivors)) + } // Remove the finalizer. controllerutil.RemoveFinalizer(obj, swapi.Finalizer) @@ -51,39 +62,72 @@ func (r *ArtifactGeneratorReconciler) finalize(ctx context.Context, return ctrl.Result{}, nil } -// finalizeExternalArtifacts deletes the ExternalArtifact resources -// referenced in the provided list, along with their associated -// artifacts in the storage backend. -func (r *ArtifactGeneratorReconciler) finalizeExternalArtifacts(ctx context.Context, +// finalizeReferences deletes the objects referenced in the provided list. +// ExternalArtifacts are deleted along with their associated artifacts in the +// storage backend. Managed namespaces are deleted only when pruning is enabled. +// It returns the references whose deletion was not confirmed by the API server, +// so the caller can keep them tracked and retry on the next reconciliation. +func (r *ArtifactGeneratorReconciler) finalizeReferences(ctx context.Context, obj *swapi.ArtifactGenerator, - refs []swapi.ExternalArtifactReference, - impersonated client.Client) { + refs []swapi.InventoryEntry, + impersonated client.Client) []swapi.InventoryEntry { log := ctrl.LoggerFrom(ctx) - - for _, eaRef := range refs { - // Delete from storage. - storagePath := gotkstorage.ArtifactPath(sourcev1.ExternalArtifactKind, eaRef.Namespace, eaRef.Name, "*") - rmDir, err := r.Storage.RemoveAll(gotkmeta.Artifact{Path: storagePath}) - if err != nil { - log.Error(err, "Failed to delete artifact from storage", "path", storagePath) - } else if rmDir != "" { - log.Info(fmt.Sprintf("%s/%s/%s deleted from storage", sourcev1.ExternalArtifactKind, eaRef.Namespace, eaRef.Name), "path", rmDir) + var survivors []swapi.InventoryEntry + + for _, ref := range refs { + if ref.Kind == swapi.NamespaceKind { + if !obj.ManagesNamespaces() || !obj.NamespacePrune() { + log.Info("Skipping managed namespace deletion, pruning is disabled", "namespace", ref.Name) + continue + } + if err := r.deleteNamespace(ctx, ref.Name, impersonated); err != nil { + log.Error(err, "Failed to delete Namespace, will retry", "namespace", ref.Name) + survivors = append(survivors, ref) + } + continue } - // Delete from cluster. - ea := &sourcev1.ExternalArtifact{ - ObjectMeta: metav1.ObjectMeta{ - Name: eaRef.Name, - Namespace: eaRef.Namespace, - }, - } - err = r.clientForNamespace(obj, impersonated, eaRef.Namespace).Delete(ctx, ea) - if err != nil && !apierrors.IsNotFound(err) { - log.Error(err, "Failed to delete ExternalArtifact") - } else { - log.Info(fmt.Sprintf("%s/%s/%s deleted from cluster", sourcev1.ExternalArtifactKind, eaRef.Namespace, eaRef.Name)) + if err := r.finalizeExternalArtifact(ctx, obj, ref, impersonated); err != nil { + log.Error(err, "Failed to delete ExternalArtifact, will retry", + "artifact", fmt.Sprintf("%s/%s/%s", sourcev1.ExternalArtifactKind, ref.Namespace, ref.Name)) + survivors = append(survivors, ref) } } + return survivors +} + +// finalizeExternalArtifact deletes the ExternalArtifact referenced in the +// provided entry, along with its associated artifact in the storage backend. +// It returns an error when the deletion was not confirmed by the API server. +func (r *ArtifactGeneratorReconciler) finalizeExternalArtifact(ctx context.Context, + obj *swapi.ArtifactGenerator, + ref swapi.InventoryEntry, + impersonated client.Client) error { + log := ctrl.LoggerFrom(ctx) + + // Delete from storage. + var retErr error + storagePath := gotkstorage.ArtifactPath(sourcev1.ExternalArtifactKind, ref.Namespace, ref.Name, "*") + if rmDir, err := r.Storage.RemoveAll(gotkmeta.Artifact{Path: storagePath}); err != nil { + retErr = fmt.Errorf("failed to delete artifact from storage: %w", err) + } else if rmDir != "" { + log.Info(fmt.Sprintf("%s/%s/%s deleted from storage", sourcev1.ExternalArtifactKind, ref.Namespace, ref.Name), "path", rmDir) + } + + // Delete from cluster. + ea := &sourcev1.ExternalArtifact{ + ObjectMeta: metav1.ObjectMeta{ + Name: ref.Name, + Namespace: ref.Namespace, + }, + } + if err := r.clientForNamespace(obj, impersonated, ref.Namespace).Delete(ctx, ea); err != nil && !apierrors.IsNotFound(err) { + retErr = errors.Join(retErr, fmt.Errorf("failed to delete ExternalArtifact: %w", err)) + } else { + log.Info(fmt.Sprintf("%s/%s/%s deleted from cluster", sourcev1.ExternalArtifactKind, ref.Namespace, ref.Name)) + } + + return retErr } // addFinalizer sets the initial status conditions, adds the finalizer diff --git a/internal/controller/artifactgenerator_finalize_test.go b/internal/controller/artifactgenerator_finalize_test.go index 9dc577c6..04393fd7 100644 --- a/internal/controller/artifactgenerator_finalize_test.go +++ b/internal/controller/artifactgenerator_finalize_test.go @@ -18,16 +18,22 @@ package controller import ( "context" + "errors" "testing" "time" . "github.com/onsi/gomega" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/reconcile" gotkmeta "github.com/fluxcd/pkg/apis/meta" gotkconditions "github.com/fluxcd/pkg/runtime/conditions" + sourcev1 "github.com/fluxcd/source-controller/api/v1" swapi "github.com/fluxcd/source-watcher/api/v2/v1beta1" ) @@ -85,6 +91,57 @@ func TestArtifactGeneratorReconciler_Finalize(t *testing.T) { g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) } +func TestArtifactGeneratorReconciler_FinalizeReferencesSurvivors(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // A client whose Delete always fails, simulating an apiserver rejection + // such as RBAC or an admission webhook. + forbidden := apierrors.NewForbidden(schema.GroupResource{Resource: "namespaces"}, "tenant", errors.New("forbidden")) + kubeClient := fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + return forbidden + }, + }).Build() + + reconciler := &ArtifactGeneratorReconciler{ + Client: kubeClient, + ControllerName: controllerName, + Storage: testStorage, + } + + obj := &swapi.ArtifactGenerator{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "src", UID: "uid"}, + Spec: swapi.ArtifactGeneratorSpec{ + Namespaces: &swapi.Namespaces{Strategy: swapi.NamespaceStrategyManaged}, + }, + } + + artifact := swapi.InventoryEntry{ + Kind: sourcev1.ExternalArtifactKind, + Name: "ea", Namespace: "tenant", Filename: "ea.tar.gz", + } + namespace := swapi.InventoryEntry{ + Kind: swapi.NamespaceKind, Name: "tenant", + } + + // Both entries are survivors when their deletion is not confirmed. + survivors := reconciler.finalizeReferences(ctx, obj, []swapi.InventoryEntry{artifact, namespace}, nil) + g.Expect(survivors).To(ConsistOf(artifact, namespace)) + + // Namespaces are not survivors when management or pruning is disabled. + unmanaged := obj.DeepCopy() + unmanaged.Spec.Namespaces = &swapi.Namespaces{Strategy: swapi.NamespaceStrategyUnmanaged} + survivors = reconciler.finalizeReferences(ctx, unmanaged, []swapi.InventoryEntry{namespace}, nil) + g.Expect(survivors).To(BeEmpty()) + + prune := false + noPrune := obj.DeepCopy() + noPrune.Spec.Namespaces.Prune = &prune + survivors = reconciler.finalizeReferences(ctx, noPrune, []swapi.InventoryEntry{namespace}, nil) + g.Expect(survivors).To(BeEmpty()) +} + func TestArtifactGeneratorReconciler_Finalize_Disabled(t *testing.T) { g := NewWithT(t) reconciler := getArtifactGeneratorReconciler() diff --git a/internal/controller/artifactgenerator_inventory_test.go b/internal/controller/artifactgenerator_inventory_test.go new file mode 100644 index 00000000..29a60c44 --- /dev/null +++ b/internal/controller/artifactgenerator_inventory_test.go @@ -0,0 +1,51 @@ +/* +Copyright 2026 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "testing" + + . "github.com/onsi/gomega" + + sourcev1 "github.com/fluxcd/source-controller/api/v1" + + swapi "github.com/fluxcd/source-watcher/api/v2/v1beta1" +) + +func TestFindOrphanedReferences(t *testing.T) { + g := NewWithT(t) + r := &ArtifactGeneratorReconciler{} + + // The previous inventory may contain entries written before the type and + // kind field was introduced. + inventory := []swapi.InventoryEntry{ + {Name: "app", Namespace: "tenant", Digest: "sha256:abc", Filename: "app.tar.gz"}, + {Name: "stale", Namespace: "tenant", Digest: "sha256:def", Filename: "stale.tar.gz"}, + {Kind: swapi.NamespaceKind, Name: "old-ns"}, + } + current := []swapi.InventoryEntry{ + {Kind: sourcev1.ExternalArtifactKind, Name: "app", Namespace: "tenant", Digest: "sha256:abc", Filename: "app.tar.gz"}, + {Kind: swapi.NamespaceKind, Name: "new-ns"}, + } + + orphans := r.findOrphanedReferences(inventory, current) + g.Expect(orphans).To(HaveLen(2)) + g.Expect(orphans).To(ConsistOf( + swapi.InventoryEntry{Name: "stale", Namespace: "tenant", Digest: "sha256:def", Filename: "stale.tar.gz"}, + swapi.InventoryEntry{Kind: swapi.NamespaceKind, Name: "old-ns"}, + )) +} diff --git a/internal/controller/artifactgenerator_namespace.go b/internal/controller/artifactgenerator_namespace.go new file mode 100644 index 00000000..f7ffaf14 --- /dev/null +++ b/internal/controller/artifactgenerator_namespace.go @@ -0,0 +1,269 @@ +/* +Copyright 2026 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "crypto/sha256" + "fmt" + "maps" + "sort" + "strings" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" + gotkmeta "github.com/fluxcd/pkg/apis/meta" + + swapi "github.com/fluxcd/source-watcher/api/v2/v1beta1" +) + +// namespaceConflict describes a managed namespace that is being adopted or +// taken over from another ArtifactGenerator. +type namespaceConflict struct { + Namespace string `json:"namespace"` + Owner string `json:"owner,omitempty"` + Adopted bool `json:"adopted"` +} + +// namespaceClient returns the client used to manage cluster-scoped Namespace +// objects. When a ServiceAccount is configured for impersonation, it is used, +// otherwise the controller client is returned. +func (r *ArtifactGeneratorReconciler) namespaceClient(impersonated client.Client) client.Client { + if impersonated != nil { + return impersonated + } + return r.Client +} + +// reconcileNamespaces creates or updates the namespaces explicitly targeted by +// the rendered output artifacts, returning their inventory entries and the +// namespaces that were adopted or taken over. +func (r *ArtifactGeneratorReconciler) reconcileNamespaces(ctx context.Context, + obj *swapi.ArtifactGenerator, + reqs []artifactRequest, + impersonated client.Client) ([]swapi.InventoryEntry, []namespaceConflict, error) { + kubeClient := r.namespaceClient(impersonated) + seen := make(map[string]struct{}) + var refs []swapi.InventoryEntry + var conflicts []namespaceConflict + for _, req := range reqs { + // Only namespaces explicitly targeted by .spec.artifacts[].namespace + // are managed, the ArtifactGenerator namespace is never managed + // implicitly through the default. + name := req.Namespace + if name == "" { + continue + } + // Never manage the ArtifactGenerator namespace itself, even when + // explicitly targeted, to avoid deleting the ArtifactGenerator and + // its dependencies when pruning. + if name == obj.Namespace { + continue + } + if _, ok := seen[name]; ok { + continue + } + seen[name] = struct{}{} + + ref, conflict, err := r.reconcileNamespace(ctx, obj, name, kubeClient) + if err != nil { + return nil, nil, err + } + refs = append(refs, *ref) + if conflict != nil { + conflicts = append(conflicts, *conflict) + } + } + return refs, conflicts, nil +} + +// reconcileNamespace ensures the Namespace exists and is managed by the +// ArtifactGenerator, applying the common metadata and the controller labels. +// Existing namespaces that are not owned by another ArtifactGenerator are +// adopted with a warning event, while namespaces owned by another +// ArtifactGenerator are taken over. +func (r *ArtifactGeneratorReconciler) reconcileNamespace(ctx context.Context, + obj *swapi.ArtifactGenerator, + name string, + kubeClient client.Client) (*swapi.InventoryEntry, *namespaceConflict, error) { + log := ctrl.LoggerFrom(ctx) + + labels, annotations := r.namespaceMetadata(obj) + digest := namespaceMetadataDigest(labels, annotations) + + // Detect adoption or ownership conflict before applying. + var conflict *namespaceConflict + changed := true + existing := &corev1.Namespace{} + err := kubeClient.Get(ctx, client.ObjectKey{Name: name}, existing) + switch { + case apierrors.IsNotFound(err): + // The namespace will be created. + case err != nil: + return nil, nil, fmt.Errorf("failed to get Namespace: %w", err) + default: + changed = !namespaceMetadataMatches(existing, labels, annotations) + owner := existing.Labels[swapi.ArtifactGeneratorLabel] + switch { + case owner == "": + msg := fmt.Sprintf("Namespace %s is not managed by any ArtifactGenerator and is being adopted by %s/%s", + name, obj.Namespace, obj.Name) + log.Info("Adopting unmanaged namespace", "namespace", name) + r.Event(existing, corev1.EventTypeWarning, swapi.NamespaceAdoptedReason, msg) + conflict = &namespaceConflict{Namespace: name, Adopted: true} + case owner != string(obj.GetUID()): + msg := fmt.Sprintf("Namespace %s is managed by ArtifactGenerator %s and is being taken over by %s/%s", + name, owner, obj.Namespace, obj.Name) + log.Info("Taking over managed namespace", "namespace", name, "owner", owner) + r.Event(existing, corev1.EventTypeWarning, swapi.OwnershipConflictReason, msg) + conflict = &namespaceConflict{Namespace: name, Owner: owner} + } + } + + ns := &corev1.Namespace{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: swapi.NamespaceKind, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: labels, + Annotations: annotations, + }, + } + + forceApply := true + if err := kubeClient.Patch(ctx, ns, client.Apply, &client.PatchOptions{ + FieldManager: r.ControllerName, + Force: &forceApply, + }); err != nil { + return nil, nil, fmt.Errorf("failed to apply Namespace: %w", err) + } + + if changed { + msg := fmt.Sprintf("Namespace %s reconciled", name) + log.Info(msg) + r.Event(obj, eventv1.EventTypeTrace, gotkmeta.ReadyCondition, msg) + } else { + log.Info("Namespace is up to date", "namespace", name) + } + + return &swapi.InventoryEntry{ + Kind: swapi.NamespaceKind, + Name: name, + Digest: digest, + }, conflict, nil +} + +// namespaceMetadata returns the labels and annotations to apply to managed +// namespaces. The common metadata is applied, with the controller labels +// taking precedence. +func (r *ArtifactGeneratorReconciler) namespaceMetadata(obj *swapi.ArtifactGenerator) (map[string]string, map[string]string) { + labels := make(map[string]string) + var annotations map[string]string + if cm := obj.Spec.CommonMetadata; cm != nil { + maps.Copy(labels, cm.Labels) + if len(cm.Annotations) > 0 { + annotations = make(map[string]string) + maps.Copy(annotations, cm.Annotations) + } + } + + labels["app.kubernetes.io/managed-by"] = r.ControllerName + labels[swapi.ArtifactGeneratorLabel] = string(obj.GetUID()) + return labels, annotations +} + +// namespaceMetadataMatches returns true when the namespace carries all the +// expected labels and annotations. +func namespaceMetadataMatches(ns *corev1.Namespace, labels, annotations map[string]string) bool { + for key, value := range labels { + if ns.Labels[key] != value { + return false + } + } + for key, value := range annotations { + if ns.Annotations[key] != value { + return false + } + } + return true +} + +// namespaceMetadataDigest computes a deterministic digest of the metadata +// applied to a managed namespace. +func namespaceMetadataDigest(labels, annotations map[string]string) string { + parts := make([]string, 0, len(labels)+len(annotations)) + for key, value := range labels { + parts = append(parts, fmt.Sprintf("label:%s=%s", key, value)) + } + for key, value := range annotations { + parts = append(parts, fmt.Sprintf("annotation:%s=%s", key, value)) + } + sort.Strings(parts) + digest := sha256.Sum256([]byte(strings.Join(parts, "|"))) + return fmt.Sprintf("sha256:%x", digest) +} + +// detectNamespacesDrift checks if the managed namespaces recorded in the +// inventory still exist, are owned by the ArtifactGenerator and carry the +// expected common metadata. +func (r *ArtifactGeneratorReconciler) detectNamespacesDrift(ctx context.Context, + obj *swapi.ArtifactGenerator, + impersonated client.Client) (bool, error) { + if !obj.ManagesNamespaces() { + return false, nil + } + + expectedLabels, expectedAnnotations := r.namespaceMetadata(obj) + kubeClient := r.namespaceClient(impersonated) + for _, ref := range obj.Status.Inventory { + if ref.Kind != swapi.NamespaceKind { + continue + } + ns := &corev1.Namespace{} + if err := kubeClient.Get(ctx, client.ObjectKey{Name: ref.Name}, ns); err != nil { + if apierrors.IsNotFound(err) { + return true, nil + } + return false, err + } + if !namespaceMetadataMatches(ns, expectedLabels, expectedAnnotations) { + return true, nil + } + } + return false, nil +} + +// deleteNamespace deletes a managed namespace. It returns an error when the +// deletion was not confirmed by the API server. +func (r *ArtifactGeneratorReconciler) deleteNamespace(ctx context.Context, + name string, + impersonated client.Client) error { + log := ctrl.LoggerFrom(ctx) + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: name}} + if err := r.namespaceClient(impersonated).Delete(ctx, ns); err != nil && !apierrors.IsNotFound(err) { + return err + } + log.Info("Namespace deleted from cluster", "namespace", name) + return nil +} diff --git a/internal/controller/artifactgenerator_namespace_test.go b/internal/controller/artifactgenerator_namespace_test.go index 456122bb..d4168b96 100644 --- a/internal/controller/artifactgenerator_namespace_test.go +++ b/internal/controller/artifactgenerator_namespace_test.go @@ -19,6 +19,7 @@ package controller import ( "context" "fmt" + "strings" "testing" . "github.com/onsi/gomega" @@ -26,6 +27,7 @@ import ( rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/rand" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -106,6 +108,81 @@ func TestArtifactGeneratorReconciler_CrossNamespace(t *testing.T) { g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) } +func TestArtifactGeneratorReconciler_CrossNamespacePathPattern(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-agns-pp-src") + g.Expect(err).ToNot(HaveOccurred()) + tgtNS1, err := testEnv.CreateNamespace(ctx, "test-agns-pp-a") + g.Expect(err).ToNot(HaveOccurred()) + tgtNS2, err := testEnv.CreateNamespace(ctx, "test-agns-pp-b") + g.Expect(err).ToNot(HaveOccurred()) + + // The ServiceAccount lives in the ArtifactGenerator namespace and is + // granted access to both target namespaces through RoleBindings. + saName := "artifact-generator-pp" + g.Expect(createImpersonationRBAC(ctx, saName, srcNS.Name, tgtNS1.Name, tgtNS2.Name)).To(Succeed()) + + objKey := client.ObjectKey{Name: "test-agns-pp", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.ServiceAccountName = saName + obj.Spec.Sources = obj.Spec.Sources[:1] + alias := obj.Spec.Sources[0].Alias + obj.Spec.PathPattern = fmt.Sprintf("@%s/apps/{tenant}", alias) + obj.Spec.OutputArtifacts = []swapi.OutputArtifact{ + { + Name: "{tenant}", + Namespace: "{tenant}", + Copy: []swapi.CopyOperation{ + {From: fmt.Sprintf("@%s/apps/{tenant}/**", alias), To: "@artifact/"}, + }, + }, + } + g.Expect(testClient.Create(ctx, obj)).To(Succeed()) + + // The captured directory names are the target namespaces. + gitFiles := []gotktestsrv.File{ + {Name: fmt.Sprintf("apps/%s/app.yaml", tgtNS1.Name), Body: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config"}, + {Name: fmt.Sprintf("apps/%s/app.yaml", tgtNS2.Name), Body: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config"}, + } + g.Expect(applyGitRepository(objKey, "main@sha256:abc123", gitFiles)).To(Succeed()) + + // Add the finalizer. + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + // Build the artifacts and reconcile the ExternalArtifacts. + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(testClient.Get(ctx, objKey, obj)).To(Succeed()) + g.Expect(gotkconditions.IsReady(obj)).To(BeTrue()) + g.Expect(obj.Status.Inventory).To(HaveLen(2)) + + for _, targetNS := range []*corev1.Namespace{tgtNS1, tgtNS2} { + eaKey := client.ObjectKey{Name: targetNS.Name, Namespace: targetNS.Name} + ea := &sourcev1.ExternalArtifact{} + g.Expect(testClient.Get(ctx, eaKey, ea)).To(Succeed()) + g.Expect(ea.Status.Artifact).ToNot(BeNil()) + g.Expect(ea.Spec.SourceRef.Namespace).To(Equal(srcNS.Name)) + } + + // Deleting the ArtifactGenerator must remove the ExternalArtifacts + // from the target namespaces. + g.Expect(testClient.Delete(ctx, obj)).To(Succeed()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + for _, targetNS := range []*corev1.Namespace{tgtNS1, tgtNS2} { + eaKey := client.ObjectKey{Name: targetNS.Name, Namespace: targetNS.Name} + err = testClient.Get(ctx, eaKey, &sourcev1.ExternalArtifact{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + } +} + func TestArtifactGeneratorReconciler_DefaultServiceAccount(t *testing.T) { g := NewWithT(t) reconciler := getArtifactGeneratorReconciler() @@ -154,23 +231,27 @@ func TestArtifactGeneratorReconciler_DefaultServiceAccount(t *testing.T) { g.Expect(testClient.Get(ctx, eaKey, &sourcev1.ExternalArtifact{})).To(Succeed()) } +// TestArtifactGeneratorReconciler_ServiceAccountSameNamespace verifies that an +// explicit .spec.serviceAccountName is used for ExternalArtifacts in the +// ArtifactGenerator namespace too, so the ServiceAccount needs permission to +// manage them there. func TestArtifactGeneratorReconciler_ServiceAccountSameNamespace(t *testing.T) { g := NewWithT(t) reconciler := getArtifactGeneratorReconciler() - reconciler.DefaultServiceAccount = "missing-default-sa" ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() srcNS, err := testEnv.CreateNamespace(ctx, "test-agns-same") g.Expect(err).ToNot(HaveOccurred()) - // The ServiceAccount does not exist, so any attempt to impersonate it - // would make the reconciliation fail. Output artifacts in the - // ArtifactGenerator namespace must be reconciled with the controller - // client instead. + // The ServiceAccount lives in the ArtifactGenerator namespace and is + // granted access to it. + saName := "test-agns-same-sa" + g.Expect(createImpersonationRBAC(ctx, saName, srcNS.Name, srcNS.Name)).To(Succeed()) + objKey := client.ObjectKey{Name: "test-agns-same", Namespace: srcNS.Name} obj := getArtifactGenerator(objKey) - obj.Spec.ServiceAccountName = "missing-sa" + obj.Spec.ServiceAccountName = saName obj.Spec.Sources = obj.Spec.Sources[:1] obj.Spec.OutputArtifacts = []swapi.OutputArtifact{ { @@ -216,6 +297,108 @@ func TestArtifactGeneratorReconciler_ServiceAccountSameNamespace(t *testing.T) { } } +// TestArtifactGeneratorReconciler_ServiceAccountSameNamespaceDenied verifies +// that an explicit .spec.serviceAccountName is used for artifacts in the +// ArtifactGenerator namespace: when the ServiceAccount has no RBAC, the +// reconciliation fails instead of falling back to the controller credentials. +func TestArtifactGeneratorReconciler_ServiceAccountSameNamespaceDenied(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-agns-same-deny") + g.Expect(err).ToNot(HaveOccurred()) + + // The ServiceAccount exists but has no RBAC bindings. + saName := "test-agns-same-deny-sa" + g.Expect(testClient.Create(ctx, &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: saName, Namespace: srcNS.Name}, + })).To(Succeed()) + + objKey := client.ObjectKey{Name: "test-agns-same-deny", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.ServiceAccountName = saName + obj.Spec.Sources = obj.Spec.Sources[:1] + obj.Spec.OutputArtifacts = []swapi.OutputArtifact{ + { + Name: fmt.Sprintf("%s-git", objKey.Name), + Copy: []swapi.CopyOperation{ + {From: fmt.Sprintf("@%s-git/**", objKey.Name), To: "@artifact/"}, + }, + }, + } + g.Expect(testClient.Create(ctx, obj)).To(Succeed()) + + gitFiles := []gotktestsrv.File{ + {Name: "app.yaml", Body: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config"}, + } + g.Expect(applyGitRepository(objKey, "main@sha256:abc123", gitFiles)).To(Succeed()) + + // Add the finalizer. + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + // The apply is rejected by RBAC, so the reconciliation must fail and no + // ExternalArtifact may be created. + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).To(HaveOccurred()) + g.Expect(apierrors.IsForbidden(err)).To(BeTrue()) + + eaKey := client.ObjectKey{Name: fmt.Sprintf("%s-git", objKey.Name), Namespace: srcNS.Name} + err = testClient.Get(ctx, eaKey, &sourcev1.ExternalArtifact{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) +} + +// TestArtifactGeneratorReconciler_DefaultServiceAccountSameNamespace verifies +// that the controller default ServiceAccount is not used for artifacts in the +// ArtifactGenerator namespace: those keep using the controller credentials +// unless .spec.serviceAccountName is set. +func TestArtifactGeneratorReconciler_DefaultServiceAccountSameNamespace(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + // The default ServiceAccount does not exist, so any attempt to impersonate + // it would make the reconciliation fail. In-namespace artifacts must not + // use it. + reconciler.DefaultServiceAccount = "missing-default-sa" + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-agns-def-same") + g.Expect(err).ToNot(HaveOccurred()) + + objKey := client.ObjectKey{Name: "test-agns-def-same", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.Sources = obj.Spec.Sources[:1] + obj.Spec.OutputArtifacts = []swapi.OutputArtifact{ + { + Name: fmt.Sprintf("%s-git", objKey.Name), + Copy: []swapi.CopyOperation{ + {From: fmt.Sprintf("@%s-git/**", objKey.Name), To: "@artifact/"}, + }, + }, + } + g.Expect(testClient.Create(ctx, obj)).To(Succeed()) + + gitFiles := []gotktestsrv.File{ + {Name: "app.yaml", Body: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config"}, + } + g.Expect(applyGitRepository(objKey, "main@sha256:abc123", gitFiles)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(testClient.Get(ctx, objKey, obj)).To(Succeed()) + g.Expect(gotkconditions.IsReady(obj)).To(BeTrue()) + + eaKey := client.ObjectKey{Name: fmt.Sprintf("%s-git", objKey.Name), Namespace: srcNS.Name} + ea := &sourcev1.ExternalArtifact{} + g.Expect(testClient.Get(ctx, eaKey, ea)).To(Succeed()) + g.Expect(ea.Status.Artifact).ToNot(BeNil()) +} + func TestArtifactGeneratorReconciler_OwnershipConflict(t *testing.T) { g := NewWithT(t) reconciler := getArtifactGeneratorReconciler() @@ -374,9 +557,507 @@ func TestArtifactGeneratorReconciler_CrossNamespaceAccessDenied(t *testing.T) { g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) } +func TestArtifactGeneratorReconciler_ManagedNamespaces(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-mns-src") + g.Expect(err).ToNot(HaveOccurred()) + + tgtName := fmt.Sprintf("test-mns-tgt-%s", rand.String(5)) + + objKey := client.ObjectKey{Name: "test-mns", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.Sources = obj.Spec.Sources[:1] + obj.Spec.Namespaces = &swapi.Namespaces{Strategy: swapi.NamespaceStrategyManaged} + obj.Spec.CommonMetadata = &swapi.CommonMetadata{ + Labels: map[string]string{"team": "platform"}, + Annotations: map[string]string{"owner": "platform"}, + } + obj.Spec.OutputArtifacts = []swapi.OutputArtifact{ + { + Name: fmt.Sprintf("%s-git", objKey.Name), + Namespace: tgtName, + Copy: []swapi.CopyOperation{ + {From: fmt.Sprintf("@%s-git/**", objKey.Name), To: "@artifact/"}, + }, + }, + } + g.Expect(testClient.Create(ctx, obj)).To(Succeed()) + + gitFiles := []gotktestsrv.File{ + {Name: "app.yaml", Body: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config"}, + } + g.Expect(applyGitRepository(objKey, "main@sha256:abc123", gitFiles)).To(Succeed()) + + // Add the finalizer. + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + // Create the namespace and reconcile the ExternalArtifact. + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(testClient.Get(ctx, objKey, obj)).To(Succeed()) + g.Expect(gotkconditions.IsReady(obj)).To(BeTrue()) + g.Expect(gotkconditions.GetMessage(obj, gotkmeta.ReadyCondition)).To(ContainSubstring("manages 1 namespace(s)")) + + // The namespace is created with the common metadata and controller labels. + ns := &corev1.Namespace{} + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: tgtName}, ns)).To(Succeed()) + g.Expect(ns.Labels).To(HaveKeyWithValue("team", "platform")) + g.Expect(ns.Labels).To(HaveKeyWithValue("app.kubernetes.io/managed-by", controllerName)) + g.Expect(ns.Labels).To(HaveKeyWithValue(swapi.ArtifactGeneratorLabel, string(obj.GetUID()))) + g.Expect(ns.Annotations).To(HaveKeyWithValue("owner", "platform")) + + // The ExternalArtifact exists in the managed namespace. + eaKey := client.ObjectKey{Name: fmt.Sprintf("%s-git", objKey.Name), Namespace: tgtName} + g.Expect(testClient.Get(ctx, eaKey, &sourcev1.ExternalArtifact{})).To(Succeed()) + + // The inventory tracks both the artifact and the namespace. + g.Expect(obj.Status.Inventory).To(HaveLen(2)) + kinds := make([]string, 0, len(obj.Status.Inventory)) + for _, ref := range obj.Status.Inventory { + kinds = append(kinds, ref.Kind) + } + g.Expect(kinds).To(ConsistOf(sourcev1.ExternalArtifactKind, swapi.NamespaceKind)) + + // Metadata drift on a managed namespace is corrected. + ns.Labels["team"] = "changed" + g.Expect(testClient.Update(ctx, ns)).To(Succeed()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: tgtName}, ns)).To(Succeed()) + g.Expect(ns.Labels).To(HaveKeyWithValue("team", "platform")) + + // Changing the desired metadata is reported as a namespace update. + g.Expect(testClient.Get(ctx, objKey, obj)).To(Succeed()) + obj.Spec.CommonMetadata.Labels["tier"] = "gold" + g.Expect(testClient.Update(ctx, obj)).To(Succeed()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: tgtName}, ns)).To(Succeed()) + g.Expect(ns.Labels).To(HaveKeyWithValue("tier", "gold")) + var reported bool + for _, e := range getEvents(objKey.Name, objKey.Namespace) { + if e.Type == corev1.EventTypeNormal && strings.Contains(e.Message, "namespaces reconciled") { + reported = true + } + } + g.Expect(reported).To(BeTrue()) + + // Deleting the ArtifactGenerator prunes the managed namespace. + g.Expect(testClient.Delete(ctx, obj)).To(Succeed()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + expectNamespaceDeleted(g, ctx, tgtName) +} + +func TestArtifactGeneratorReconciler_ManagedNamespacesAdopt(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-mns-adopt-src") + g.Expect(err).ToNot(HaveOccurred()) + tgtNS, err := testEnv.CreateNamespace(ctx, "test-mns-adopt-tgt") + g.Expect(err).ToNot(HaveOccurred()) + + objKey := client.ObjectKey{Name: "test-mns-adopt", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.Sources = obj.Spec.Sources[:1] + obj.Spec.Namespaces = &swapi.Namespaces{Strategy: swapi.NamespaceStrategyManaged} + obj.Spec.CommonMetadata = &swapi.CommonMetadata{ + Labels: map[string]string{"team": "platform"}, + } + obj.Spec.OutputArtifacts = []swapi.OutputArtifact{ + { + Name: fmt.Sprintf("%s-git", objKey.Name), + Namespace: tgtNS.Name, + Copy: []swapi.CopyOperation{ + {From: fmt.Sprintf("@%s-git/**", objKey.Name), To: "@artifact/"}, + }, + }, + } + g.Expect(testClient.Create(ctx, obj)).To(Succeed()) + + gitFiles := []gotktestsrv.File{ + {Name: "app.yaml", Body: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config"}, + } + g.Expect(applyGitRepository(objKey, "main@sha256:abc123", gitFiles)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + // The pre-existing namespace is adopted and labeled. + ns := &corev1.Namespace{} + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: tgtNS.Name}, ns)).To(Succeed()) + g.Expect(ns.Labels).To(HaveKeyWithValue(swapi.ArtifactGeneratorLabel, string(obj.GetUID()))) + g.Expect(ns.Labels).To(HaveKeyWithValue("team", "platform")) + + g.Expect(testClient.Get(ctx, objKey, obj)).To(Succeed()) + var nsDigest string + for _, ref := range obj.Status.Inventory { + if ref.Kind == swapi.NamespaceKind && ref.Name == tgtNS.Name { + nsDigest = ref.Digest + } + } + g.Expect(nsDigest).ToNot(BeEmpty()) + g.Expect(obj.HasNamespaceInInventory(tgtNS.Name, nsDigest)).To(BeTrue()) + + // The adoption is summarized as a warning event on the ArtifactGenerator. + g.Expect(eventsWithReason(getEvents(objKey.Name, objKey.Namespace), swapi.NamespaceAdoptedReason)).To(HaveLen(1)) +} + +func TestArtifactGeneratorReconciler_ManagedNamespacesTakeover(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-mns-take-src") + g.Expect(err).ToNot(HaveOccurred()) + + tgtName := fmt.Sprintf("test-mns-take-tgt-%s", rand.String(5)) + + // Pre-create the namespace owned by another ArtifactGenerator. + other := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: tgtName, + Labels: map[string]string{ + swapi.ArtifactGeneratorLabel: "11111111-1111-1111-1111-111111111111", + }, + }, + } + g.Expect(testClient.Create(ctx, other)).To(Succeed()) + + objKey := client.ObjectKey{Name: "test-mns-take", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.Sources = obj.Spec.Sources[:1] + obj.Spec.Namespaces = &swapi.Namespaces{Strategy: swapi.NamespaceStrategyManaged} + obj.Spec.OutputArtifacts = []swapi.OutputArtifact{ + { + Name: fmt.Sprintf("%s-git", objKey.Name), + Namespace: tgtName, + Copy: []swapi.CopyOperation{ + {From: fmt.Sprintf("@%s-git/**", objKey.Name), To: "@artifact/"}, + }, + }, + } + g.Expect(testClient.Create(ctx, obj)).To(Succeed()) + + gitFiles := []gotktestsrv.File{ + {Name: "app.yaml", Body: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config"}, + } + g.Expect(applyGitRepository(objKey, "main@sha256:abc123", gitFiles)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + // The namespace is taken over and labeled with the new owner. + ns := &corev1.Namespace{} + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: tgtName}, ns)).To(Succeed()) + g.Expect(testClient.Get(ctx, objKey, obj)).To(Succeed()) + g.Expect(ns.Labels).To(HaveKeyWithValue(swapi.ArtifactGeneratorLabel, string(obj.GetUID()))) + + // The takeover is summarized as an ownership conflict event on the AG. + g.Expect(eventsWithReason(getEvents(objKey.Name, objKey.Namespace), swapi.OwnershipConflictReason)).To(HaveLen(1)) +} + +func TestArtifactGeneratorReconciler_ManagedNamespacesNoPrune(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-mns-noprune-src") + g.Expect(err).ToNot(HaveOccurred()) + + tgtName := fmt.Sprintf("test-mns-noprune-tgt-%s", rand.String(5)) + + prune := false + objKey := client.ObjectKey{Name: "test-mns-noprune", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.Sources = obj.Spec.Sources[:1] + obj.Spec.Namespaces = &swapi.Namespaces{ + Strategy: swapi.NamespaceStrategyManaged, + Prune: &prune, + } + obj.Spec.OutputArtifacts = []swapi.OutputArtifact{ + { + Name: fmt.Sprintf("%s-git", objKey.Name), + Namespace: tgtName, + Copy: []swapi.CopyOperation{ + {From: fmt.Sprintf("@%s-git/**", objKey.Name), To: "@artifact/"}, + }, + }, + } + g.Expect(testClient.Create(ctx, obj)).To(Succeed()) + + gitFiles := []gotktestsrv.File{ + {Name: "app.yaml", Body: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config"}, + } + g.Expect(applyGitRepository(objKey, "main@sha256:abc123", gitFiles)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: tgtName}, &corev1.Namespace{})).To(Succeed()) + + // Deleting the ArtifactGenerator leaves the managed namespace in place. + g.Expect(testClient.Get(ctx, objKey, obj)).To(Succeed()) + g.Expect(testClient.Delete(ctx, obj)).To(Succeed()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + ns := &corev1.Namespace{} + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: tgtName}, ns)).To(Succeed()) + g.Expect(ns.DeletionTimestamp).To(BeNil()) +} + +func TestArtifactGeneratorReconciler_ManagedNamespacesPathPattern(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-mns-pp-src") + g.Expect(err).ToNot(HaveOccurred()) + + tgt1 := fmt.Sprintf("test-mns-pp-a-%s", rand.String(5)) + tgt2 := fmt.Sprintf("test-mns-pp-b-%s", rand.String(5)) + + objKey := client.ObjectKey{Name: "test-mns-pp", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.Sources = obj.Spec.Sources[:1] + obj.Spec.Namespaces = &swapi.Namespaces{Strategy: swapi.NamespaceStrategyManaged} + alias := obj.Spec.Sources[0].Alias + obj.Spec.PathPattern = fmt.Sprintf("@%s/apps/{tenant}", alias) + obj.Spec.OutputArtifacts = []swapi.OutputArtifact{ + { + Name: "{tenant}", + Namespace: "{tenant}", + Copy: []swapi.CopyOperation{ + {From: fmt.Sprintf("@%s/apps/{tenant}/**", alias), To: "@artifact/"}, + }, + }, + } + g.Expect(testClient.Create(ctx, obj)).To(Succeed()) + + body := "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config" + g.Expect(applyGitRepository(objKey, "main@sha256:abc123", []gotktestsrv.File{ + {Name: fmt.Sprintf("apps/%s/app.yaml", tgt1), Body: body}, + {Name: fmt.Sprintf("apps/%s/app.yaml", tgt2), Body: body}, + })).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + for _, name := range []string{tgt1, tgt2} { + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: name}, &corev1.Namespace{})).To(Succeed()) + } + + // Removing a directory prunes its managed namespace. + g.Expect(applyGitRepository(objKey, "main@sha256:def456", []gotktestsrv.File{ + {Name: fmt.Sprintf("apps/%s/app.yaml", tgt1), Body: body}, + })).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: tgt1}, &corev1.Namespace{})).To(Succeed()) + expectNamespaceDeleted(g, ctx, tgt2) +} + +// expectNamespaceDeleted asserts that a namespace is either gone or in the +// process of being deleted. envtest does not run the namespace controller, so +// namespaces may remain in Terminating state after deletion. +func expectNamespaceDeleted(g Gomega, ctx context.Context, name string) { + ns := &corev1.Namespace{} + err := testClient.Get(ctx, client.ObjectKey{Name: name}, ns) + if err == nil { + g.Expect(ns.DeletionTimestamp).ToNot(BeNil()) + } else { + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + } +} + +func TestArtifactGeneratorReconciler_ManagedNamespacesImpersonation(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-mns-imp-src") + g.Expect(err).ToNot(HaveOccurred()) + tgtNS, err := testEnv.CreateNamespace(ctx, "test-mns-imp-tgt") + g.Expect(err).ToNot(HaveOccurred()) + + // The ServiceAccount manages ExternalArtifacts in the target namespace and + // namespaces at cluster scope. + saName := "test-mns-imp-sa" + g.Expect(createImpersonationRBAC(ctx, saName, srcNS.Name, tgtNS.Name)).To(Succeed()) + g.Expect(grantNamespaceManagement(ctx, saName, srcNS.Name)).To(Succeed()) + + objKey := client.ObjectKey{Name: "test-mns-imp", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.ServiceAccountName = saName + obj.Spec.Sources = obj.Spec.Sources[:1] + obj.Spec.Namespaces = &swapi.Namespaces{Strategy: swapi.NamespaceStrategyManaged} + obj.Spec.OutputArtifacts = []swapi.OutputArtifact{ + { + Name: fmt.Sprintf("%s-git", objKey.Name), + Namespace: tgtNS.Name, + Copy: []swapi.CopyOperation{ + {From: fmt.Sprintf("@%s-git/**", objKey.Name), To: "@artifact/"}, + }, + }, + } + g.Expect(testClient.Create(ctx, obj)).To(Succeed()) + + gitFiles := []gotktestsrv.File{ + {Name: "app.yaml", Body: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config"}, + } + g.Expect(applyGitRepository(objKey, "main@sha256:abc123", gitFiles)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(testClient.Get(ctx, objKey, obj)).To(Succeed()) + g.Expect(gotkconditions.IsReady(obj)).To(BeTrue()) + + ns := &corev1.Namespace{} + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: tgtNS.Name}, ns)).To(Succeed()) + g.Expect(ns.Labels).To(HaveKeyWithValue(swapi.ArtifactGeneratorLabel, string(obj.GetUID()))) + + eaKey := client.ObjectKey{Name: fmt.Sprintf("%s-git", objKey.Name), Namespace: tgtNS.Name} + g.Expect(testClient.Get(ctx, eaKey, &sourcev1.ExternalArtifact{})).To(Succeed()) +} + +func TestArtifactGeneratorReconciler_ManagedNamespacesPruneRetry(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-mns-retry-src") + g.Expect(err).ToNot(HaveOccurred()) + tgtNS, err := testEnv.CreateNamespace(ctx, "test-mns-retry-tgt") + g.Expect(err).ToNot(HaveOccurred()) + + // The ServiceAccount can manage ExternalArtifacts and namespaces, but + // cannot delete namespaces. + saName := "test-mns-retry-sa" + g.Expect(createImpersonationRBAC(ctx, saName, srcNS.Name, tgtNS.Name)).To(Succeed()) + g.Expect(grantNamespaceManagement(ctx, saName, srcNS.Name, "get", "list", "watch", "create", "update", "patch")).To(Succeed()) + + objKey := client.ObjectKey{Name: "test-mns-retry", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.ServiceAccountName = saName + obj.Spec.Sources = obj.Spec.Sources[:1] + obj.Spec.Namespaces = &swapi.Namespaces{Strategy: swapi.NamespaceStrategyManaged} + obj.Spec.OutputArtifacts = []swapi.OutputArtifact{ + { + Name: fmt.Sprintf("%s-git", objKey.Name), + Namespace: tgtNS.Name, + Copy: []swapi.CopyOperation{ + {From: fmt.Sprintf("@%s-git/**", objKey.Name), To: "@artifact/"}, + }, + }, + } + g.Expect(testClient.Create(ctx, obj)).To(Succeed()) + + gitFiles := []gotktestsrv.File{ + {Name: "app.yaml", Body: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config"}, + } + g.Expect(applyGitRepository(objKey, "main@sha256:abc123", gitFiles)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + // Deleting the ArtifactGenerator deletes the ExternalArtifact but the + // namespace delete is rejected, so the finalizer is retained and the + // namespace stays tracked in the inventory. + g.Expect(testClient.Get(ctx, objKey, obj)).To(Succeed()) + g.Expect(testClient.Delete(ctx, obj)).To(Succeed()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).To(HaveOccurred()) + + g.Expect(testClient.Get(ctx, objKey, obj)).To(Succeed()) + g.Expect(obj.Finalizers).To(ContainElement(swapi.Finalizer)) + g.Expect(obj.Status.Inventory).To(HaveLen(1)) + g.Expect(obj.Status.Inventory[0].Kind).To(Equal(swapi.NamespaceKind)) + g.Expect(gotkconditions.GetReason(obj, gotkmeta.ReadyCondition)).To(Equal(gotkmeta.PruneFailedReason)) + + // Grant delete and retry: the finalizer is removed and the namespace deleted. + role := &rbacv1.ClusterRole{} + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: saName}, role)).To(Succeed()) + role.Rules[0].Verbs = append(role.Rules[0].Verbs, "delete") + g.Expect(testClient.Update(ctx, role)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(apierrors.IsNotFound(testClient.Get(ctx, objKey, &swapi.ArtifactGenerator{}))).To(BeTrue()) + expectNamespaceDeleted(g, ctx, tgtNS.Name) +} + +// grantNamespaceManagement grants an existing ServiceAccount permission to +// manage Namespace objects at cluster scope. When no verbs are provided, all +// the namespace verbs are granted. +func grantNamespaceManagement(ctx context.Context, saName, saNamespace string, verbs ...string) error { + if len(verbs) == 0 { + verbs = []string{"get", "list", "watch", "create", "update", "patch", "delete"} + } + role := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: saName}, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"namespaces"}, + Verbs: verbs, + }, + }, + } + if err := testClient.Create(ctx, role); err != nil { + return err + } + + binding := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: saName}, + Subjects: []rbacv1.Subject{ + { + Kind: rbacv1.ServiceAccountKind, + Name: saName, + Namespace: saNamespace, + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "ClusterRole", + Name: saName, + }, + } + return testClient.Create(ctx, binding) +} + // createImpersonationRBAC creates a ServiceAccount in saNamespace and grants -// it permission to manage ExternalArtifacts in the target namespace. -func createImpersonationRBAC(ctx context.Context, saName, saNamespace, targetNamespace string) error { +// it permission to manage ExternalArtifacts in the given target namespaces. +func createImpersonationRBAC(ctx context.Context, saName, saNamespace string, targetNamespaces ...string) error { sa := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: saName, @@ -387,6 +1068,17 @@ func createImpersonationRBAC(ctx context.Context, saName, saNamespace, targetNam return err } + for _, targetNamespace := range targetNamespaces { + if err := grantImpersonationInNamespace(ctx, saName, saNamespace, targetNamespace); err != nil { + return err + } + } + return nil +} + +// grantImpersonationInNamespace grants an existing ServiceAccount permission +// to manage ExternalArtifacts in the target namespace. +func grantImpersonationInNamespace(ctx context.Context, saName, saNamespace, targetNamespace string) error { role := &rbacv1.Role{ ObjectMeta: metav1.ObjectMeta{ Name: saName, diff --git a/internal/controller/artifactgenerator_pathpattern.go b/internal/controller/artifactgenerator_pathpattern.go index fe3f1bfa..fb0330fe 100644 --- a/internal/controller/artifactgenerator_pathpattern.go +++ b/internal/controller/artifactgenerator_pathpattern.go @@ -109,8 +109,14 @@ func buildArtifactRequests(obj *swapi.ArtifactGenerator, localSources map[string } var reqs []artifactRequest - // Track rendered artifact names to detect collisions after lowercasing. - seenNames := make(map[string]string) + // Track rendered artifacts to detect collisions after lowercasing. + // Artifacts are uniquely identified by namespace and name, so the same + // name may be reused in different namespaces. + type artifactKey struct { + namespace string + name string + } + seenArtifacts := make(map[artifactKey]string) err = filepath.WalkDir(srcDir, func(path string, d fs.DirEntry, err error) error { if err != nil { @@ -172,13 +178,23 @@ func buildArtifactRequests(obj *swapi.ArtifactGenerator, localSources map[string obj.Spec.PathPattern, req.Name, strings.Join(errs, "; ")) } - // Validate for duplicate names. - if prevDir, exists := seenNames[req.Name]; exists { + // Validate the rendered EA namespace with ValidateNamespaceName. + if req.Namespace != "" { + if errs := apivalidation.ValidateNamespaceName(req.Namespace, false); len(errs) > 0 { + return newTerminalPathPatternError( + "pathPattern %q: rendered artifact namespace %q is not a valid Kubernetes namespace: %s", + obj.Spec.PathPattern, req.Namespace, strings.Join(errs, "; ")) + } + } + + // Validate for duplicate names within the same namespace. + key := artifactKey{namespace: obj.GetArtifactNamespace(&req.OutputArtifact), name: req.Name} + if prevDir, exists := seenArtifacts[key]; exists { return newTerminalPathPatternError( - "pathPattern %q: directories %q and %q both resolve to artifact name %q", - obj.Spec.PathPattern, prevDir, rel, req.Name) + "pathPattern %q: directories %q and %q both resolve to artifact name %q in namespace %q", + obj.Spec.PathPattern, prevDir, rel, req.Name, key.namespace) } - seenNames[req.Name] = rel + seenArtifacts[key] = rel reqs = append(reqs, req) } @@ -226,18 +242,25 @@ func validatePathPatternCaptures(pathPattern, patternStr string) error { } // renderArtifactRequest creates an artifactRequest by substituting capture placeholders. -// Artifact names and labels use normalized captures, while copy paths use raw captures. +// Artifact names, namespaces, and labels use normalized captures, while copy paths +// use raw captures. func renderArtifactRequest(oa swapi.OutputArtifact, normalizedCaptures, rawCaptures map[string]string) (artifactRequest, error) { name, err := renderTemplateString(oa.Name, normalizedCaptures) if err != nil { return artifactRequest{}, err } + namespace, err := renderTemplateString(oa.Namespace, normalizedCaptures) + if err != nil { + return artifactRequest{}, err + } + req := artifactRequest{ OutputArtifact: oa, Labels: normalizedCaptures, } req.Name = name + req.Namespace = namespace // Deep copy the Copy slice to avoid mutating the original OutputArtifact spec, // and substitute capture placeholders in each copy operation. diff --git a/internal/controller/artifactgenerator_pathpattern_test.go b/internal/controller/artifactgenerator_pathpattern_test.go index 9027b6e1..eb885b77 100644 --- a/internal/controller/artifactgenerator_pathpattern_test.go +++ b/internal/controller/artifactgenerator_pathpattern_test.go @@ -191,6 +191,139 @@ func TestBuildArtifactRequests(t *testing.T) { } }) + t.Run("renders artifact namespace from captures", func(t *testing.T) { + g := gomega.NewWithT(t) + obj := &swapi.ArtifactGenerator{ + Spec: swapi.ArtifactGeneratorSpec{ + PathPattern: "@repo/apps/{app}/envs/{env}", + OutputArtifacts: []swapi.OutputArtifact{ + { + Name: "{app}-{env}", + Namespace: "{env}-ns", + Copy: []swapi.CopyOperation{ + {From: "apps/{app}/envs/{env}", To: "."}, + }, + }, + }, + }, + } + reqs, err := buildArtifactRequests(obj, localSources) + g.Expect(err).ToNot(gomega.HaveOccurred()) + g.Expect(reqs).To(gomega.HaveLen(2)) + + namespaces := []string{reqs[0].Namespace, reqs[1].Namespace} + g.Expect(namespaces).To(gomega.ConsistOf("dev-ns", "prod-ns")) + }) + + t.Run("static artifact namespace with captures in name", func(t *testing.T) { + g := gomega.NewWithT(t) + obj := &swapi.ArtifactGenerator{ + Spec: swapi.ArtifactGeneratorSpec{ + PathPattern: "@repo/apps/{app}/envs/{env}", + OutputArtifacts: []swapi.OutputArtifact{ + { + Name: "{app}-{env}", + Namespace: "static-ns", + Copy: []swapi.CopyOperation{ + {From: "apps/{app}/envs/{env}", To: "."}, + }, + }, + }, + }, + } + reqs, err := buildArtifactRequests(obj, localSources) + g.Expect(err).ToNot(gomega.HaveOccurred()) + g.Expect(reqs).To(gomega.HaveLen(2)) + for _, r := range reqs { + g.Expect(r.Namespace).To(gomega.Equal("static-ns")) + } + }) + + t.Run("invalid rendered namespace", func(t *testing.T) { + g := gomega.NewWithT(t) + obj := &swapi.ArtifactGenerator{ + Spec: swapi.ArtifactGeneratorSpec{ + PathPattern: "@repo/apps/{app}/envs/{env}", + OutputArtifacts: []swapi.OutputArtifact{ + { + Name: "{app}-{env}", + Namespace: "ns_{env}", + Copy: []swapi.CopyOperation{ + {From: "apps/{app}/envs/{env}", To: "."}, + }, + }, + }, + }, + } + _, err := buildArtifactRequests(obj, localSources) + g.Expect(err).To(gomega.HaveOccurred()) + g.Expect(isTerminalPathPatternError(err)).To(gomega.BeTrue()) + g.Expect(err.Error()).To(gomega.ContainSubstring("not a valid Kubernetes namespace")) + }) + + t.Run("unknown capture placeholder in namespace", func(t *testing.T) { + g := gomega.NewWithT(t) + obj := &swapi.ArtifactGenerator{ + Spec: swapi.ArtifactGeneratorSpec{ + PathPattern: "@repo/apps/{app}", + OutputArtifacts: []swapi.OutputArtifact{ + {Name: "app-{app}", Namespace: "{missing}"}, + }, + }, + } + _, err := buildArtifactRequests(obj, localSources) + g.Expect(err).To(gomega.HaveOccurred()) + g.Expect(err.Error()).To(gomega.ContainSubstring("capture variable \"missing\" is not defined by pathPattern")) + }) + + t.Run("same artifact name in different namespaces", func(t *testing.T) { + g := gomega.NewWithT(t) + obj := &swapi.ArtifactGenerator{ + Spec: swapi.ArtifactGeneratorSpec{ + PathPattern: "@repo/apps/{app}/envs/{env}", + OutputArtifacts: []swapi.OutputArtifact{ + { + Name: "{app}", + Namespace: "{env}", + Copy: []swapi.CopyOperation{ + {From: "apps/{app}/envs/{env}", To: "."}, + }, + }, + }, + }, + } + reqs, err := buildArtifactRequests(obj, localSources) + g.Expect(err).ToNot(gomega.HaveOccurred()) + g.Expect(reqs).To(gomega.HaveLen(2)) + for _, r := range reqs { + g.Expect(r.Name).To(gomega.Equal("auth")) + } + namespaces := []string{reqs[0].Namespace, reqs[1].Namespace} + g.Expect(namespaces).To(gomega.ConsistOf("dev", "prod")) + }) + + t.Run("duplicate name in same namespace", func(t *testing.T) { + g := gomega.NewWithT(t) + obj := &swapi.ArtifactGenerator{ + Spec: swapi.ArtifactGeneratorSpec{ + PathPattern: "@repo/apps/{app}/envs/{env}", + OutputArtifacts: []swapi.OutputArtifact{ + { + Name: "{app}", + Namespace: "fixed-ns", + Copy: []swapi.CopyOperation{ + {From: "apps/{app}/envs/{env}", To: "."}, + }, + }, + }, + }, + } + _, err := buildArtifactRequests(obj, localSources) + g.Expect(err).To(gomega.HaveOccurred()) + g.Expect(err.Error()).To(gomega.ContainSubstring("both resolve to artifact name")) + g.Expect(err.Error()).To(gomega.ContainSubstring("in namespace \"fixed-ns\"")) + }) + t.Run("unknown capture placeholder", func(t *testing.T) { g := gomega.NewWithT(t) obj := &swapi.ArtifactGenerator{ diff --git a/internal/controller/artifactgenerator_validation.go b/internal/controller/artifactgenerator_validation.go index 76e020dd..af75a61a 100644 --- a/internal/controller/artifactgenerator_validation.go +++ b/internal/controller/artifactgenerator_validation.go @@ -46,15 +46,21 @@ func (r *ArtifactGeneratorReconciler) validateSpec(obj *swapi.ArtifactGenerator) } } - // Validate output artifact. - nameMap := make(map[string]bool) + // Validate output artifacts. Artifacts are uniquely identified by their + // namespace and name, as the same name may be used in different namespaces. + type artifactKey struct { + namespace string + name string + } + artifactMap := make(map[artifactKey]bool) for _, artifact := range obj.Spec.OutputArtifacts { - // Check for duplicate artifact names. - if nameMap[artifact.Name] { + key := artifactKey{namespace: obj.GetArtifactNamespace(&artifact), name: artifact.Name} + if artifactMap[key] { return r.newTerminalErrorFor(obj, swapi.ValidationFailedReason, - "duplicate artifact name '%s' found", artifact.Name) + "duplicate artifact name '%s' found in namespace '%s'", artifact.Name, key.namespace) } + artifactMap[key] = true if obj.Spec.PathPattern == "" { if errs := apivalidation.NameIsDNSSubdomain(artifact.Name, false); len(errs) > 0 { @@ -63,6 +69,15 @@ func (r *ArtifactGeneratorReconciler) validateSpec(obj *swapi.ArtifactGenerator) "artifact name %q is not a valid Kubernetes object name: %s", artifact.Name, strings.Join(errs, "; ")) } + + if artifact.Namespace != "" { + if errs := apivalidation.ValidateNamespaceName(artifact.Namespace, false); len(errs) > 0 { + return r.newTerminalErrorFor(obj, + swapi.ValidationFailedReason, + "artifact namespace %q is not a valid Kubernetes namespace: %s", + artifact.Namespace, strings.Join(errs, "; ")) + } + } } // Check that the revision source alias exists. @@ -72,7 +87,6 @@ func (r *ArtifactGeneratorReconciler) validateSpec(obj *swapi.ArtifactGenerator) "artifact %s revision source alias '%s' not found", artifact.Name, strings.TrimPrefix(artifact.Revision, "@")) } - nameMap[artifact.Name] = true } return nil diff --git a/internal/controller/artifactgenerator_validation_test.go b/internal/controller/artifactgenerator_validation_test.go index 28d75b62..65ac450a 100644 --- a/internal/controller/artifactgenerator_validation_test.go +++ b/internal/controller/artifactgenerator_validation_test.go @@ -19,6 +19,7 @@ package controller import ( "context" "errors" + "strings" "testing" . "github.com/onsi/gomega" @@ -316,6 +317,50 @@ func TestArtifactGenerator_crdValidation(t *testing.T) { }, expectError: true, }, + { + name: "templated artifact namespace without pathPattern", + setupObj: func() *swapi.ArtifactGenerator { + objKey := client.ObjectKey{ + Name: "test-templated-artifact-namespace-no-pattern", + Namespace: ns.Name, + } + obj := getArtifactGenerator(objKey) + obj.Spec.OutputArtifacts[0].Namespace = "{env}" + return obj + }, + expectError: true, + }, + { + name: "artifact namespace exceeding max length", + setupObj: func() *swapi.ArtifactGenerator { + objKey := client.ObjectKey{ + Name: "test-long-artifact-namespace", + Namespace: ns.Name, + } + obj := getArtifactGenerator(objKey) + obj.Spec.OutputArtifacts[0].Namespace = strings.Repeat("a", 64) + return obj + }, + expectError: true, + }, + { + name: "templated artifact namespace with pathPattern", + setupObj: func() *swapi.ArtifactGenerator { + objKey := client.ObjectKey{ + Name: "test-templated-artifact-namespace", + Namespace: ns.Name, + } + obj := getArtifactGenerator(objKey) + alias := obj.Spec.Sources[0].Alias + obj.Spec.PathPattern = "@" + alias + "/apps/{app}/envs/{env}" + obj.Spec.OutputArtifacts[0].Name = "{app}-{env}" + obj.Spec.OutputArtifacts[0].Namespace = "{env}" + obj.Spec.OutputArtifacts[0].Copy[0].From = "@" + alias + "/apps/{app}/envs/{env}/**" + obj.Spec.OutputArtifacts[0].Copy[0].To = "@artifact/" + return obj + }, + expectError: false, + }, { name: "valid service account name", setupObj: func() *swapi.ArtifactGenerator { @@ -342,6 +387,49 @@ func TestArtifactGenerator_crdValidation(t *testing.T) { }, expectError: true, }, + { + name: "valid namespace strategy managed", + setupObj: func() *swapi.ArtifactGenerator { + objKey := client.ObjectKey{ + Name: "test-valid-namespace-strategy", + Namespace: ns.Name, + } + obj := getArtifactGenerator(objKey) + prune := false + obj.Spec.Namespaces = &swapi.Namespaces{ + Strategy: swapi.NamespaceStrategyManaged, + Prune: &prune, + } + return obj + }, + expectError: false, + }, + { + name: "invalid namespace strategy name", + setupObj: func() *swapi.ArtifactGenerator { + objKey := client.ObjectKey{ + Name: "test-invalid-namespace-strategy", + Namespace: ns.Name, + } + obj := getArtifactGenerator(objKey) + obj.Spec.Namespaces = &swapi.Namespaces{Strategy: "Invalid"} + return obj + }, + expectError: true, + }, + { + name: "namespace strategy missing name", + setupObj: func() *swapi.ArtifactGenerator { + objKey := client.ObjectKey{ + Name: "test-namespace-strategy-missing-name", + Namespace: ns.Name, + } + obj := getArtifactGenerator(objKey) + obj.Spec.Namespaces = &swapi.Namespaces{} + return obj + }, + expectError: true, + }, } for _, tt := range tests {