Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACM-8739: ACM Policy that applies stringdata in a secret regression with templates #179

Merged

Conversation

JeffeyL
Copy link
Contributor

@JeffeyL JeffeyL commented Nov 22, 2023

Description: On second evaluation of a secret with stringData, the existingObject is modified while evaluating stringData for comparison. Since the order in which the keys are evaluated is varied, if the underlying object is modified first this may cause issues for future key evaluations.

Solution: when evaluating stringData, operate on a copy of the existing object's data so the actual object is not modified.

Ref: https://issues.redhat.com/browse/ACM-8739

@JeffeyL
Copy link
Contributor Author

JeffeyL commented Nov 22, 2023

The original 2.8 ticket is here: https://issues.redhat.com/browse/ACM-8415, I created a new ticket since this problem may also affect the most recent version.

encodedValue, _, err := unstructured.NestedStringMap(existingObj.Object, "data")
if err != nil {
message := "Error accessing encoded data"

return message, false, mergedValue, false
}

existingValue = make(map[string]interface{}, len(encodedValue))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change this

existingValue.(map[string]interface{})[k] = string(decodedVal)
? because of this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yiraeChristineKim
Copy link
Contributor

/hold

Solution: when decoding, operate on a copy of the existing object's data

Ref: https://issues.redhat.com/browse/ACM-8739
Signed-off-by: Jeffrey Luo <jeluo@redhat.com>
@openshift-ci openshift-ci bot added the lgtm label Nov 23, 2023
Copy link

openshift-ci bot commented Nov 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JeffeyL, yiraeChristineKim

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@yiraeChristineKim
Copy link
Contributor

/unhold

@openshift-merge-bot openshift-merge-bot bot merged commit 2d10d53 into open-cluster-management-io:main Nov 23, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants