Re-ordering ManagedClusterAddOn configs has no effect

[KevinFCormier]

Describe the bug

Re-ordering entries in ManagedClusterAddOn.spec.configs does not update
the order in status.configReferences. The order is determined by the
first reconcile and never changes, making it impossible to alter config
precedence after initial creation.

Config precedence depends on index ordering — the documented contract on
GetAddOnDeploymentConfigValues (in addon-framework) states that the
highest-index entry takes precedence. However, changing which config
appears last in spec.configs has no effect because
status.configReferences retains its original order.

To Reproduce

Prerequisites: a hub cluster with OCM addon-manager running. This example
uses cluster1 as the managed cluster name, consistent with the
OCM contributing guidelines
for kind-based test environments.

1. Create an AddOnTemplate with a variable placeholder:

kubectl apply -f - <<'EOF'
apiVersion: addon.open-cluster-management.io/v1alpha1
kind: AddOnTemplate
metadata:
  name: ordering-demo
spec:
  addonName: ordering-demo
  agentSpec:
    workload:
      manifests:
        - apiVersion: v1
          kind: ConfigMap
          metadata:
            name: ordering-demo
            namespace: open-cluster-management-agent-addon
          data:
            greeting: "{{GREETING}}"
EOF

2. Create two AddOnDeploymentConfig resources with different values:

kubectl apply -f - <<'EOF'
apiVersion: addon.open-cluster-management.io/v1alpha1
kind: AddOnDeploymentConfig
metadata:
  name: greeting-hello
  namespace: cluster1
spec:
  customizedVariables:
    - name: GREETING
      value: "hello"
---
apiVersion: addon.open-cluster-management.io/v1alpha1
kind: AddOnDeploymentConfig
metadata:
  name: greeting-goodbye
  namespace: cluster1
spec:
  customizedVariables:
    - name: GREETING
      value: "goodbye"
EOF

3. Create a ClusterManagementAddOn referencing the template (no global
   ADC default, so per-cluster configs are the only source):

kubectl apply -f - <<'EOF'
apiVersion: addon.open-cluster-management.io/v1alpha1
kind: ClusterManagementAddOn
metadata:
  name: ordering-demo
  annotations:
    addon.open-cluster-management.io/lifecycle: "addon-manager"
spec:
  addOnMeta:
    displayName: Config Ordering Demo
  installStrategy:
    type: Manual
  supportedConfigs:
    - group: addon.open-cluster-management.io
      resource: addontemplates
      defaultConfig:
        name: ordering-demo
    - group: addon.open-cluster-management.io
      resource: addondeploymentconfigs
EOF

4. Create the ManagedClusterAddOn with two ADC references — hello
   first, goodbye second:

kubectl apply -f - <<'EOF'
apiVersion: addon.open-cluster-management.io/v1alpha1
kind: ManagedClusterAddOn
metadata:
  name: ordering-demo
  namespace: cluster1
spec:
  configs:
    - group: addon.open-cluster-management.io
      resource: addondeploymentconfigs
      name: greeting-hello
      namespace: cluster1
    - group: addon.open-cluster-management.io
      resource: addondeploymentconfigs
      name: greeting-goodbye
      namespace: cluster1
EOF

5. Wait for reconciliation, then inspect status.configReferences:

kubectl get managedclusteraddons ordering-demo -n cluster1 \
  -o jsonpath='{range .status.configReferences[*]}{.resource}: {.desiredConfig.name}{"\n"}{end}'

Note the order — greeting-hello appears before greeting-goodbye.

6. Re-order the configs to put goodbye first:

kubectl apply -f - <<'EOF'
apiVersion: addon.open-cluster-management.io/v1alpha1
kind: ManagedClusterAddOn
metadata:
  name: ordering-demo
  namespace: cluster1
spec:
  configs:
    - group: addon.open-cluster-management.io
      resource: addondeploymentconfigs
      name: greeting-goodbye
      namespace: cluster1
    - group: addon.open-cluster-management.io
      resource: addondeploymentconfigs
      name: greeting-hello
      namespace: cluster1
EOF

7. Wait for reconciliation and inspect again:

kubectl get managedclusteraddons ordering-demo -n cluster1 \
  -o jsonpath='{range .status.configReferences[*]}{.resource}: {.desiredConfig.name}{"\n"}{end}'

The order in status.configReferences is unchanged — greeting-hello
still appears before greeting-goodbye, despite the spec now listing
goodbye first.

Cleanup:

kubectl delete clustermanagementaddons ordering-demo
kubectl delete addontemplates ordering-demo
kubectl delete addondeploymentconfigs greeting-hello greeting-goodbye -n cluster1

Expected behavior

When the user re-orders entries in ManagedClusterAddOn.spec.configs,
the order in status.configReferences should be updated to reflect the
new ordering, so that precedence changes take effect.

Environment ie: OCM version, Kubernetes version and provider:

• OCM addon-manager (all versions with multi-GK config support)
• Kubernetes 4.x / OpenShift 4.x

Additional context

The root cause is in mergeAddonConfig in
pkg/addon/controllers/addonconfiguration/addon_configuration_reconciler.go:

