Skip to content

ROX-18978, ROX-20225: Policy updates#7371

Merged
theencee merged 1 commit intomasterfrom
nc/policy_nftables
Oct 27, 2023
Merged

ROX-18978, ROX-20225: Policy updates#7371
theencee merged 1 commit intomasterfrom
nc/policy_nftables

Conversation

@theencee
Copy link
Contributor

@theencee theencee commented Aug 10, 2023 •
edited
Loading

Description

Combining two changes into one PR because they all deal with one migration.

The changes include:

  • Update the policy migrator code to make it a little more efficient (avoid an unnecessary Exists call). Unfortunately this does lead to some code duplication, but given that the code is used in old migrations, it's best to be careful.
  • Some policies have duplicate exclusions for some reason which used to cause issues with migrator. That's fixed now.

And the actual policy changes:

  • ROX-18978: Updates two policies that check for iptables process to also check nft (which is for nftables)
  • ROX-20225: Exclude deployment aide-worker-fileintegrity in namespace openshift-file-integrity from the policy "Process with UID 0"

If you are only interested in reviewing the policy, I recommend looking at only the files in the pkg/default/policies/files directory since that's the actual policies. Everything else is scaffolding for migrations.

Checklist

  • Investigated and inspected CI test results
  • Unit test and regression tests added
    [ ] Evaluated and added CHANGELOG entry if required
    [ ] Determined and documented upgrade steps
    [ ] Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

Manual testing

  1. Installed fresh version of ACS and validated all the changes
  2. Start with 4.2.2 and then upgraded to this build. Validated all the changes
  3. Start with 4.2.12 Upgrade to this build. Rollback to 4.2.2. Roll forward to this version.

For #3 the most important thing is that central didn't error out or crash. On revert, all of the changes (new criteria, and exclusion) all remained. But that should be ok. On roll forward, the policy criteria remain. The exclusion unfortunately generates a duplicate, but I think that's a side effect we can take (especially since some existing policies already have dupes for exclusions).

Screenshot 2023-10-26 at 5 01 25 PM

In addition to reviewing your code, reviewers must also review your testing
instructions and make sure they are sufficient.

@theencee theencee requested review from a team as code owners August 10, 2023 01:59
@theencee theencee force-pushed the nc/policy_nftables branch from 67a9779 to 3e5536b Compare August 10, 2023 02:04
@dashrews78 dashrews78 mentioned this pull request Aug 10, 2023
Closed
5 tasks
@theencee theencee changed the title ROX-18978: Update policies to also check for nftables ROX-18978, ROX-20223, ROX-20224, ROX-20225: Policy category and exclusions updates Oct 13, 2023
@theencee theencee changed the title ROX-18978, ROX-20223, ROX-20224, ROX-20225: Policy category and exclusions updates ROX-18978, ROX-20223, ROX-20224, ROX-20225: Policy updates Oct 13, 2023
@ghost
Copy link

ghost commented Oct 13, 2023 •
edited by ghost
Loading

Images are ready for the commit at 0fb59f8.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.2.x-564-g0fb59f8259.

@theencee theencee force-pushed the nc/policy_nftables branch 2 times, most recently from 30574b6 to 5dba287 Compare October 14, 2023 04:32
@theencee
Copy link
Contributor Author

theencee commented Oct 18, 2023

/retest

dashrews78
dashrews78 approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@dashrews78 dashrews78 left a comment

Choose a reason for hiding this comment

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

Code changes look good. Please have someone else review the policies.
Before merging make sure no one else took migration 192 as @ksurabhi91 and @rhybrillou have PRs out using 192.

clickboo
clickboo reviewed Oct 19, 2023
View reviewed changes
srcporter
srcporter approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@srcporter srcporter left a comment

Choose a reason for hiding this comment

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

looks great!

@theencee
Copy link
Contributor Author

theencee commented Oct 25, 2023

/retest

@theencee
Copy link
Contributor Author

theencee commented Oct 25, 2023

/test gke-operator-e2e-tests gke-upgrade-tests gke-nongroovy-e2e-tests gke-qa-e2e-tests

@theencee
Copy link
Contributor Author

theencee commented Oct 25, 2023

/test ocp-4-13-qa-e2e-tests

@theencee theencee force-pushed the nc/policy_nftables branch 5 times, most recently from 2c71af9 to 270543a Compare October 26, 2023 21:34
@theencee theencee changed the title ROX-18978, ROX-20223, ROX-20224, ROX-20225: Policy updates ROX-18978, ROX-20225: Policy updates Oct 26, 2023
@theencee
Copy link
Contributor Author

theencee commented Oct 26, 2023

Removed all changes to do with categories for this PR

@theencee
Copy link
Contributor Author

theencee commented Oct 27, 2023

/test gke-upgrade-tests

@theencee
Copy link
Contributor Author

theencee commented Oct 27, 2023

/test gke-upgrade-tests ocp-4-10-qa-e2e-tests

@openshift-ci
Copy link

openshift-ci bot commented Oct 27, 2023

@theencee: The following tests 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/ocp-4-13-ebpf-qa-e2e-tests f1ad832 link false /test ocp-4-13-ebpf-qa-e2e-tests
ci/prow/ocp-4-10-ebpf-qa-e2e-tests f1ad832 link false /test ocp-4-10-ebpf-qa-e2e-tests
ci/prow/gke-upgrade-tests 0fb59f8 link false /test gke-upgrade-tests
ci/prow/ocp-4-10-qa-e2e-tests 0fb59f8 link false /test ocp-4-10-qa-e2e-tests

Full PR test history. Your PR dashboard.

Details

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.

@theencee
Copy link
Contributor Author

theencee commented Oct 27, 2023

The ocp-4-10-qa-e2e-tests failure looks like it's due to a flaky AuditLogsAlertsTest which we know about and is unrelated.

@theencee theencee merged commit 355b472 into master Oct 27, 2023
@theencee theencee deleted the nc/policy_nftables branch October 27, 2023 07:39
@theencee theencee added this to the 4.3.0-rc.1 milestone Oct 27, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2023

Merge commit has been cherry-picked to branch release-4.3.

ghost pushed a commit that referenced this pull request Oct 27, 2023
@theencee @web-flow
ROX-18978, ROX-20225: Policy updates (#7371)
2082d0e 
(cherry picked from commit 355b472)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clickboo clickboo clickboo left review comments

@dashrews78 dashrews78 dashrews78 approved these changes

@boazmichaely boazmichaely Awaiting requested review from boazmichaely

+1 more reviewer

@srcporter srcporter srcporter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/central

Projects

None yet

Milestone

4.3.0-rc.1

Development

Successfully merging this pull request may close these issues.

4 participants

@theencee @srcporter @clickboo @dashrews78