Skip to content

GetAddOnConfigRef returns first matching ConfigReference instead of last #387

Description

@KevinFCormier

Describe the bug

GetAddOnConfigRef returns the first matching ConfigReference for
a given group+resource, but the documented contract on
GetAddOnDeploymentConfigValues states that the last entry should
take precedence.

The comment on GetAddOnDeploymentConfigValues (in
pkg/addonfactory/addondeploymentconfig.go) says:

"If there are multiple AddOnDeploymentConfig objects in the AddOn
ConfigReferences, the big index object will override the one from small
index"

However, GetAddOnConfigRef (in pkg/utils/addon_config.go) iterates
forward and returns on first match, so the small index object always
wins. The function's own TODO comment acknowledges this:

"(TODO) this needs to be reconsidered if we support multiple same GK
in the config references"

Multiple same-GK config references have been supported since OCM v0.15.0,
making this a functional bug.

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
  1. 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
  1. 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
  1. 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
  1. Wait for the addon to reconcile, then check which value was selected:
kubectl get configmap ordering-demo -n open-cluster-management-agent-addon -o jsonpath='{.data.greeting}'

The result is hello (the first entry), not goodbye (the last entry).

Per the documented contract, the last entry (goodbye) should take
precedence.

Cleanup:

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

Expected behavior

When multiple config references of the same group+resource exist in
status.configReferences, the last entry should take precedence,
matching the documented contract on GetAddOnDeploymentConfigValues.

Environment ie: OCM version, Kubernetes version and provider:

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

Additional context

The root cause is in pkg/utils/addon_config.go:

func GetAddOnConfigRef(
    configReferences []addonapiv1beta1.ConfigReference,
    group, resource string) (bool, addonapiv1beta1.ConfigReference) {

    for _, config := range configReferences {
        if config.Group == group && config.Resource == resource {
            return true, config    // returns on FIRST match
        }
    }
    return false, addonapiv1beta1.ConfigReference{}
}

The fix is to iterate the full list and return the last match:

func GetAddOnConfigRef(
    configReferences []addonapiv1beta1.ConfigReference,
    group, resource string) (bool, addonapiv1beta1.ConfigReference) {

    found := false
    var result addonapiv1beta1.ConfigReference
    for _, config := range configReferences {
        if config.Group == group && config.Resource == resource {
            found = true
            result = config
        }
    }
    return found, result
}

Related issues

There is a separate bug in ocm where re-ordering
entries in ManagedClusterAddOn.spec.configs does not update the order
in status.configReferences, compounding this issue. See open-cluster-management-io/ocm#1600

Risks
This PR does introduce a change in behaviour, however, I think it is unlikely that users are dependent on this selection order because the presence of a 2nd config currently has no impact (When the config is a AddOnDeploymentConfig, it would be nice if variables are merged between multiple configs - I opened an enhancement to implement that: open-cluster-management-io/ocm#1603)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions