-
Notifications
You must be signed in to change notification settings - Fork 59
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
Update Talos v1.9.1 #553
base: main
Are you sure you want to change the base?
Update Talos v1.9.1 #553
Conversation
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
WalkthroughThis pull request focuses on upgrading the Talos Kubernetes distribution from version v1.8.4 to v1.9.1. The changes span multiple configuration files across the Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 checkov (3.2.334)packages/system/kubevirt/templates/disable-selinux-workaround.yaml[HIGH] 4-41: Container should not be privileged (CKV_K8S_16) 🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
blocked by siderolabs/talos#10083 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/system/kubevirt/templates/disable-selinux-workaround.yaml (1)
1-2
: Document the workaround's purpose and impact.While the link to the Talos issue is helpful, please enhance the documentation to:
- Explain why SELinux needs to be disabled
- Document the security implications
- Note any plans to remove this workaround once the Talos issue is resolved
Add more context in the comments:
# See: # - https://github.com/siderolabs/talos/issues/10083 +# Purpose: Temporary workaround to disable SELinux enforcement for KubeVirt compatibility +# Security Impact: Reduces system security by disabling SELinux +# TODO: Remove this workaround once https://github.com/siderolabs/talos/issues/10083 is resolved
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/system/kubevirt/templates/disable-selinux-workaround.yaml
(1 hunks)
🧰 Additional context used
🪛 checkov (3.2.334)
packages/system/kubevirt/templates/disable-selinux-workaround.yaml
[HIGH] 4-41: Container should not be privileged
(CKV_K8S_16)
🔇 Additional comments (1)
packages/system/kubevirt/templates/disable-selinux-workaround.yaml (1)
33-41
: 🛠️ Refactor suggestionConsider restricting host access scope.
The DaemonSet currently requests full host access:
hostIPC: true
hostNetwork: true
hostPID: true
- Root directory mount
Consider limiting the scope to only what's necessary:
- hostIPC: true - hostNetwork: true - hostPID: true volumes: - hostPath: - path: / + path: /sys/fs/selinux name: host-rootLet's verify if full host access is required:
🧰 Tools
🪛 checkov (3.2.334)
[HIGH] 4-41: Container should not be privileged
(CKV_K8S_16)
containers: | ||
- command: | ||
- sh | ||
- -exc | ||
- test -f /sys/fs/selinux/enforce && mount -t tmpfs tmpfs /host/sys/fs/selinux; sleep infinity | ||
image: docker.io/library/alpine | ||
name: mount | ||
securityContext: | ||
privileged: true | ||
volumeMounts: | ||
- mountPath: /host | ||
mountPropagation: Bidirectional | ||
name: host-root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Workaround is currently necessary but requires security improvements
Based on the active KubeVirt issue #13607 and related discussions, this workaround is still necessary as KubeVirt fails to work properly when SELinux filesystem exists but is disabled or in permissive mode. However, we can improve the security posture while maintaining functionality:
- The container requires privileged access to mask SELinux filesystem
- Host root mount with bidirectional propagation is needed only for the specific SELinux path
- Additional security contexts can be restricted
Here are the specific issues found:
- Current implementation mounts entire host root (/) which is excessive
- Container has unrestricted host access (hostPID, hostIPC, hostNetwork) which isn't needed
- No resource limits or security policies are defined
Recommended security improvements:
spec:
template:
spec:
containers:
- securityContext:
privileged: true
allowPrivilegeEscalation: false
capabilities:
drop: ["ALL"]
add: ["SYS_ADMIN"] # Required for mount operations
resources:
limits:
memory: "64Mi"
cpu: "100m"
volumeMounts:
- mountPath: /host/sys/fs/selinux
mountPropagation: Bidirectional
name: selinux-path
volumes:
- hostPath:
path: /sys/fs/selinux
type: Directory
name: selinux-path
🔗 Analysis chain
Review security implications of privileged container.
The container requires significant privileges and host access:
- Runs as privileged
- Mounts host root with bidirectional propagation
- Modifies host's SELinux configuration
While this appears necessary for the workaround, it poses security risks:
- Full host access could be exploited if the container is compromised
- Disabling SELinux reduces the host's security posture
Let's verify if this workaround is still needed:
Consider implementing additional security measures:
- Add seccomp profile
- Limit host mount to only necessary paths
- Add network policy to restrict container communication
Would you like me to propose a more restrictive configuration?
Let me check the related KubeVirt issue to understand if this workaround is still necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the Talos issue is still open
gh issue view 10083 --repo siderolabs/talos --json state,title,comments
# Look for any alternative solutions or related discussions
rg -l "selinux|SELinux" --type yaml
Length of output: 6574
Script:
#!/bin/bash
# Check the referenced KubeVirt issue
gh issue view 13607 --repo kubevirt/kubevirt --json state,title,body,comments
# Check if there are any alternative solutions in the codebase
rg -A 5 -B 5 "selinux|SELinux" packages/system/kubevirt/
Length of output: 15510
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
This PR includes a new image based on Talos Linux v1.9.1
Summary by CodeRabbit
New Features
Chores