mergedConfigs := make(addonConfigMap)
// First go through the configReferences listed in mca status,
// if the existing config (gvk + namespace + name) is also in the desiredConfigMap,
// append it to mergedConfigs,
// this will save the LastAppliedConfig and LastObservedGeneration from mca status.
for _, configRef := range mcaCopy.Status.ConfigReferences {
    gr := configRef.ConfigGroupResource
    if _, ok := mergedConfigs[gr]; !ok {
        mergedConfigs[gr] = []addonv1beta1.ConfigReference{}
    }
    if _, ok := desiredConfigMap.containsConfig(gr, configRef.DesiredConfig.ConfigReferent); ok {
        mergedConfigs[gr] = append(mergedConfigs[gr], configRef)
    }
}
// Then go through the desiredConfigMap ...

Because the old status.configReferences are iterated first and their
order is preserved, any re-ordering in the desired config map is ignored
for entries that already exist. New entries are appended at the end, but
existing entries keep their original positions.

The fix is to use the desiredConfigMap as the source of truth for
ordering, while preserving LastAppliedConfig and
LastObservedGeneration via a lookup map keyed by
group+resource+name+namespace:

// Build a lookup from old status to preserve rollout tracking fields
oldStatusByKey := map[configKey]addonv1beta1.ConfigReference{}
for _, configRef := range mcaCopy.Status.ConfigReferences {
    key := configKey{...}
    oldStatusByKey[key] = configRef
}

// Iterate desiredConfigMap in sorted GR order — source of truth for ordering
configRefs := []addonv1beta1.ConfigReference{}
for _, gr := range desiredConfigMap.orderedKeys() {
    for _, configRef := range desiredConfigMap[gr] {
        key := configKey{...}
        if old, ok := oldStatusByKey[key]; ok {
            old.DesiredConfig = configRef.DesiredConfig.DeepCopy()
            configRefs = append(configRefs, old)
        } else {
            configRefs = append(configRefs, configRef)
        }
    }
}

Related issue: There is a separate bug in addon-framework where
GetAddOnConfigRef returns the first match instead of the last, which
compounds this issue. See open-cluster-management-io/addon-framework#387

[qiujian16]

I think I understand the bug, but a more fundamental question is should we allow setting two configs with same gvr in the mca? What is the use case if we have to do that? Should we just disallow that in API level.

[tesshuflower]

@qiujian16 this is a good question, and I was wondering this myself - I did get a bit of an answer recently while looking into: #1608. It seems at the CMA level we support this, at least under placements.

My understanding is some downstream projects (observability in particular) use this. I think their use-case is that users can set multiple different prometheus configs, each with different rules ,and my assumption is their addon controller will combine or merge all these configs together. If we support it at the CMA level, does it make sense to do the same thing at MCA level?

I know @KevinFCormier was also looking at whether merging is possible when there are multiple configs. Maybe it's really a decision for the addon implementer?

I.e. If multiple configs are set for a GVK type:

• Do we use just 1 (the helper functions in addon-framework do this currently, as per this issue)
• Do we merge them together (need to use same priority ordering we choose from above I'd assume) - If we want to generalize this flow, perhaps a helper function could be made available from the addon-framework and implementer can decide which to use. I suppose the downside is inconsistent behaviour between addons, but it seems perhaps this is already the case with the observability example above.

[qiujian16]

I know @KevinFCormier was also looking at whether merging is possible when there are multiple configs. Maybe it's really a decision for the addon implementer?

I think so, the implementer has to decide how configuration is merged. But maybe we should also provide some type of API enhancement so user can choose configuration override or merge? Also in the status, should we provided the hash of merged config ins tead of separated one for each configuration?

And I am curious whether rollout should be triggered when the configs are reordered. It may not be necessary since the reorder may not impact the final config, but I am not sure how it can be represented.

[KevinFCormier]

And I am curious whether rollout should be triggered when the configs are reordered. It may not be necessary since the reorder may not impact the final config, but I am not sure how it can be represented.

@qiujian16 If we allow multiple configs of the same group+kind, then I think order must matter to determine precedence, so we would need new rollout when the order changes.

[qiujian16]

yeah...but what if the final config does not change? An addon may choose to merge configs such that even config order changes, the final configuration applied to the agent manifests is the same.

[KevinFCormier]

yeah...but what if the final config does not change? An addon may choose to merge configs such that even config order changes, the final configuration applied to the agent manifests is the same.

@qiujian16 I'm not really sure I follow what you're saying... an addon could also choose to merge configs in a way that the order matters, so I think the status.configReferences needs to respect ordering so that the addon can then choose what to do with it.

Ultimately I'm proposing that by default, we only keep the highest precedence config of a given group-kind. (Default overrideStrategy of Replace.) But for cases with overrideStrategy of Merge, then this defect still applies since ordering may matter (like in the case of layering sets of variables). See #1603 for details.

[qiujian16]

my point is computing hash for each configuration and use that to trigger rolling seems not correct if we want to support multiple same gvk and various configuration merge approaches. It seems to me that the addon should provide hash for all configurations together and set to the status of mca. e.g. hash(mergedConfig(config1, config2, ....)), where mergedConfig func should be implemented by the addon. It will need some update on the addon-framework and mca api, but should be flexible enough to support any configuration rendering/merging strategy.