OCPBUGS-73929: lib/resourcemerge/core: Reconcile ConfigMap binaryData too

[wking]

Since 4.20, some Cluster-API ConfigMap manifests have wanted to set this:

$ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.20.10-x86_64
$ grep -r binaryData manifests
manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml:binaryData:

so we need to compare the in-cluster ConfigMap with this property as well, when deciding if the resource needs updating.

[openshift-ci-robot]

@wking: This pull request references Jira Issue OCPBUGS-73929, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.22.0) matches configured target version for branch (4.22.0)
• bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jiajliu

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Since 4.20, some Cluster-API ConfigMap manifests have wanted to set this:

$ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.20.10-x86_64
$ grep -r binaryData manifests
manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml:binaryData:

so we need to compare the in-cluster ConfigMap with this property as well, when deciding if the resource needs updating.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Walkthrough

Added merging of ConfigMap BinaryData: a new helper merges byte-slice maps and EnsureConfigMap now invokes it to incorporate BinaryData before merging Data.

Changes

Cohort / File(s)	Summary
ConfigMap merge call lib/resourcemerge/core.go	Invoke new mergeByteSliceMap(modified, &existing.BinaryData, required.BinaryData) in EnsureConfigMap to include BinaryData in the merge flow.
Byte-slice map merge helper lib/resourcemerge/meta.go	Add mergeByteSliceMap(modified *bool, existing *map[string][]byte, required map[string][]byte) which initializes existing if nil and updates keys whose byte-slice values differ (compared by string conversion), marking modified.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

lib/resourcemerge/meta.go (1)

43-55: Prefer bytes.Equal (and copy) for BinaryData merges.
String conversion allocates and obscures intent; assigning v shares the backing array. Using bytes.Equal and copying makes the comparison explicit and avoids aliasing surprises.

♻️ Proposed refactor

 import (
+	"bytes"
 	"k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@
 	for k, v := range required {
-		if existingV, ok := (*existing)[k]; !ok || string(v) != string(existingV) {
+		if existingV, ok := (*existing)[k]; !ok || !bytes.Equal(v, existingV) {
 			*modified = true
-			(*existing)[k] = v
+			(*existing)[k] = append([]byte(nil), v...)
 		}
 	}
 }

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 2c87f33 and d294e8c.

📒 Files selected for processing (2)

• lib/resourcemerge/core.go
• lib/resourcemerge/meta.go

🚧 Files skipped from review as they are similar to previous changes (1)

• lib/resourcemerge/core.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• lib/resourcemerge/meta.go

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[JoelSpeed]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[wking]

Seems straightforward enough that I don't think we need to wait on pre-merge verification, and I can verify post-merge:

/verified by @wking

[openshift-ci-robot]

@wking: This PR has been marked as verified by @wking.

Details

In response to this:

Seems straightforward enough that I don't think we need to wait on pre-merge verification, and I can verify post-merge:

/verified by @wking

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[wking]

launch 4.22,openshift/cluster-version-operator#1294 aws,techpreview ClusterBot cluster (logs), original content:

$ oc -n openshift-cluster-api get -o jsonpath-as-json='{.binaryData.components\-zstd}' configmap aws | cut -b -80
[
    "KLUv/QRolCYC2g+iWDOgaoxqDt6xwE7Zvyk57mwiafsazqcoDN8KhhZ3P5BbAujrQ8ApDwHPgX1
]

Inject my own stuff:

$ echo testing | base64
dGVzdGluZwo=
$ oc -n openshift-cluster-api patch configmap aws --type json -p '[{"op": "add", "path": "/binaryData/components-zstd", "value": "dGVzdGluZwo="}]'
configmap/aws patched

See if the CVO stomped it back. Not yet...

$ oc -n openshift-cluster-version logs -l k8s-app=cluster-version-operator --tail -1 | grep 'configmap.*cluster-api\|Result of work' | tail -n3
I0116 19:05:48.465959       1 task_graph.go:564] Result of work: errs=[]
I0116 19:09:32.898192       1 task_graph.go:564] Result of work: errs=[]
I0116 19:13:17.442367       1 task_graph.go:564] Result of work: errs=[]
$ oc -n openshift-cluster-api get -o jsonpath-as-json='{.binaryData.components\-zstd}' configmap aws | cut -b -80
[
    "dGVzdGluZwo="
]

Wait for the CVO to get through another round of reconciliation... and there's the expected stomp:

$ oc -n openshift-cluster-version logs -l k8s-app=cluster-version-operator --tail -1 | grep -i 'openshift-cluster-api/aws\|Result of work' | tail -n3
I0116 19:13:17.442367       1 task_graph.go:564] Result of work: errs=[]
I0116 19:17:00.760830       1 core.go:138] Updating ConfigMap openshift-cluster-api/aws due to diff:   &v1.ConfigMap{
I0116 19:17:02.027428       1 task_graph.go:564] Result of work: errs=[]
$ oc -n openshift-cluster-api get -o jsonpath-as-json='{.binaryData.components\-zstd}' configmap aws | cut -b -80
[
    "KLUv/QRolCYC2g+iWDOgaoxqDt6xwE7Zvyk57mwiafsazqcoDN8KhhZ3P5BbAujrQ8ApDwHPgX1
]

Looks good to me :)

[openshift-ci]

@wking: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[openshift-ci-robot]

@wking: Jira Issue Verification Checks: Jira Issue OCPBUGS-73929
✔️ This pull request was pre-merge verified.
✔️ All associated pull requests have merged.
✔️ All associated, merged pull requests were pre-merge verified.

Jira Issue OCPBUGS-73929 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓

Details

In response to this:

Since 4.20, some Cluster-API ConfigMap manifests have wanted to set this:

$ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.20.10-x86_64
$ grep -r binaryData manifests
manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml:binaryData:

so we need to compare the in-cluster ConfigMap with this property as well, when deciding if the resource needs updating.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[damdo]

/cherry-pick release-4.21 release-4.20

[openshift-cherrypick-robot]

@damdo: new pull request created: #1295

Details

In response to this:

/cherry-pick release-4.21 release-4.20

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.