Skip to content

Commit e91466f

Browse files
dgrove-ossdgrove-oss
authored
Admission Control: validate path (#11)
Validate that every path provided in a Component.PodSet actually refers to a portion of the Component.template from which we can construct a PodTemplateSpec.
1 parent e05180e commit e91466f

File tree

3 files changed

+142
-76
lines changed

3 files changed

+142
-76
lines changed

‎internal/controller/appwrapper_webhook.go‎

Lines changed: 65 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -37,74 +37,90 @@ type AppWrapperWebhook struct {
3737

3838
var _ webhook.CustomDefaulter = &AppWrapperWebhook{}
3939

40-
// Default implements webhook.CustomDefaulter so a webhook will be registered for the type
40+
// Default ensures that Suspend is set appropriately when an AppWrapper is created
4141
func (w *AppWrapperWebhook) Default(ctx context.Context, obj runtime.Object) error {
42-
job := obj.(*workloadv1beta2.AppWrapper)
43-
log.FromContext(ctx).Info("Applying defaults", "job", job)
44-
jobframework.ApplyDefaultForSuspend((*AppWrapper)(job), w.ManageJobsWithoutQueueName)
42+
aw := obj.(*workloadv1beta2.AppWrapper)
43+
log.FromContext(ctx).Info("Applying defaults", "job", aw)
44+
jobframework.ApplyDefaultForSuspend((*AppWrapper)(aw), w.ManageJobsWithoutQueueName)
4545
return nil
4646
}
4747

4848
//+kubebuilder:webhook:path=/validate-workload-codeflare-dev-v1beta2-appwrapper,mutating=false,failurePolicy=fail,sideEffects=None,groups=workload.codeflare.dev,resources=appwrappers,verbs=create;update,versions=v1beta2,name=vappwrapper.kb.io,admissionReviewVersions=v1
4949

5050
var _ webhook.CustomValidator = &AppWrapperWebhook{}
5151

52-
// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type
52+
// ValidateCreate validates invariants when an AppWrapper is created
5353
func (w *AppWrapperWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
54-
job := obj.(*workloadv1beta2.AppWrapper)
55-
log.FromContext(ctx).Info("Validating create", "job", job)
56-
return nil, w.validateCreate(job).ToAggregate()
57-
}
54+
aw := obj.(*workloadv1beta2.AppWrapper)
55+
log.FromContext(ctx).Info("Validating create", "job", aw)
5856

59-
func (w *AppWrapperWebhook) validateCreate(job *workloadv1beta2.AppWrapper) field.ErrorList {
60-
var allErrors field.ErrorList
57+
allErrors := w.validateAppWrapperInvariants(ctx, aw)
6158

62-
if w.ManageJobsWithoutQueueName || jobframework.QueueName((*AppWrapper)(job)) != "" {
63-
components := job.Spec.Components
64-
componentsPath := field.NewPath("spec").Child("components")
65-
podSpecCount := 0
66-
for idx, component := range components {
67-
podSetsPath := componentsPath.Index(idx).Child("podSets")
68-
for psIdx, ps := range component.PodSets {
69-
podSetPath := podSetsPath.Index(psIdx)
70-
if ps.Path == "" {
71-
allErrors = append(allErrors, field.Required(podSetPath.Child("path"), "podspec must specify path"))
72-
}
59+
if w.ManageJobsWithoutQueueName || jobframework.QueueName((*AppWrapper)(aw)) != "" {
60+
allErrors = append(allErrors, jobframework.ValidateCreateForQueueName((*AppWrapper)(aw))...)
61+
}
7362

74-
// TODO: Validatate the ps.Path resolves to a PodSpec
63+
return nil, allErrors.ToAggregate()
64+
}
7565

76-
// TODO: RBAC check to make sure that the user has the ability to create the wrapped resources
66+
// ValidateUpdate validates invariants when an AppWrapper is updated
67+
func (w *AppWrapperWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
68+
oldAW := oldObj.(*workloadv1beta2.AppWrapper)
69+
newAW := newObj.(*workloadv1beta2.AppWrapper)
70+
log.FromContext(ctx).Info("Validating update", "job", newAW)
7771

78-
podSpecCount += 1
79-
}
80-
}
81-
if podSpecCount == 0 {
82-
allErrors = append(allErrors, field.Invalid(componentsPath, components, "components contains no podspecs"))
83-
}
84-
if podSpecCount > 8 {
85-
allErrors = append(allErrors, field.Invalid(componentsPath, components, fmt.Sprintf("components contains %v podspecs; at most 8 are allowed", podSpecCount)))
86-
}
72+
allErrors := w.validateAppWrapperInvariants(ctx, newAW)
73+
74+
if w.ManageJobsWithoutQueueName || jobframework.QueueName((*AppWrapper)(newAW)) != "" {
75+
allErrors = append(allErrors, jobframework.ValidateUpdateForQueueName((*AppWrapper)(oldAW), (*AppWrapper)(newAW))...)
76+
allErrors = append(allErrors, jobframework.ValidateUpdateForWorkloadPriorityClassName((*AppWrapper)(oldAW), (*AppWrapper)(newAW))...)
8777
}
8878

89-
allErrors = append(allErrors, jobframework.ValidateCreateForQueueName((*AppWrapper)(job))...)
90-
return allErrors
79+
return nil, allErrors.ToAggregate()
9180
}
9281

93-
// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type
94-
func (w *AppWrapperWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
95-
oldJob := oldObj.(*workloadv1beta2.AppWrapper)
96-
newJob := newObj.(*workloadv1beta2.AppWrapper)
97-
if w.ManageJobsWithoutQueueName || jobframework.QueueName((*AppWrapper)(newJob)) != "" {
98-
log.FromContext(ctx).Info("Validating update", "job", newJob)
99-
allErrors := jobframework.ValidateUpdateForQueueName((*AppWrapper)(oldJob), (*AppWrapper)(newJob))
100-
allErrors = append(allErrors, w.validateCreate(newJob)...)
101-
allErrors = append(allErrors, jobframework.ValidateUpdateForWorkloadPriorityClassName((*AppWrapper)(oldJob), (*AppWrapper)(newJob))...)
102-
return nil, allErrors.ToAggregate()
103-
}
82+
// ValidateDelete is a noop for us, but is required to implement the CustomValidator interface
83+
func (w *AppWrapperWebhook) ValidateDelete(context.Context, runtime.Object) (admission.Warnings, error) {
10484
return nil, nil
10585
}
10686

107-
// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type
108-
func (w *AppWrapperWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
109-
return nil, nil
87+
// validateAppWrapperInvariants checks AppWrapper-specific invariants
88+
func (w *AppWrapperWebhook) validateAppWrapperInvariants(_ context.Context, aw *workloadv1beta2.AppWrapper) field.ErrorList {
89+
allErrors := field.ErrorList{}
90+
components := aw.Spec.Components
91+
componentsPath := field.NewPath("spec").Child("components")
92+
podSpecCount := 0
93+
94+
for idx, component := range components {
95+
96+
// Each PodSet.Path must specify a path within Template to a v1.PodSpecTemplate
97+
podSetsPath := componentsPath.Index(idx).Child("podSets")
98+
for psIdx, ps := range component.PodSets {
99+
podSetPath := podSetsPath.Index(psIdx)
100+
if ps.Path == "" {
101+
allErrors = append(allErrors, field.Required(podSetPath.Child("path"), "podspec must specify path"))
102+
}
103+
if _, err := getPodTemplateSpec(component.Template.Raw, ps.Path); err != nil {
104+
allErrors = append(allErrors, field.Invalid(podSetPath.Child("path"), ps.Path,
105+
fmt.Sprintf("path does not refer to a v1.PodSpecTemplate: %v", err)))
106+
}
107+
podSpecCount += 1
108+
}
109+
110+
// TODO: RBAC check to make sure that the user has permissions to create the component
111+
112+
// TODO: We could attempt to validate the object is namespaced and the namespace is the same as the AppWrapper's namespace
113+
// This is currently enforced when the resources are created.
114+
115+
}
116+
117+
// Enforce Kueue limitation that 0 < podSpecCount <= 8
118+
if podSpecCount == 0 {
119+
allErrors = append(allErrors, field.Invalid(componentsPath, components, "components contains no podspecs"))
120+
}
121+
if podSpecCount > 8 {
122+
allErrors = append(allErrors, field.Invalid(componentsPath, components, fmt.Sprintf("components contains %v podspecs; at most 8 are allowed", podSpecCount)))
123+
}
124+
125+
return allErrors
110126
}

‎internal/controller/utils.go‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
/*
2+
Copyright 2024 IBM Corporation.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package controller
18+
19+
import (
20+
"fmt"
21+
"strings"
22+
23+
v1 "k8s.io/api/core/v1"
24+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
25+
"k8s.io/apimachinery/pkg/runtime"
26+
)
27+
28+
// getPodTemplateSpec parses raw as JSON and extracts a Kueue-compatible PodTemplateSpec at the given path within it
29+
func getPodTemplateSpec(raw []byte, path string) (*v1.PodTemplateSpec, error) {
30+
obj := &unstructured.Unstructured{}
31+
if _, _, err := unstructured.UnstructuredJSONScheme.Decode(raw, nil, obj); err != nil {
32+
return nil, err
33+
}
34+
35+
// Walk down the path
36+
parts := strings.Split(path, ".")
37+
p := obj.UnstructuredContent()
38+
var ok bool
39+
for i := 1; i < len(parts); i++ {
40+
p, ok = p[parts[i]].(map[string]interface{})
41+
if !ok {
42+
return nil, fmt.Errorf("path element %v not found (segment %v of %v)", parts[i], i, len(parts))
43+
}
44+
}
45+
46+
// Extract the PodSpec that should be at candidatePTS.spec
47+
candidatePTS := p
48+
spec, ok := candidatePTS["spec"].(map[string]interface{})
49+
if !ok {
50+
return nil, fmt.Errorf("content at %v does not contain a spec", path)
51+
}
52+
podSpec := &v1.PodSpec{}
53+
if err := runtime.DefaultUnstructuredConverter.FromUnstructuredWithValidation(spec, podSpec, true); err != nil {
54+
return nil, fmt.Errorf("content at %v.spec not parseable as a v1.PodSpec: %w", path, err)
55+
}
56+
57+
// Construct the filtered PodTemplateSpec, copying only the metadata expected by Kueue
58+
template := &v1.PodTemplateSpec{Spec: *podSpec}
59+
if metadata, ok := candidatePTS["metadata"].(map[string]interface{}); ok {
60+
if labels, ok := metadata["labels"].(map[string]string); ok {
61+
template.ObjectMeta.Labels = labels
62+
}
63+
if annotations, ok := metadata["annotations"].(map[string]string); ok {
64+
template.ObjectMeta.Annotations = annotations
65+
}
66+
}
67+
68+
return template, nil
69+
}

‎internal/controller/workload_controller.go‎

Lines changed: 8 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,8 @@ package controller
1818

1919
import (
2020
"fmt"
21-
"strings"
2221

23-
v1 "k8s.io/api/core/v1"
2422
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
25-
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
26-
"k8s.io/apimachinery/pkg/runtime"
2723
"k8s.io/apimachinery/pkg/runtime/schema"
2824
"sigs.k8s.io/controller-runtime/pkg/client"
2925
kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1"
@@ -75,35 +71,20 @@ func (aw *AppWrapper) PodSets() []kueue.PodSet {
7571
podSets := []kueue.PodSet{}
7672
i := 0
7773
for _, component := range aw.Spec.Components {
78-
LOOP:
7974
for _, podSet := range component.PodSets {
8075
replicas := int32(1)
8176
if podSet.Replicas != nil {
8277
replicas = *podSet.Replicas
8378
}
84-
obj := &unstructured.Unstructured{}
85-
if _, _, err := unstructured.UnstructuredJSONScheme.Decode(component.Template.Raw, nil, obj); err != nil {
86-
continue LOOP // TODO handle error
79+
template, err := getPodTemplateSpec(component.Template.Raw, podSet.Path)
80+
if err == nil {
81+
podSets = append(podSets, kueue.PodSet{
82+
Name: aw.Name + "-" + fmt.Sprint(i),
83+
Template: *template,
84+
Count: replicas,
85+
})
86+
i++
8787
}
88-
parts := strings.Split(podSet.Path, ".")
89-
p := obj.UnstructuredContent()
90-
var ok bool
91-
for i := 1; i < len(parts); i++ {
92-
p, ok = p[parts[i]].(map[string]interface{})
93-
if !ok {
94-
continue LOOP // TODO handle error
95-
}
96-
}
97-
var template v1.PodTemplateSpec
98-
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(p, &template); err != nil {
99-
continue LOOP // TODO handle error
100-
}
101-
podSets = append(podSets, kueue.PodSet{
102-
Name: aw.Name + "-" + fmt.Sprint(i),
103-
Template: template,
104-
Count: replicas,
105-
})
106-
i++
10788
}
10889
}
10990
return podSets

0 commit comments

Comments
 (0)