Skip to content

Commit fd73280

Browse files
JoohospoltiVedantMahabaleshwarkar
authored
[Cherrypick] Use annotation instead of label to enable auth for RawDeployment (#519)
* Make label and annotation propagation configurable (kserve#4030) * Make label and annotation propagation configurable chore: Make the DisallaowedAnnotations and Labels configurable through ConfigMap so users can configured it quickly. fixes kserve#3710 Signed-off-by: Spolti <[email protected]> * generate boilerplate code Signed-off-by: Spolti <[email protected]> * Edgar's review changes Signed-off-by: Spolti <[email protected]> --------- Signed-off-by: Spolti <[email protected]> (cherry picked from commit 654d314) Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * Use annotation instead of label to enable auth for RawDeployment Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> Signed-off-by: Jooho Lee <[email protected]> * address pr comments Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * change code to be compatible with isvcConfig.serviceAnnotationDisallowedList and added the disallowed list to odh isvc config patch Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * Remove comments from inferenceservice-config configmap manifests (#518) Signed-off-by: Jooho Lee <[email protected]> --------- Signed-off-by: Spolti <[email protected]> Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> Signed-off-by: Jooho Lee <[email protected]> Co-authored-by: Filippe Spolti <[email protected]> Co-authored-by: Vedant Mahabaleshwarkar <[email protected]>
1 parent 0f345de commit fd73280

23 files changed

+543
-231
lines changed

‎config/configmap/inferenceservice.yaml

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,38 @@ data:
4141
"defaultImageVersion": "latest"
4242
}
4343
}
44-
44+
# ====================================== ISVC CONFIGURATION ======================================
45+
# Example
46+
inferenceService: |-
47+
{
48+
"serviceAnnotationDisallowedList": [
49+
"my.custom.annotation/1"
50+
],
51+
"serviceLabelDisallowedList": [
52+
"my.custom.label.1"
53+
]
54+
}
55+
# Example of isvc configuration
56+
inferenceService: |-
57+
{
58+
# ServiceAnnotationDisallowedList is a list of annotations that are not allowed to be propagated to Knative
59+
# revisions, which prevents the reconciliation loop to be triggered if the annotations is
60+
# configured here are used.
61+
# Default values are:
62+
# "autoscaling.knative.dev/min-scale",
63+
# "autoscaling.knative.dev/max-scale",
64+
# "internal.serving.kserve.io/storage-initializer-sourceuri",
65+
# "kubectl.kubernetes.io/last-applied-configuration"
66+
# Any new value will be appended to the list.
67+
"serviceAnnotationDisallowedList": [
68+
"my.custom.annotation/1"
69+
],
70+
# ServiceLabelDisallowedList is a list of labels that are not allowed to be propagated to Knative revisions
71+
# which prevents the reconciliation loop to be triggered if the labels is configured here are used.
72+
"serviceLabelDisallowedList": [
73+
"my.custom.label.1"
74+
]
75+
}
4576
# ====================================== STORAGE INITIALIZER CONFIGURATION ======================================
4677
# Example
4778
storageInitializer: |-

‎config/overlays/odh/inferenceservice-config-patch.yaml

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,3 +84,14 @@ data:
8484
"enableMetricAggregation": "false",
8585
"enablePrometheusScraping" : "false"
8686
}
87+
88+
inferenceService: |-
89+
{
90+
"serviceAnnotationDisallowedList": [
91+
"autoscaling.knative.dev/min-scale",
92+
"autoscaling.knative.dev/max-scale",
93+
"internal.serving.kserve.io/storage-initializer-sourceuri",
94+
"kubectl.kubernetes.io/last-applied-configuration",
95+
"security.opendatahub.io/enable-auth"
96+
]
97+
}

‎hack/violation_exceptions.list

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ API rule violation: list_type_missing,github.com/kserve/kserve/pkg/apis/serving/
1111
API rule violation: list_type_missing,github.com/kserve/kserve/pkg/apis/serving/v1alpha1,TrainedModelList,Items
1212
API rule violation: list_type_missing,github.com/kserve/kserve/pkg/apis/serving/v1beta1,ComponentStatusSpec,Traffic
1313
API rule violation: list_type_missing,github.com/kserve/kserve/pkg/apis/serving/v1beta1,InferenceServiceList,Items
14+
API rule violation: list_type_missing,github.com/kserve/kserve/pkg/apis/serving/v1beta1,InferenceServicesConfig,ServiceAnnotationDisallowedList
15+
API rule violation: list_type_missing,github.com/kserve/kserve/pkg/apis/serving/v1beta1,InferenceServicesConfig,ServiceLabelDisallowedList
1416
API rule violation: list_type_missing,github.com/kserve/kserve/pkg/apis/serving/v1beta1,LoggerSpec,MetadataHeaders
1517
API rule violation: list_type_missing,github.com/kserve/kserve/pkg/apis/serving/v1beta1,PodSpec,Containers
1618
API rule violation: list_type_missing,github.com/kserve/kserve/pkg/apis/serving/v1beta1,PodSpec,EphemeralContainers

