Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RHOAIENG-16851] - Rawdeployment bug fixes #462

Merged
merged 4 commits into from
Jan 15, 2025
Merged

[RHOAIENG-16851] - Rawdeployment bug fixes #462

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c95dac
[RHOAIENG-16851] fix scheme bugs in status.url and status.address.url…
VedantMahabaleshwarkar Jan 8, 2025
ef6b4d6
[RHOAIENG-16851] Remove component url temporarily
VedantMahabaleshwarkar Jan 8, 2025
be3f5f3
[RHOAIENG-16851] Use transformer spec to set upstream port in oauth-p…
VedantMahabaleshwarkar Jan 13, 2025
3fc65e2
[RHOAIENG-16851] address feedback
VedantMahabaleshwarkar Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions pkg/apis/serving/v1beta1/inference_service_status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,9 +336,12 @@ func (ss *InferenceServiceStatus) PropagateRawStatus(
}

condition := getDeploymentCondition(deploymentList, appsv1.DeploymentAvailable)
if condition != nil && condition.Status == v1.ConditionTrue {
statusSpec.URL = url
}
// currently the component url is disabled as this url generated based on ingressConfig. This is incompatible with
// rawdeployment changes as the url depends on the route creation, if a route is requested.
// TODO: add back component url deterministicly
spolti marked this conversation as resolved.
Show resolved Hide resolved
// if condition != nil && condition.Status == v1.ConditionTrue {
// statusSpec.URL = url
//}
readyCondition := readyConditionsMap[component]
ss.SetCondition(readyCondition, condition)
ss.Components[component] = statusSpec
Expand Down
50 changes: 25 additions & 25 deletions pkg/controller/v1beta1/inferenceservice/rawkube_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,10 +425,10 @@ var _ = Describe("v1beta1 inference service controller", func() {
Components: map[v1beta1.ComponentType]v1beta1.ComponentStatusSpec{
v1beta1.PredictorComponent: {
LatestCreatedRevision: "",
URL: &apis.URL{
Scheme: "http",
Host: "raw-foo-predictor-default.example.com",
},
//URL: &apis.URL{
// Scheme: "http",
// Host: "raw-foo-predictor-default.example.com",
//},
},
},
ModelStatus: v1beta1.ModelStatus{
Expand Down Expand Up @@ -840,10 +840,10 @@ var _ = Describe("v1beta1 inference service controller", func() {
Components: map[v1beta1.ComponentType]v1beta1.ComponentStatusSpec{
v1beta1.PredictorComponent: {
LatestCreatedRevision: "",
URL: &apis.URL{
Scheme: "http",
Host: "raw-foo-customized-predictor-default.example.com",
},
//URL: &apis.URL{
// Scheme: "http",
// Host: "raw-foo-customized-predictor-default.example.com",
//},
},
},
ModelStatus: v1beta1.ModelStatus{
Expand Down Expand Up @@ -1246,10 +1246,10 @@ var _ = Describe("v1beta1 inference service controller", func() {
Components: map[v1beta1.ComponentType]v1beta1.ComponentStatusSpec{
v1beta1.PredictorComponent: {
LatestCreatedRevision: "",
URL: &apis.URL{
Scheme: "http",
Host: "raw-foo-2-predictor-default.example.com",
},
//URL: &apis.URL{
// Scheme: "http",
// Host: "raw-foo-2-predictor-default.example.com",
//},
},
},
ModelStatus: v1beta1.ModelStatus{
Expand Down Expand Up @@ -1723,10 +1723,10 @@ var _ = Describe("v1beta1 inference service controller", func() {
Components: map[v1beta1.ComponentType]v1beta1.ComponentStatusSpec{
v1beta1.PredictorComponent: {
LatestCreatedRevision: "",
URL: &apis.URL{
Scheme: "http",
Host: fmt.Sprintf("%s-predictor-default.example.com", serviceName),
},
//URL: &apis.URL{
// Scheme: "http",
// Host: fmt.Sprintf("%s-predictor-default.example.com", serviceName),
//},
},
},
ModelStatus: v1beta1.ModelStatus{
Expand Down Expand Up @@ -2156,10 +2156,10 @@ var _ = Describe("v1beta1 inference service controller", func() {
Components: map[v1beta1.ComponentType]v1beta1.ComponentStatusSpec{
v1beta1.PredictorComponent: {
LatestCreatedRevision: "",
URL: &apis.URL{
Scheme: "http",
Host: fmt.Sprintf("%s-predictor.%s.%s", serviceName, serviceKey.Namespace, domain),
},
//URL: &apis.URL{
// Scheme: "http",
// Host: fmt.Sprintf("%s-predictor.%s.%s", serviceName, serviceKey.Namespace, domain),
//},
},
},
ModelStatus: v1beta1.ModelStatus{
Expand Down Expand Up @@ -2652,17 +2652,17 @@ var _ = Describe("v1beta1 inference service controller", func() {
},
Address: &duckv1.Addressable{
URL: &apis.URL{
Scheme: "http",
Scheme: "https",
Host: fmt.Sprintf("%s-predictor.%s.svc.cluster.local:8443", serviceKey.Name, serviceKey.Namespace),
},
},
Components: map[v1beta1.ComponentType]v1beta1.ComponentStatusSpec{
v1beta1.PredictorComponent: {
LatestCreatedRevision: "",
URL: &apis.URL{
Scheme: "http",
Host: "raw-auth-predictor-default.example.com",
},
//URL: &apis.URL{
// Scheme: "http",
// Host: "raw-auth-predictor-default.example.com",
//},
},
},
ModelStatus: v1beta1.ModelStatus{
Expand Down
13 changes: 11 additions & 2 deletions pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,15 +269,24 @@ func createRawWorkerDeployment(componentMeta metav1.ObjectMeta,
}

func GetKServeContainerPort(podSpec *corev1.PodSpec) string {
var kserveContainerPort string

for _, container := range podSpec.Containers {
if container.Name == "kserve-container" {
if container.Name == "transformer-container" {
if len(container.Ports) > 0 {
return strconv.Itoa(int(container.Ports[0].ContainerPort))
}
}
if container.Name == "kserve-container" {
if len(container.Ports) > 0 {
kserveContainerPort = strconv.Itoa(int(container.Ports[0].ContainerPort))
spolti marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
return ""

return kserveContainerPort
}

func generateOauthProxyContainer(clientset kubernetes.Interface, isvc string, namespace string, upstreamPort string, sa string) (*corev1.Container, error) {
isvcConfigMap, err := clientset.CoreV1().ConfigMaps(constants.KServeNamespace).Get(context.TODO(), constants.InferenceServiceConfigMapName, metav1.GetOptions{})
if err != nil {
Expand Down
14 changes: 12 additions & 2 deletions pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/kube_ingress_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,15 @@ func createRawURL(client client.Client, isvc *v1beta1.InferenceService, authEnab
return nil, err
}
} else {
var scheme string
if authEnabled {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about it, why are you enforcing auth only with SSL?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing this, the code looks cleaner.

var scheme string
		url = &apis.URL{
			Host:   getRawServiceHost(isvc, client),
			Scheme: "http",
			Scheme: scheme,
			Path:   "",
		}
		if authEnabled {
			url.Scheme = "https"
		}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another question, as ssl is being enforced, how the certificates are configured?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

certs : https://github.com/opendatahub-io/kserve/blob/master/pkg/controller/v1beta1/inferenceservice/reconcilers/service/service_reconciler.go#L176

All reported ISVC status schemes are according to what is expected in each case according to #419 (comment)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, code change is included in latest commit

scheme = "https"
} else {
scheme = "http"
}
url = &apis.URL{
Host: getRawServiceHost(isvc, client),
Scheme: "http",
Scheme: scheme,
Path: "",
}
if authEnabled {
Expand Down Expand Up @@ -337,13 +343,17 @@ func (r *RawIngressReconciler) Reconcile(isvc *v1beta1.InferenceService) error {
return err
}
internalHost := getRawServiceHost(isvc, r.client)
var scheme string
if authEnabled {
internalHost += ":" + strconv.Itoa(constants.OauthProxyPort)
scheme = "https"
} else {
scheme = "http"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here:

        scheme = "http"
	if authEnabled {
		internalHost += ":" + strconv.Itoa(constants.OauthProxyPort)
		scheme = "https"
	}

considering, the logic you wrote is the correct behavior for both cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in latest commit

}
isvc.Status.Address = &duckv1.Addressable{
URL: &apis.URL{
Host: internalHost,
Scheme: r.ingressConfig.UrlScheme,
Scheme: scheme,
Path: "",
},
}
Expand Down
Loading