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

fixup the operator-sdk bundle #19

Merged
merged 2 commits into from
Jun 18, 2024
Merged

Conversation

astoycos
Copy link
Member

@astoycos astoycos commented Jun 5, 2024

  • Make sure the bundle is installed in the bpfman namespace rather than the openshift-bpfman namespace.

  • Update the SCC to be a bit more tight specifically making it run with

runAsUser:
type: MustRunAsNonRoot

  • Fixup some documentation

  • Make the operator deploy our custom bpfman-restricted SCC if running in OCP

Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 5.88235% with 64 lines in your changes missing coverage. Please review.

Project coverage is 26.74%. Comparing base (1861e30) to head (eb369fa).
Report is 13 commits behind head on main.

Files Patch % Lines
cmd/bpfman-operator/main.go 0.00% 33 Missing ⚠️
controllers/bpfman-operator/configmap.go 11.42% 30 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #19      +/-   ##
==========================================
+ Coverage   26.36%   26.74%   +0.37%     
==========================================
  Files          75       75              
  Lines        5112     6146    +1034     
==========================================
+ Hits         1348     1644     +296     
- Misses       3600     4337     +737     
- Partials      164      165       +1     
Flag Coverage Δ
unittests 26.74% <5.88%> (+0.37%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- Make sure the bundle is installed in the
`bpfman` namespace rather than the openshift-bpfman
namespace.

- Update the SCC to be a bit more tight specifically
making it run with

runAsUser:
  type: MustRunAsNonRoot

- Fixup some documentation

Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
@astoycos astoycos force-pushed the operatorhub-updates branch 3 times, most recently from bc7ea8e to aa17521 Compare June 6, 2024 18:18
@astoycos astoycos marked this pull request as ready for review June 6, 2024 18:19
// users can bind to in order to utilize bpfman in an unprivileged way.
func LoadRestrictedSecurityContext(path string) *osv1.SecurityContextConstraints {
// Load static SCC yaml from disk
file, err := os.Open(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

why u decided to load static file instead of setting scc obj using APIs ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just how we've been doing it for all of the objects deployed by the bpfman-operator (i.e the bpfman-daemonset, csiDriver, and now SecurityContextConstraint) and IMO it usually ends up being a bit less code

Copy link
Member Author

Choose a reason for hiding this comment

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

Additionally they all really didn't need to be configured at runtime at all (except for the bpfman ds) so I figured having them static on disk made sense

@astoycos astoycos added the hold label Jun 6, 2024
@astoycos
Copy link
Member Author

astoycos commented Jun 6, 2024

Hold until I can manually test on OCP, cluster bot is fighting me ATM

@astoycos
Copy link
Member Author

astoycos commented Jun 7, 2024

Tested on OCP 4.15 👍

@msherif1234
Copy link
Contributor

is there a need to write specific section in the readme for deploying on OCP cluster ?

@astoycos
Copy link
Member Author

It's already there right? https://github.com/bpfman/bpfman-operator#deploy-to-openshift-cluster

get bpfman up and running quickly simply click 'install' to deploy the bpfman-operator
in the bpfman namespace via operator-hub.\n## Configuration\n\nThe `bpfman-config`
configmap is automatically created in the `bpfman` namespace and used to configure
the bpfman deployment.\n\nTo edit the config simply run\n\n```bash\nkubectl edit
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the bundle more OCP specific? If so, should kubectl be replaced with oc?

Copy link
Member Author

Choose a reason for hiding this comment

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

So technically the operatorhub is for everyone, and community operators prod is for OCP I think we can leave it as kubectl here and add a carry commit to https://github.com/redhat-openshift-ecosystem/community-operators-prod

@astoycos astoycos requested a review from Billy99 June 17, 2024 15:44
cmd/bpfman-operator/main.go Outdated Show resolved Hide resolved
cmd/bpfman-operator/main.go Show resolved Hide resolved
controllers/bpfman-operator/configmap.go Show resolved Hide resolved
Billy99
Billy99 previously approved these changes Jun 18, 2024
Copy link
Contributor

@Billy99 Billy99 left a comment

Choose a reason for hiding this comment

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

/LGTM

As part of our default deployment in openshift we
need to deploy a custom SCC which can be used
by applications in order to receive access to their
eBPF maps without running as root.

update bundle based on changes.

Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
@astoycos astoycos added the hold label Jun 18, 2024
@astoycos astoycos removed the hold label Jun 18, 2024
@astoycos astoycos merged commit 5453572 into bpfman:main Jun 18, 2024
12 checks passed
msherif1234 added a commit to msherif1234/bpfman-operator that referenced this pull request Oct 11, 2024
…ator

Red Hat Konflux update ocp-bpfman-operator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants