fix(tenantresource): ensure original map is not modified in prepareAdditionalMetadata

[yehlo]

Saw that the original map was returned when additionalMetadata was specified leading to the controller adding the labels and annotations as specified here: https://github.com/projectcapsule/capsule/blob/v0.10.0/controllers/resources/processor.go#L133 to the globaltenantresource.spec.resources.additionalMetadata.

Leading to the tenantresource.spec looking like this:

spec:
  pruningOnDelete: true
  resources:
  - additionalMetadata:
      annotations:
        foo: bar
        capsule.clastix.io/tenant: tenant-name
      labels:
        foo: bar
        capsule.clastix.io/resources: "0"
        capsule.clastix.io/tenant: tenant-name

I made sure to just create a new map and return this instead.

[Svarrogh1337]

@yehlo the idea behind prepareAdditionalMetadata is that we either return empty map or the original one if it's defined in the spec. We are not doing any modification at this point, so making copy of the map is unnecessary.
This was done in order to prevent #1387 ( fixed in #1413 ).

Do you see any misbehaviour with this approach?

[yehlo]

Hi @Svarrogh1337 If the original map is returned instead of a copy it will be updated through the reconciliation leading to the globalTenantResource object receiving an update.

This leads to the original map being mutated here:
annotations: https://github.com/projectcapsule/capsule/blob/v0.10.0/controllers/resources/processor.go#L133
labels: https://github.com/projectcapsule/capsule/blob/v0.10.0/controllers/resources/processor.go#L135-L136

As the map is a pointer to the actual resource received here https://github.com/projectcapsule/capsule/blob/v0.10.0/controllers/resources/global.go#L51-L61 the map within k8s will be mutated aswell.

This leads to the following misbehaviour:

I create a globalTenantResource that looks like this:

apiVersion: capsule.clastix.io/v1beta2
kind: GlobalTenantResource
metadata:
  name: my-replication
spec:
  pruningOnDelete: true
  resources:
  - additionalMetadata:
      annotations:
        foo: "bar"
      labels:
        foo: "bar"
    namespacedItems:
    - apiVersion: v1
      kind: Configmap
      namespace: my-conf
      selector:
        matchLabels:
          do-replicate: "true"
  resyncPeriod: 60s
  tenantSelector:
    matchLabels:
      receive-replicated: "true"

While reconciling all tenants matching the tenantSelector the processor.go will add labels specific to the tenant to the map. (s. linked up)

While this map will then be added as metadata on resource creation, which results in the expected behaviour creating a configmap like this:

apiVersion: v1
kind: LimitRange
metadata:
  annotations:
    foo: "bar"
    capsule.clastix.io/tenant: my-tenant # mutated default tenant annotation
  creationTimestamp: "2025-08-08T06:37:28Z"
  labels:
    foo: "bar"
    capsule.clastix.io/resources: "0" # mutated default tenant label
    capsule.clastix.io/tenant: my-tenant # mutated default tenant label
  name: my-conf
  namespace: my-tenant-namespace
data:
  foo: "bar"

It will also result in the original globalTenantResource being mutated with the tenant processed (as the map is a pointer) leading to the resource looking like this:

apiVersion: capsule.clastix.io/v1beta2
kind: GlobalTenantResource
metadata:
  name: my-replication
spec:
  pruningOnDelete: true
  resources:
  - additionalMetadata:
      annotations:
        foo: "bar"
        capsule.clastix.io/tenant: my-tenant # added due to the map being a pointer
      labels:
        foo: "bar"
        capsule.clastix.io/resources: "0" # added due to the map being a pointer
        capsule.clastix.io/tenant: my-tenant # added due to the map being a pointer
    namespacedItems:
    - apiVersion: v1
      kind: Configmap
      namespace: my-conf
      selector:
        matchLabels:
          do-replicate: "true"
  resyncPeriod: 60s
  tenantSelector:
    matchLabels:
      receive-replicated: "true"

While this does not impact the replicated resource it results in diffs within GitOps processes resulting in endless reapplies.

Note

Please note that this only happens if additionalMetadata was specified on your globalTenantResource object

Hope this helps clarifying what I tried to solve with this PR.

[oliverbaehler]

@Svarrogh1337 why do we need that function in the first place? As @yehlo the map given as args is always a reference, so any modification to it behaives like you know it from pointers.

Cant we just drop the function and directly loop or even deepycopy in the metadata check? It really doesnt add much value to abstract this to a function.

Probably even simpler is if we just pass it directly to createOrUpdate and check for nil in there. Blablabla. Just the above.

[Svarrogh1337]

@Svarrogh1337 why do we need that function in the first place? As @yehlo the map given as args is always a reference, so any modification to it behaives like you know it from pointers.

Cant we just drop the function and directly loop or even deepycopy in the metadata check? It really doesnt add much value to abstract this to a function.

Probably even simpler is if we just pass it directly to createOrUpdate and check for nil in there. Blablabla. Just the above.

I was thinking of dropping it in favour of DeepCopy indeed. @yehlo wdyt?

[yehlo]

@Svarrogh1337 sure I don't mind though maps don't support the DeepCopy() method.

I can import the maps module and use it's Clone() method and check for nil on the supplied values.

Removing the prepareMetadata would end in something like this (not tested):

if spec.AdditionalMetadata != nil {
  if spec.AdditionalMetadata.Annotations != nil {
    objAnnotations = maps.Clone(spec.AdditionalMetadata.Annotations)
  } else {
    objAnnotations = make(map[string]string)
  }
  if spec.AdditionalMetadata.Labels != nil {
    objLabels = maps.Clone(spec.AdditionalMetadata.Labels)
  } else {
    objLabels = make(map[string]string)
  }
} 

I'd recommend keeping the method to keep it DRY and either stick to looping over keys (as seen in this PR) or use maps.Clone().

I might aswell just be overthinking.

Let me know what you'd prefer and I'll update accordingly.

[yehlo]

Alright I now left it as is and fixed the commit message to not include camelCase.

[oliverbaehler]

Good, i will add e2e tests for this in my refactor branch. Thanks for the contribution! 🚀