Skip to content

Commit 2fbf3a3

Browse files
committed
fix(admission): restrict NvFractions device annotations to Binder
Signed-off-by: davidLif <davidshani12@gmail.com>
1 parent 2851e7e commit 2fbf3a3

17 files changed

Lines changed: 221 additions & 45 deletions

File tree

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
kind: Added
2+
body: |-
3+
Configure NvFractions GPU sharing mode

cmd/admission/app/options.go

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13,23 +13,24 @@ import (
1313
)
1414

1515
type Options struct {
16-
SchedulerName string
17-
QPS float64
18-
Burst int
19-
RateLimiterBaseDelaySeconds int
20-
RateLimiterMaxDelaySeconds int
21-
EnableLeaderElection bool
22-
MetricsAddr string
23-
ProbeAddr string
24-
WebhookPort int
25-
FakeGPUNodes bool
26-
GPUSharingEnabled bool
27-
NRIPluginEnabled bool
28-
HamiCoreEnabled bool
29-
NvFractionsEnabled bool
30-
BlockNvidiaVisibleDevices bool
31-
GPUPodRuntimeClassName string
32-
GPUFractionRuntimeClassName string
16+
SchedulerName string
17+
QPS float64
18+
Burst int
19+
RateLimiterBaseDelaySeconds int
20+
RateLimiterMaxDelaySeconds int
21+
EnableLeaderElection bool
22+
MetricsAddr string
23+
ProbeAddr string
24+
WebhookPort int
25+
FakeGPUNodes bool
26+
GPUSharingEnabled bool
27+
NRIPluginEnabled bool
28+
HamiCoreEnabled bool
29+
NvFractionsEnabled bool
30+
BinderServiceAccountUsername string
31+
BlockNvidiaVisibleDevices bool
32+
GPUPodRuntimeClassName string
33+
GPUFractionRuntimeClassName string
3334
}
3435

