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

CMP-2130: Implement support for profile versioning #425

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

rhmdnd
Copy link

@rhmdnd rhmdnd commented Sep 28, 2023

This commit adds support for an optional version attribute for Profile
custom resources. This attribute is parsed out of the datastream and set
on the Profile by the compliance operator. It's not intended for end
users to supply their own version.

Future patches may expand on this concept to support multiple versions
of a single profile.

@openshift-ci-robot
Copy link
Collaborator

@rhmdnd: This pull request references CMP-2130 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

This commit adds support for an optional version attribute for Profile
custom resources. This attribute is parsed out of the datastream and set
on the Profile by the compliance operator. It's not intended for end
users to supply their own version.

Future patches may expand on this concept to support multiple versions
of a single profile.

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/test-infra repository.

CHANGELOG.md Outdated Show resolved Hide resolved
config/manager/kustomization.yaml Outdated Show resolved Hide resolved
@rhmdnd
Copy link
Author

rhmdnd commented Oct 13, 2023

Need an end-to-end integration test, which shouldn't be too difficult now that the some of the profile versions have landed in the cac/content repository.

@xiaojiey
Copy link
Collaborator

/hold

@xiaojiey
Copy link
Collaborator

xiaojiey commented Oct 17, 2023

Verification pass with 4.14.0-0.nightly-2023-10-16-104540 + CO built with code in the PR:
$ oc get customresourcedefinitions.apiextensions.k8s.io profiles.compliance.openshift.io -o=jsonpath={.spec.versions[0].additionalPrinterColumns} | jq -r
[
{
"jsonPath": ".version",
"name": "Version",
"type": "string"
}
]
$ oc get profile ocp4-cis -o=jsonpath={.version}
1.4.0
$ oc get profile.compliance
NAME VERSION
ocp4-cis 1.4.0
ocp4-cis-node 1.4.0
ocp4-e8
ocp4-high Revision 4
ocp4-high-node Revision 4
ocp4-moderate Revision 4
ocp4-moderate-node Revision 4
ocp4-nerc-cip
ocp4-nerc-cip-node
ocp4-pci-dss 3.2.1
ocp4-pci-dss-node 3.2.1
ocp4-stig V1R1
ocp4-stig-node V1R1
rhcos4-anssi-bp28-enhanced
rhcos4-anssi-bp28-high
rhcos4-anssi-bp28-intermediary
rhcos4-anssi-bp28-minimal
rhcos4-e8
rhcos4-high Revision 4
rhcos4-moderate Revision 4
rhcos4-nerc-cip
rhcos4-stig V1R1

@xiaojiey
Copy link
Collaborator

/label qe-approved
/unhold

Copy link
Member

@yuumasato yuumasato 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 to me.
@Vincent056 Can you check it as well?

@rhmdnd
Copy link
Author

rhmdnd commented Oct 17, 2023

/test e2e-aws-parallel

Profile parsing timed out, but I wasn't able to recreate it locally. Retesting.

Copy link

@Vincent056 Vincent056 left a comment

Choose a reason for hiding this comment

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

/lgtm

@yuumasato
Copy link
Member

/retest

This commit adds support for an optional version attribute for Profile
custom resources. This attribute is parsed out of the datastream and set
on the Profile by the compliance operator. It's not intended for end
users to supply their own version.
@xiaojiey
Copy link
Collaborator

Verification pass with 4.14.0-rc.7 + code in the PR.
@rhmdnd A minor issue is: some profiles have an empty value for the field version. Such as nerc-cip and e8 profiles. It is expected? Thanks.
$ oc get customresourcedefinitions.apiextensions.k8s.io profiles.compliance.openshift.io -o=jsonpath={.spec.versions[0].additionalPrinterColumns} | jq -r
[
{
"jsonPath": ".metadata.creationTimestamp",
"name": "Age",
"type": "date"
},
{
"jsonPath": ".version",
"name": "Version",
"type": "string"
}
]
$ oc get profile.compliance
NAME AGE VERSION
ocp4-cis 2m20s 1.4.0
ocp4-cis-node 2m20s 1.4.0
ocp4-e8 2m20s
ocp4-high 2m20s Revision 4
ocp4-high-node 2m20s Revision 4
ocp4-moderate 2m20s Revision 4
ocp4-moderate-node 2m20s Revision 4
ocp4-nerc-cip 2m20s
ocp4-nerc-cip-node 2m20s
ocp4-pci-dss 2m20s 3.2.1
ocp4-pci-dss-node 2m20s 3.2.1
ocp4-stig 2m20s V1R1
ocp4-stig-node 2m20s V1R1
rhcos4-anssi-bp28-enhanced 2m17s
rhcos4-anssi-bp28-high 2m16s
rhcos4-anssi-bp28-intermediary 2m16s
rhcos4-anssi-bp28-minimal 2m16s
rhcos4-e8 2m16s
rhcos4-high 2m16s Revision 4
rhcos4-moderate 2m16s Revision 4
rhcos4-nerc-cip 2m16s
$ oc get profile ocp4-cis -o=jsonpath={.version}
1.4.0

@rhmdnd
Copy link
Author

rhmdnd commented Oct 31, 2023

A minor issue is: some profiles have an empty value for the field version. Such as nerc-cip and e8 profiles. It is expected?

NERC-CIP profile controls are versioned, and we haven't built that into this construct (we're not sure if we will). From what I can tell the E8 profile uses an "updated date" in place of a version.

In these cases, we aren't required to version all profiles before landing the implementation since the version attribute is optional.

}

// +kubebuilder:object:root=true

// Profile is the Schema for the profiles API
// +kubebuilder:resource:path=profiles,scope=Namespaced,shortName=profs;prof
// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"

Choose a reason for hiding this comment

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

wondering if we update the profile, do we still have the old creationTimestamp

Copy link
Author

Choose a reason for hiding this comment

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

I think it would, since this patch isn't changing the behavior of the creation time.

Do you think that behavior needs to be different.

In my thinking, the profiles are still created from the profile parsers. Multiple versions of the same profile will have the same creation timestamp since they're all created when the profile parser runs. We don't support a way to update a profile besides uploading a new content image, which creates a new profile, right?

Choose a reason for hiding this comment

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

From today's discussion during sync up meeting, I think adding age here makes sense, because we had shown age in the past, and adding this here preserve such behavior.

Copy link

@Vincent056 Vincent056 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 1, 2023
Copy link

openshift-ci bot commented Nov 1, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhmdnd, Vincent056

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

@rhmdnd
Copy link
Author

rhmdnd commented Nov 3, 2023

This PR will go hand-in-hand with ComplianceAsCode/content#11241

@rhmdnd rhmdnd closed this Nov 3, 2023
@rhmdnd rhmdnd reopened this Nov 3, 2023
@rhmdnd
Copy link
Author

rhmdnd commented Nov 3, 2023

/retest

@openshift-ci openshift-ci bot merged commit ae5267f into ComplianceAsCode:master Nov 3, 2023
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.

7 participants