-
Notifications
You must be signed in to change notification settings - Fork 37
refactor(mlflow): use external gateway URL for tracking #294
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,8 +22,6 @@ package mlflow | |
| import ( | ||
| "bytes" | ||
| "context" | ||
| "crypto/tls" | ||
| "crypto/x509" | ||
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
|
|
@@ -40,10 +38,6 @@ const ( | |
| // DefaultTokenPath is the projected SA token path in a pod. | ||
| DefaultTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token" | ||
|
|
||
| // DefaultCACertPath is the service-serving CA certificate path. | ||
| // On OpenShift, the service-ca.crt is projected into the SA token volume. | ||
| DefaultCACertPath = "/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt" | ||
|
|
||
| // WorkspaceHeader is the MLflow workspace header (namespace-based isolation). | ||
| WorkspaceHeader = "X-MLFLOW-WORKSPACE" | ||
| ) | ||
|
|
@@ -56,10 +50,6 @@ type Client struct { | |
| // TokenPath is the path to the SA token file. Defaults to DefaultTokenPath. | ||
| TokenPath string | ||
|
|
||
| // CACertPath is the path to the CA certificate for TLS verification. | ||
| // Defaults to the in-cluster SA CA cert. | ||
| CACertPath string | ||
|
|
||
| // HTTPClient is the HTTP client to use. If nil, a default client with 30s timeout is used. | ||
| HTTPClient *http.Client | ||
|
|
||
|
|
@@ -111,32 +101,14 @@ func IsResourceAlreadyExists(err error) bool { | |
| func (c *Client) httpClient() *http.Client { | ||
| c.httpOnce.Do(func() { | ||
| if c.HTTPClient == nil { | ||
| tlsCfg := &tls.Config{MinVersion: tls.VersionTLS12} | ||
| if caCert, err := os.ReadFile(c.caCertPath()); err == nil { | ||
| pool, err := x509.SystemCertPool() | ||
| if err != nil { | ||
| // Fall back to an empty pool; the service-CA cert will still be appended. | ||
| pool = x509.NewCertPool() | ||
| } | ||
| pool.AppendCertsFromPEM(caCert) | ||
| tlsCfg.RootCAs = pool | ||
| } | ||
| c.HTTPClient = &http.Client{ | ||
| Timeout: 30 * time.Second, | ||
| Transport: &http.Transport{TLSClientConfig: tlsCfg}, | ||
| Timeout: 30 * time.Second, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: The default |
||
| } | ||
| } | ||
| }) | ||
| return c.HTTPClient | ||
| } | ||
|
|
||
| func (c *Client) caCertPath() string { | ||
| if c.CACertPath != "" { | ||
| return c.CACertPath | ||
| } | ||
| return DefaultCACertPath | ||
| } | ||
|
|
||
| func (c *Client) tokenPath() string { | ||
| if c.TokenPath != "" { | ||
| return c.TokenPath | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,13 +51,8 @@ type MLflow struct { | |
| } | ||
|
|
||
| type MLflowStatus struct { | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
| Address *MLflowAddressStatus `json:"address,omitempty"` | ||
| } | ||
|
|
||
| // MLflowAddressStatus holds the internal in-cluster endpoint for the MLflow Service. | ||
| type MLflowAddressStatus struct { | ||
| // URL is the in-cluster HTTPS URL for the managed MLflow Service. | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
| // URL is the external gateway URL for the MLflow server (e.g. via the RHOAI data-science gateway). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Since E2E tests pass, the upstream CRD likely already uses the flat |
||
| URL string `json:"url,omitempty"` | ||
| } | ||
|
|
||
|
|
@@ -95,9 +90,6 @@ func (in *MLflowStatus) DeepCopyInto(out *MLflowStatus) { | |
| in.Conditions[i].DeepCopyInto(&out.Conditions[i]) | ||
| } | ||
| } | ||
| if in.Address != nil { | ||
| out.Address = &MLflowAddressStatus{URL: in.Address.URL} | ||
| } | ||
| } | ||
|
|
||
| func (in *MLflowList) DeepCopyObject() runtime.Object { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Nice logic inversion — checking for presence first (
URL != "") is cleaner than the old guard-and-continue pattern.