‎pkg/apis/serving/v1beta1/component.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,6 @@ type ComponentExtensionSpec struct {
117117
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/
118118
// +optional
119119
Annotations map[string]string `json:"annotations,omitempty"`
120-
121120
// The deployment strategy to use to replace existing pods with new ones. Only applicable for raw deployment mode.
122121
// +optional
123122
DeploymentStrategy *appsv1.DeploymentStrategy `json:"deploymentStrategy,omitempty"`

‎pkg/apis/serving/v1beta1/configmap.go

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,13 @@ import (
3131

3232
// ConfigMap Keys
3333
const (
34-
ExplainerConfigKeyName = "explainers"
35-
IngressConfigKeyName = "ingress"
36-
DeployConfigName = "deploy"
37-
LocalModelConfigName = "localModel"
38-
SecurityConfigName = "security"
39-
ServiceConfigName = "service"
34+
ExplainerConfigKeyName = "explainers"
35+
InferenceServiceConfigKeyName = "inferenceService"
36+
IngressConfigKeyName = "ingress"
37+
DeployConfigName = "deploy"
38+
LocalModelConfigName = "localModel"
39+
SecurityConfigName = "security"
40+
ServiceConfigName = "service"
4041
)
4142

4243
const (
@@ -62,6 +63,11 @@ type ExplainersConfig struct {
6263
type InferenceServicesConfig struct {
6364
// Explainer configurations
6465
Explainers ExplainersConfig `json:"explainers"`
66+
// ServiceAnnotationDisallowedList is a list of annotations that are not allowed to be propagated to Knative
67+
// revisions
68+
ServiceAnnotationDisallowedList []string `json:"serviceAnnotationDisallowedList,omitempty"`
69+
// ServiceLabelDisallowedList is a list of labels that are not allowed to be propagated to Knative revisions
70+
ServiceLabelDisallowedList []string `json:"serviceLabelDisallowedList,omitempty"`
6571
}
6672

6773
// +kubebuilder:object:generate=false
@@ -115,7 +121,9 @@ type ServiceConfig struct {
115121
}
116122

117123
func NewInferenceServicesConfig(clientset kubernetes.Interface) (*InferenceServicesConfig, error) {
118-
configMap, err := clientset.CoreV1().ConfigMaps(constants.KServeNamespace).Get(context.TODO(), constants.InferenceServiceConfigMapName, metav1.GetOptions{})
124+
configMap, err := clientset.CoreV1().ConfigMaps(constants.KServeNamespace).Get(
125+
context.TODO(), constants.InferenceServiceConfigMapName, metav1.GetOptions{})
126+
119127
if err != nil {
120128
return nil, err
121129
}
@@ -127,6 +135,27 @@ func NewInferenceServicesConfig(clientset kubernetes.Interface) (*InferenceServi
127135
return nil, err
128136
}
129137
}
138+
139+
if isvc, ok := configMap.Data[InferenceServiceConfigKeyName]; ok {
140+
errisvc := json.Unmarshal([]byte(isvc), &icfg)
141+
if errisvc != nil {
142+
return nil, fmt.Errorf("unable to parse isvc config json: %w", errisvc)
143+
}
144+
if icfg.ServiceAnnotationDisallowedList == nil {
145+
icfg.ServiceAnnotationDisallowedList = constants.ServiceAnnotationDisallowedList
146+
} else {
147+
icfg.ServiceAnnotationDisallowedList = append(
148+
constants.ServiceAnnotationDisallowedList,
149+
icfg.ServiceAnnotationDisallowedList...)
150+
}
151+
if icfg.ServiceLabelDisallowedList == nil {
152+
icfg.ServiceLabelDisallowedList = constants.RevisionTemplateLabelDisallowedList
153+
} else {
154+
icfg.ServiceLabelDisallowedList = append(
155+
constants.RevisionTemplateLabelDisallowedList,
156+
icfg.ServiceLabelDisallowedList...)
157+
}
158+
}
130159
return icfg, nil
131160
}
132161

‎pkg/apis/serving/v1beta1/configmap_test.go

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,17 @@ var (
4949
ServiceConfigData = fmt.Sprintf(`{
5050
"serviceClusterIPNone" : %t
5151
}`, true)
52+
53+
ISCVWithData = fmt.Sprintf(`{
54+
"serviceAnnotationDisallowedList": ["%s","%s"],
55+
"serviceLabelDisallowedList": ["%s","%s"]
56+
}`, "my.custom.annotation/1", "my.custom.annotation/2",
57+
"my.custom.label.1", "my.custom.label.2")
58+
59+
ISCVNoData = fmt.Sprintf(`{
60+
"serviceAnnotationDisallowedList": %s,
61+
"serviceLabelDisallowedList": %s
62+
}`, []string{}, []string{})
5263
)
5364

5465
func TestNewInferenceServiceConfig(t *testing.T) {
@@ -147,5 +158,35 @@ func TestNewServiceConfig(t *testing.T) {
147158
g.Expect(err).Should(gomega.BeNil())
148159
g.Expect(nv).ShouldNot(gomega.BeNil())
149160
g.Expect(nv.ServiceClusterIPNone).Should(gomega.BeFalse())
161+
}
162+
163+
func TestInferenceServiceDisallowedLists(t *testing.T) {
164+
g := gomega.NewGomegaWithT(t)
165+
withData := fakeclientset.NewSimpleClientset(&v1.ConfigMap{
166+
ObjectMeta: metav1.ObjectMeta{Name: constants.InferenceServiceConfigMapName, Namespace: constants.KServeNamespace},
167+
Data: map[string]string{
168+
InferenceServiceConfigKeyName: ISCVWithData,
169+
},
170+
})
171+
isvcConfigWithData, err := NewInferenceServicesConfig(withData)
172+
g.Expect(err).Should(gomega.BeNil())
173+
g.Expect(isvcConfigWithData).ShouldNot(gomega.BeNil())
150174

175+
annotations := append(constants.ServiceAnnotationDisallowedList, []string{"my.custom.annotation/1", "my.custom.annotation/2"}...)
176+
g.Expect(isvcConfigWithData.ServiceAnnotationDisallowedList).To(gomega.Equal(annotations))
177+
labels := append(constants.RevisionTemplateLabelDisallowedList, []string{"my.custom.label.1", "my.custom.label.2"}...)
178+
g.Expect(isvcConfigWithData.ServiceLabelDisallowedList).To(gomega.Equal(labels))
179+
180+
// with no data
181+
withoutData := fakeclientset.NewSimpleClientset(&v1.ConfigMap{
182+
ObjectMeta: metav1.ObjectMeta{Name: constants.InferenceServiceConfigMapName, Namespace: constants.KServeNamespace},
183+
Data: map[string]string{
184+
InferenceServiceConfigKeyName: ISCVNoData,
185+
},
186+
})
187+
isvcConfigWithoutData, err := NewInferenceServicesConfig(withoutData)
188+
g.Expect(err).Should(gomega.BeNil())
189+
g.Expect(isvcConfigWithoutData).ShouldNot(gomega.BeNil())
190+
g.Expect(isvcConfigWithoutData.ServiceAnnotationDisallowedList).To(gomega.Equal(constants.ServiceAnnotationDisallowedList))
191+
g.Expect(isvcConfigWithoutData.ServiceLabelDisallowedList).To(gomega.Equal(constants.RevisionTemplateLabelDisallowedList))
151192
}

‎pkg/constants/constants.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -372,8 +372,6 @@ var (
372372
autoscaling.MaxScaleAnnotationKey,
373373
StorageInitializerSourceUriInternalAnnotationKey,
374374
"kubectl.kubernetes.io/last-applied-configuration",
375-
// remove when https://issues.redhat.com/browse/RHOAIENG-15662 is merged on community and ported to ODH
376-
// Plus, this annotation must be moved to the inferenceservice-config
377375
"security.opendatahub.io/enable-auth",
378376
}
379377

‎pkg/controller/v1beta1/inferenceservice/components/explainer.go

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,19 @@ func NewExplainer(client client.Client, clientset kubernetes.Interface, scheme *
7171
func (e *Explainer) Reconcile(isvc *v1beta1.InferenceService) (ctrl.Result, error) {
7272
e.Log.Info("Reconciling Explainer", "ExplainerSpec", isvc.Spec.Explainer)
7373
explainer := isvc.Spec.Explainer.GetImplementation()
74-
annotations := utils.Filter(isvc.Annotations, func(key string) bool {
75-
return !utils.Includes(constants.ServiceAnnotationDisallowedList, key)
76-
})
74+
var annotations map[string]string
75+
if e.deploymentMode == constants.RawDeployment {
76+
annotations = utils.Filter(isvc.Annotations, func(key string) bool {
77+
// https://issues.redhat.com/browse/RHOAIENG-20326
78+
// For RawDeployment, we allow the security.opendatahub.io/enable-auth annotation
79+
return !utils.Includes(isvcutils.FilterList(e.inferenceServiceConfig.ServiceAnnotationDisallowedList, constants.ODHKserveRawAuth), key)
80+
})
81+
} else {
82+
annotations = utils.Filter(isvc.Annotations, func(key string) bool {
83+
return !utils.Includes(e.inferenceServiceConfig.ServiceAnnotationDisallowedList, key)
84+
})
85+
}
86+
7787
// KNative does not support INIT containers or mounting, so we add annotations that trigger the
7888
// StorageInitializer injector to mutate the underlying deployment to provision model data
7989
if sourceURI := explainer.GetStorageUri(); sourceURI != nil {
@@ -104,11 +114,20 @@ func (e *Explainer) Reconcile(isvc *v1beta1.InferenceService) (ctrl.Result, erro
104114
}
105115

106116
// Labels and annotations from explainer component
107-
// Label filter will be handled in ksvc_reconciler
117+
// Label filter will be handled in ksvc_reconciler and raw reconciler
108118
explainerLabels := isvc.Spec.Explainer.Labels
109-
explainerAnnotations := utils.Filter(isvc.Spec.Explainer.Annotations, func(key string) bool {
110-
return !utils.Includes(constants.ServiceAnnotationDisallowedList, key)
111-
})
119+
var explainerAnnotations map[string]string
120+
if e.deploymentMode == constants.RawDeployment {
121+
explainerAnnotations = utils.Filter(isvc.Spec.Explainer.Annotations, func(key string) bool {
122+
// https://issues.redhat.com/browse/RHOAIENG-20326
123+
// For RawDeployment, we allow the security.opendatahub.io/enable-auth annotation
124+
return !utils.Includes(isvcutils.FilterList(e.inferenceServiceConfig.ServiceAnnotationDisallowedList, constants.ODHKserveRawAuth), key)
125+
})
126+
} else {
127+
explainerAnnotations = utils.Filter(isvc.Spec.Explainer.Annotations, func(key string) bool {
128+
return !utils.Includes(e.inferenceServiceConfig.ServiceAnnotationDisallowedList, key)
129+
})
130+
}
112131

113132
// Labels and annotations priority: explainer component > isvc
114133
// Labels and annotations from high priority will overwrite that from low priority
@@ -170,7 +189,7 @@ func (e *Explainer) Reconcile(isvc *v1beta1.InferenceService) (ctrl.Result, erro
170189
isvc.Status.PropagateRawStatus(v1beta1.ExplainerComponent, deployment, r.URL)
171190
} else {
172191
r := knative.NewKsvcReconciler(e.client, e.scheme, objectMeta, &isvc.Spec.Explainer.ComponentExtensionSpec,
173-
&podSpec, isvc.Status.Components[v1beta1.ExplainerComponent])
192+
&podSpec, isvc.Status.Components[v1beta1.ExplainerComponent], e.inferenceServiceConfig.ServiceLabelDisallowedList)
174193

175194
if err := controllerutil.SetControllerReference(isvc, r.Service, e.scheme); err != nil {
176195
return ctrl.Result{}, errors.Wrapf(err, "fails to set owner reference for explainer")

‎pkg/controller/v1beta1/inferenceservice/components/predictor.go

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -88,10 +88,21 @@ func (p *Predictor) Reconcile(isvc *v1beta1.InferenceService) (ctrl.Result, erro
8888
if isvc.Spec.Predictor.WorkerSpec != nil {
8989
multiNodeEnabled = true
9090
}
91+
var annotations map[string]string
92+
if p.deploymentMode == constants.RawDeployment {
93+
annotations = utils.Filter(isvc.Annotations, func(key string) bool {
94+
// https://issues.redhat.com/browse/RHOAIENG-20326
95+
// For RawDeployment, we allow the security.opendatahub.io/enable-auth annotation
96+
return !utils.Includes(isvcutils.FilterList(p.inferenceServiceConfig.ServiceAnnotationDisallowedList, constants.ODHKserveRawAuth), key)
97+
})
98+
} else {
99+
annotations = utils.Filter(isvc.Annotations, func(key string) bool {
100+
return !utils.Includes(p.inferenceServiceConfig.ServiceAnnotationDisallowedList, key)
101+
})
102+
}
91103

92-
annotations := utils.Filter(isvc.Annotations, func(key string) bool {
93-
return !utils.Includes(constants.ServiceAnnotationDisallowedList, key)
94-
})
104+
p.Log.V(1).Info("Predictor custom annotations", "annotations", p.inferenceServiceConfig.ServiceAnnotationDisallowedList)
105+
p.Log.V(1).Info("Predictor custom labels", "labels", p.inferenceServiceConfig.ServiceLabelDisallowedList)
95106

96107
addLoggerAnnotations(isvc.Spec.Predictor.Logger, annotations)
97108
addBatcherAnnotations(isvc.Spec.Predictor.Batcher, annotations)
@@ -228,9 +239,17 @@ func (p *Predictor) Reconcile(isvc *v1beta1.InferenceService) (ctrl.Result, erro
228239

229240
// Label filter will be handled in ksvc_reconciler
230241
sRuntimeLabels = sRuntime.ServingRuntimePodSpec.Labels
231-
sRuntimeAnnotations = utils.Filter(sRuntime.ServingRuntimePodSpec.Annotations, func(key string) bool {
232-
return !utils.Includes(constants.ServiceAnnotationDisallowedList, key)
233-
})
242+
if p.deploymentMode == constants.RawDeployment {
243+
sRuntimeAnnotations = utils.Filter(sRuntime.ServingRuntimePodSpec.Annotations, func(key string) bool {
244+
// https://issues.redhat.com/browse/RHOAIENG-20326
245+
// For RawDeployment, we allow the security.opendatahub.io/enable-auth annotation
246+
return !utils.Includes(isvcutils.FilterList(p.inferenceServiceConfig.ServiceAnnotationDisallowedList, constants.ODHKserveRawAuth), key)
247+
})
248+
} else {
249+
sRuntimeAnnotations = utils.Filter(sRuntime.ServingRuntimePodSpec.Annotations, func(key string) bool {
250+
return !utils.Includes(p.inferenceServiceConfig.ServiceAnnotationDisallowedList, key)
251+
})
252+
}
234253
} else {
235254
container = predictor.GetContainer(isvc.ObjectMeta, isvc.Spec.Predictor.GetExtensions(), p.inferenceServiceConfig)
236255

@@ -260,11 +279,20 @@ func (p *Predictor) Reconcile(isvc *v1beta1.InferenceService) (ctrl.Result, erro
260279
}
261280

262281
// Labels and annotations from predictor component
263-
// Label filter will be handled in ksvc_reconciler
282+
// Label filter will be handled in ksvc_reconciler and raw reconciler
264283
predictorLabels := isvc.Spec.Predictor.Labels
265-
predictorAnnotations := utils.Filter(isvc.Spec.Predictor.Annotations, func(key string) bool {
266-
return !utils.Includes(constants.ServiceAnnotationDisallowedList, key)
267-
})
284+
var predictorAnnotations map[string]string
285+
if p.deploymentMode == constants.RawDeployment {
286+
predictorAnnotations = utils.Filter(isvc.Spec.Predictor.Annotations, func(key string) bool {
287+
// https://issues.redhat.com/browse/RHOAIENG-20326
288+
// For RawDeployment, we allow the security.opendatahub.io/enable-auth annotation
289+
return !utils.Includes(isvcutils.FilterList(p.inferenceServiceConfig.ServiceAnnotationDisallowedList, constants.ODHKserveRawAuth), key)
290+
})
291+
} else {
292+
predictorAnnotations = utils.Filter(isvc.Spec.Predictor.Annotations, func(key string) bool {
293+
return !utils.Includes(p.inferenceServiceConfig.ServiceAnnotationDisallowedList, key)
294+
})
295+
}
268296

269297
// Labels and annotations priority: predictor component > isvc > ServingRuntimePodSpec
270298
// Labels and annotations from high priority will overwrite that from low priority
@@ -368,7 +396,7 @@ func (p *Predictor) Reconcile(isvc *v1beta1.InferenceService) (ctrl.Result, erro
368396
} else {
369397
podLabelKey = constants.RevisionLabel
370398
r := knative.NewKsvcReconciler(p.client, p.scheme, objectMeta, &isvc.Spec.Predictor.ComponentExtensionSpec,
371-
&podSpec, isvc.Status.Components[v1beta1.PredictorComponent])
399+
&podSpec, isvc.Status.Components[v1beta1.PredictorComponent], p.inferenceServiceConfig.ServiceLabelDisallowedList)
372400
if err := controllerutil.SetControllerReference(isvc, r.Service, p.scheme); err != nil {
373401
return ctrl.Result{}, errors.Wrapf(err, "fails to set owner reference for predictor")
374402
}

0 commit comments

Comments
 (0)