diff --git a/README.md b/README.md index 54b8596f1..c37821b69 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,7 @@ Details: - [Serving Configuration](https://knative.dev/docs/install/operator/configuring-serving-cr/) - [Eventing Configuration](https://knative.dev/docs/install/operator/configuring-eventing-cr/) - [Upgrade](docs/upgrade.md) +- [Customizing generated resources with patches](docs/resource-patches.md) - [Multi-cluster deployment](docs/multicluster.md) - [Development](docs/development.md) - [Multi-cluster E2E testing](docs/development/e2e-multicluster.md) diff --git a/config/charts/knative-operator/templates/crds/knativeeventings.yaml b/config/charts/knative-operator/templates/crds/knativeeventings.yaml index fd3a9aeb2..59a20682a 100644 --- a/config/charts/knative-operator/templates/crds/knativeeventings.yaml +++ b/config/charts/knative-operator/templates/crds/knativeeventings.yaml @@ -1685,6 +1685,57 @@ spec: description: Labels overrides labels for the namespace and its template. type: object type: object + patches: + description: Patches customize generated resources after all built-in transformations have been applied. + items: + description: ResourcePatch customizes one generated Kubernetes resource. + properties: + patch: + description: Patch defines the patch type and content to apply. + properties: + content: + description: Content is a JSON or YAML patch document. + minLength: 1 + type: string + type: + description: 'Type is the patch type: "json", "merge", or "strategic".' + enum: + - json + - merge + - strategic + type: string + required: + - content + - type + type: object + target: + description: Target identifies exactly one generated resource to patch. + properties: + apiVersion: + description: APIVersion is the API version of the target resource, for example "apps/v1". + minLength: 1 + type: string + kind: + description: Kind is the Kubernetes kind of the target resource, for example "Deployment". + minLength: 1 + type: string + name: + description: Name is the name of the target resource, for example "activator". + minLength: 1 + type: string + namespace: + description: Namespace optionally disambiguates resources with the same API version, kind, and name. + type: string + required: + - apiVersion + - kind + - name + type: object + required: + - patch + - target + type: object + type: array podDisruptionBudgets: description: PodDisruptionBudgetOverride overrides PodDisruptionBudget configurations via minAvailable. items: diff --git a/config/charts/knative-operator/templates/crds/knativeservings.yaml b/config/charts/knative-operator/templates/crds/knativeservings.yaml index 0a06ab12a..17ddbe922 100644 --- a/config/charts/knative-operator/templates/crds/knativeservings.yaml +++ b/config/charts/knative-operator/templates/crds/knativeservings.yaml @@ -2012,6 +2012,57 @@ spec: description: Labels overrides labels for the namespace and its template. type: object type: object + patches: + description: Patches customize generated resources after all built-in transformations have been applied. + items: + description: ResourcePatch customizes one generated Kubernetes resource. + properties: + patch: + description: Patch defines the patch type and content to apply. + properties: + content: + description: Content is a JSON or YAML patch document. + minLength: 1 + type: string + type: + description: 'Type is the patch type: "json", "merge", or "strategic".' + enum: + - json + - merge + - strategic + type: string + required: + - content + - type + type: object + target: + description: Target identifies exactly one generated resource to patch. + properties: + apiVersion: + description: APIVersion is the API version of the target resource, for example "apps/v1". + minLength: 1 + type: string + kind: + description: Kind is the Kubernetes kind of the target resource, for example "Deployment". + minLength: 1 + type: string + name: + description: Name is the name of the target resource, for example "activator". + minLength: 1 + type: string + namespace: + description: Namespace optionally disambiguates resources with the same API version, kind, and name. + type: string + required: + - apiVersion + - kind + - name + type: object + required: + - patch + - target + type: object + type: array podDisruptionBudgets: description: PodDisruptionBudgetOverride overrides PodDisruptionBudget configurations via minAvailable. items: diff --git a/config/crd/bases/operator.knative.dev_knativeeventings.yaml b/config/crd/bases/operator.knative.dev_knativeeventings.yaml index f38e89798..b752f33d5 100644 --- a/config/crd/bases/operator.knative.dev_knativeeventings.yaml +++ b/config/crd/bases/operator.knative.dev_knativeeventings.yaml @@ -1760,6 +1760,64 @@ spec: template. type: object type: object + patches: + description: Patches customize generated resources after all built-in + transformations have been applied. + items: + description: ResourcePatch customizes one generated Kubernetes resource. + properties: + patch: + description: Patch defines the patch type and content to apply. + properties: + content: + description: Content is a JSON or YAML patch document. + minLength: 1 + type: string + type: + description: 'Type is the patch type: "json", "merge", or + "strategic".' + enum: + - json + - merge + - strategic + type: string + required: + - content + - type + type: object + target: + description: Target identifies exactly one generated resource + to patch. + properties: + apiVersion: + description: APIVersion is the API version of the target + resource, for example "apps/v1". + minLength: 1 + type: string + kind: + description: Kind is the Kubernetes kind of the target resource, + for example "Deployment". + minLength: 1 + type: string + name: + description: Name is the name of the target resource, for + example "activator". + minLength: 1 + type: string + namespace: + description: Namespace optionally disambiguates resources + with the same API version, kind, and name. + type: string + required: + - apiVersion + - kind + - name + type: object + required: + - patch + - target + type: object + type: array podDisruptionBudgets: description: PodDisruptionBudgetOverride overrides PodDisruptionBudget configurations via minAvailable. diff --git a/config/crd/bases/operator.knative.dev_knativeservings.yaml b/config/crd/bases/operator.knative.dev_knativeservings.yaml index e607cbe02..ee552c6a1 100644 --- a/config/crd/bases/operator.knative.dev_knativeservings.yaml +++ b/config/crd/bases/operator.knative.dev_knativeservings.yaml @@ -2118,6 +2118,64 @@ spec: template. type: object type: object + patches: + description: Patches customize generated resources after all built-in + transformations have been applied. + items: + description: ResourcePatch customizes one generated Kubernetes resource. + properties: + patch: + description: Patch defines the patch type and content to apply. + properties: + content: + description: Content is a JSON or YAML patch document. + minLength: 1 + type: string + type: + description: 'Type is the patch type: "json", "merge", or + "strategic".' + enum: + - json + - merge + - strategic + type: string + required: + - content + - type + type: object + target: + description: Target identifies exactly one generated resource + to patch. + properties: + apiVersion: + description: APIVersion is the API version of the target + resource, for example "apps/v1". + minLength: 1 + type: string + kind: + description: Kind is the Kubernetes kind of the target resource, + for example "Deployment". + minLength: 1 + type: string + name: + description: Name is the name of the target resource, for + example "activator". + minLength: 1 + type: string + namespace: + description: Namespace optionally disambiguates resources + with the same API version, kind, and name. + type: string + required: + - apiVersion + - kind + - name + type: object + required: + - patch + - target + type: object + type: array podDisruptionBudgets: description: PodDisruptionBudgetOverride overrides PodDisruptionBudget configurations via minAvailable. diff --git a/docs/resource-patches.md b/docs/resource-patches.md new file mode 100644 index 000000000..2f78fa814 --- /dev/null +++ b/docs/resource-patches.md @@ -0,0 +1,100 @@ +# Customizing generated resources with patches + +Cluster administrators can use `spec.patches` on a `KnativeServing` or +`KnativeEventing` resource when an existing typed override does not cover the +generated Kubernetes resource they need to change. Patches run after the +Operator's built-in transformations and apply in declaration order. Each patch +sees the result of earlier patches, so a later patch can override their changes. + +## Patch target + +Each patch must identify exactly one generated resource by API version, kind, +and name. Set `namespace` only when resources in more than one namespace have +the same identity. Reconciliation fails if the target matches zero or multiple +resources; this makes a renamed or removed target visible during an upgrade. + +```yaml +spec: + patches: + - target: + apiVersion: autoscaling/v2 + kind: HorizontalPodAutoscaler + name: webhook + patch: + type: strategic + content: | + spec: + minReplicas: 2 + maxReplicas: 10 +``` + +Patches apply to all resources assembled for the component, including Serving +or Eventing core resources, enabled Ingress or Source resources, extension +resources, and `additionalManifests`. + +| Patch type | Use it for | Content format | +| --- | --- | --- | +| `strategic` | Kubernetes resources with named lists, such as Deployment containers | Strategic Merge Patch in YAML or JSON | +| `merge` | Maps and scalar fields, including custom resources | RFC 7386 Merge Patch in YAML or JSON | +| `json` | Exact add, replace, or remove operations | RFC 6902 JSON Patch in YAML or JSON | + +Strategic Merge Patch requires a Kubernetes type registered in the Operator's +scheme. Use `merge` or `json` for an unregistered custom resource. A patch may +not change the target's API version, kind, name, or namespace. + +## Letting KEDA manage replicas + +Knative 1.23 includes the following HorizontalPodAutoscalers. An ingress or +broker HPA is present only when its corresponding optional component is +enabled. + +| Custom resource | Workload | HPA name | +| --- | --- | --- | +| `KnativeServing` | `activator` | `activator` | +| `KnativeServing` | `webhook` | `webhook` | +| `KnativeServing` with Kourier | `3scale-kourier-gateway` | `3scale-kourier-gateway` | +| `KnativeEventing` | `eventing-webhook` | `eventing-webhook` | +| `KnativeEventing` with MTChannelBasedBroker | `mt-broker-ingress` | `broker-ingress-hpa` | +| `KnativeEventing` with MTChannelBasedBroker | `mt-broker-filter` | `broker-filter-hpa` | + +Run `kubectl get hpa -n knative-serving` or `kubectl get hpa -n knative-eventing` +to confirm which HPAs are present in the cluster. + +To replace the bundled `activator` HPA with KEDA, remove the HPA and leave +`spec.replicas` unmanaged by the Operator: + +```yaml +apiVersion: operator.knative.dev/v1beta1 +kind: KnativeServing +metadata: + name: knative-serving + namespace: knative-serving +spec: + patches: + - target: + apiVersion: autoscaling/v2 + kind: HorizontalPodAutoscaler + name: activator + patch: + type: strategic + content: | + $patch: delete + - target: + apiVersion: apps/v1 + kind: Deployment + name: activator + patch: + type: strategic + content: | + spec: + replicas: null +``` + +The root-level `$patch: delete` directive means the exact HPA identity must +remain absent. Configure the external autoscaler to use another HPA name. +Removing the delete patch causes the bundled HPA to be created again on the +next reconciliation. + +A root-level `$patch: delete` cannot target a CustomResourceDefinition or +Namespace. Knative Serving also requires its `apps/v1` Deployment named +`webhook` during installation, so that Deployment cannot be deleted. diff --git a/go.mod b/go.mod index 08fa0e489..15166bfa2 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module knative.dev/operator go 1.25.0 require ( + github.com/evanphx/json-patch/v5 v5.9.11 github.com/go-logr/zapr v1.3.0 github.com/google/go-cmp v0.7.0 github.com/google/go-github/v33 v33.0.0 @@ -69,7 +70,6 @@ require ( github.com/dimchansky/utfbom v1.1.1 // indirect github.com/elliotchance/orderedmap v1.8.0 // indirect github.com/emicklei/go-restful/v3 v3.13.0 // indirect - github.com/evanphx/json-patch/v5 v5.9.11 // indirect github.com/fatih/color v1.18.0 // indirect github.com/felixge/httpsnoop v1.0.4 // indirect github.com/fxamacker/cbor/v2 v2.9.0 // indirect diff --git a/pkg/apis/operator/base/common.go b/pkg/apis/operator/base/common.go index 6b4f813bf..37c94499d 100644 --- a/pkg/apis/operator/base/common.go +++ b/pkg/apis/operator/base/common.go @@ -66,6 +66,8 @@ type KComponentSpec interface { GetManifests() []Manifest // GetAdditionalManifests gets the list of additional manifests, which should be installed GetAdditionalManifests() []Manifest + // GetPatches gets the patches to apply to generated resources. + GetPatches() []ResourcePatch // GetNamespaceConfiguration gets the labels and annotations for the namespace GetNamespaceConfiguration() *NamespaceConfiguration @@ -179,6 +181,10 @@ type CommonSpec struct { // +optional AdditionalManifests []Manifest `json:"additionalManifests,omitempty"` + // Patches customize generated resources after all built-in transformations have been applied. + // +optional + Patches []ResourcePatch `json:"patches,omitempty"` + // HighAvailability allows specification of HA control plane. // +optional HighAvailability *HighAvailability `json:"high-availability,omitempty"` @@ -228,6 +234,11 @@ func (c *CommonSpec) GetAdditionalManifests() []Manifest { return c.AdditionalManifests } +// GetPatches implements KComponentSpec. +func (c *CommonSpec) GetPatches() []ResourcePatch { + return c.Patches +} + // GetHighAvailability implements KComponentSpec. func (c *CommonSpec) GetHighAvailability() *HighAvailability { return c.HighAvailability @@ -443,6 +454,59 @@ type Manifest struct { Url string `json:"URL"` } +// ResourcePatch customizes one generated Kubernetes resource. +type ResourcePatch struct { + // Target identifies exactly one generated resource to patch. + // +kubebuilder:validation:Required + Target PatchTarget `json:"target"` + + // Patch defines the patch type and content to apply. + // +kubebuilder:validation:Required + Patch PatchSpec `json:"patch"` +} + +// PatchTarget identifies a generated Kubernetes resource. +type PatchTarget struct { + // APIVersion is the API version of the target resource, for example "apps/v1". + // +kubebuilder:validation:MinLength=1 + APIVersion string `json:"apiVersion"` + + // Kind is the Kubernetes kind of the target resource, for example "Deployment". + // +kubebuilder:validation:MinLength=1 + Kind string `json:"kind"` + + // Name is the name of the target resource, for example "activator". + // +kubebuilder:validation:MinLength=1 + Name string `json:"name"` + + // Namespace optionally disambiguates resources with the same API version, kind, and name. + // +optional + Namespace string `json:"namespace,omitempty"` +} + +// PatchSpec defines a patch to apply to a generated resource. +type PatchSpec struct { + // Type is the patch type: "json", "merge", or "strategic". + // +kubebuilder:validation:Enum=json;merge;strategic + Type PatchType `json:"type"` + + // Content is a JSON or YAML patch document. + // +kubebuilder:validation:MinLength=1 + Content string `json:"content"` +} + +// PatchType is the algorithm used to apply a resource patch. +type PatchType string + +const ( + // JSONPatchType is an RFC 6902 JSON Patch. + JSONPatchType PatchType = "json" + // MergePatchType is an RFC 7386 JSON Merge Patch. + MergePatchType PatchType = "merge" + // StrategicMergePatchType is a Kubernetes Strategic Merge Patch. + StrategicMergePatchType PatchType = "strategic" +) + // HighAvailability specifies options for deploying Knative Serving control // plane in a highly available manner. Note that HighAvailability is still in // progress and does not currently provide a completely HA control plane. diff --git a/pkg/apis/operator/base/zz_generated.deepcopy.go b/pkg/apis/operator/base/zz_generated.deepcopy.go index c80d9ccc4..4596c9a08 100644 --- a/pkg/apis/operator/base/zz_generated.deepcopy.go +++ b/pkg/apis/operator/base/zz_generated.deepcopy.go @@ -137,6 +137,11 @@ func (in *CommonSpec) DeepCopyInto(out *CommonSpec) { *out = make([]Manifest, len(*in)) copy(*out, *in) } + if in.Patches != nil { + in, out := &in.Patches, &out.Patches + *out = make([]ResourcePatch, len(*in)) + copy(*out, *in) + } if in.HighAvailability != nil { in, out := &in.HighAvailability, &out.HighAvailability *out = new(HighAvailability) @@ -572,6 +577,38 @@ func (in *NatssSourceConfiguration) DeepCopy() *NatssSourceConfiguration { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PatchSpec) DeepCopyInto(out *PatchSpec) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PatchSpec. +func (in *PatchSpec) DeepCopy() *PatchSpec { + if in == nil { + return nil + } + out := new(PatchSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PatchTarget) DeepCopyInto(out *PatchTarget) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PatchTarget. +func (in *PatchTarget) DeepCopy() *PatchTarget { + if in == nil { + return nil + } + out := new(PatchTarget) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PodDisruptionBudgetOverride) DeepCopyInto(out *PodDisruptionBudgetOverride) { *out = *in @@ -686,6 +723,24 @@ func (in *Registry) DeepCopy() *Registry { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ResourcePatch) DeepCopyInto(out *ResourcePatch) { + *out = *in + out.Target = in.Target + out.Patch = in.Patch + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ResourcePatch. +func (in *ResourcePatch) DeepCopy() *ResourcePatch { + if in == nil { + return nil + } + out := new(ResourcePatch) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ResourceRequirementsOverride) DeepCopyInto(out *ResourceRequirementsOverride) { *out = *in diff --git a/pkg/reconciler/common/patches.go b/pkg/reconciler/common/patches.go new file mode 100644 index 000000000..b746d5711 --- /dev/null +++ b/pkg/reconciler/common/patches.go @@ -0,0 +1,264 @@ +/* +Copyright 2026 The Knative 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 common + +import ( + "context" + "encoding/json" + "errors" + "fmt" + + jsonpatch "github.com/evanphx/json-patch/v5" + mf "github.com/manifestival/manifestival" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/strategicpatch" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/yaml" + + "knative.dev/operator/pkg/apis/operator/base" +) + +const strategicMergeDeleteDirective = "delete" + +type resourceIdentity struct { + apiVersion string + kind string + namespace string + name string +} + +// ResourcePatchStages carries resources removed by Apply to the post-readiness +// Delete stage. Delete removes them directly so root-level delete directives +// also work when status has no previous manifest for obsolete-resource cleanup. +type ResourcePatchStages struct { + protectedDeletes []base.PatchTarget + deletedResources mf.Manifest +} + +// NewResourcePatchStages creates state shared by the Apply and Delete stages. +// protectedDeletes identifies component resources that later reconciliation +// stages require and therefore cannot be deleted. +func NewResourcePatchStages(protectedDeletes ...base.PatchTarget) *ResourcePatchStages { + return &ResourcePatchStages{protectedDeletes: protectedDeletes} +} + +// Apply applies user-provided patches to the final manifest and records +// resources selected by root-level delete directives. +func (stages *ResourcePatchStages) Apply(_ context.Context, manifest *mf.Manifest, instance base.KComponent) error { + deletedResources, err := applyResourcePatches(manifest, instance.GetSpec().GetPatches(), stages.protectedDeletes) + if err != nil { + instance.GetStatus().MarkInstallFailed(err.Error()) + return err + } + stages.deletedResources = deletedResources + return nil +} + +// Delete removes resources recorded by Apply from the cluster. +func (stages *ResourcePatchStages) Delete(_ context.Context, _ *mf.Manifest, instance base.KComponent) error { + if err := stages.deletedResources.Delete(); err != nil { + err = fmt.Errorf("delete resources removed by patches: %w", err) + instance.GetStatus().MarkInstallFailed(err.Error()) + return err + } + return nil +} + +// ApplyResourcePatches applies user-provided patches in declaration order. A +// strategic merge patch with a root-level "$patch: delete" directive removes +// the matching resource from the manifest. +func ApplyResourcePatches(manifest *mf.Manifest, patches []base.ResourcePatch) error { + _, err := applyResourcePatches(manifest, patches, nil) + return err +} + +func applyResourcePatches(manifest *mf.Manifest, patches []base.ResourcePatch, protectedDeletes []base.PatchTarget) (mf.Manifest, error) { + deletedResources := manifest.Filter(mf.Nothing) + for patchIndex, resourcePatch := range patches { + deletedResource, err := applyPatchToManifest(manifest, resourcePatch, protectedDeletes) + if err != nil { + return deletedResources, fmt.Errorf("patch %d target %s: %w", patchIndex, describeTarget(resourcePatch.Target), err) + } + if deletedResource != nil { + deletedResources = deletedResources.Append(*deletedResource) + } + } + return deletedResources, nil +} + +// applyPatchToManifest removes or rewrites the single resource selected by the +// patch target. +func applyPatchToManifest(manifest *mf.Manifest, resourcePatch base.ResourcePatch, protectedDeletes []base.PatchTarget) (*mf.Manifest, error) { + matchesTarget := resourceMatchesTarget(resourcePatch.Target) + matchedResources := manifest.Filter(matchesTarget).Resources() + if len(matchedResources) == 0 { + return nil, errors.New("did not match any generated resource") + } + if len(matchedResources) > 1 { + if resourcePatch.Target.Namespace == "" { + return nil, fmt.Errorf("matched %d generated resources; specify target.namespace", len(matchedResources)) + } + return nil, fmt.Errorf("matched %d generated resources; target must match exactly one resource", len(matchedResources)) + } + + patchJSON, err := yaml.YAMLToJSON([]byte(resourcePatch.Patch.Content)) + if err != nil { + return nil, fmt.Errorf("convert patch content to JSON: %w", err) + } + isDelete, err := isStrategicMergeDelete(resourcePatch.Patch.Type, patchJSON) + if err != nil { + return nil, err + } + + if isDelete { + if mf.CRDs(&matchedResources[0]) { + return nil, errors.New("deleting CustomResourceDefinitions is not supported") + } + if matchedResources[0].GroupVersionKind() == corev1.SchemeGroupVersion.WithKind("Namespace") { + return nil, errors.New("deleting Namespaces is not supported") + } + for _, protectedTarget := range protectedDeletes { + if resourceMatchesTarget(protectedTarget)(&matchedResources[0]) { + return nil, fmt.Errorf("deleting protected resource %s is not supported", describeTarget(protectedTarget)) + } + } + deletedResource := manifest.Filter(matchesTarget) + *manifest = manifest.Filter(mf.Not(matchesTarget)) + return &deletedResource, nil + } + + patchedManifest, err := manifest.Transform(func(resource *unstructured.Unstructured) error { + if !matchesTarget(resource) { + return nil + } + return applyPatchToResource(resource, resourcePatch.Patch.Type, patchJSON) + }) + if err != nil { + return nil, err + } + *manifest = patchedManifest + return nil, nil +} + +// resourceMatchesTarget returns a predicate for selecting the generated +// resource a patch target refers to. An empty target namespace matches any +// namespace. +func resourceMatchesTarget(target base.PatchTarget) mf.Predicate { + return func(resource *unstructured.Unstructured) bool { + if resource.GetAPIVersion() != target.APIVersion { + return false + } + if resource.GetKind() != target.Kind { + return false + } + if resource.GetName() != target.Name { + return false + } + return target.Namespace == "" || resource.GetNamespace() == target.Namespace + } +} + +func applyPatchToResource(resource *unstructured.Unstructured, patchType base.PatchType, patchJSON []byte) error { + originalIdentity := identityOf(resource) + originalJSON, err := resource.MarshalJSON() + if err != nil { + return fmt.Errorf("marshal generated resource: %w", err) + } + + var patchedJSON []byte + switch patchType { + case base.JSONPatchType: + decoded, err := jsonpatch.DecodePatch(patchJSON) + if err != nil { + return fmt.Errorf("decode JSON patch: %w", err) + } + patchedJSON, err = decoded.Apply(originalJSON) + if err != nil { + return fmt.Errorf("apply JSON patch: %w", err) + } + case base.MergePatchType: + patchedJSON, err = jsonpatch.MergePatch(originalJSON, patchJSON) + if err != nil { + return fmt.Errorf("apply merge patch: %w", err) + } + case base.StrategicMergePatchType: + resourceType, err := scheme.Scheme.New(resource.GroupVersionKind()) + if runtime.IsNotRegisteredError(err) { + return fmt.Errorf( + "strategic merge patch is not supported for %s; use a json or merge patch", + resource.GroupVersionKind(), + ) + } + if err != nil { + return fmt.Errorf("resolve strategic merge patch schema for %s: %w", resource.GroupVersionKind(), err) + } + patchedJSON, err = strategicpatch.StrategicMergePatch(originalJSON, patchJSON, resourceType) + if err != nil { + return fmt.Errorf("apply strategic merge patch: %w", err) + } + default: + return fmt.Errorf("unsupported patch type %q", patchType) + } + + patched := &unstructured.Unstructured{} + if err := patched.UnmarshalJSON(patchedJSON); err != nil { + return fmt.Errorf("decode patched resource: %w", err) + } + if patchedIdentity := identityOf(patched); patchedIdentity != originalIdentity { + return errors.New("patch must not change apiVersion, kind, metadata.name, or metadata.namespace") + } + *resource = *patched + return nil +} + +func isStrategicMergeDelete(patchType base.PatchType, patchJSON []byte) (bool, error) { + if patchType != base.StrategicMergePatchType { + return false, nil + } + patchDocument := map[string]interface{}{} + if err := json.Unmarshal(patchJSON, &patchDocument); err != nil { + return false, fmt.Errorf("decode strategic merge patch: %w", err) + } + directive, found := patchDocument["$patch"] + if !found { + return false, nil + } + directiveString, ok := directive.(string) + if !ok { + return false, errors.New("root $patch directive must be a string") + } + return directiveString == strategicMergeDeleteDirective, nil +} + +func identityOf(resource *unstructured.Unstructured) resourceIdentity { + return resourceIdentity{ + apiVersion: resource.GetAPIVersion(), + kind: resource.GetKind(), + namespace: resource.GetNamespace(), + name: resource.GetName(), + } +} + +func describeTarget(target base.PatchTarget) string { + namespace := target.Namespace + if namespace == "" { + namespace = "*" + } + return fmt.Sprintf("%s, Kind=%s, %s/%s", target.APIVersion, target.Kind, namespace, target.Name) +} diff --git a/pkg/reconciler/common/patches_test.go b/pkg/reconciler/common/patches_test.go new file mode 100644 index 000000000..379ea2caf --- /dev/null +++ b/pkg/reconciler/common/patches_test.go @@ -0,0 +1,406 @@ +/* +Copyright 2026 The Knative 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 common + +import ( + "context" + "strings" + "testing" + + mf "github.com/manifestival/manifestival" + mffake "github.com/manifestival/manifestival/fake" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "knative.dev/operator/pkg/apis/operator/base" + "knative.dev/operator/pkg/apis/operator/v1beta1" + apistest "knative.dev/pkg/apis/testing" +) + +func TestApplyResourcePatchesAppliesAllTypesInOrder(t *testing.T) { + manifest := manifestForPatchTest(t, deploymentForPatchTest(t, "activator", "knative-serving", 1)) + patches := []base.ResourcePatch{ + resourcePatchForTest(base.JSONPatchType, "apps/v1", "Deployment", "activator", ` +- op: add + path: /metadata/annotations + value: + example.com/json: applied +`), + resourcePatchForTest(base.MergePatchType, "apps/v1", "Deployment", "activator", ` +metadata: + labels: + example.com/merge: applied +spec: + replicas: 2 +`), + resourcePatchForTest(base.StrategicMergePatchType, "apps/v1", "Deployment", "activator", ` +spec: + replicas: 3 +`), + } + + if err := ApplyResourcePatches(&manifest, patches); err != nil { + t.Fatalf("ApplyResourcePatches() = %v", err) + } + got := manifest.Resources()[0] + if value := got.GetAnnotations()["example.com/json"]; value != "applied" { + t.Fatalf("JSON patch annotation = %q, want applied", value) + } + if value := got.GetLabels()["example.com/merge"]; value != "applied" { + t.Fatalf("merge patch label = %q, want applied", value) + } + if replicas, _, err := unstructured.NestedInt64(got.Object, "spec", "replicas"); err != nil || replicas != 3 { + t.Fatalf("strategic merge patch replicas = %d, %v, want 3, nil", replicas, err) + } +} + +func TestApplyResourcePatchesUsesNamespaceToDisambiguateTarget(t *testing.T) { + manifest := manifestForPatchTest(t, + deploymentForPatchTest(t, "activator", "one", 1), + deploymentForPatchTest(t, "activator", "two", 1), + ) + patch := resourcePatchForTest(base.MergePatchType, "apps/v1", "Deployment", "activator", ` +metadata: + labels: + example.com/patched: "true" +`) + patch.Target.Namespace = "two" + + if err := ApplyResourcePatches(&manifest, []base.ResourcePatch{patch}); err != nil { + t.Fatalf("ApplyResourcePatches() = %v", err) + } + resources := manifest.Resources() + if value := resources[0].GetLabels()["example.com/patched"]; value != "" { + t.Fatalf("namespace one patch label = %q, want empty", value) + } + if value := resources[1].GetLabels()["example.com/patched"]; value != "true" { + t.Fatalf("namespace two patch label = %q, want true", value) + } +} + +func TestApplyResourcePatchesDeletesResource(t *testing.T) { + hpa := resourceForPatchTest("autoscaling/v2", "HorizontalPodAutoscaler", "activator", "knative-serving") + deployment := deploymentForPatchTest(t, "activator", "knative-serving", 1) + manifest := manifestForPatchTest(t, hpa, deployment) + patch := resourcePatchForTest(base.StrategicMergePatchType, "autoscaling/v2", "HorizontalPodAutoscaler", "activator", `$patch: delete`) + + if err := ApplyResourcePatches(&manifest, []base.ResourcePatch{patch}); err != nil { + t.Fatalf("ApplyResourcePatches() = %v", err) + } + resources := manifest.Resources() + if len(resources) != 1 || resources[0].GetKind() != "Deployment" { + t.Fatalf("resources = %#v, want only Deployment", resources) + } +} + +func TestResourcePatchStagesDeleteLiveResourceWithoutManifestStatus(t *testing.T) { + client := mffake.New() + hpa := resourceForPatchTest("autoscaling/v2", "HorizontalPodAutoscaler", "activator", "knative-serving") + manifest, err := mf.ManifestFrom(mf.Slice([]unstructured.Unstructured{hpa}), mf.UseClient(client)) + if err != nil { + t.Fatalf("ManifestFrom() = %v", err) + } + if err := manifest.Apply(); err != nil { + t.Fatalf("initial Apply() = %v", err) + } + instance := &v1beta1.KnativeServing{ + Spec: v1beta1.KnativeServingSpec{CommonSpec: base.CommonSpec{ + Patches: []base.ResourcePatch{ + resourcePatchForTest(base.StrategicMergePatchType, "autoscaling/v2", "HorizontalPodAutoscaler", "activator", `$patch: delete`), + }, + }}, + } + + patchStages := NewResourcePatchStages() + if err := patchStages.Apply(context.Background(), &manifest, instance); err != nil { + t.Fatalf("ResourcePatchStages.Apply() = %v", err) + } + if _, err := client.Get(&hpa); err != nil { + t.Fatalf("Get() before deferred delete = %v", err) + } + if err := patchStages.Delete(context.Background(), &manifest, instance); err != nil { + t.Fatalf("ResourcePatchStages.Delete() = %v", err) + } + if _, err := client.Get(&hpa); !apierrors.IsNotFound(err) { + t.Fatalf("Get() after delete = %v, want NotFound", err) + } +} + +func TestDeletedResourceIsPrunedAndCanBeRecreated(t *testing.T) { + client := mffake.New() + hpa := resourceForPatchTest("autoscaling/v2", "HorizontalPodAutoscaler", "activator", "knative-serving") + installed, err := mf.ManifestFrom(mf.Slice([]unstructured.Unstructured{hpa}), mf.UseClient(client)) + if err != nil { + t.Fatalf("ManifestFrom() = %v", err) + } + if err := installed.Apply(); err != nil { + t.Fatalf("initial Apply() = %v", err) + } + + desired := installed.Append() + patch := resourcePatchForTest(base.StrategicMergePatchType, "autoscaling/v2", "HorizontalPodAutoscaler", "activator", `$patch: delete`) + if err := ApplyResourcePatches(&desired, []base.ResourcePatch{patch}); err != nil { + t.Fatalf("ApplyResourcePatches() = %v", err) + } + instance := &v1beta1.KnativeServing{Status: v1beta1.KnativeServingStatus{Manifests: []string{"installed"}}} + deleteObsolete := DeleteObsoleteResources(context.Background(), instance, func(context.Context, base.KComponent) (*mf.Manifest, error) { + return &installed, nil + }) + if err := deleteObsolete(context.Background(), &desired, instance); err != nil { + t.Fatalf("DeleteObsoleteResources() = %v", err) + } + if _, err := client.Get(&hpa); !apierrors.IsNotFound(err) { + t.Fatalf("Get() after delete = %v, want NotFound", err) + } + + if err := installed.Apply(); err != nil { + t.Fatalf("Apply() after removing the patch = %v", err) + } + if _, err := client.Get(&hpa); err != nil { + t.Fatalf("Get() after recreate = %v", err) + } +} + +func TestApplyResourcePatchesValidatesTargetAndIdentity(t *testing.T) { + tests := []struct { + name string + resources []unstructured.Unstructured + patch base.ResourcePatch + targetNamespace string + wantError string + }{ + { + name: "target does not exist", + resources: []unstructured.Unstructured{deploymentForPatchTest(t, "controller", "knative-serving", 1)}, + patch: resourcePatchForTest(base.MergePatchType, "apps/v1", "Deployment", "activator", `{}`), + wantError: "did not match any generated resource", + }, + { + name: "target ambiguity is reported before patch errors", + resources: []unstructured.Unstructured{ + deploymentForPatchTest(t, "activator", "one", 1), + deploymentForPatchTest(t, "activator", "two", 1), + }, + patch: resourcePatchForTest(base.JSONPatchType, "apps/v1", "Deployment", "activator", `not: an-array`), + wantError: "specify target.namespace", + }, + { + name: "duplicate target in one namespace", + resources: []unstructured.Unstructured{ + deploymentForPatchTest(t, "activator", "knative-serving", 1), + deploymentForPatchTest(t, "activator", "knative-serving", 2), + }, + patch: resourcePatchForTest(base.MergePatchType, "apps/v1", "Deployment", "activator", `{}`), + targetNamespace: "knative-serving", + wantError: "target must match exactly one resource", + }, + { + name: "identity changes", + resources: []unstructured.Unstructured{deploymentForPatchTest(t, "activator", "knative-serving", 1)}, + patch: resourcePatchForTest(base.MergePatchType, "apps/v1", "Deployment", "activator", ` +metadata: + name: changed +`), + wantError: "must not change apiVersion, kind, metadata.name, or metadata.namespace", + }, + { + name: "strategic merge unsupported for custom resource", + resources: []unstructured.Unstructured{resourceForPatchTest("example.dev/v1", "Example", "sample", "knative-serving")}, + patch: resourcePatchForTest(base.StrategicMergePatchType, "example.dev/v1", "Example", "sample", `spec: {enabled: true}`), + wantError: "use a json or merge patch", + }, + { + name: "CRD deletion is rejected", + resources: []unstructured.Unstructured{resourceForPatchTest("apiextensions.k8s.io/v1", "CustomResourceDefinition", "examples.example.dev", "")}, + patch: resourcePatchForTest(base.StrategicMergePatchType, "apiextensions.k8s.io/v1", "CustomResourceDefinition", "examples.example.dev", `$patch: delete`), + wantError: "deleting CustomResourceDefinitions is not supported", + }, + { + name: "Namespace deletion is rejected", + resources: []unstructured.Unstructured{resourceForPatchTest("v1", "Namespace", "knative-serving", "")}, + patch: resourcePatchForTest(base.StrategicMergePatchType, "v1", "Namespace", "knative-serving", `$patch: delete`), + wantError: "deleting Namespaces is not supported", + }, + { + name: "unsupported patch type", + resources: []unstructured.Unstructured{deploymentForPatchTest(t, "activator", "knative-serving", 1)}, + patch: resourcePatchForTest(base.PatchType("unknown"), "apps/v1", "Deployment", "activator", `{}`), + wantError: "unsupported patch type", + }, + { + name: "malformed JSON patch", + resources: []unstructured.Unstructured{deploymentForPatchTest(t, "activator", "knative-serving", 1)}, + patch: resourcePatchForTest(base.JSONPatchType, "apps/v1", "Deployment", "activator", `not: an-array`), + wantError: "decode JSON patch", + }, + { + name: "non-string root strategic directive", + resources: []unstructured.Unstructured{deploymentForPatchTest(t, "activator", "knative-serving", 1)}, + patch: resourcePatchForTest(base.StrategicMergePatchType, "apps/v1", "Deployment", "activator", `$patch: 1`), + wantError: "root $patch directive must be a string", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + manifest := manifestForPatchTest(t, test.resources...) + test.patch.Target.Namespace = test.targetNamespace + err := ApplyResourcePatches(&manifest, []base.ResourcePatch{test.patch}) + if err == nil || !strings.Contains(err.Error(), test.wantError) { + t.Fatalf("ApplyResourcePatches() = %v, want error containing %q", err, test.wantError) + } + }) + } +} + +func TestResourcePatchRunsAfterBuiltInTransforms(t *testing.T) { + replicas := int32(5) + instance := &v1beta1.KnativeServing{ + ObjectMeta: metav1.ObjectMeta{Namespace: "knative-serving"}, + Spec: v1beta1.KnativeServingSpec{CommonSpec: base.CommonSpec{ + HighAvailability: &base.HighAvailability{Replicas: &replicas}, + Patches: []base.ResourcePatch{ + resourcePatchForTest(base.StrategicMergePatchType, "apps/v1", "Deployment", "controller", ` +spec: + replicas: null +`), + }, + }}, + } + manifest := manifestForPatchTest(t, deploymentForPatchTest(t, "controller", "upstream-namespace", 1)) + + if err := Transform(context.Background(), &manifest, instance); err != nil { + t.Fatalf("Transform() = %v", err) + } + patchStages := NewResourcePatchStages() + if err := patchStages.Apply(context.Background(), &manifest, instance); err != nil { + t.Fatalf("ResourcePatchStages.Apply() = %v", err) + } + got := manifest.Resources()[0] + if got.GetNamespace() != "knative-serving" { + t.Fatalf("namespace = %q, want knative-serving", got.GetNamespace()) + } + if _, found, err := unstructured.NestedFieldNoCopy(got.Object, "spec", "replicas"); err != nil || found { + t.Fatalf("spec.replicas found = %v, err = %v, want false, nil", found, err) + } +} + +func TestResourcePatchFailureMarksInstallFailed(t *testing.T) { + instance := &v1beta1.KnativeServing{ + ObjectMeta: metav1.ObjectMeta{Namespace: "knative-serving"}, + Spec: v1beta1.KnativeServingSpec{CommonSpec: base.CommonSpec{ + Patches: []base.ResourcePatch{ + resourcePatchForTest(base.MergePatchType, "apps/v1", "Deployment", "missing", `{}`), + }, + }}, + } + manifest := manifestForPatchTest(t, deploymentForPatchTest(t, "controller", "upstream-namespace", 1)) + + patchStages := NewResourcePatchStages() + if err := patchStages.Apply(context.Background(), &manifest, instance); err == nil { + t.Fatal("ResourcePatchStages.Apply() = nil, want patch target error") + } + apistest.CheckConditionFailed(&instance.Status, base.InstallSucceeded, t) +} + +func TestApplyPatchesRejectsProtectedDelete(t *testing.T) { + instance := &v1beta1.KnativeServing{ + Spec: v1beta1.KnativeServingSpec{CommonSpec: base.CommonSpec{ + Patches: []base.ResourcePatch{ + resourcePatchForTest(base.StrategicMergePatchType, "apps/v1", "Deployment", "webhook", `$patch: delete`), + }, + }}, + } + manifest := manifestForPatchTest(t, deploymentForPatchTest(t, "webhook", "knative-serving", 1)) + protectedWebhook := base.PatchTarget{APIVersion: "apps/v1", Kind: "Deployment", Name: "webhook"} + + err := NewResourcePatchStages(protectedWebhook).Apply(context.Background(), &manifest, instance) + if err == nil || !strings.Contains(err.Error(), "deleting protected resource") { + t.Fatalf("ResourcePatchStages.Apply() = %v, want protected resource error", err) + } +} + +func TestManifestivalPreservesExternallyManagedReplicas(t *testing.T) { + client := mffake.New() + manifest, err := mf.ManifestFrom( + mf.Slice([]unstructured.Unstructured{deploymentForPatchTestWithoutReplicas("activator", "knative-serving")}), + mf.UseClient(client), + ) + if err != nil { + t.Fatalf("ManifestFrom() = %v", err) + } + if err := manifest.Apply(); err != nil { + t.Fatalf("initial Apply() = %v", err) + } + + deploymentIdentity := deploymentForPatchTestWithoutReplicas("activator", "knative-serving") + liveDeployment, err := client.Get(&deploymentIdentity) + if err != nil { + t.Fatalf("Get() = %v", err) + } + if err := unstructured.SetNestedField(liveDeployment.Object, int64(7), "spec", "replicas"); err != nil { + t.Fatalf("SetNestedField() = %v", err) + } + if err := client.Update(liveDeployment); err != nil { + t.Fatalf("Update() = %v", err) + } + + if err := manifest.Apply(); err != nil { + t.Fatalf("second Apply() = %v", err) + } + liveDeployment, err = client.Get(&deploymentIdentity) + if err != nil { + t.Fatalf("Get() after reconcile = %v", err) + } + if replicas, _, err := unstructured.NestedInt64(liveDeployment.Object, "spec", "replicas"); err != nil || replicas != 7 { + t.Fatalf("live replicas = %d, %v, want 7, nil", replicas, err) + } +} + +func resourcePatchForTest(patchType base.PatchType, apiVersion, kind, name, content string) base.ResourcePatch { + return base.ResourcePatch{ + Target: base.PatchTarget{APIVersion: apiVersion, Kind: kind, Name: name}, + Patch: base.PatchSpec{Type: patchType, Content: content}, + } +} + +func manifestForPatchTest(t *testing.T, resources ...unstructured.Unstructured) mf.Manifest { + t.Helper() + manifest, err := mf.ManifestFrom(mf.Slice(resources)) + if err != nil { + t.Fatalf("ManifestFrom() = %v", err) + } + return manifest +} + +func resourceForPatchTest(apiVersion, kind, name, namespace string) unstructured.Unstructured { + return *NamespacedResource(apiVersion, kind, namespace, name) +} + +func deploymentForPatchTest(t *testing.T, name, namespace string, replicas int64) unstructured.Unstructured { + t.Helper() + deployment := deploymentForPatchTestWithoutReplicas(name, namespace) + if err := unstructured.SetNestedField(deployment.Object, replicas, "spec", "replicas"); err != nil { + t.Fatalf("SetNestedField() = %v", err) + } + return deployment +} + +func deploymentForPatchTestWithoutReplicas(name, namespace string) unstructured.Unstructured { + return resourceForPatchTest("apps/v1", "Deployment", name, namespace) +} diff --git a/pkg/reconciler/knativeeventing/eventing_tls_test.go b/pkg/reconciler/knativeeventing/eventing_tls_test.go new file mode 100644 index 000000000..f8e73b491 --- /dev/null +++ b/pkg/reconciler/knativeeventing/eventing_tls_test.go @@ -0,0 +1,66 @@ +/* +Copyright 2026 The Knative 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 knativeeventing + +import ( + "context" + "strings" + "testing" + + mf "github.com/manifestival/manifestival" + mffake "github.com/manifestival/manifestival/fake" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "knative.dev/operator/pkg/apis/operator/base" + "knative.dev/operator/pkg/apis/operator/v1beta1" + "knative.dev/operator/pkg/reconciler/common" +) + +func TestPatchesRunAfterTLSResourceFiltering(t *testing.T) { + certificate := unstructured.Unstructured{} + certificate.SetAPIVersion("cert-manager.io/v1") + certificate.SetKind("Certificate") + certificate.SetNamespace("knative-eventing") + certificate.SetName("eventing-webhook") + manifest, err := mf.ManifestFrom( + mf.Slice([]unstructured.Unstructured{certificate}), + mf.UseClient(mffake.New()), + ) + if err != nil { + t.Fatalf("ManifestFrom() = %v", err) + } + instance := &v1beta1.KnativeEventing{ + ObjectMeta: metav1.ObjectMeta{Namespace: "knative-eventing"}, + Spec: v1beta1.KnativeEventingSpec{CommonSpec: base.CommonSpec{ + Patches: []base.ResourcePatch{{ + Target: base.PatchTarget{ + APIVersion: "cert-manager.io/v1", + Kind: "Certificate", + Name: "eventing-webhook", + }, + Patch: base.PatchSpec{Type: base.MergePatchType, Content: `{}`}, + }}, + }}, + } + + patchStages := common.NewResourcePatchStages() + stages := common.Stages{(&Reconciler{}).handleTLSResources, patchStages.Apply} + if _, err := stages.Execute(context.Background(), &manifest, instance); err == nil || !strings.Contains(err.Error(), "did not match any generated resource") { + t.Fatalf("Stages.Execute() = %v, want missing patch target error", err) + } +} diff --git a/pkg/reconciler/knativeeventing/knativeeventing.go b/pkg/reconciler/knativeeventing/knativeeventing.go index 7e8eeb5d9..0033b0c1d 100644 --- a/pkg/reconciler/knativeeventing/knativeeventing.go +++ b/pkg/reconciler/knativeeventing/knativeeventing.go @@ -128,6 +128,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ke *v1beta1.KnativeEvent } var state common.ReconcileState + patchStages := common.NewResourcePatchStages() stages := common.Stages{ common.ResolveTargetCluster(r.clusterProvider, &state), @@ -139,9 +140,11 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ke *v1beta1.KnativeEvent return r.transform(ctx, manifest, comp, state.AnchorOwner) }, r.handleTLSResources, + patchStages.Apply, manifests.Install, manifests.SetManifestPaths, // setting path right after applying manifests to populate paths common.CheckDeployments, + patchStages.Delete, common.MarkStatusSuccess, common.DeleteObsoleteResources(ctx, ke, r.installed), } @@ -191,9 +194,10 @@ func (r *Reconciler) installed(ctx context.Context, instance base.KComponent) (* } installed = r.manifest.Append(installed) - // Per the manifests, that have been installed in the cluster, we only need to inject the correct namespace - // in the stages. - stages := common.Stages{r.injectNamespace} + // Core resources only need their installation namespace restored. Extension + // resources also need the extension transforms that determined their final + // identities so obsolete-resource cleanup can find them. + stages := common.Stages{r.injectNamespace, r.appendInstalledExtensionManifests} _, err = stages.Execute(ctx, &installed, instance) return &installed, err } @@ -206,3 +210,19 @@ func (r *Reconciler) appendExtensionManifests(ctx context.Context, manifest *mf. *manifest = manifest.Append(platformManifests...) return nil } + +func (r *Reconciler) appendInstalledExtensionManifests(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error { + platformManifests, err := r.extension.Manifests(instance) + if err != nil { + return err + } + if len(platformManifests) == 0 { + return nil + } + extensionManifest := manifest.Filter(mf.Nothing).Append(platformManifests...) + if err := r.transform(ctx, &extensionManifest, instance, nil); err != nil { + return err + } + *manifest = manifest.Append(extensionManifest) + return nil +} diff --git a/pkg/reconciler/knativeserving/knativeserving.go b/pkg/reconciler/knativeserving/knativeserving.go index b1446f40f..1bea543af 100644 --- a/pkg/reconciler/knativeserving/knativeserving.go +++ b/pkg/reconciler/knativeserving/knativeserving.go @@ -121,6 +121,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ks *v1beta1.KnativeServi } var state common.ReconcileState + patchStages := common.NewResourcePatchStages(base.PatchTarget{APIVersion: "apps/v1", Kind: "Deployment", Name: "webhook"}) stages := common.Stages{ common.ResolveTargetCluster(r.clusterProvider, &state), @@ -132,11 +133,13 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ks *v1beta1.KnativeServi func(ctx context.Context, manifest *mf.Manifest, comp base.KComponent) error { return r.transform(ctx, manifest, comp, state.AnchorOwner) }, + patchStages.Apply, manifests.Install, manifests.SetManifestPaths, // setting path right after applying manifests to populate paths common.CheckWebhookDeployment, // Wait for webhook to be ready before creating Certificate resources common.InstallWebhookDependentResources, common.CheckDeployments, + patchStages.Delete, common.MarkStatusSuccess, common.DeleteObsoleteResources(ctx, ks, r.installed), } @@ -188,9 +191,10 @@ func (r *Reconciler) installed(ctx context.Context, instance base.KComponent) (* } installed = r.manifest.Append(installed) - // Per the manifests, that have been installed in the cluster, we only need to inject the correct namespace - // in the stages. - stages := common.Stages{r.injectNamespace} + // Core resources only need their installation namespace restored. Extension + // resources also need the extension transforms that determined their final + // identities so obsolete-resource cleanup can find them. + stages := common.Stages{r.injectNamespace, r.appendInstalledExtensionManifests} _, err = stages.Execute(ctx, &installed, instance) return &installed, err } @@ -203,3 +207,19 @@ func (r *Reconciler) appendExtensionManifests(ctx context.Context, manifest *mf. *manifest = manifest.Append(platformManifests...) return nil } + +func (r *Reconciler) appendInstalledExtensionManifests(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error { + platformManifests, err := r.extension.Manifests(instance) + if err != nil { + return err + } + if len(platformManifests) == 0 { + return nil + } + extensionManifest := manifest.Filter(mf.Nothing).Append(platformManifests...) + if err := r.transform(ctx, &extensionManifest, instance, nil); err != nil { + return err + } + *manifest = manifest.Append(extensionManifest) + return nil +} diff --git a/pkg/reconciler/knativeserving/knativeserving_test.go b/pkg/reconciler/knativeserving/knativeserving_test.go new file mode 100644 index 000000000..83fe7c6c5 --- /dev/null +++ b/pkg/reconciler/knativeserving/knativeserving_test.go @@ -0,0 +1,113 @@ +/* +Copyright 2026 The Knative 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 knativeserving + +import ( + "context" + "testing" + + mf "github.com/manifestival/manifestival" + mffake "github.com/manifestival/manifestival/fake" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "knative.dev/operator/pkg/apis/operator/base" + "knative.dev/operator/pkg/apis/operator/v1beta1" + "knative.dev/operator/pkg/reconciler/common" +) + +type extensionForPatchTest struct { + manifest mf.Manifest +} + +func (e extensionForPatchTest) Manifests(base.KComponent) ([]mf.Manifest, error) { + return []mf.Manifest{e.manifest}, nil +} + +func (extensionForPatchTest) Transformers(base.KComponent) []mf.Transformer { + return []mf.Transformer{mf.InjectNamespace("extension-system")} +} + +func (extensionForPatchTest) Reconcile(context.Context, base.KComponent) error { + return nil +} + +func (extensionForPatchTest) Finalize(context.Context, base.KComponent) error { + return nil +} + +func TestDeletedExtensionResourceIsPruned(t *testing.T) { + common.ClearCache() + client := mffake.New() + baseManifest, err := mf.ManifestFrom( + mf.Slice([]unstructured.Unstructured{}), + mf.UseClient(client), + ) + if err != nil { + t.Fatalf("ManifestFrom() = %v", err) + } + extensionResource := common.NamespacedResource("v1", "ConfigMap", "upstream", "extension-config") + extensionManifest, err := mf.ManifestFrom(mf.Slice([]unstructured.Unstructured{*extensionResource})) + if err != nil { + t.Fatalf("ManifestFrom() for extension = %v", err) + } + reconciler := &Reconciler{ + manifest: baseManifest, + extension: extensionForPatchTest{manifest: extensionManifest}, + } + instance := &v1beta1.KnativeServing{ + ObjectMeta: metav1.ObjectMeta{Name: "knative-serving", Namespace: "knative-serving"}, + Spec: v1beta1.KnativeServingSpec{CommonSpec: base.CommonSpec{ + Patches: []base.ResourcePatch{{ + Target: base.PatchTarget{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "extension-config", + Namespace: "extension-system", + }, + Patch: base.PatchSpec{Type: base.StrategicMergePatchType, Content: `$patch: delete`}, + }}, + }}, + Status: v1beta1.KnativeServingStatus{ + Manifests: []string{"../common/testdata/kodata/additional-manifests/additional-sa.yaml"}, + }, + } + + installed, err := reconciler.installed(context.Background(), instance) + if err != nil { + t.Fatalf("installed() = %v", err) + } + if err := installed.Apply(); err != nil { + t.Fatalf("installed.Apply() = %v", err) + } + desired := installed.Append() + if err := common.ApplyResourcePatches(&desired, instance.GetSpec().GetPatches()); err != nil { + t.Fatalf("ApplyResourcePatches() = %v", err) + } + deleteObsolete := common.DeleteObsoleteResources(context.Background(), instance, func(context.Context, base.KComponent) (*mf.Manifest, error) { + return installed, nil + }) + if err := deleteObsolete(context.Background(), &desired, instance); err != nil { + t.Fatalf("DeleteObsoleteResources() = %v", err) + } + + extensionIdentity := common.NamespacedResource("v1", "ConfigMap", "extension-system", "extension-config") + if _, err := client.Get(extensionIdentity); !apierrors.IsNotFound(err) { + t.Fatalf("Get() after delete = %v, want NotFound", err) + } +}