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

Store rendered control files #10656

Merged

Conversation

ggbecker
Copy link
Member

@ggbecker ggbecker commented May 30, 2023

Description:

  • Store rendered (resolved) control files when building the content.
  • The files are stored under build/$product/controls/
  • This PR also adds support for reusing controls across different policies.

Rationale:

  • Files can be used to display a policy and the relationship of controls with rules in the project.

Note: Seeking feedback and help on how to implement this properly.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label May 30, 2023
@openshift-ci
Copy link

openshift-ci bot commented May 30, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@github-actions
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

@ggbecker ggbecker force-pushed the store-rendered-control-files branch from d9a9fb0 to 8515e44 Compare May 30, 2023 12:34
ssg/controls.py Outdated Show resolved Hide resolved
@jan-cerny jan-cerny changed the title Store rendered control files. Store rendered control files May 31, 2023
@jan-cerny
Copy link
Collaborator

Files can be used to display a policy and the relationship of controls with rules in the project.

Questions: What is the use-case for this? In what way do you want to show the relationship of controls and rules? What is the benefit of viewing "resolved controls" instead of the original source or the HTML output online?

@ggbecker
Copy link
Member Author

ggbecker commented Jun 5, 2023

Files can be used to display a policy and the relationship of controls with rules in the project.

Questions: What is the use-case for this? In what way do you want to show the relationship of controls and rules? What is the benefit of viewing "resolved controls" instead of the original source or the HTML output online?

This is a requirement we have been discussing for OpenShift products. They want to have an ability to ship the contents of a resolved control to customers. And the controls file resolved will contain only things that are applicable to a given product and any jinja macros (although there is only one in the project in controls file) will also be resolved and the file can be interpreted as pure yaml file.

These files will be bundled whenever the content is built by OpenShift for example and they need to be "versioned" opposed the the HTML output online which represents latest upstream code always.

@jan-cerny
Copy link
Collaborator

@ggbecker Thanks for explanation! If it needs to be in a YAML format I would prefer a "nice" YAML, with no build system internal details and no empty fields, so that it would be better readable.

@ggbecker
Copy link
Member Author

ggbecker commented Jun 6, 2023

@ggbecker Thanks for explanation! If it needs to be in a YAML format I would prefer a "nice" YAML, with no build system internal details and no empty fields, so that it would be better readable.

Agree with the part that no build system internal details, but the empty fields might be useful in the future depending on the needs of a policy. This PR definitely needs more iterations.

It doesn't need to be a "nice" YAML since this will be consumed most likely by Compliance Operator which will parse the file and produce something else for example. So the empty fields are not really a problem.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Jun 13, 2023
@ggbecker
Copy link
Member Author

patch.txt

@ggbecker ggbecker force-pushed the store-rendered-control-files branch from 3f2fc21 to a428a22 Compare June 14, 2023 08:56
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Jun 14, 2023
@matejak matejak force-pushed the store-rendered-control-files branch from 2f7307b to 734d945 Compare June 21, 2023 08:07
@matejak
Copy link
Member

matejak commented Jun 21, 2023

It turned out that this feature interferes with the nested controls feature, when we define (sub)controls inside another controls, and the information is then discarded, so the compiled controls don't have it - they just have the expected rule selections without knowing how those got there.
I have handled that, so subcontrols are formally recorded, and I have extended the mechanics to allow not only definition of subcontrols, but also reference of controls from other policies, all handled the same way.
At this time, the PR contains "ugly" code, but it also contains tests of this functionality.

@matejak matejak force-pushed the store-rendered-control-files branch from c45558f to 545c2ab Compare June 21, 2023 12:27
@ggbecker
Copy link
Member Author

/packit retest-failed

@ggbecker ggbecker marked this pull request as ready for review June 26, 2023 10:05
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Jun 26, 2023
@ggbecker
Copy link
Member Author

It looks good to me, but I think we should provide some examples of how to use the nested controls feature and how people can benefit from it to avoid code duplication.

@jan-cerny jan-cerny added the Highlight This PR/Issue should make it to the featured changelog. label Jun 26, 2023
@jan-cerny jan-cerny added Infrastructure Our content build system New Feature Issues or pull requests related to new Features. labels Jun 26, 2023
@jan-cerny jan-cerny added this to the 0.1.69 milestone Jun 26, 2023
Copy link
Member Author

@ggbecker ggbecker left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the follow up work @matejak .

@jan-cerny
Copy link
Collaborator

@ggbecker What about the CodeClimate problems? And do you need a 3rd person to a review or do you plan to be the assignee?

@matejak
Copy link
Member

matejak commented Jun 26, 2023

So I believe that as Becker and me got our hands dirty with this one, we need e.g. you, @jan-cerny as the 3rd person.

Anyway, I wouldn't document the feature further than it is proposed in the README, because we don't have a meaningful and illustrative use case. Moreover, there are open questions about inheritance of attributes that will influence the usefulness of sub-controls in practice.
Right now, the PR is about policy being able to save its compiled version into a file, and about controls remembering their sub-controls, and being able to load themselves consistently, taking those sub-controls into the account.
Ability to include control references also in control definitions is a side-effect of compilation, and I propose to explore this new capability in subsequent PRs, along with documentation.

Regarding Code Climate, I think that findings are false positives, because the explicit pass makes the code more understandable, so I propose to waive it.

@jan-cerny jan-cerny self-assigned this Jun 26, 2023
Copy link
Collaborator

@jan-cerny jan-cerny left a comment

Choose a reason for hiding this comment

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

I have tried to reference some controls from other controls and I have watched that the rule selections propageted.

@jan-cerny
Copy link
Collaborator

CI fail on Rawhide is caused by OpenSCAP and isn't related to the contents of this PR, see OpenSCAP/openscap#1995.

@codeclimate
Copy link

codeclimate bot commented Jun 26, 2023

Code Climate has analyzed commit f0506b9 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Bug Risk 1

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

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

View more on Code Climate.

@jan-cerny jan-cerny merged commit 3b0b961 into ComplianceAsCode:master Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Highlight This PR/Issue should make it to the featured changelog. Infrastructure Our content build system New Feature Issues or pull requests related to new Features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants