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

OCP Update variable filter to consider go_template #11906

Merged
merged 1 commit into from May 3, 2024

Conversation

Vincent056
Copy link
Contributor

@Vincent056 Vincent056 commented Apr 26, 2024

Description:

Update the variable filter to find if a rule is using go-template, if so find any var being used, and add them to the var list for that rule, Compliance Operator use [go template](https://pkg.go.dev/text/template) to do additional processing, sometimes we reference a xccdf variable within templated content.

Copy link

Start a new ephemeral environment with changes proposed in this pull request:

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

Copy link

github-actions bot commented Apr 26, 2024

🤖 A k8s content image for this PR is available at:
ghcr.io/complianceascode/k8scontent:11906
This image was built from commit: 59162ea

Click here to see how to deploy it

If you alread have Compliance Operator deployed:
utils/build_ds_container.py -i ghcr.io/complianceascode/k8scontent:11906

Otherwise deploy the content and operator together by checking out ComplianceAsCode/compliance-operator and:
CONTENT_IMAGE=ghcr.io/complianceascode/k8scontent:11906 make deploy-local

@Vincent056 Vincent056 force-pushed the filter branch 3 times, most recently from 8fed1ef to e1bdec0 Compare April 26, 2024 20:53
@rhmdnd
Copy link
Collaborator

rhmdnd commented Apr 26, 2024

/test

Copy link

openshift-ci bot commented Apr 26, 2024

@rhmdnd: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

  • /test 4.13-e2e-aws-ocp4-cis
  • /test 4.13-e2e-aws-ocp4-cis-node
  • /test 4.13-e2e-aws-ocp4-e8
  • /test 4.13-e2e-aws-ocp4-high
  • /test 4.13-e2e-aws-ocp4-high-node
  • /test 4.13-e2e-aws-ocp4-moderate
  • /test 4.13-e2e-aws-ocp4-moderate-node
  • /test 4.13-e2e-aws-ocp4-pci-dss
  • /test 4.13-e2e-aws-ocp4-pci-dss-node
  • /test 4.13-e2e-aws-ocp4-stig
  • /test 4.13-e2e-aws-ocp4-stig-node
  • /test 4.13-e2e-aws-rhcos4-e8
  • /test 4.13-e2e-aws-rhcos4-high
  • /test 4.13-e2e-aws-rhcos4-moderate
  • /test 4.13-e2e-aws-rhcos4-stig
  • /test 4.13-images
  • /test 4.14-images
  • /test 4.15-e2e-aws-ocp4-cis
  • /test 4.15-e2e-aws-ocp4-cis-node
  • /test 4.15-e2e-aws-ocp4-e8
  • /test 4.15-e2e-aws-ocp4-high
  • /test 4.15-e2e-aws-ocp4-high-node
  • /test 4.15-e2e-aws-ocp4-moderate
  • /test 4.15-e2e-aws-ocp4-moderate-node
  • /test 4.15-e2e-aws-ocp4-pci-dss
  • /test 4.15-e2e-aws-ocp4-pci-dss-node
  • /test 4.15-e2e-aws-ocp4-stig
  • /test 4.15-e2e-aws-ocp4-stig-node
  • /test 4.15-e2e-aws-rhcos4-e8
  • /test 4.15-e2e-aws-rhcos4-high
  • /test 4.15-e2e-aws-rhcos4-moderate
  • /test 4.15-e2e-aws-rhcos4-stig
  • /test 4.15-images
  • /test 4.16-e2e-aws-ocp4-cis
  • /test 4.16-e2e-aws-ocp4-cis-node
  • /test 4.16-e2e-aws-ocp4-e8
  • /test 4.16-e2e-aws-ocp4-high
  • /test 4.16-e2e-aws-ocp4-high-node
  • /test 4.16-e2e-aws-ocp4-moderate
  • /test 4.16-e2e-aws-ocp4-moderate-node
  • /test 4.16-e2e-aws-ocp4-pci-dss
  • /test 4.16-e2e-aws-ocp4-pci-dss-node
  • /test 4.16-e2e-aws-ocp4-stig
  • /test 4.16-e2e-aws-ocp4-stig-node
  • /test 4.16-e2e-aws-rhcos4-e8
  • /test 4.16-e2e-aws-rhcos4-high
  • /test 4.16-e2e-aws-rhcos4-moderate
  • /test 4.16-e2e-aws-rhcos4-stig
  • /test 4.16-images
  • /test e2e-aws-ocp4-cis
  • /test e2e-aws-ocp4-cis-node
  • /test e2e-aws-ocp4-e8
  • /test e2e-aws-ocp4-high
  • /test e2e-aws-ocp4-high-node
  • /test e2e-aws-ocp4-moderate
  • /test e2e-aws-ocp4-moderate-node
  • /test e2e-aws-ocp4-pci-dss
  • /test e2e-aws-ocp4-pci-dss-node
  • /test e2e-aws-ocp4-stig
  • /test e2e-aws-ocp4-stig-node
  • /test e2e-aws-rhcos4-e8
  • /test e2e-aws-rhcos4-high
  • /test e2e-aws-rhcos4-moderate
  • /test e2e-aws-rhcos4-stig
  • /test images

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-ComplianceAsCode-content-master-4.13-images
  • pull-ci-ComplianceAsCode-content-master-4.14-images
  • pull-ci-ComplianceAsCode-content-master-4.15-images
  • pull-ci-ComplianceAsCode-content-master-4.16-images
  • pull-ci-ComplianceAsCode-content-master-images

In response to this:

/test

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.

@rhmdnd
Copy link
Collaborator

rhmdnd commented Apr 26, 2024

/test e2e-aws-ocp4-pci-dss
/test e2e-aws-ocp4-pci-dss-node

@xiaojiey
Copy link
Collaborator

/hold for test

@openshift-ci openshift-ci bot added the do-not-merge/hold Used by openshift-ci-robot bot. label Apr 28, 2024
@xiaojiey
Copy link
Collaborator

Verification pass with 4.16.0-0.nightly-2024-04-26-145258 + ghcr.io/complianceascode/k8scontent:11906:

1. e2e failures on master branch:
$ oc compliance bind -N test-pci-dss profile/ocp4-pci-dss profile/ocp4-pci-dss-node
Creating ScanSettingBinding test-pci-dss
$ oc get suite -w
NAME           PHASE     RESULT
test           RUNNING   NOT-AVAILABLE
test-pci-dss   PENDING
...
test-pci-dss   RUNNING     NOT-AVAILABLE
...
test-pci-dss   RUNNING     NOT-AVAILABLE
^C
$ oc get pod
NAME                                              READY   STATUS       RESTARTS        AGE
compliance-operator-6bf6fbc8c8-v7wtj              1/1     Running      1 (5m17s ago)   5m23s
ocp4-openshift-compliance-pp-7c6d678c7f-m8whd     1/1     Running      0               5m15s
ocp4-pci-dss-api-checks-pod                       0/2     Init:Error   4 (55s ago)     2m
ocp4-pci-dss-rs-665d6b5cb8-bq28d                  1/1     Running      0               2m
rhcos4-openshift-compliance-pp-86665cf945-dfs4x   1/1     Running      0               5m15s
[3:10](https://redhat-internal.slack.com/archives/DNPDDTQA3/p1714288242759759)
$ oc logs pod/ocp4-pci-dss-api-checks-pod --all-containers
...
Fetching URI: '/api/v1/namespaces/-/pods?labelSelector=app%3Dkube-controller-manager'
FATAL:Error fetching resources: couldn't filter '{"kind":"PodList","apiVersion":"v1","metadata":{"resourceVersion":"92523"},"items":[]}
': cannot iterate over: null
Error from server (BadRequest): container "log-collector" in pod "ocp4-pci-dss-api-checks-pod" is waiting to start: PodInitializing
2. Issue get fixed this this PR:
$ oc compliance bind -N test-11906 profile/upstream-ocp4-pci-dss profile/upstream-ocp4-pci-dss-node
Creating ScanSettingBinding test-11906
$ oc get suite -w
NAME           PHASE     RESULT
test           RUNNING   NOT-AVAILABLE
test-11906     PENDING
...
test-11906     DONE          NON-COMPLIANT
test-11906     DONE          NON-COMPLIANT

@xiaojiey
Copy link
Collaborator

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Used by openshift-ci-robot bot. label Apr 28, 2024
@xiaojiey
Copy link
Collaborator

/lgtm

@xiaojiey
Copy link
Collaborator

@Vincent056 seems one issue to fix: https://codeclimate.com/github/ComplianceAsCode/content/pull/11906. Could you please take a look? Thanks.

@rhmdnd
Copy link
Collaborator

rhmdnd commented Apr 29, 2024

/test e2e-aws-ocp4-pci-dss
/test e2e-aws-ocp4-pci-dss-node

@Vincent056 Vincent056 force-pushed the filter branch 3 times, most recently from b0124e3 to f2a9860 Compare April 29, 2024 17:58
@Vincent056
Copy link
Contributor Author

/test e2e-aws-ocp4-pci-dss
/test e2e-aws-ocp4-pci-dss-node

@rhmdnd
Copy link
Collaborator

rhmdnd commented Apr 29, 2024

/test 4.15-e2e-aws-ocp4-high
/test 4.15-e2e-aws-ocp4-high-node
/test 4.15-e2e-aws-rhcos4-high

Copy link

openshift-ci bot commented Apr 29, 2024

@Vincent056: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/4.15-e2e-aws-rhcos4-high f2a9860 link true /test 4.15-e2e-aws-rhcos4-high

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@rhmdnd
Copy link
Collaborator

rhmdnd commented Apr 30, 2024

The stig e2e failure is unrelated and being addressed in a separate PR.

Copy link
Collaborator

@rhmdnd rhmdnd left a comment

Choose a reason for hiding this comment

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

/lgtm

Looks good from a OpenShift content perspective.

@rhmdnd
Copy link
Collaborator

rhmdnd commented Apr 30, 2024

@Mab879 does this seem reasonable from the build perspective?

@rhmdnd
Copy link
Collaborator

rhmdnd commented May 1, 2024

@Honny1 how do you feel about this approach given the recent refactors to the build system made in #11858 ?

Copy link
Collaborator

@Honny1 Honny1 left a comment

Choose a reason for hiding this comment

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

I built and compared ocp4 ds from this PR branch and the main branch and found no problem. However, I would like to ask you to create tests for this problem to avoid similar problems in future changes. Also, I would like to ask why you are processing the DS after the build? Wouldn't it be better to extend the macros during the build?

build-scripts/build_xccdf.py Outdated Show resolved Hide resolved
def get_variables_from_go_templating(rule, var_ids):
go_templating_pattern = re.compile(r"{{(.*?)}}")
go_templating_var_pattern = re.compile(r"\.([a-zA-Z0-9_]+)")
for ele in rule.itertext():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can go_template be used only in the text part of a rule or can it be present in an XML element attribute?

Copy link
Contributor Author

@Vincent056 Vincent056 May 2, 2024

Choose a reason for hiding this comment

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

they will present in the text part of the rule as well as in the remediations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think fix is also part of the rule

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I was curious. Yes, the fix element is a sub-element of the Rule element.

Update the variable filter to find if a rule is using go-template, if so find any var being used, add them to var list for that rule
@Vincent056
Copy link
Contributor Author

Vincent056 commented May 2, 2024

I built and compared ocp4 ds from this PR branch and the main branch and found no problem. However, I would like to ask you to create tests for this problem to avoid similar problems in future changes. Also, I would like to ask why you are processing the DS after the build? Wouldn't it be better to extend the macros during the build?

Thanks for the review, we have e2e test, but we don't run that in every PR, for example you could run /test 4.15-e2e-aws-ocp4-high for one of the test, I can add the unit test in another PR since this is blocking our CI and release.

The go_template is use to in our operator when consuming the datastream file, so that we can process the remediation to use xccdf variables as well as other part of rule to be more dynamic. @Honny1

Copy link

codeclimate bot commented May 2, 2024

Code Climate has analyzed commit 59162ea and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 59.4% (0.0% change).

View more on Code Climate.

@Honny1
Copy link
Collaborator

Honny1 commented May 2, 2024

@Vincent056
Okay, please add a unit test in another PR.

Well, I think the sub element would do the same magic. See rule: xccdf_org.ssgproject.content_rule_enable_fips_mode
Or is there some other reason for this approach?

@Vincent056
Copy link
Contributor Author

Vincent056 commented May 2, 2024

xccdf_org.ssgproject.content_rule_enable_fips_mode

@Honny1 great, I will do that, we were also depending on go_templating to be able to fetch different kube API, which is defined in warning part of the rule in the compliance operator dynamically based on value of the xccdf variables, among other things such as render value of referenced variables in rule description

@Honny1
Copy link
Collaborator

Honny1 commented May 2, 2024

@Vincent056 Okay

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.

Thank you for the review @Honny1

/lgtm

@yuumasato
Copy link
Member

/packit help

@yuumasato
Copy link
Member

/packit retest-failed

@yuumasato yuumasato merged commit bd9ef20 into ComplianceAsCode:master May 3, 2024
113 checks passed
@yuumasato yuumasato added this to the 0.1.74 milestone May 3, 2024
@yuumasato yuumasato self-assigned this May 3, 2024
@theboringstuff
Copy link

theboringstuff commented May 3, 2024

This will not be included in v0.1.73? Is there WA for this issue? I encounter similar problem for openshift cluster

Fetching URI: '/api/v1/namespaces/-/pods?labelSelector=app%3Dkube-controller-manager'
debug: Applying filter '[[.items[0].spec.containers[0].args[] | select(. | match("--port=[1-9]*[1-9]+") )] | length | if . == 0 then true else false end]' to path '/api/v1/namespaces/-/pods?labelSelector=app%3Dkube-controller-manager'
debug: Error while filtering: cannot iterate over: null
debug: Persisting warnings to output file
FATAL:Error fetching resources: couldn't filter '{"kind":"PodList","apiVersion":"v1","metadata":{"resourceVersion":"190509111"},"items":[]}
': cannot iterate over: null

Are there older content/CO versions where this issue is not present (to use as a WA)?

@yuumasato
Copy link
Member

As #11858 is going into 0.1.73, I think it makes sense to backport this fix there.
@ComplianceAsCode/red-hatters do you guys concur?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Our content build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants