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

Make the kubelet root directory configurable #1438

Merged
merged 8 commits into from
Jan 31, 2023
Merged

Make the kubelet root directory configurable #1438

merged 8 commits into from
Jan 31, 2023

Conversation

ccojocar
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:

This makes the kubelet root directory configurable. kubelet has a command option which allows to
configure its root directory. This is the place form where the configuration is loaded, including
the seccomp profiles.

This is useful in a cluster which doesn't use the default /var/lib/kubelet path.

Which issue(s) this PR fixes:

Does this PR have test?

Yes

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Make the kubelet root directory configurable via KUBELET_DIR environment variable.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 27, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 27, 2023
@ccojocar
Copy link
Contributor Author

/assign @saschagrunert

@codecov-commenter
Copy link

Codecov Report

Merging #1438 (0989402) into main (91b4819) will decrease coverage by 0.08%.
The diff coverage is 44.44%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1438      +/-   ##
==========================================
- Coverage   44.13%   44.05%   -0.08%     
==========================================
  Files          50       50              
  Lines        5651     5661      +10     
==========================================
  Hits         2494     2494              
- Misses       3037     3047      +10     
  Partials      120      120

@ccojocar
Copy link
Contributor Author

/retest

@ccojocar
Copy link
Contributor Author

/test all

@ccojocar
Copy link
Contributor Author

/retest

@ccojocar ccojocar force-pushed the kubelet-rootpath-config branch 2 times, most recently from 03d5954 to 0989402 Compare January 27, 2023 18:38
@saschagrunert
Copy link
Member

/retest

saschagrunert
saschagrunert reviewed Jan 30, 2023
View reviewed changes
installation-usage.md
Comment on lines +193 to +197
## Configure a custom kubelet root directory

You can configure a custom kubelet root directory in case your cluster is not using the default `/var/lib/kubelet` path.
You can achieve this by setting the environment variable `KUBELET_DIR` in the operator deployment. This environment variable will
be then set in the manager container as well as it will be propagated into the containers part of spod daemonset.
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better / possible to have this as part of the SPOD instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The challenge is that the kubelet path is required in the non-root enabler sidecar which currently doesn't read any SPOD CRD before copying the profiles into the kubelet directory.

It seems to be used inside of nodestatus controller as well.

The idea is to have this environment variable propagated everywhere that all components used the same custom kubelet directory including both manager and daemon instances.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, make sense. Any further thoughts on that @kubernetes-sigs/security-profiles-operator-maintainers ?

Copy link
Member

@pjbgf pjbgf Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Relying on env-vars would make it a lot easier to enable support for non symmetrical configuration across node pools. +1 from me on the approach.

pjbgf
pjbgf reviewed Jan 30, 2023
View reviewed changes
internal/pkg/config/config.go Outdated Show resolved Hide resolved
internal/pkg/config/config.go Outdated Show resolved Hide resolved
installation-usage.md
Comment on lines +193 to +197
## Configure a custom kubelet root directory

You can configure a custom kubelet root directory in case your cluster is not using the default `/var/lib/kubelet` path.
You can achieve this by setting the environment variable `KUBELET_DIR` in the operator deployment. This environment variable will
be then set in the manager container as well as it will be propagated into the containers part of spod daemonset.
Copy link
Member

@pjbgf pjbgf Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Relying on env-vars would make it a lot easier to enable support for non symmetrical configuration across node pools. +1 from me on the approach.

pjbgf
pjbgf reviewed Jan 30, 2023
View reviewed changes
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

Two nits, apart from that it looks good to me.

@ccojocar
Copy link
Contributor Author

Two nits, apart from that it looks good to me.

@pjbgf Thanks for review! I addressed your comments.

@ccojocar ccojocar force-pushed the kubelet-rootpath-config branch from c200170 to bfa94b6 Compare January 30, 2023 10:25
@ccojocar
Copy link
Contributor Author

/test all

pjbgf
pjbgf approved these changes Jan 30, 2023
View reviewed changes
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 30, 2023
@ccojocar
Copy link
Contributor Author

@saschagrunert it seems that are some recent changes in the llvm.sh script which is causing the verify step to fail. The script is calling now the sudo command when is adding the llvm package key. This command doesn't seem to be available in the golang base image. The hack is to add the key before running the script in order to avoid running into sudo command later.

The fix is in this commit 7932975

@ccojocar
Copy link
Contributor Author

/test all

@ccojocar ccojocar force-pushed the kubelet-rootpath-config branch from 2a53c47 to bfa94b6 Compare January 30, 2023 12:27
@ccojocar
Copy link
Contributor Author

It seems that they fixed the llvm.sh script in the meantime and the sudo was removed.

@ccojocar
Copy link
Contributor Author

The llvm.sh seems to be broken:

Hit:3 http://deb.debian.org/debian bullseye-updates InRelease
Get:4 https://apt.llvm.org/bullseye llvm-toolchain-bullseye-14 InRelease [6836 B]
Err:4 https://apt.llvm.org/bullseye llvm-toolchain-bullseye-14 InRelease
  The following signatures couldn't be verified because the public key is not available: NO_PUBKEY 15CF4D18AF4F7421
Reading package lists...
W: GPG error: https://apt.llvm.org/bullseye llvm-toolchain-bullseye-14 InRelease: The following signatures couldn't be verified because the public key is not available: NO_PUBKEY 15CF4D18AF4F7421
E: The repository 'http://apt.llvm.org/bullseye llvm-toolchain-bullseye-14 InRelease' is not signed.

@ccojocar ccojocar force-pushed the kubelet-rootpath-config branch from bfa94b6 to ddc4231 Compare January 31, 2023 08:45
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ccojocar, pjbgf, saschagrunert

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:
  • OWNERS [pjbgf,saschagrunert]

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

@ccojocar ccojocar force-pushed the kubelet-rootpath-config branch from ddc4231 to 416302e Compare January 31, 2023 09:48
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2023
@saschagrunert
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2023
@k8s-ci-robot k8s-ci-robot merged commit a086a9c into kubernetes-sigs:main Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pjbgf pjbgf pjbgf approved these changes

@saschagrunert saschagrunert saschagrunert approved these changes

@JAORMX JAORMX Awaiting requested review from JAORMX

@Vincent056 Vincent056 Awaiting requested review from Vincent056

Assignees

@saschagrunert saschagrunert

@pjbgf pjbgf

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ccojocar @codecov-commenter @saschagrunert @k8s-ci-robot @pjbgf