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

OPCT-6 - fix/rbac: manage custom ServiceAccount and roles #34

Merged

Conversation

mtulio
Copy link
Contributor

@mtulio mtulio commented Nov 22, 2022

https://issues.redhat.com/browse/OPCT-6

This PR creates custom permissions to be used on Sonobuoy's service accounts.

The default sonobuoy's permissions are crashing the cluster upgrade feature (implemented on #33), described on OPCT-6. The goal is to avoid using system groups to not fall into pod admissions security admissions introduced on kube 1.24 (4.11).

The groups system:authenticated and system:serviceaccounts previously used were crashing the upgrade. Ref KCS 5875621

$ oc adm policy who-can use scc anyuid | grep serviceaccounts
        system:serviceaccounts 

Tests checklist:

  • Run a full test on regular mode on baseline platform (AWS) and check the results. Expected: no failures increased when comparing with latest baseline results payload
  • Run the full test on upgrade feature (SPLAT-651/OPCT-5: support upgrade execution mode #33) to validate full functional in both modes (regular and upgrade).

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 22, 2022
@mtulio mtulio marked this pull request as ready for review November 22, 2022 04:22
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2022
@mtulio mtulio assigned mtulio and bostrt and unassigned mtulio Nov 22, 2022
@mtulio mtulio changed the title SPLAT-874 - fix/scc-upgrade: SPLAT874 - creating custom RBAC resources SPLAT-874 - fix/rbac-upgrade: SPLAT874 - create and bind serviceaccount to SCC cluster roles Nov 22, 2022
@mtulio mtulio marked this pull request as draft November 22, 2022 14:19
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2022
@mtulio
Copy link
Contributor Author

mtulio commented Nov 22, 2022

@bostrt
Copy link
Contributor

bostrt commented Nov 30, 2022

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2022
@mtulio mtulio changed the title SPLAT-874 - fix/rbac-upgrade: SPLAT874 - create and bind serviceaccount to SCC cluster roles SPLAT-874 - fix/rbac-upgrade: SPLAT874 - create serviceaccount to SCC cluster roles Dec 1, 2022
@mtulio mtulio marked this pull request as ready for review December 21, 2022 17:37
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2022
@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 22, 2022
@mtulio mtulio changed the title SPLAT-874 - fix/rbac-upgrade: SPLAT874 - create serviceaccount to SCC cluster roles SPLAT-874 - fix/rbac-upgrade: create serviceaccount to SCC cluster roles Jan 5, 2023
@mtulio mtulio marked this pull request as draft January 6, 2023 13:38
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 6, 2023
@mtulio mtulio mentioned this pull request Jan 6, 2023
12 tasks
@bostrt
Copy link
Contributor

bostrt commented Jan 6, 2023

/unhold
/lgtm

This lgtm so far but I'll run a quick test before approving. @mtulio can you remove draft state from this PR?

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 6, 2023
pkg/types.go Outdated Show resolved Hide resolved
pkg/run/run.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot requested a review from bostrt January 9, 2023 20:03
@mtulio mtulio force-pushed the fix-rbac-upgrade branch 2 times, most recently from 543dd23 to a0b0488 Compare January 9, 2023 20:07
@mtulio
Copy link
Contributor Author

mtulio commented Jan 9, 2023

@bostrt this PR is ready for review and validation. It is the same RBAC changes used on #33 .

@bostrt
Copy link
Contributor

bostrt commented Jan 9, 2023

/lgtm

I have a test running in AWS right now and if it looks good, I'll approve.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2023
pkg/run/run.go Outdated Show resolved Hide resolved
pkg/run/run.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2023
@mtulio
Copy link
Contributor Author

mtulio commented Jan 10, 2023

@bostrt I ran tests onto 4.11 (regular mode) and got good results, similar failures ratio as v0.2.1+ (less than 1%). I am ok to a final review, then backport to v0.2.

$ echo $CLUSTER
opct-411pr34

$ ./.$CLUSTER-openshift-provider-cert version
OpenShift Provider Certification Tool: 0.0.0+aed8313
Sonobuoy Version: v0.56.10

$ ./.$CLUSTER-openshift-provider-cert results .$CLUSTER/clusters/$CLUSTER/opct/*.tar.gz  |egrep ^'(Plugin|Run)' -A 5
Plugin: 99-openshift-artifacts-collector
Status: passed
Total: 3
Passed: 3
Failed: 0
Skipped: 0
--
Plugin: 10-openshift-kube-conformance
Status: failed
Total: 662
Passed: 657
Failed: 5
Skipped: 0
--
Plugin: 20-openshift-conformance-validated
Status: failed
Total: 3805
Passed: 1717
Failed: 38
Skipped: 2050
--
Run Details:
API Server version: v1.24.6+5658434
Node health: 7/7 (100%)
Pods health: 262/262 (100%)

$ bc <<< "scale=4; (5/662)*100"
.7500
$ bc <<< "scale=4; (38/3805)*100"
.9900

/unhold

@mtulio
Copy link
Contributor Author

mtulio commented Jan 10, 2023

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2023
@mtulio
Copy link
Contributor Author

mtulio commented Jan 10, 2023

@bostrt I am thinking that we don't need to backport this PR to v0.2 for the following reasons:

  • 4.12 is not supported on v0.2
  • upgrade feature is not supported on v0.2

Let me know your thoughts.

@mtulio mtulio changed the title SPLAT-874 - fix/rbac: manage custom ServiceAccount and roles OPCT-6 - fix/rbac: manage custom ServiceAccount and roles Jan 10, 2023
@bostrt
Copy link
Contributor

bostrt commented Jan 10, 2023

@bostrt I am thinking that we don't need to backport this PR to v0.2 for the following reasons:

* 4.12 is not supported on v0.2

* upgrade feature is not supported on v0.2

Let me know your thoughts.

@mtulio I was originally thinking we could separate the RBAC work from the upgrade work in 0.3 but if you want to work on stabilizing 0.2 that works for me.

@mtulio
Copy link
Contributor Author

mtulio commented Jan 10, 2023

@mtulio I was originally thinking we could separate the RBAC work from the upgrade work in 0.3 but if you want to work on stabilizing 0.2 that works for me.

@bostrt yes, we are working to stabilize v0.3/upgrades, but I was thinking in scenarios if we need it on the older releases. But nothing came to my mind.

I am ok to merge here and not back port to v0.2.

@bostrt
Copy link
Contributor

bostrt commented Jan 10, 2023

I am ok to merge here and not back port to v0.2.

This works for me too. If anything comes up, we can always backport.

@bostrt
Copy link
Contributor

bostrt commented Jan 11, 2023

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jan 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bostrt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2023
@openshift-merge-robot openshift-merge-robot merged commit d31aa37 into redhat-openshift-ecosystem:main Jan 11, 2023
@mtulio mtulio deleted the fix-rbac-upgrade branch January 11, 2023 19:44
openshift-merge-robot pushed a commit that referenced this pull request Jan 26, 2023
https://issues.redhat.com/browse/SPLAT-651

Support upgrade conformance.
- Introduces new flags to control whether the execution needs to run the
upgrade cluster or not:
  - `--mode=upgrade`
- `--upgrade-to-image=<release_digest>` (`$(oc adm release info 4.Y+1.Z
-o jsonpath={.image}`)
- Create config map with plugin variables (the sonobuoy native feature
wipes all existing from `podSpec` which is undesired)
- add a new plugin instance of openshift-tests to run upgrades:
`05-openshift-cluster-upgrade`

Blocked by:
- [x]
#31
- [x]
#34

Blocked by Plugin release:

- [x]
redhat-openshift-ecosystem/provider-certification-plugins#24

Checklist:
- [x] CLI changes to run in upgrade mode
- [x] CLI changes to get the release image digest
- [x] Plugin implementation:
redhat-openshift-ecosystem/provider-certification-plugins#24
- [x] Validate y-stream upgrades
- [x] Fix RBAC #34 for Cluster upgrade
- [x] Fix SecurityContextMode for Sonobuoy aggregator stuck on
4.10->4.11 #39
- [x] MachineConfigPool validation: 'opct' object is validated if
present when running `mode=upgrade` on the runtime (plugin execution).
Failures will be raised by the plugin when the MCP is not present (the
User Docs should keep it very explicit): Tests described here:
redhat-openshift-ecosystem/provider-certification-plugins#24 (comment)
- [x] User Documentation

Tests checklist:
- [x] upgrade 4.12-> 4.13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants