Skip to content

Commit 791beca

Browse files
israel-hdezisrael-hdez
authored
Fix oauth-proxy should not be injected to InferenceGraph (#543)
* Fix oauth-proxy should not be injected to InferenceGraph InferenceGraphs do not require oauth-proxy for auth purposes. The auth validation is implemented in kserve-router. This is ensuring that oauth-proxy is not injected to InferenceGraph pods that are deployed in Raw mode. Also, this is fixing some issues with the management of the related `Service` and `Route`. Signed-off-by: Edgar Hernández <[email protected]> * Fix feedback: Jooho Signed-off-by: Edgar Hernández <[email protected]> --------- Signed-off-by: Edgar Hernández <[email protected]>
1 parent 6598f58 commit 791beca

File tree

7 files changed

+50
-32
lines changed

7 files changed

+50
-32
lines changed

‎pkg/controller/v1alpha1/inferencegraph/controller.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@ func (r *InferenceGraphReconciler) Reconcile(ctx context.Context, req ctrl.Reque
249249
}
250250
hostname, err := routeReconciler.Reconcile(ctx, graph)
251251
url.Host = hostname
252+
url.Scheme = "https"
252253
if err != nil {
253254
return ctrl.Result{}, errors.Wrapf(err, "fails to reconcile Route for InferenceGraph")
254255
}

‎pkg/controller/v1alpha1/inferencegraph/controller_test.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -656,6 +656,10 @@ var _ = Describe("Inference Graph controller test", func() {
656656
return true
657657
}, timeout, interval).Should(BeTrue())
658658

659+
// ODH Svc checks
660+
Expect(actualK8sServiceCreated.Spec.Ports[0].Port).To(Equal(int32(443)))
661+
Expect(actualK8sServiceCreated.Spec.Ports[0].TargetPort.IntVal).To(Equal(int32(8080)))
662+
659663
//No KNative Service should get created in Raw deployment mode
660664
actualKnServiceCreated := &knservingv1.Service{}
661665
Eventually(func() bool {
@@ -706,6 +710,7 @@ var _ = Describe("Inference Graph controller test", func() {
706710
k8sClient.Get(ctx, serviceKey, inferenceGraphSubmitted)
707711
return inferenceGraphSubmitted.Status.URL.Host
708712
}, timeout, interval).Should(Equal(osRoute.Status.Ingress[0].Host))
713+
Expect(inferenceGraphSubmitted.Status.URL.Scheme).To(Equal("https"))
709714
})
710715

711716
It("Should not create ingress when cluster-local visibility is configured", func() {
@@ -772,6 +777,7 @@ var _ = Describe("Inference Graph controller test", func() {
772777
_ = k8sClient.Get(ctx, serviceKey, ig)
773778
return ig.Status.URL.Host
774779
}, timeout, interval).Should(Equal(fmt.Sprintf("%s.%s.svc.cluster.local", graphName, "default")))
780+
Expect(ig.Status.URL.Scheme).To(Equal("https"))
775781
})
776782

777783
It("Should reconfigure InferenceGraph as private when cluster-local visibility is configured", func() {
@@ -1010,7 +1016,7 @@ var _ = Describe("Inference Graph controller test", func() {
10101016
g.Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: inferenceGraph.GetNamespace(), Name: inferenceGraph.GetName()}, &igDeployment)).To(Succeed())
10111017
g.Expect(igDeployment.Spec.Template.Spec.AutomountServiceAccountToken).To(Equal(proto.Bool(true)))
10121018
g.Expect(igDeployment.Spec.Template.Spec.ServiceAccountName).To(Equal(getServiceAccountNameForGraph(inferenceGraph)))
1013-
// g.Expect(igDeployment.Spec.Template.Spec.Containers).To(HaveLen(1)) // TODO: Restore in RHOAIENG-21300
1019+
g.Expect(igDeployment.Spec.Template.Spec.Containers).To(HaveLen(1))
10141020
g.Expect(igDeployment.Spec.Template.Spec.Containers[0].Args).To(ContainElements("--enable-auth", "--inferencegraph-name", inferenceGraph.GetName()))
10151021
}, timeout, interval).Should(Succeed())
10161022
})

‎pkg/controller/v1alpha1/inferencegraph/openshift_route_reconciler.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,9 @@ func (r *OpenShiftRouteReconciler) buildOpenShiftRoute(inferenceGraph *v1alpha1.
104104
Port: &v1.RoutePort{
105105
TargetPort: intstr.FromString(inferenceGraph.GetName()),
106106
},
107+
TLS: &v1.TLSConfig{
108+
Termination: v1.TLSTerminationReencrypt,
109+
},
107110
},
108111
}
109112

‎pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,12 @@ func createRawDeploymentODH(clientset kubernetes.Interface, resourceType constan
9494
headDeployment := deploymentList[0]
9595
if val, ok := componentMeta.Annotations[constants.ODHKserveRawAuth]; ok && strings.EqualFold(val, "true") {
9696
enableAuth = true
97-
err := addOauthContainerToDeployment(clientset, headDeployment, componentMeta, componentExt, podSpec)
98-
if err != nil {
99-
return nil, err
97+
98+
if resourceType != constants.InferenceGraphResource { // InferenceGraphs don't use oauth-proxy
99+
err := addOauthContainerToDeployment(clientset, headDeployment, componentMeta, componentExt, podSpec)
100+
if err != nil {
101+
return nil, err
102+
}
100103
}
101104
}
102105
if (resourceType == constants.InferenceServiceResource && enableAuth) || resourceType == constants.InferenceGraphResource {

‎pkg/controller/v1beta1/inferenceservice/reconcilers/raw/raw_kube_reconciler.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ func NewRawKubeReconciler(client client.Client,
8585
client: client,
8686
scheme: scheme,
8787
Deployment: depl,
88-
Service: service.NewServiceReconciler(client, scheme, componentMeta, componentExt, podSpec, multiNodeEnabled, serviceConfig),
88+
Service: service.NewServiceReconciler(client, scheme, resourceType, componentMeta, componentExt, podSpec, multiNodeEnabled, serviceConfig),
8989
Scaler: as,
9090
URL: url,
9191
}, nil

‎pkg/controller/v1beta1/inferenceservice/reconcilers/service/service_reconciler.go

Lines changed: 30 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -48,19 +48,20 @@ type ServiceReconciler struct {
4848

4949
func NewServiceReconciler(client client.Client,
5050
scheme *runtime.Scheme,
51+
resourceType constants.ResourceType,
5152
componentMeta metav1.ObjectMeta,
5253
componentExt *v1beta1.ComponentExtensionSpec,
5354
podSpec *corev1.PodSpec, multiNodeEnabled bool,
5455
serviceConfig *v1beta1.ServiceConfig) *ServiceReconciler {
5556
return &ServiceReconciler{
5657
client: client,
5758
scheme: scheme,
58-
ServiceList: createService(componentMeta, componentExt, podSpec, multiNodeEnabled, serviceConfig),
59+
ServiceList: createService(resourceType, componentMeta, componentExt, podSpec, multiNodeEnabled, serviceConfig),
5960
componentExt: componentExt,
6061
}
6162
}
6263

63-
func createService(componentMeta metav1.ObjectMeta, componentExt *v1beta1.ComponentExtensionSpec,
64+
func createService(resourceType constants.ResourceType, componentMeta metav1.ObjectMeta, componentExt *v1beta1.ComponentExtensionSpec,
6465
podSpec *corev1.PodSpec, multiNodeEnabled bool, serviceConfig *v1beta1.ServiceConfig) []*corev1.Service {
6566
var svcList []*corev1.Service
6667
var isWorkerContainer bool
@@ -75,11 +76,11 @@ func createService(componentMeta metav1.ObjectMeta, componentExt *v1beta1.Compon
7576

7677
if !multiNodeEnabled {
7778
// If multiNodeEnabled is false, only defaultSvc will be created.
78-
defaultSvc := createDefaultSvc(componentMeta, componentExt, podSpec, serviceConfig)
79+
defaultSvc := createDefaultSvc(resourceType, componentMeta, componentExt, podSpec, serviceConfig)
7980
svcList = append(svcList, defaultSvc)
8081
} else if multiNodeEnabled && !isWorkerContainer {
8182
// If multiNodeEnabled is true, both defaultSvc and headSvc will be created.
82-
defaultSvc := createDefaultSvc(componentMeta, componentExt, podSpec, serviceConfig)
83+
defaultSvc := createDefaultSvc(resourceType, componentMeta, componentExt, podSpec, serviceConfig)
8384
svcList = append(svcList, defaultSvc)
8485

8586
headSvc := createHeadlessSvc(componentMeta)
@@ -89,7 +90,7 @@ func createService(componentMeta metav1.ObjectMeta, componentExt *v1beta1.Compon
8990
return svcList
9091
}
9192

92-
func createDefaultSvc(componentMeta metav1.ObjectMeta, componentExt *v1beta1.ComponentExtensionSpec,
93+
func createDefaultSvc(resourceType constants.ResourceType, componentMeta metav1.ObjectMeta, componentExt *v1beta1.ComponentExtensionSpec,
9394
podSpec *corev1.PodSpec, serviceConfig *v1beta1.ServiceConfig) *corev1.Service {
9495
var servicePorts []corev1.ServicePort
9596

@@ -175,28 +176,32 @@ func createDefaultSvc(componentMeta metav1.ObjectMeta, componentExt *v1beta1.Com
175176
}
176177
service.ObjectMeta.Annotations[constants.OpenshiftServingCertAnnotation] = componentMeta.Name + constants.ServingCertSecretSuffix
177178

178-
if val, ok := componentMeta.Annotations[constants.ODHKserveRawAuth]; ok && strings.EqualFold(val, "true") {
179-
httpsPort := corev1.ServicePort{
180-
Name: "https",
181-
Port: constants.OauthProxyPort,
182-
TargetPort: intstr.IntOrString{
183-
Type: intstr.String,
184-
StrVal: "https",
185-
},
186-
Protocol: corev1.ProtocolTCP,
187-
}
188-
ports := service.Spec.Ports
189-
replaced := false
190-
for i, port := range ports {
191-
if port.Port == constants.CommonDefaultHttpPort {
192-
ports[i] = httpsPort
193-
replaced = true
179+
if resourceType == constants.InferenceGraphResource {
180+
servicePorts[0].Port = int32(443)
181+
} else {
182+
if val, ok := componentMeta.Annotations[constants.ODHKserveRawAuth]; ok && strings.EqualFold(val, "true") {
183+
httpsPort := corev1.ServicePort{
184+
Name: "https",
185+
Port: constants.OauthProxyPort,
186+
TargetPort: intstr.IntOrString{
187+
Type: intstr.String,
188+
StrVal: "https",
189+
},
190+
Protocol: corev1.ProtocolTCP,
194191
}
192+
ports := service.Spec.Ports
193+
replaced := false
194+
for i, port := range ports {
195+
if port.Port == constants.CommonDefaultHttpPort {
196+
ports[i] = httpsPort
197+
replaced = true
198+
}
199+
}
200+
if !replaced {
201+
ports = append(ports, httpsPort)
202+
}
203+
service.Spec.Ports = ports
195204
}
196-
if !replaced {
197-
ports = append(ports, httpsPort)
198-
}
199-
service.Spec.Ports = ports
200205
}
201206

202207
if serviceConfig != nil && serviceConfig.ServiceClusterIPNone {

‎pkg/controller/v1beta1/inferenceservice/reconcilers/service/service_reconciler_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ func TestCreateDefaultDeployment(t *testing.T) {
224224
}
225225
for _, tt := range tests {
226226
t.Run(tt.name, func(t *testing.T) {
227-
got := createService(tt.args.componentMeta, tt.args.componentExt, tt.args.podSpec, tt.args.multiNodeEnabled, emptyServiceConfig)
227+
got := createService(constants.InferenceServiceResource, tt.args.componentMeta, tt.args.componentExt, tt.args.podSpec, tt.args.multiNodeEnabled, emptyServiceConfig)
228228
for i, service := range got {
229229
if diff := cmp.Diff(tt.expected[i], service); diff != "" {
230230
t.Errorf("Test %q unexpected service (-want +got): %v", tt.name, diff)
@@ -282,7 +282,7 @@ func runTestServiceCreate(serviceConfig *v1beta1.ServiceConfig, expectedClusterI
282282
componentExt := &v1beta1.ComponentExtensionSpec{}
283283
podSpec := &corev1.PodSpec{}
284284

285-
service := createService(componentMeta, componentExt, podSpec, false, serviceConfig)
285+
service := createService(constants.InferenceServiceResource, componentMeta, componentExt, podSpec, false, serviceConfig)
286286
assert.Equal(t, componentMeta, service[0].ObjectMeta, "Expected ObjectMeta to be equal")
287287
assert.Equal(t, map[string]string{"app": "isvc.test-service"}, service[0].Spec.Selector, "Expected Selector to be equal")
288288
assert.Equal(t, expectedClusterIP, service[0].Spec.ClusterIP, "Expected ClusterIP to be equal")

0 commit comments

Comments
 (0)