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

Add support for recording the diff in the ConfigurationPolicy status #246

Merged

Conversation

mprahl
Copy link
Member

@mprahl mprahl commented May 17, 2024

A new recordDiff option of "InStatus" allows the diff to be stored in
the object properties in the ConfigurationPolicy status.

The new default recordDiff value is "InStatus" unless sensitive data may
be in the diff. Then the user must explicitly set recordDiff.

Relates:
https://issues.redhat.com/browse/ACM-11421

@mprahl mprahl force-pushed the diff-in-status branch 3 times, most recently from 2c99486 to 376c11c Compare May 17, 2024 18:17
@mprahl mprahl changed the title WIP: Add support for recording the diff in the ConfigurationPolicy status Add support for recording the diff in the ConfigurationPolicy status May 17, 2024
@mprahl
Copy link
Member Author

mprahl commented May 17, 2024

/cc @yiraeChristineKim

apiVersion: "route.openshift.io/v1",
kind: "Route",
expected: RecordDiffSensored,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
},
},
{
testName: "Route-recordDiff-unset",
apiVersion: "route.openshift.io/v1",
kind: "Route",
recordDiff: RecordDiffLog,
expected: RecordDiffSensored,
},

Add this test too?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the user explicitly sets recordDiff we want to honor that. RecordDiffSensored is just to detect when sensitive data is present and we don't want to store the diff in the status by default.

}

// handleDiff will generate the diff and then log it or return it based on the input recordDiff value. If recordDiff
// is set to None or is set to InStatus with enforce, no diff is generated. When recordDiff is set to Sensored, a
Copy link
Contributor

@yiraeChristineKim yiraeChristineKim May 17, 2024

Choose a reason for hiding this comment

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

inform + inStatus creates diff? why? why enfore+instatus doesn't create diff?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the policy is enforced, then the diff is no longer relevant after the update so it shouldn't get stored in the status.

Copy link
Member

Choose a reason for hiding this comment

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

I was confused about this as well, can the comment explain that detail?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment that should help clarify this.

Copy link
Member

@JustinKuli JustinKuli left a comment

Choose a reason for hiding this comment

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

Looks good overall.

api/v1/configurationpolicy_types.go Outdated Show resolved Hide resolved
api/v1/configurationpolicy_types_test.go Outdated Show resolved Hide resolved
controllers/configurationpolicy_controller.go Outdated Show resolved Hide resolved
controllers/configurationpolicy_controller.go Show resolved Hide resolved
controllers/configurationpolicy_controller.go Outdated Show resolved Hide resolved
@JustinKuli
Copy link
Member

The Jira mentions "the new options of InStatus and Automatic" but I think this implementation doesn't have Automatic - it just has some behavior when the field isn't set. That's fine with me, just wanted to make sure that detail doesn't get lost.

A new recordDiff option of "InStatus" allows the diff to be stored in
the object properties in the ConfigurationPolicy status.

The new default recordDiff value is "InStatus" unless sensitive data may
be in the diff. Then the user must explicitly set recordDiff.

Relates:
https://issues.redhat.com/browse/ACM-11421

Signed-off-by: mprahl <mprahl@users.noreply.github.com>
Signed-off-by: mprahl <mprahl@users.noreply.github.com>
Copy link
Member

@JustinKuli JustinKuli left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for those updates.

@openshift-ci openshift-ci bot added the lgtm label May 20, 2024
Copy link

openshift-ci bot commented May 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, mprahl

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

@openshift-merge-bot openshift-merge-bot bot merged commit 883e801 into open-cluster-management-io:main May 20, 2024
9 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.

3 participants