3536
// ResolvedGPUFractionRuntimeClassName returns the effective runtime class name
@@ -93,6 +94,9 @@ func InitOptions() *Options {
9394
fs.BoolVar(&options.NvFractionsEnabled,
9495
"nv-fractions-enabled", false,
9596
"Specifies if the NvFractions GPU-sharing admission plugin is enabled")
97+
fs.StringVar(&options.BinderServiceAccountUsername,
98+
"binder-service-account-username", "",
99+
"The Kubernetes username allowed to write NvFractions device annotations")
96100
fs.BoolVar(&options.BlockNvidiaVisibleDevices,
97101
"block-nvidia-visible-devices", false,
98102
"Reject pods that set the NVIDIA_VISIBLE_DEVICES environment variable to values "+

cmd/admission/main.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ func registerPlugins(app *app.App) error {
4949
// admission plugin: it owns validation and mutation for fraction pods and
5050
// does not use the shared GPU configmap.
5151
if app.Options.NvFractionsEnabled {
52-
admissionPlugins.RegisterPlugin(nvfractions.New())
52+
admissionPlugins.RegisterPlugin(nvfractions.New(app.Options.BinderServiceAccountUsername))
5353
} else {
5454
admissionGpuSharingPlugin := gpusharing.New(
5555
app.Client, app.Options.GPUSharingEnabled, app.Options.NRIPluginEnabled)

pkg/admission/plugins/plugins.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import (
1212

1313
type Plugin interface {
1414
Name() string
15-
Validate(*v1.Pod) error
15+
Validate(context.Context, *v1.Pod, *v1.Pod) error
1616
Mutate(*v1.Pod) error
1717
}
1818

@@ -32,9 +32,9 @@ func (bp *KaiAdmissionPlugins) RegisterPlugin(plugin Plugin) {
3232
bp.plugins = append(bp.plugins, plugin)
3333
}
3434

35-
func (bp *KaiAdmissionPlugins) Validate(pod *v1.Pod) error {
35+
func (bp *KaiAdmissionPlugins) Validate(ctx context.Context, oldPod, pod *v1.Pod) error {
3636
for _, p := range bp.plugins {
37-
err := p.Validate(pod)
37+
err := p.Validate(ctx, oldPod, pod)
3838
if err != nil {
3939
logger := log.FromContext(context.Background())
4040
logger.Error(err, "pod validation failed for pod",

pkg/admission/webhook/v1alpha2/deviceaccess/device_access.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package deviceaccess
55

66
import (
7+
"context"
78
"fmt"
89
"slices"
910

@@ -27,7 +28,7 @@ func (da *DeviceAccess) Name() string {
2728
return "deviceaccess"
2829
}
2930

30-
func (da *DeviceAccess) Validate(pod *v1.Pod) error {
31+
func (da *DeviceAccess) Validate(_ context.Context, _, pod *v1.Pod) error {
3132
containerRef, err := fractionContainerRef(pod)
3233
if err != nil {
3334
return err

pkg/admission/webhook/v1alpha2/deviceaccess/device_access_test.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package deviceaccess
55

66
import (
7+
"context"
78
"testing"
89

910
"github.com/stretchr/testify/assert"
@@ -158,7 +159,7 @@ func TestValidate(t *testing.T) {
158159
plugin := New()
159160
for _, tt := range tests {
160161
t.Run(tt.name, func(t *testing.T) {
161-
err := plugin.Validate(tt.pod)
162+
err := plugin.Validate(context.Background(), nil, tt.pod)
162163
if tt.expectedErr == "" {
163164
assert.NoError(t, err)
164165
} else {
@@ -308,6 +309,6 @@ func TestFractionPodWithoutRegularContainers(t *testing.T) {
308309
Spec: v1.PodSpec{InitContainers: []v1.Container{cpuContainer("init-container-0")}},
309310
}
310311
plugin := New()
311-
assert.NoError(t, plugin.Validate(pod))
312+
assert.NoError(t, plugin.Validate(context.Background(), nil, pod))
312313
assert.NoError(t, plugin.Mutate(pod))
313314
}

pkg/admission/webhook/v1alpha2/gpusharing/gpu_sharing.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package gpusharing
55

66
import (
7+
"context"
78
"fmt"
89

910
"strconv"
@@ -40,7 +41,7 @@ func (p *GPUSharing) Name() string {
4041
return "gpusharing"
4142
}
4243

43-
func (p *GPUSharing) Validate(pod *v1.Pod) error {
44+
func (p *GPUSharing) Validate(_ context.Context, _, pod *v1.Pod) error {
4445
if !p.gpuSharingEnabled && resources.RequestsGPUFraction(pod) {
4546
return fmt.Errorf(
4647
"attempting to create a pod %s/%s with gpu sharing request, while GPU sharing is disabled",

pkg/admission/webhook/v1alpha2/gpusharing/gpu_sharing_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package gpusharing
55

66
import (
7+
"context"
78
"fmt"
89
"strings"
910
"testing"
@@ -737,7 +738,7 @@ func TestValidate(t *testing.T) {
737738
t.Run(tt.name, func(t *testing.T) {
738739
kubeClient := fake.NewClientBuilder().WithRuntimeObjects(tt.pod).Build()
739740
gpuSharingPlugin := New(kubeClient, tt.GPUSharingEnabled, false)
740-
err := gpuSharingPlugin.Validate(tt.pod)
741+
err := gpuSharingPlugin.Validate(context.Background(), nil, tt.pod)
741742
if err == nil && tt.error != nil {
742743
t.Errorf("Validate() expected and error but actual is nil")
743744
return

pkg/admission/webhook/v1alpha2/hamicore/hamicore.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
package hamicore
55

66
import (
7+
"context"
8+
79
v1 "k8s.io/api/core/v1"
810
"k8s.io/utils/ptr"
911

@@ -22,7 +24,7 @@ func (p *HamiCore) Name() string {
2224
return "hamicore"
2325
}
2426

25-
func (p *HamiCore) Validate(_ *v1.Pod) error {
27+
func (p *HamiCore) Validate(context.Context, *v1.Pod, *v1.Pod) error {
2628
return nil
2729
}
2830

pkg/admission/webhook/v1alpha2/nvfractions/nvfractions.go

Lines changed: 100 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,13 @@
44
package nvfractions
55

66
import (
7+
"context"
78
"fmt"
89
"strconv"
10+
"strings"
911

1012
v1 "k8s.io/api/core/v1"
13+
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
1114

1215
"github.com/kai-scheduler/KAI-scheduler/pkg/binder/common"
1316
"github.com/kai-scheduler/KAI-scheduler/pkg/common/constants"
@@ -19,18 +22,25 @@ import (
1922
// gpu-fraction-container-name, and NvFractions annotations) and normalizes
2023
// legacy memory annotations to the NvFractions form, without using the shared
2124
// GPU configmap.
22-
type NvFractions struct{}
25+
type NvFractions struct {
26+
binderServiceAccountUsername string
27+
}
2328

24-
func New() *NvFractions {
25-
return &NvFractions{}
29+
func New(binderServiceAccountUsername string) *NvFractions {
30+
return &NvFractions{
31+
binderServiceAccountUsername: binderServiceAccountUsername,
32+
}
2633
}
2734

2835
func (p *NvFractions) Name() string {
2936
return "nvfractions"
3037
}
3138

32-
func (p *NvFractions) Validate(pod *v1.Pod) error {
33-
return resources.ValidateGPUFractionRequest(pod)
39+
func (p *NvFractions) Validate(ctx context.Context, oldPod, pod *v1.Pod) error {
40+
if err := p.validateDeviceAnnotation(ctx, oldPod, pod); err != nil {
41+
return err
42+
}
43+
return resources.ValidateGPUFractionRequest(stripNvFractionsDeviceAnnotations(pod))
3444
}
3545

3646
func (p *NvFractions) Mutate(pod *v1.Pod) error {
@@ -68,3 +78,88 @@ func adjustFractionalMemoryAnnotations(pod *v1.Pod, containerName string) error
6878
pod.Annotations[resources.CalcGpuFractionAnnotationForContainer(containerName)] = memoryQuantity.String()
6979
return nil
7080
}
81+
82+
// validateDeviceAnnotation makes sure that only the binder service account can modify the nvfractions device annotations.
83+
// This is done to prevent malicious actors from modifying the nvfractions device annotations to gain access to the GPU.
84+
func (p *NvFractions) validateDeviceAnnotation(ctx context.Context, oldPod, pod *v1.Pod) error {
85+
hasNvFractionsDeviceAnnotationChange := false
86+
for annotationKey := range pod.Annotations {
87+
if !isNvFractionsDeviceAnnotation(annotationKey) {
88+
continue
89+
}
90+
if err := validateDeviceAnnotationContainerExists(pod, annotationKey); err != nil {
91+
return err
92+
}
93+
94+
if oldPod != nil {
95+
hasNvFractionsDeviceAnnotationChange = isAnnotationChanged(annotationKey, oldPod, pod)
96+
} else {
97+
hasNvFractionsDeviceAnnotationChange = true
98+
}
99+
100+
if hasNvFractionsDeviceAnnotationChange {
101+
break
102+
}
103+
}
104+
105+
if !hasNvFractionsDeviceAnnotationChange {
106+
return nil
107+
}
108+
109+
if len(p.binderServiceAccountUsername) == 0 {
110+
return fmt.Errorf("binder service account username is not configured, cannot validate nvfractions device annotations change")
111+
}
112+
113+
request, err := admission.RequestFromContext(ctx)
114+
if err != nil {
115+
return fmt.Errorf("failed to extract admission request: %w", err)
116+
}
117+
if request.UserInfo.Username != p.binderServiceAccountUsername {
118+
return fmt.Errorf("%s annotations may only be modified by %s",
119+
constants.NvFractionsVisibleDevicesSuffix, p.binderServiceAccountUsername)
120+
}
121+
122+
return nil
123+
}
124+
125+
func isAnnotationChanged(annotationKey string, oldPod *v1.Pod, pod *v1.Pod) bool {
126+
oldAnnotationValue, oldHasNvFractionsDeviceAnnotation := oldPod.Annotations[annotationKey]
127+
if !oldHasNvFractionsDeviceAnnotation {
128+
return true
129+
}
130+
newAnnotationValue := pod.Annotations[annotationKey]
131+
if oldAnnotationValue != newAnnotationValue {
132+
return true
133+
}
134+
return false
135+
}
136+
137+
func stripNvFractionsDeviceAnnotations(pod *v1.Pod) *v1.Pod {
138+
if pod == nil || len(pod.Annotations) == 0 {
139+
return pod
140+
}
141+
142+
podCopy := pod.DeepCopy()
143+
for key := range podCopy.Annotations {
144+
if isNvFractionsDeviceAnnotation(key) {
145+
delete(podCopy.Annotations, key)
146+
}
147+
}
148+
return podCopy
149+
}
150+
151+
func isNvFractionsDeviceAnnotation(annotationKey string) bool {
152+
return strings.HasPrefix(annotationKey, constants.NvFractionsAnnotationPrefix) &&
153+
strings.HasSuffix(annotationKey, constants.NvFractionsVisibleDevicesSuffix)
154+
}
155+
156+
func validateDeviceAnnotationContainerExists(pod *v1.Pod, annotationKey string) error {
157+
containerName := strings.TrimPrefix(annotationKey, constants.NvFractionsAnnotationPrefix)
158+
containerName = strings.TrimSuffix(containerName, constants.NvFractionsVisibleDevicesSuffix)
159+
for _, container := range pod.Spec.Containers {
160+
if container.Name == containerName {
161+
return nil
162+
}
163+
}
164+
return fmt.Errorf("container %s not found in pod spec, but a fractional annotation referencing it was found", containerName)
165+
}

0 commit comments

Comments
 (0)