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.

Sign up for GitHub

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

Jump to bottom

Update kubelet event creation limit to 50 #10950

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Update kubelet event creation limit to 50 #10950

merged 2 commits into from
Aug 9, 2023

Conversation

yuumasato
Copy link
Member

Description:

  • Update default event creation limit to 50.
  • The value of 5 is still selectable via tailored profile.

Rationale:

  • The kubelet event creation limit bumped from 5 to 50 in OCP CIS 1.4.0.
  • The default value in OCP 4.14 is also bumped from 5 to 50.

This keeps the rule aligned with CIS and OCP.

  • Fixes OCPBUGS-16727

@yuumasato
Update default event creation limit to 50
afa8e6e 
The kubelet event creation limit bumped from 5 to 50 in OCP CIS 1.4.0.
The default value in OCP 4.14 was also bumpted from 5 to 50.

This keeps the rule aligned with CIS and OCP.
The value of 5 is still selectable via tailored profile.
@yuumasato yuumasato added OpenShift OpenShift product related. CIS CIS Benchmark related. labels Aug 3, 2023
@yuumasato yuumasato added this to the 0.1.70 milestone Aug 3, 2023
@yuumasato yuumasato requested a review from rhmdnd August 3, 2023 09:34
@github-actions
Copy link

github-actions bot commented Aug 3, 2023

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

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

yuumasato
yuumasato commented Aug 3, 2023
View reviewed changes
products/ocp4/profiles/cis.profile
@@ -30,6 +30,7 @@ selections:
- cis_ocp_1_4_0:all
### Variables
- var_openshift_audit_profile=WriteRequestBodies
- var_event_record_qps=50
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit superfluous, since the default value is being changed to 50.
But this is more explicit though.

@rhmdnd
Copy link
Collaborator

rhmdnd commented Aug 3, 2023

/test

@openshift-ci
Copy link

openshift-ci bot commented Aug 3, 2023

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

  • /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-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 Aug 3, 2023

/test e2e-aws-ocp4-cis
/test e2e-aws-ocp4-cis-node

@yuumasato
Copy link
Member Author

The tests are running on 4.13 and its default value is 5, so the rule failed.

    helpers.go:808: Result - Name: e2e-cis-kubelet-configure-event-creation - Status: FAIL - Severity: medium
    helpers.go:815: E2E-FAILURE: The expected result for the kubelet_configure_event_creation rule didn't match. Expected 'PASS', Got 'FAIL'

Should we actually change the the rule to pass if the event limit creation is higher than 5?

@rhmdnd
Copy link
Collaborator

rhmdnd commented Aug 4, 2023

Should we actually change the the rule to pass if the event limit creation is higher than 5?

I think we should remain aligned with the CIS guidance. We should update the test to show this fails for 4.13 (and we'll need to update it again when we start using 4.14 in CI).

https://github.com/ComplianceAsCode/content/blob/master/applications/openshift/kubelet/kubelet_configure_event_creation/tests/ocp4/e2e.yml#L2

@yuumasato
Copy link
Member Author

/test e2e-aws-ocp4-cis
/test e2e-aws-ocp4-cis-node

@yuumasato
Copy link
Member Author

/retest

@rhmdnd
Copy link
Collaborator

rhmdnd commented Aug 8, 2023

/test e2e-aws-ocp4-cis

Failure due to timeout.

@yuumasato
Keep the rule's e2e result aligned with CI version
794fabc 
This rules default result is FAIL on 4.13.
It will be PASS when CI is updated to 4.14.
@yuumasato yuumasato force-pushed the update_max_event_creations_per_second branch from 08264dc to 794fabc Compare August 9, 2023 09:09
@yuumasato
Copy link
Member Author

/test e2e-aws-ocp4-cis

@codeclimate
Copy link

codeclimate bot commented Aug 9, 2023

Code Climate has analyzed commit 794fabc 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 53.3% (0.0% change).

View more on Code Climate.

@yuumasato
Copy link
Member Author

This should be good to go.

I also had to add result_after_remediation: PASS, otherwise the e2e test thought the result should be FAIL after remediation.

rhmdnd
rhmdnd reviewed Aug 9, 2023
View reviewed changes
applications/openshift/kubelet/kubelet_configure_event_creation/tests/ocp4/e2e.yml
@@ -1,2 +1,4 @@
---
default_result: PASS
# This rule fails by default on 4.13, and passes by default on 4.14
default_result: FAIL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok - good to know. We'll need to flip this bit when we start using 4.14 in CaC CI

https://github.com/openshift/release/blob/master/ci-operator/config/ComplianceAsCode/content/ComplianceAsCode-content-master.yaml#L12

rhmdnd
rhmdnd approved these changes Aug 9, 2023
View reviewed changes
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

@rhmdnd rhmdnd merged commit 57a6d29 into ComplianceAsCode:master Aug 9, 2023
29 of 30 checks passed
@yuumasato yuumasato deleted the update_max_event_creations_per_second branch August 9, 2023 15:56
@yuumasato yuumasato mentioned this pull request Aug 24, 2023
Merged
@Mab879 Mab879 added the Update Rule Issues or pull requests related to Rules updates. label Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhmdnd rhmdnd rhmdnd approved these changes

@Vincent056 Vincent056 Awaiting requested review from Vincent056

@xiaojiey xiaojiey Awaiting requested review from xiaojiey

Assignees

@rhmdnd rhmdnd

Labels
CIS CIS Benchmark related. OpenShift OpenShift product related. Update Rule Issues or pull requests related to Rules updates.
Projects
None yet
Milestone
0.1.70
Development

Successfully merging this pull request may close these issues.
3 participants
@yuumasato @rhmdnd @Mab879