address more feedback

Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

pkg/controller/v1beta1/inferenceservice/rawkube_controller_test.go

@@ -1375,7 +1375,7 @@ var _ = Describe("v1beta1 inference service controller", func() {
 			actualIngress := &netv1.Ingress{}
 			predictorIngressKey := types.NamespacedName{Name: serviceKey.Name,
 				Namespace: serviceKey.Namespace}
-			Eventually(func() error { return k8sClient.Get(context.TODO(), predictorIngressKey, actualIngress) }, timeout).
+			Consistently(func() error { return k8sClient.Get(context.TODO(), predictorIngressKey, actualIngress) }, timeout).
 				ShouldNot(Succeed())
 		})
 	})
@@ -2432,7 +2432,6 @@ var _ = Describe("v1beta1 inference service controller", func() {
 										//`--cookie-secret=SECRET`,
 										`--openshift-delegate-urls={"/": {"namespace": "` + serviceKey.Namespace + `", "resource": "inferenceservices", "group": "serving.kserve.io", "name": "` + serviceName + `", "verb": "get"}}`,
 										`--openshift-sar={"namespace": "` + serviceKey.Namespace + `", "resource": "inferenceservices", "group": "serving.kserve.io", "name": "` + serviceName + `", "verb": "get"}`,
-										`--skip-auth-regex="(^/metrics|^/apis/v1beta1/healthz)"`,
 									},
 									Ports: []v1.ContainerPort{
 										{

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

@@ -109,7 +109,10 @@ func createRawDeployment(clientset kubernetes.Interface, componentMeta metav1.Ob
 
 	defaultDeployment, err := createRawDefaultDeployment(clientset, componentMeta, componentExt, podSpec)
 	if err != nil {
-		return nil, err
+		// the calling function will ignore the deploymentlist if an error is also returned
+		// This is required for the deployment_reconciler_tests
+		deploymentList = append(deploymentList, defaultDeployment)
+		return deploymentList, err
 	}
 	if multiNodeEnabled {
 		// Use defaut value(1) if tensor-parallel-size is not set (gpu count)
@@ -146,6 +149,26 @@ func createRawDefaultDeployment(clientset kubernetes.Interface, componentMeta me
 	podMetadata := componentMeta
 	podMetadata.Labels["app"] = constants.GetRawServiceLabel(componentMeta.Name)
 	setDefaultPodSpec(podSpec)
+
+	deployment := &appsv1.Deployment{
+		ObjectMeta: componentMeta,
+		Spec: appsv1.DeploymentSpec{
+			Selector: &metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					"app": constants.GetRawServiceLabel(componentMeta.Name),
+				},
+			},
+			Template: corev1.PodTemplateSpec{
+				ObjectMeta: podMetadata,
+				Spec:       *podSpec,
+			},
+		},
+	}
+	if componentExt.DeploymentStrategy != nil {
+		deployment.Spec.Strategy = *componentExt.DeploymentStrategy
+	}
+	setDefaultDeploymentSpec(&deployment.Spec)
+
 	var isvcname string
 	var upstreamPort string
 	if val, ok := componentMeta.Labels[constants.InferenceServiceLabel]; ok {
@@ -167,9 +190,12 @@ func createRawDefaultDeployment(clientset kubernetes.Interface, componentMeta me
 		}
 		oauthProxyContainer, err := generateOauthProxyContainer(clientset, isvcname, componentMeta.Namespace, upstreamPort)
 		if err != nil {
-			return nil, err
+			// return the deployment without the oauth proxy container if there was an error
+			// This is required for the deployment_reconciler_tests
+			return deployment, err
 		}
-		podSpec.Containers = append(podSpec.Containers, oauthProxyContainer)
+		updatedPodSpec := podSpec.DeepCopy()
+		updatedPodSpec.Containers = append(updatedPodSpec.Containers, *oauthProxyContainer)
 		tlsSecretVolume := corev1.Volume{
 			Name: tlsVolumeName,
 			VolumeSource: corev1.VolumeSource{
@@ -179,26 +205,9 @@ func createRawDefaultDeployment(clientset kubernetes.Interface, componentMeta me
 				},
 			},
 		}
-		podSpec.Volumes = append(podSpec.Volumes, tlsSecretVolume)
+		updatedPodSpec.Volumes = append(updatedPodSpec.Volumes, tlsSecretVolume)
+		deployment.Spec.Template.Spec = *updatedPodSpec
 	}
-	deployment := &appsv1.Deployment{
-		ObjectMeta: componentMeta,
-		Spec: appsv1.DeploymentSpec{
-			Selector: &metav1.LabelSelector{
-				MatchLabels: map[string]string{
-					"app": constants.GetRawServiceLabel(componentMeta.Name),
-				},
-			},
-			Template: corev1.PodTemplateSpec{
-				ObjectMeta: podMetadata,
-				Spec:       *podSpec,
-			},
-		},
-	}
-	if componentExt.DeploymentStrategy != nil {
-		deployment.Spec.Strategy = *componentExt.DeploymentStrategy
-	}
-	setDefaultDeploymentSpec(&deployment.Spec)
 	return deployment, nil
 }
 func createRawWorkerDeployment(componentMeta metav1.ObjectMeta,
@@ -241,19 +250,19 @@ func GetKServeContainerPort(podSpec *corev1.PodSpec) string {
 	}
 	return ""
 }
-func generateOauthProxyContainer(clientset kubernetes.Interface, isvc string, namespace string, upstreamPort string) (corev1.Container, error) {
-	configMap, err := clientset.CoreV1().ConfigMaps(constants.KServeNamespace).Get(context.TODO(), constants.InferenceServiceConfigMapName, metav1.GetOptions{})
+func generateOauthProxyContainer(clientset kubernetes.Interface, isvc string, namespace string, upstreamPort string) (*corev1.Container, error) {
+	isvcConfigMap, err := clientset.CoreV1().ConfigMaps(constants.KServeNamespace).Get(context.TODO(), constants.InferenceServiceConfigMapName, metav1.GetOptions{})
 	if err != nil {
-		return corev1.Container{}, err
+		return nil, err
 	}
-	oauthProxyJSON := strings.TrimSpace(configMap.Data["oauthProxy"])
+	oauthProxyJSON := strings.TrimSpace(isvcConfigMap.Data["oauthProxy"])
 	oauthProxyConfig := v1beta1.OauthConfig{}
 	if err := json.Unmarshal([]byte(oauthProxyJSON), &oauthProxyConfig); err != nil {
-		return corev1.Container{}, err
+		return nil, err
 	}
 	if oauthProxyConfig.Image == "" || oauthProxyConfig.MemoryRequest == "" || oauthProxyConfig.MemoryLimit == "" ||
 		oauthProxyConfig.CpuRequest == "" || oauthProxyConfig.CpuLimit == "" {
-		return corev1.Container{}, fmt.Errorf("one or more oauthProxyConfig fields are empty")
+		return nil, fmt.Errorf("one or more oauthProxyConfig fields are empty")
 	}
 	oauthImage := oauthProxyConfig.Image
 	oauthMemoryRequest := oauthProxyConfig.MemoryRequest
@@ -263,13 +272,13 @@ func generateOauthProxyContainer(clientset kubernetes.Interface, isvc string, na
 
 	cookieSecret, err := generateCookieSecret()
 	if err != nil {
-		return corev1.Container{}, err
+		return nil, err
 	}
 
-	return corev1.Container{
+	return &corev1.Container{
 		Name: "oauth-proxy",
 		Args: []string{
-			`--https-address=:8443`,
+			`--https-address=:` + strconv.Itoa(constants.OauthProxyPort),
 			`--provider=openshift`,
 			`--skip-provider-button`,
 			`--upstream=http://localhost:` + upstreamPort,
@@ -278,7 +287,6 @@ func generateOauthProxyContainer(clientset kubernetes.Interface, isvc string, na
 			`--cookie-secret=` + cookieSecret,
 			`--openshift-delegate-urls={"/": {"namespace": "` + namespace + `", "resource": "inferenceservices", "group": "serving.kserve.io", "name": "` + isvc + `", "verb": "get"}}`,
 			`--openshift-sar={"namespace": "` + namespace + `", "resource": "inferenceservices", "group": "serving.kserve.io", "name": "` + isvc + `", "verb": "get"}`,
-			`--skip-auth-regex="(^/metrics|^/apis/v1beta1/healthz)"`,
 		},
 		Image: oauthImage,
 		Ports: []corev1.ContainerPort{

pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler_test.go

@@ -394,6 +394,10 @@ func TestCreateDefaultDeployment(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got, _ := createRawDeployment(clientset, tt.args.objectMeta, tt.args.workerObjectMeta, tt.args.componentExt, tt.args.podSpec, tt.args.workerPodSpec)
+			if len(got) == 0 {
+				t.Errorf("Got empty deployment")
+			}
+
 			for i, deploy := range got {
 				if diff := cmp.Diff(tt.expected[i], deploy, cmpopts.IgnoreFields(appsv1.Deployment{}, "Spec.Template.Spec.SecurityContext"),
 					cmpopts.IgnoreFields(appsv1.Deployment{}, "Spec.Template.Spec.RestartPolicy"),
@@ -462,6 +466,9 @@ func TestCreateDefaultDeployment(t *testing.T) {
 
 			// update objectMeta using modify func
 			got, _ := createRawDeployment(clientset, ttArgs.objectMeta, ttArgs.workerObjectMeta, ttArgs.componentExt, tt.modifyArgs(ttArgs).podSpec, tt.modifyArgs(ttArgs).workerPodSpec)
+			if len(got) == 0 {
+				t.Errorf("Got empty deployment")
+			}
 
 			// update expected value using modifyExpected func
 			expected := tt.modifyExpected(ttExpected)
@@ -765,6 +772,9 @@ func TestCreateDefaultDeployment(t *testing.T) {
 
 			// update objectMeta using modify func
 			got, _ := createRawDeployment(clientset, tt.modifyObjectMetaArgs(ttArgs).objectMeta, tt.modifyWorkerObjectMetaArgs(ttArgs).workerObjectMeta, ttArgs.componentExt, tt.modifyPodSpecArgs(ttArgs).podSpec, tt.modifyWorkerPodSpecArgs(ttArgs).workerPodSpec)
+			if len(got) == 0 {
+				t.Errorf("Got empty deployment")
+			}
 
 			// update expected value using modifyExpected func
 			expected := tt.modifyExpected(ttExpected)

pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/kube_ingress_reconciler.go

@@ -267,6 +267,7 @@ func semanticIngressEquals(desired, existing *netv1.Ingress) bool {
 	return equality.Semantic.DeepEqual(desired.Spec, existing.Spec)
 }
 
+// Check for route created by odh-model-controller. If the route is found, use it as the isvc URL
 func getRouteURLIfExists(cli client.Client, isvc *v1beta1.InferenceService) (*apis.URL, error) {
 	foundRoute := false
 	routeReady := false
@@ -372,7 +373,7 @@ func (r *RawIngressReconciler) Reconcile(isvc *v1beta1.InferenceService) error {
 	}
 	internalHost := getRawServiceHost(isvc, r.client)
 	if authEnabled {
-		internalHost += ":8443"
+		internalHost += ":" + strconv.Itoa(constants.OauthProxyPort)
 	}
 	isvc.Status.Address = &duckv1.Addressable{
 		URL: &apis.URL{

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

@@ -110,7 +110,7 @@ func createDefaultSvc(componentMeta metav1.ObjectMeta, componentExt *v1beta1.Com
 				},
 				Protocol: container.Ports[0].Protocol,
 			}
-			if servicePort.Name == "" {
+			if len(servicePort.Name) == 0 {
 				servicePort.Name = "http"
 			}
 			servicePorts = append(servicePorts, servicePort)

pkg/controller/v1beta1/inferenceservice/utils/utils.go

@@ -444,6 +444,8 @@ func MergeServingRuntimeAndInferenceServiceSpecs(srContainers []v1.Container, is
 	return containerIndexInSR, mergedContainer, mergedPodSpec, nil
 }
 
+// Since the "cookie-secret" arg in the oauth proxy container is generated randomly,
+// we exclude that arg while comparing existing and desired deployment specs
 func RemoveCookieSecretArg(deployment appsv1.Deployment) *appsv1.Deployment {
 	dep := deployment.DeepCopy()
 	for i, container := range dep.Spec.Template.Spec.Containers {