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

Fix issue with ambiguity of control product #12454

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

teacup-on-rockingchair
Copy link
Contributor

Description:

  • Fix issue with ambiguity of control product and partial match of product names vs product specific controls

Rationale:

  • When product member is initialized during loading of yaml file it could be ambiguously created as a string, when control is specific for only one product or list when multiple products are specified in the yaml. The problem with partial match of product names comes later in the add_references method, where the current product of the build is matched vs the control product, and the condition used is product not in self.product.
  • In case of list this condition will check if any of the members of the list is exact match to the product.
  • In case of string though, which is the more common case it will check if the string of the product name, for which we are building is partially matched (contained) in the self.product.

Fixes:

  • The issue was found while analyzing complaint from a contributor Joel Njanga(@barbarello), while he was trying to add support for al2 platform and it was conflicting with existing platform al2023. The discussion can be seen in the gitter/matrix discussion channel

…uct names vs product specific controls

When product member is initialised during loading of yaml file it could be ambigously created as a string, when control
is specific for only one product or list when multiple products are specified in the yaml.
The problem with partial match of product names comes later in the add_references method,
 where the current product of the build is matched vs the control product, and the condition used is `product not in self.product`.
In case of list this condition will check if any of the members of the list is exact match to the product.
In case of string though, which is the more common case it will check if the string of the product name, for which we are building is
partially matched (contained) in the self.product.
The issue was found while analysing complaint from a contributor Joel Njanga(@barbarello), while he was trying to add support for al2 platform and it was conflicting with exsting platform al2023
@teacup-on-rockingchair teacup-on-rockingchair added the Infrastructure Our content build system label Oct 2, 2024
Copy link

github-actions bot commented Oct 2, 2024

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 Oct 2, 2024

🤖 A k8s content image for this PR is available at:
ghcr.io/complianceascode/k8scontent:12454
This image was built from commit: 83eaa21

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:12454

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

@Mab879
Copy link
Member

Mab879 commented Oct 2, 2024

Thanks for the pull request. It appears that the CI failures on the Python Unit tests are valid. Please take look.

If you need any help please let us know.

@teacup-on-rockingchair
Copy link
Contributor Author

Thanks for the pull request. It appears that the CI failures on the Python Unit tests are valid. Please take look.

If you need any help please let us know.

Thanks @Mab879 missed to include the consideration if product remains None

@teacup-on-rockingchair teacup-on-rockingchair added this to the 0.1.75 milestone Oct 3, 2024
@jan-cerny jan-cerny self-assigned this Oct 9, 2024
@jan-cerny
Copy link
Collaborator

@teacup-on-rockingchair Excellent catch!

I think it was originally designed as a filed that should contain just a single product but then it evolved. Unfortunately, at this moment it isn't well handled.

Please update this documentation:

product: product ID, set if the policy is specific to a single product.

Also, consider renaming it to "products" and enforcing it being a list.

ssg/controls.py Outdated
@@ -349,7 +349,11 @@ def load(self):
self.title = ssg.utils.required_key(yaml_contents, "title")
self.source = yaml_contents.get("source", "")
self.reference_type = yaml_contents.get("reference_type", None)
self.product = yaml_contents.get("product", None)
yaml_product = yaml_contents.get("product", None)
if type(yaml_product) is list:
Copy link
Collaborator

Choose a reason for hiding this comment

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

using isinstance() is preffered over type()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks changed it in 83eaa21

Thanks to @jan-cerny for the note 🙇
Copy link

codeclimate bot commented Oct 9, 2024

Code Climate has analyzed commit 83eaa21 and detected 0 issues on this pull request.

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

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

View more on Code Climate.

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.

3 participants