Skip to content

feat: Add kubernetes device ownership#69

Merged
vigh-m merged 2 commits intobottlerocket-os:developfrom
vigh-m:extend-kubernetes-settings
Dec 24, 2024
Merged

feat: Add kubernetes device ownership#69
vigh-m merged 2 commits intobottlerocket-os:developfrom
vigh-m:extend-kubernetes-settings

Conversation

@vigh-m
Copy link
Copy Markdown
Contributor

@vigh-m vigh-m commented Dec 20, 2024

Description of changes:

  • Create a new setting to hold the device-ownership-from-security-context boolean. This is to support the same setting in the containerd CRI plugin as described here

Testing done:

  • The setting shows up as expected in an AMI built using this settings-sdk

    # apiclient get settings.kubernetes.device-ownership-from-security-context
    {
      "settings": {
        "kubernetes": {
          "device-ownership-from-security-context": false
        }
      }
    }
    
  • Changing the setting works as defined:

    [root@admin]# apiclient set settings.kubernetes.device-ownership-from-security-context="true"
    [root@admin]# apiclient get settings.kubernetes.device-ownership-from-security-context
    {
      "settings": {
        "kubernetes": {
          "device-ownership-from-security-context": true
        }
      }
    }
    [root@admin]# sheltie cat /etc/containerd/config.toml | grep device
    device_ownership_from_security_context = true
    
  • The setting has the desired effect on device ownership

    • When true:
      # kubectl exec -it single-node-test -- bash
      ubuntu@single-node-test:/$ ls -lah /dev/ | grep neuron
      crw-------. 1 ubuntu 2000 244, 0 Dec 20 00:25 neuron0
      
    • When false:
      # kubectl exec -it single-node-test -- bash
      ubuntu@single-node-test:/$ ls -lah /dev/ | grep neuron
      crw-------. 1 root root 244, 0 Dec 19 20:03 neuron0
      

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@yeazelm
Copy link
Copy Markdown
Contributor

yeazelm commented Dec 23, 2024

The change looks good but nit about the commit message: I'd like a basic explanation about the change more than just the title to help folks piece together what it is intended for or why it was added since this is in a different repo from where the interesting parts will land.

@vigh-m vigh-m force-pushed the extend-kubernetes-settings branch from 7376696 to 1309714 Compare December 24, 2024 20:26
@vigh-m
Copy link
Copy Markdown
Contributor Author

vigh-m commented Dec 24, 2024

Ack. Updated the commit message with some more context.

Add a new setting to gate the containerd CRI plugin
`device-ownership-from-security-context`.  This option will allow
containers to gain ownership of requested devices without needing root
containers or devices.
@vigh-m vigh-m force-pushed the extend-kubernetes-settings branch from 1309714 to 288b2ba Compare December 24, 2024 20:35
@vigh-m vigh-m requested a review from cbgbt December 24, 2024 21:21
@vigh-m vigh-m force-pushed the extend-kubernetes-settings branch from 2bbeec6 to f1b216a Compare December 24, 2024 22:02
Copy link
Copy Markdown
Contributor

@cbgbt cbgbt left a comment

Choose a reason for hiding this comment

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

Thanks!

@vigh-m vigh-m merged commit 2a7c098 into bottlerocket-os:develop Dec 24, 2024
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.

4 participants