diff --git a/api/v1beta1/artifactgenerator_types.go b/api/v1beta1/artifactgenerator_types.go index 9d169e0..faaaedf 100644 --- a/api/v1beta1/artifactgenerator_types.go +++ b/api/v1beta1/artifactgenerator_types.go @@ -35,6 +35,7 @@ const ( AccessDeniedReason = "AccessDenied" ValidationFailedReason = "ValidationFailed" SourceFetchFailedReason = "SourceFetchFailed" + OwnershipConflictReason = "OwnershipConflict" OverwriteStrategy = "Overwrite" MergeStrategy = "Merge" ExtractStrategy = "Extract" @@ -69,6 +70,22 @@ type ArtifactGeneratorSpec struct { // +required 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. + // +kubebuilder:validation:Pattern="^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=63 + // +optional + ServiceAccountName string `json:"serviceAccountName,omitempty"` + // PathPattern specifies a directory traversal pattern to match within the sources. // The format is "@/". Named captures in the pattern (e.g. "{app}") // can be used as placeholders in OutputArtifacts fields. @@ -125,6 +142,16 @@ type OutputArtifact struct { // +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 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 + // +optional + Namespace string `json:"namespace,omitempty"` + // Revision is the revision of the generated artifact. // If specified, it must point to an existing source alias in the format "@". // If not specified, the revision is automatically set to the digest of the artifact content. @@ -266,6 +293,16 @@ func (in *ArtifactGenerator) IsDisabled() bool { return ok && strings.ToLower(val) == DisabledValue } +// GetArtifactNamespace returns the namespace where the ExternalArtifact +// generated for the given OutputArtifact is created. It defaults to the +// ArtifactGenerator namespace. +func (in *ArtifactGenerator) GetArtifactNamespace(outputArtifact *OutputArtifact) string { + if outputArtifact.Namespace != "" { + return outputArtifact.Namespace + } + return in.Namespace +} + // HasArtifactInInventory returns true if the artifact with the given // kind, name, namespace, and digest exists in the inventory. func (in *ArtifactGenerator) HasArtifactInInventory(name, namespace, digest string) bool { diff --git a/api/v1beta1/artifactgenerator_types_test.go b/api/v1beta1/artifactgenerator_types_test.go new file mode 100644 index 0000000..434e8f2 --- /dev/null +++ b/api/v1beta1/artifactgenerator_types_test.go @@ -0,0 +1,36 @@ +/* +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 v1beta1_test + +import ( + "testing" + + "github.com/fluxcd/source-watcher/api/v2/v1beta1" +) + +func TestArtifactGeneratorGetArtifactNamespace(t *testing.T) { + obj := &v1beta1.ArtifactGenerator{} + obj.Namespace = "generator-ns" + + if got := obj.GetArtifactNamespace(&v1beta1.OutputArtifact{}); got != "generator-ns" { + t.Errorf("GetArtifactNamespace() = %q, want %q", got, "generator-ns") + } + + if got := obj.GetArtifactNamespace(&v1beta1.OutputArtifact{Namespace: "target-ns"}); got != "target-ns" { + t.Errorf("GetArtifactNamespace() = %q, want %q", got, "target-ns") + } +} diff --git a/cmd/main.go b/cmd/main.go index 5b6dcc2..bb80115 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -80,6 +80,7 @@ func main() { httpRetry int reconciliationTimeout time.Duration requeueDependency time.Duration + defaultServiceAccount string // GitOps Toolkit (gotk) runtime options. // https://pkg.go.dev/github.com/fluxcd/pkg/runtime @@ -105,6 +106,8 @@ func main() { "The maximum duration of a reconciliation.") 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.") aclOptions.BindFlags(flag.CommandLine) artifactOptions.BindFlags(flag.CommandLine) @@ -217,6 +220,7 @@ func main() { DependencyRequeueInterval: requeueDependency, DirectSourceFetch: directSourceFetch, NoCrossNamespaceRefs: aclOptions.NoCrossNamespaceRefs, + DefaultServiceAccount: defaultServiceAccount, }).SetupWithManager(ctx, mgr, controller.ArtifactGeneratorReconcilerOptions{ RateLimiter: gotkctrl.GetRateLimiter(rateLimiterOptions), }); err != nil { diff --git a/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml b/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml index 64f81b8..67dc28b 100644 --- a/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml +++ b/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml @@ -135,6 +135,16 @@ spec: maxLength: 253 minLength: 1 type: string + namespace: + description: |- + Namespace is the namespace of the generated artifact. + If not provided, defaults to the same namespace as the ArtifactGenerator. + 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])?$ + type: string originRevision: description: |- OriginRevision is used to set the 'org.opencontainers.image.revision' @@ -186,6 +196,22 @@ spec: maxLength: 1024 pattern: ^@([a-z0-9]([a-z0-9_-]*[a-z0-9])?)/(.*)$ type: string + 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. + maxLength: 63 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + type: string sources: description: |- Sources is a list of references to the Flux source-controller diff --git a/docs/README.md b/docs/README.md index e6d5e53..5b61fc1 100644 --- a/docs/README.md +++ b/docs/README.md @@ -14,6 +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. | | `--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 03535b9..308d434 100644 --- a/docs/spec/v1beta1/artifactgenerators.md +++ b/docs/spec/v1beta1/artifactgenerators.md @@ -281,6 +281,9 @@ 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). - `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. @@ -436,6 +439,55 @@ Any existing label or annotation on the generated resources will be overridden i a common one. Note that the `app.kubernetes.io/managed-by` and `source.extensions.fluxcd.io/generator` labels are reserved by the controller and cannot be overridden by common metadata. +### Cross-namespace Artifacts + +By default, the generated ExternalArtifacts are created in the same namespace as the +ArtifactGenerator. The `.spec.artifacts[].namespace` field can be used to create an +ExternalArtifact in a different namespace. This is useful for multi-tenant clusters +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. + +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. + +For example, the following generator creates an ExternalArtifact in the `tenant-app` +namespace, using the `tenant-artifacts` ServiceAccount: + +```yaml +apiVersion: source.extensions.fluxcd.io/v1beta1 +kind: ArtifactGenerator +metadata: + name: tenant-app + namespace: flux-system +spec: + serviceAccountName: tenant-artifacts + sources: + - alias: repo + kind: GitRepository + name: my-monorepo + artifacts: + - name: tenant-app + namespace: tenant-app + copy: + - from: "@repo/tenants/tenant-app/**" + to: "@artifact/" +``` + +**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. + ## Working with ArtifactGenerators ### Suspend and Resume Reconciliation @@ -563,6 +615,7 @@ Events are emitted for the following scenarios: - 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). 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 dc6c140..2c19f63 100644 --- a/internal/controller/artifactgenerator_controller.go +++ b/internal/controller/artifactgenerator_controller.go @@ -40,6 +40,7 @@ import ( gotkmeta "github.com/fluxcd/pkg/apis/meta" gotkstroage "github.com/fluxcd/pkg/artifact/storage" gotkfetch "github.com/fluxcd/pkg/http/fetch" + gotkclient "github.com/fluxcd/pkg/runtime/client" gotkconditions "github.com/fluxcd/pkg/runtime/conditions" gotkjitter "github.com/fluxcd/pkg/runtime/jitter" gotkpatch "github.com/fluxcd/pkg/runtime/patch" @@ -63,6 +64,7 @@ type ArtifactGeneratorReconciler struct { DependencyRequeueInterval time.Duration NoCrossNamespaceRefs bool DirectSourceFetch bool + DefaultServiceAccount string } // +kubebuilder:rbac:groups=source.extensions.fluxcd.io,resources=artifactgenerators,verbs=get;list;watch;create;update;patchStatus;delete @@ -91,9 +93,29 @@ 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. + var impersonated client.Client + if r.needsImpersonation(obj) { + var err error + impersonated, err = r.newImpersonatedClient(ctx, obj) + if err != nil { + err = fmt.Errorf("failed to build the impersonation client: %w", err) + gotkconditions.MarkFalse(obj, + gotkmeta.ReadyCondition, + swapi.AccessDeniedReason, + "%s", err.Error()) + r.Event(obj, corev1.EventTypeWarning, swapi.AccessDeniedReason, err.Error()) + return ctrl.Result{}, err + } + } + // Finalize the reconciliation and release resources if the object is being deleted. if !obj.ObjectMeta.DeletionTimestamp.IsZero() { - return r.finalize(ctx, obj) + return r.finalize(ctx, obj, impersonated) } // Add the finalizer if it does not exist. @@ -115,13 +137,65 @@ func (r *ArtifactGeneratorReconciler) Reconcile(ctx context.Context, req ctrl.Re } // Run drift detection and reconciliation. - return r.reconcile(ctx, obj, patcher) + 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. +func (r *ArtifactGeneratorReconciler) needsImpersonation(obj *swapi.ArtifactGenerator) bool { + if r.DefaultServiceAccount == "" && obj.Spec.ServiceAccountName == "" { + return false + } + + for i := range obj.Spec.OutputArtifacts { + if obj.GetArtifactNamespace(&obj.Spec.OutputArtifacts[i]) != obj.Namespace { + return true + } + } + + for _, ref := range obj.Status.Inventory { + if ref.Namespace != obj.Namespace { + return true + } + } + + return false +} + +// newImpersonatedClient returns a client that impersonates the ServiceAccount +// configured on the ArtifactGenerator, or the controller default when the +// object does not specify one. +func (r *ArtifactGeneratorReconciler) newImpersonatedClient(ctx context.Context, + obj *swapi.ArtifactGenerator) (client.Client, error) { + impersonator := gotkclient.NewImpersonator(r.Client, + gotkclient.WithScheme(r.Scheme), + gotkclient.WithServiceAccount(r.DefaultServiceAccount, obj.Spec.ServiceAccountName, obj.Namespace)) + kubeClient, _, err := impersonator.GetClient(ctx) + if err != nil { + return nil, err + } + return kubeClient, nil +} + +// 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. +func (r *ArtifactGeneratorReconciler) clientForNamespace(obj *swapi.ArtifactGenerator, + impersonated client.Client, namespace string) client.Client { + if impersonated != nil && namespace != obj.Namespace { + return impersonated + } + return r.Client } // reconcile contains the main reconciliation logic for the ArtifactGenerator. func (r *ArtifactGeneratorReconciler) reconcile(ctx context.Context, obj *swapi.ArtifactGenerator, - patcher *gotkpatch.SerialPatcher) (ctrl.Result, error) { + patcher *gotkpatch.SerialPatcher, + impersonated client.Client) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx) oldObj := obj.DeepCopy() @@ -157,7 +231,7 @@ func (r *ArtifactGeneratorReconciler) reconcile(ctx context.Context, // Detect drift between the actual state and the desired state. // If no drift is detected in sources and the stored artifacts pass the // integrity verification, the reconciliation is complete and we can exit early. - hasDrifted, reason := r.detectDrift(ctx, obj, observedSourcesDigest) + 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)) log.Info(msg) @@ -218,10 +292,15 @@ func (r *ArtifactGeneratorReconciler) reconcile(ctx context.Context, // Prepare a slice to hold the references to the created ExternalArtifact objects. eaRefs := make([]swapi.ExternalArtifactReference, 0, len(reqs)) + // Collect ExternalArtifacts whose ownership is being taken over from + // another ArtifactGenerator to emit a single error log and a single + // summary event for this reconciliation. + var ownershipConflicts []ownershipConflict + for _, req := range reqs { oa := req.OutputArtifact // Build the artifact using the local sources. - artifact, err := artifactBuilder.Build(ctx, &oa, localSources, obj.Namespace, tmpDir) + artifact, err := artifactBuilder.Build(ctx, &oa, localSources, obj.GetArtifactNamespace(&oa), tmpDir) if err != nil { msg := fmt.Sprintf("%s build failed: %s", oa.Name, err.Error()) gotkconditions.MarkFalse(obj, @@ -238,7 +317,7 @@ func (r *ArtifactGeneratorReconciler) reconcile(ctx context.Context, // Reconcile the ExternalArtifact corresponding to the built artifact. // The ExternalArtifact will reference the artifact stored in the storage backend. // If the ExternalArtifact already exists, its status will be updated with the new artifact details. - eaRef, err := r.reconcileExternalArtifact(ctx, obj, &oa, artifact, req.Labels) + eaRef, conflict, err := r.reconcileExternalArtifact(ctx, obj, &oa, artifact, req.Labels, impersonated) if err != nil { msg := fmt.Sprintf("%s reconcile failed: %s", oa.Name, err.Error()) gotkconditions.MarkFalse(obj, @@ -248,12 +327,26 @@ func (r *ArtifactGeneratorReconciler) reconcile(ctx context.Context, r.Event(obj, corev1.EventTypeWarning, gotkmeta.ReconciliationFailedReason, msg) return ctrl.Result{}, err } + if conflict != nil { + ownershipConflicts = append(ownershipConflicts, *conflict) + } eaRefs = append(eaRefs, *eaRef) } + // Log the ownership conflicts once per reconciliation with the exact + // ExternalArtifact references, and emit a single warning event on the + // ArtifactGenerator with only the count to keep the message short. + if len(ownershipConflicts) > 0 { + log.Error(fmt.Errorf("ownership conflict detected for %d ExternalArtifact(s)", len(ownershipConflicts)), + "taking over ExternalArtifacts from other ArtifactGenerators", + "artifacts", ownershipConflicts) + r.Event(obj, corev1.EventTypeWarning, swapi.OwnershipConflictReason, + 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, orphans) + r.finalizeExternalArtifacts(ctx, obj, orphans, impersonated) } // Garbage collect old artifacts in storage according to the retention policy. @@ -438,14 +531,21 @@ func (r *ArtifactGeneratorReconciler) fetchSources(ctx context.Context, // reconcileExternalArtifact ensures the ExternalArtifact object // exists and is up to date with the provided artifact details. -// It returns a reference to the ExternalArtifact. +// It returns a reference to the ExternalArtifact and, when the ExternalArtifact +// was taken over from another ArtifactGenerator, the detected conflict. func (r *ArtifactGeneratorReconciler) reconcileExternalArtifact(ctx context.Context, obj *swapi.ArtifactGenerator, outputArtifact *swapi.OutputArtifact, artifact *gotkmeta.Artifact, - dynamicLabels map[string]string) (*swapi.ExternalArtifactReference, error) { + dynamicLabels map[string]string, + impersonated client.Client) (*swapi.ExternalArtifactReference, *ownershipConflict, error) { log := ctrl.LoggerFrom(ctx) + // Select the client based on the target namespace. Output artifacts in the + // ArtifactGenerator namespace are always reconciled with the controller + // client, even when a ServiceAccount is configured for impersonation. + kubeClient := r.clientForNamespace(obj, impersonated, obj.GetArtifactNamespace(outputArtifact)) + // Prepare labels for the ExternalArtifact with the managed-by and generator labels. labels := make(map[string]string) var annotations map[string]string @@ -471,7 +571,7 @@ func (r *ArtifactGeneratorReconciler) reconcileExternalArtifact(ctx context.Cont }, ObjectMeta: metav1.ObjectMeta{ Name: outputArtifact.Name, - Namespace: obj.Namespace, + Namespace: obj.GetArtifactNamespace(outputArtifact), Labels: labels, Annotations: annotations, }, @@ -485,13 +585,19 @@ func (r *ArtifactGeneratorReconciler) reconcileExternalArtifact(ctx context.Cont }, } + // Detect ownership conflicts before applying the ExternalArtifact. + conflict, err := r.detectOwnershipConflict(ctx, obj, kubeClient, ea) + if err != nil { + return nil, nil, err + } + // Apply the ExternalArtifact object. forceApply := true - if err := r.Patch(ctx, ea, client.Apply, &client.PatchOptions{ + if err := kubeClient.Patch(ctx, ea, client.Apply, &client.PatchOptions{ FieldManager: r.ControllerName, Force: &forceApply, }); err != nil { - return nil, fmt.Errorf("failed to apply ExternalArtifact: %w", err) + return nil, nil, fmt.Errorf("failed to apply ExternalArtifact: %w", err) } // Update the status of the ExternalArtifact with the artifact details. @@ -514,8 +620,8 @@ func (r *ArtifactGeneratorReconciler) reconcileExternalArtifact(ctx context.Cont FieldManager: r.ControllerName, }, } - if err := r.Status().Patch(ctx, ea, client.Apply, statusOpts); err != nil { - return nil, fmt.Errorf("failed to patchStatus ExternalArtifact status: %w", err) + if err := kubeClient.Status().Patch(ctx, ea, client.Apply, statusOpts); err != nil { + return nil, nil, fmt.Errorf("failed to patchStatus ExternalArtifact status: %w", err) } // Log if the artifact is up to date or emit an event if it is new or has changed. @@ -534,7 +640,59 @@ func (r *ArtifactGeneratorReconciler) reconcileExternalArtifact(ctx context.Cont Namespace: ea.Namespace, Digest: artifact.Digest, Filename: filepath.Base(artifact.Path), - }, nil + }, conflict, nil +} + +// ownershipConflict describes an ExternalArtifact that is being taken over +// from another ArtifactGenerator. +type ownershipConflict struct { + // ExternalArtifact is the namespace/name of the ExternalArtifact. + ExternalArtifact string `json:"externalArtifact"` + // ArtifactGenerator identifies the previous owner, as namespace/name when + // known, otherwise the generator label value. + ArtifactGenerator string `json:"artifactGenerator"` +} + +// detectOwnershipConflict checks whether the ExternalArtifact is currently +// owned by a different ArtifactGenerator. When the generator label points to +// another ArtifactGenerator, the ownership is about to flip: a warning event +// is emitted on the ExternalArtifact so that accidental overlaps are visible +// to users. The ownership transfer is allowed to proceed to support moving +// ExternalArtifacts from one ArtifactGenerator to another. The caller +// aggregates the returned conflicts into a single log line and a single +// ArtifactGenerator event. +func (r *ArtifactGeneratorReconciler) detectOwnershipConflict(ctx context.Context, + obj *swapi.ArtifactGenerator, + kubeClient client.Client, + ea *sourcev1.ExternalArtifact) (*ownershipConflict, error) { + existing := &sourcev1.ExternalArtifact{} + err := kubeClient.Get(ctx, client.ObjectKeyFromObject(ea), existing) + if apierrors.IsNotFound(err) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("failed to get ExternalArtifact: %w", err) + } + + owner := existing.Labels[swapi.ArtifactGeneratorLabel] + if owner == "" || owner == string(obj.GetUID()) { + return nil, nil + } + + previous := owner + if ref := existing.Spec.SourceRef; ref != nil { + previous = fmt.Sprintf("%s/%s", ref.Namespace, ref.Name) + } + conflict := &ownershipConflict{ + ExternalArtifact: fmt.Sprintf("%s/%s", existing.Namespace, existing.Name), + ArtifactGenerator: previous, + } + + msg := fmt.Sprintf("ExternalArtifact %s is owned by ArtifactGenerator %s and is being taken over by %s/%s", + conflict.ExternalArtifact, conflict.ArtifactGenerator, obj.Namespace, obj.Name) + r.Event(existing, corev1.EventTypeWarning, swapi.OwnershipConflictReason, msg) + + return conflict, nil } // findOrphanedReferences identifies ExternalArtifact references diff --git a/internal/controller/artifactgenerator_drift.go b/internal/controller/artifactgenerator_drift.go index 02cbbb7..8710fb7 100644 --- a/internal/controller/artifactgenerator_drift.go +++ b/internal/controller/artifactgenerator_drift.go @@ -45,7 +45,8 @@ import ( // - "NoDriftDetected" - no drift detected and the storage is up to date func (r *ArtifactGeneratorReconciler) detectDrift(ctx context.Context, obj *swapi.ArtifactGenerator, - currentSourcesDigest string) (bool, string) { + currentSourcesDigest string, + impersonated client.Client) (bool, string) { // Setup logger on debug level. log := ctrl.LoggerFrom(ctx).V(1) @@ -98,7 +99,7 @@ func (r *ArtifactGeneratorReconciler) detectDrift(ctx context.Context, } } - eaDrift, err := r.detectExternalArtifactsDrift(ctx, obj) + eaDrift, err := r.detectExternalArtifactsDrift(ctx, obj, impersonated) if err != nil { log.Error(err, "Failed to verify in-cluster external artifacts for drift") return true, "ExternalArtifactsNotFound" @@ -114,27 +115,44 @@ func (r *ArtifactGeneratorReconciler) detectDrift(ctx context.Context, // detectExternalArtifactsDrift checks if any ExternalArtifact objects // managed by the ArtifactGenerator have been modified or deleted. func (r *ArtifactGeneratorReconciler) detectExternalArtifactsDrift(ctx context.Context, - obj *swapi.ArtifactGenerator) (bool, error) { - - eaList := &sourcev1.ExternalArtifactList{} - if err := r.List(ctx, eaList, client.InNamespace(obj.Namespace), - client.MatchingLabels{ - swapi.ArtifactGeneratorLabel: string(obj.GetUID()), - }); err != nil { - return true, fmt.Errorf("error listing external artifacts: %w", err) + obj *swapi.ArtifactGenerator, + impersonated client.Client) (bool, error) { + + // Group the inventory references by namespace, as the generated + // ExternalArtifacts may live in namespaces other than the one of + // the ArtifactGenerator. + namespaces := make(map[string]struct{}) + for _, ref := range obj.Status.Inventory { + namespaces[ref.Namespace] = struct{}{} } - // Check if the number of ExternalArtifacts in the cluster matches the inventory - if len(eaList.Items) != len(obj.Status.Inventory) { - return true, nil - } + // Check if the number of ExternalArtifacts in the cluster matches the inventory. + total := 0 + for namespace := range namespaces { + // Select the client based on the namespace of the ExternalArtifacts. + kubeClient := r.clientForNamespace(obj, impersonated, namespace) + + eaList := &sourcev1.ExternalArtifactList{} + if err := kubeClient.List(ctx, eaList, client.InNamespace(namespace), + client.MatchingLabels{ + swapi.ArtifactGeneratorLabel: string(obj.GetUID()), + }); err != nil { + return true, fmt.Errorf("error listing external artifacts: %w", err) + } + total += len(eaList.Items) - // Check if the ExternalArtifacts in the cluster match the inventory - for _, ea := range eaList.Items { - if !obj.HasArtifactInInventory(ea.Name, ea.Namespace, ea.Status.Artifact.Digest) { - return true, nil + // Check if the ExternalArtifacts in the cluster match the inventory. + for _, ea := range eaList.Items { + if ea.Status.Artifact == nil || + !obj.HasArtifactInInventory(ea.Name, ea.Namespace, ea.Status.Artifact.Digest) { + return true, nil + } } } + if total != len(obj.Status.Inventory) { + return true, nil + } + return false, nil } diff --git a/internal/controller/artifactgenerator_drift_test.go b/internal/controller/artifactgenerator_drift_test.go index 8f740b1..862cf05 100644 --- a/internal/controller/artifactgenerator_drift_test.go +++ b/internal/controller/artifactgenerator_drift_test.go @@ -74,11 +74,11 @@ func TestArtifactGeneratorReconciler_DetectDrift(t *testing.T) { g.Expect(artifact).ToNot(BeNil()) // Generate the ExternalArtifact in cluster - _, err = reconciler.reconcileExternalArtifact(ctx, &swapi.ArtifactGenerator{ + _, _, err = reconciler.reconcileExternalArtifact(ctx, &swapi.ArtifactGenerator{ ObjectMeta: metav1.ObjectMeta{ Name: "test-generator", Namespace: ns.Name, - }}, outputArtifact, artifact, nil) + }}, outputArtifact, artifact, nil, nil) g.Expect(err).ToNot(HaveOccurred()) tests := []struct { @@ -367,7 +367,7 @@ func TestArtifactGeneratorReconciler_DetectDrift(t *testing.T) { tt.setupFunc() } - hasDrift, reason := reconciler.detectDrift(ctx, tt.obj, tt.currentDigest) + hasDrift, reason := reconciler.detectDrift(ctx, tt.obj, tt.currentDigest, nil) gt.Expect(hasDrift).To(Equal(tt.expectedDrift)) gt.Expect(reason).To(Equal(tt.expectedReason)) }) diff --git a/internal/controller/artifactgenerator_finalize.go b/internal/controller/artifactgenerator_finalize.go index d341a63..cbf1b3e 100644 --- a/internal/controller/artifactgenerator_finalize.go +++ b/internal/controller/artifactgenerator_finalize.go @@ -24,6 +24,7 @@ import ( 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" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" gotkmeta "github.com/fluxcd/pkg/apis/meta" @@ -36,11 +37,12 @@ import ( // finalize handles the finalization of the object during deletion. func (r *ArtifactGeneratorReconciler) finalize(ctx context.Context, - obj *swapi.ArtifactGenerator) (ctrl.Result, error) { + obj *swapi.ArtifactGenerator, + impersonated client.Client) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx) // Delete ExternalArtifacts found in the inventory. - r.finalizeExternalArtifacts(ctx, obj.Status.Inventory) + r.finalizeExternalArtifacts(ctx, obj, obj.Status.Inventory, impersonated) // Remove the finalizer. controllerutil.RemoveFinalizer(obj, swapi.Finalizer) @@ -53,7 +55,9 @@ func (r *ArtifactGeneratorReconciler) finalize(ctx context.Context, // referenced in the provided list, along with their associated // artifacts in the storage backend. func (r *ArtifactGeneratorReconciler) finalizeExternalArtifacts(ctx context.Context, - refs []swapi.ExternalArtifactReference) { + obj *swapi.ArtifactGenerator, + refs []swapi.ExternalArtifactReference, + impersonated client.Client) { log := ctrl.LoggerFrom(ctx) for _, eaRef := range refs { @@ -73,7 +77,7 @@ func (r *ArtifactGeneratorReconciler) finalizeExternalArtifacts(ctx context.Cont Namespace: eaRef.Namespace, }, } - err = r.Client.Delete(ctx, ea) + err = r.clientForNamespace(obj, impersonated, eaRef.Namespace).Delete(ctx, ea) if err != nil && !apierrors.IsNotFound(err) { log.Error(err, "Failed to delete ExternalArtifact") } else { diff --git a/internal/controller/artifactgenerator_namespace_test.go b/internal/controller/artifactgenerator_namespace_test.go new file mode 100644 index 0000000..456122b --- /dev/null +++ b/internal/controller/artifactgenerator_namespace_test.go @@ -0,0 +1,431 @@ +/* +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" + "fmt" + "testing" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + gotkmeta "github.com/fluxcd/pkg/apis/meta" + gotkconditions "github.com/fluxcd/pkg/runtime/conditions" + gotktestsrv "github.com/fluxcd/pkg/testserver" + sourcev1 "github.com/fluxcd/source-controller/api/v1" + + swapi "github.com/fluxcd/source-watcher/api/v2/v1beta1" +) + +func TestArtifactGeneratorReconciler_CrossNamespace(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-agns-src") + g.Expect(err).ToNot(HaveOccurred()) + tgtNS, err := testEnv.CreateNamespace(ctx, "test-agns-tgt") + g.Expect(err).ToNot(HaveOccurred()) + + // The ServiceAccount lives in the ArtifactGenerator namespace and is + // granted access to the target namespace through a RoleBinding. + saName := "artifact-generator" + g.Expect(createImpersonationRBAC(ctx, saName, srcNS.Name, tgtNS.Name)).To(Succeed()) + + objKey := client.ObjectKey{Name: "test-agns", 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), + 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()) + + // Add the finalizer. + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + // Build the artifact 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(obj.Status.Inventory).To(HaveLen(1)) + g.Expect(obj.Status.Inventory[0].Namespace).To(Equal(tgtNS.Name)) + + // The ExternalArtifact must exist in the target namespace only. + eaKey := client.ObjectKey{Name: fmt.Sprintf("%s-git", objKey.Name), Namespace: tgtNS.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)) + + err = testClient.Get(ctx, client.ObjectKey{Name: eaKey.Name, Namespace: srcNS.Name}, &sourcev1.ExternalArtifact{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + + // Deleting the ArtifactGenerator must remove the ExternalArtifact + // from the target namespace. + g.Expect(testClient.Delete(ctx, obj)).To(Succeed()) + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + 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() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-agns-def-src") + g.Expect(err).ToNot(HaveOccurred()) + tgtNS, err := testEnv.CreateNamespace(ctx, "test-agns-def-tgt") + g.Expect(err).ToNot(HaveOccurred()) + + // Multi-tenancy lockdown: the controller default ServiceAccount is used + // when the ArtifactGenerator does not specify one. + saName := "artifact-generator-default" + reconciler.DefaultServiceAccount = saName + g.Expect(createImpersonationRBAC(ctx, saName, srcNS.Name, tgtNS.Name)).To(Succeed()) + + objKey := client.ObjectKey{Name: "test-agns-def", 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), + 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()) + + 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_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. + objKey := client.ObjectKey{Name: "test-agns-same", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.ServiceAccountName = "missing-sa" + 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/"}, + }, + }, + { + Name: fmt.Sprintf("%s-git-same-ns", objKey.Name), + Namespace: srcNS.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()) + + // 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 _, name := range []string{ + fmt.Sprintf("%s-git", objKey.Name), + fmt.Sprintf("%s-git-same-ns", objKey.Name), + } { + ea := &sourcev1.ExternalArtifact{} + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: name, Namespace: srcNS.Name}, ea)).To(Succeed()) + g.Expect(ea.Status.Artifact).ToNot(BeNil()) + } +} + +func TestArtifactGeneratorReconciler_OwnershipConflict(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-agns-owner-src") + g.Expect(err).ToNot(HaveOccurred()) + tgtNS, err := testEnv.CreateNamespace(ctx, "test-agns-owner-tgt") + g.Expect(err).ToNot(HaveOccurred()) + + // The ServiceAccount lives in the ArtifactGenerator namespace and is + // granted access to the target namespace through a RoleBinding. + saName := "artifact-generator-owner" + g.Expect(createImpersonationRBAC(ctx, saName, srcNS.Name, tgtNS.Name)).To(Succeed()) + + objKey := client.ObjectKey{Name: "test-agns-owner", Namespace: srcNS.Name} + obj := getArtifactGenerator(objKey) + obj.Spec.ServiceAccountName = saName + obj.Spec.Sources = obj.Spec.Sources[:1] + eaNames := []string{ + fmt.Sprintf("%s-git", objKey.Name), + fmt.Sprintf("%s-git-second", objKey.Name), + } + obj.Spec.OutputArtifacts = nil + for _, name := range eaNames { + obj.Spec.OutputArtifacts = append(obj.Spec.OutputArtifacts, swapi.OutputArtifact{ + Name: 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()) + + // Pre-create the ExternalArtifacts in the target namespace, owned by a + // different ArtifactGenerator. + for _, name := range eaNames { + otherEA := &sourcev1.ExternalArtifact{ + TypeMeta: metav1.TypeMeta{ + APIVersion: sourcev1.GroupVersion.String(), + Kind: sourcev1.ExternalArtifactKind, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: tgtNS.Name, + Labels: map[string]string{ + swapi.ArtifactGeneratorLabel: "11111111-1111-1111-1111-111111111111", + }, + }, + Spec: sourcev1.ExternalArtifactSpec{ + SourceRef: &gotkmeta.NamespacedObjectKindReference{ + APIVersion: swapi.GroupVersion.String(), + Kind: swapi.ArtifactGeneratorKind, + Name: "other-generator", + Namespace: "other-namespace", + }, + }, + } + g.Expect(testClient.Create(ctx, otherEA)).To(Succeed()) + } + + // Add the finalizer. + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + // Build the artifacts and take over the ExternalArtifacts. + _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: objKey}) + g.Expect(err).ToNot(HaveOccurred()) + + // The ExternalArtifacts are now owned by the current ArtifactGenerator. + g.Expect(testClient.Get(ctx, objKey, obj)).To(Succeed()) + for _, name := range eaNames { + ea := &sourcev1.ExternalArtifact{} + g.Expect(testClient.Get(ctx, client.ObjectKey{Name: name, Namespace: tgtNS.Name}, ea)).To(Succeed()) + g.Expect(ea.Labels).To(HaveKeyWithValue(swapi.ArtifactGeneratorLabel, string(obj.GetUID()))) + + // One warning event per taken over ExternalArtifact. + g.Expect(eventsWithReason(getEvents(name, tgtNS.Name), swapi.OwnershipConflictReason)). + To(HaveLen(1)) + } + + // A single summary warning event is emitted for the ArtifactGenerator, + // counting the ExternalArtifacts that had a conflict. + agConflicts := eventsWithReason(getEvents(obj.Name, obj.Namespace), swapi.OwnershipConflictReason) + g.Expect(agConflicts).To(HaveLen(1)) + g.Expect(agConflicts[0].Type).To(Equal(corev1.EventTypeWarning)) + g.Expect(agConflicts[0].Message).To(Equal("ownership conflict detected for 2 ExternalArtifact(s)")) +} + +// eventsWithReason returns the events with the given reason. +func eventsWithReason(events []corev1.Event, reason string) []corev1.Event { + var result []corev1.Event + for _, e := range events { + if e.Reason == reason { + result = append(result, e) + } + } + return result +} + +func TestArtifactGeneratorReconciler_CrossNamespaceAccessDenied(t *testing.T) { + g := NewWithT(t) + reconciler := getArtifactGeneratorReconciler() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + srcNS, err := testEnv.CreateNamespace(ctx, "test-agns-deny-src") + g.Expect(err).ToNot(HaveOccurred()) + tgtNS, err := testEnv.CreateNamespace(ctx, "test-agns-deny-tgt") + g.Expect(err).ToNot(HaveOccurred()) + + // The ServiceAccount exists but has no RBAC bindings in the target namespace. + saName := "artifact-generator-denied" + g.Expect(testClient.Create(ctx, &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: saName, Namespace: srcNS.Name}, + })).To(Succeed()) + + objKey := client.ObjectKey{Name: "test-agns-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), + 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()) + + // 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: tgtNS.Name} + err = testClient.Get(ctx, eaKey, &sourcev1.ExternalArtifact{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) +} + +// 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 { + sa := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: saName, + Namespace: saNamespace, + }, + } + if err := testClient.Create(ctx, sa); err != nil { + return err + } + + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: saName, + Namespace: targetNamespace, + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{sourcev1.GroupVersion.Group}, + Resources: []string{"externalartifacts"}, + Verbs: []string{"get", "list", "watch", "create", "update", "patch", "delete"}, + }, + { + APIGroups: []string{sourcev1.GroupVersion.Group}, + Resources: []string{"externalartifacts/status"}, + Verbs: []string{"get", "update", "patch"}, + }, + }, + } + if err := testClient.Create(ctx, role); err != nil { + return err + } + + binding := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: saName, + Namespace: targetNamespace, + }, + Subjects: []rbacv1.Subject{ + { + Kind: rbacv1.ServiceAccountKind, + Name: saName, + Namespace: saNamespace, + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "Role", + Name: saName, + }, + } + return testClient.Create(ctx, binding) +} diff --git a/internal/controller/artifactgenerator_validation_test.go b/internal/controller/artifactgenerator_validation_test.go index 7d8cfde..28d75b6 100644 --- a/internal/controller/artifactgenerator_validation_test.go +++ b/internal/controller/artifactgenerator_validation_test.go @@ -290,6 +290,58 @@ func TestArtifactGenerator_crdValidation(t *testing.T) { }, expectError: true, }, + { + name: "valid artifact namespace", + setupObj: func() *swapi.ArtifactGenerator { + objKey := client.ObjectKey{ + Name: "test-valid-artifact-namespace", + Namespace: ns.Name, + } + obj := getArtifactGenerator(objKey) + obj.Spec.OutputArtifacts[0].Namespace = "another-namespace" + return obj + }, + expectError: false, + }, + { + name: "invalid artifact namespace", + setupObj: func() *swapi.ArtifactGenerator { + objKey := client.ObjectKey{ + Name: "test-invalid-artifact-namespace", + Namespace: ns.Name, + } + obj := getArtifactGenerator(objKey) + obj.Spec.OutputArtifacts[0].Namespace = "Invalid-Namespace" + return obj + }, + expectError: true, + }, + { + name: "valid service account name", + setupObj: func() *swapi.ArtifactGenerator { + objKey := client.ObjectKey{ + Name: "test-valid-service-account", + Namespace: ns.Name, + } + obj := getArtifactGenerator(objKey) + obj.Spec.ServiceAccountName = "artifact-generator" + return obj + }, + expectError: false, + }, + { + name: "invalid service account name", + setupObj: func() *swapi.ArtifactGenerator { + objKey := client.ObjectKey{ + Name: "test-invalid-service-account", + Namespace: ns.Name, + } + obj := getArtifactGenerator(objKey) + obj.Spec.ServiceAccountName = "Invalid ServiceAccount" + return obj + }, + expectError: true, + }, } for _, tt := range tests { diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 2d12725..7e1f9bc 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -30,6 +30,7 @@ import ( clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/yaml" gotkconfig "github.com/fluxcd/pkg/artifact/config" @@ -98,7 +99,34 @@ func TestMain(m *testing.M) { }() <-testEnv.Manager.Elected() + // Point controller-runtime's config loader at the test API server so that + // the impersonation client can build a REST config. The admin user has + // impersonation permissions. + adminUser, err := testEnv.AddUser(envtest.User{ + Name: "testenv-admin", + Groups: []string{"system:masters"}, + }, nil) + if err != nil { + panic(fmt.Sprintf("Failed to create testenv-admin user: %v", err)) + } + kubeconfig, err := adminUser.KubeConfig() + if err != nil { + panic(fmt.Sprintf("Failed to create testenv-admin kubeconfig: %v", err)) + } + kubeconfigFile, err := os.CreateTemp("", "source-watcher-kubeconfig-*") + if err != nil { + panic(fmt.Sprintf("Failed to create kubeconfig file: %v", err)) + } + if _, err := kubeconfigFile.Write(kubeconfig); err != nil { + panic(fmt.Sprintf("Failed to write kubeconfig file: %v", err)) + } + if err := kubeconfigFile.Close(); err != nil { + panic(fmt.Sprintf("Failed to close kubeconfig file: %v", err)) + } + os.Setenv("KUBECONFIG", kubeconfigFile.Name()) + code := m.Run() + _ = os.Remove(kubeconfigFile.Name()) fmt.Println("Stopping the test environment") if err := testEnv.Stop(); err != nil {