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.

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

Add logic on nbc-webhook to pickup internal or external registry #329

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
115 changes: 115 additions & 0 deletions components/odh-notebook-controller/controllers/notebook_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"encoding/json"
"fmt"
"net/http"
"sort"
"strings"

"github.com/go-logr/logr"
nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1"
Expand All @@ -28,7 +30,10 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/rest"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
Expand All @@ -40,6 +45,7 @@ import (
type NotebookWebhook struct {
Log logr.Logger
Client client.Client
Config *rest.Config
Decoder *admission.Decoder
OAuthConfig OAuthConfig
}
Expand Down Expand Up @@ -246,6 +252,12 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm
if err != nil {
return admission.Errored(http.StatusInternalServerError, err)
}

// Check Imagestream Info
err = SetContainerImageFromRegistry(ctx, w.Config, notebook, log)
if err != nil {
return admission.Errored(http.StatusInternalServerError, err)
}
}

// Inject the OAuth proxy if the annotation is present but only if Service Mesh is disabled
Expand Down Expand Up @@ -438,3 +450,106 @@ func InjectCertConfig(notebook *nbv1.Notebook, configMapName string) error {
}
return nil
}

// SetContainerImageFromRegistry checks if there is an internal registry and takes the corresponding actions to set the container.image value.
// If an internal registry is detected, it uses the default values specified in the Notebook Custom Resource (CR).
// Otherwise, it checks the last-image-selection annotation to find the image stream and fetches the image from status.dockerImageReference,
// assigning it to the container.image value.
func SetContainerImageFromRegistry(ctx context.Context, config *rest.Config, notebook *nbv1.Notebook, log logr.Logger) error {
// Create a dynamic client
dynamicClient, err := dynamic.NewForConfig(config)
if err != nil {
log.Error(err, "Error creating dynamic client")
return err
}
// Specify the GroupVersionResource for imagestreams
ims := schema.GroupVersionResource{
Group: "image.openshift.io",
Version: "v1",
Resource: "imagestreams",
}

annotations := notebook.GetAnnotations()
if annotations != nil {
if imageSelection, exists := annotations["notebooks.opendatahub.io/last-image-selection"]; exists {
// Check if the image selection has an internal registry, if so will pickup this. This value constructed on the initialization of the Notebook CR.
if strings.Contains(notebook.Spec.Template.Spec.Containers[0].Image, "image-registry.openshift-image-registry.svc:5000") {
Copy link
Member

@jstourac jstourac May 28, 2024

Choose a reason for hiding this comment

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

Are we sure the container we're interested in is first always in all cases? Wouldn't it be better to check based on the container name which should match name of the Notebook? The other container - oauth-proxy - contains direct quay link from the beginning.

Copy link

@shalberd shalberd May 29, 2024

Choose a reason for hiding this comment

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

it is that way the kind: Notebooks with their podspec are assembled by odh dashboard, i.e. that the notebook container is always the first one, index 0. But it is not guaranteed.
Here, also, as you can see in my comments elsewhere, the admission plugin works with fieldPath-based lookups of the container by container name, which is a good idea.

Copy link

@shalberd shalberd May 29, 2024

Choose a reason for hiding this comment

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

"The other container - oauth-proxy - contains direct quay link from the beginning."
Correct, that is done by odh notebook controller. It is part of an oauth-argument in the odh-notebook-controller-manager pod container and evaluated by odh notebook controller.

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 - are we sure that the value for the internal registry will always be this way? Is there a possibility that there could be some custom value?

Why don't we want to check presence of internal registry via ImageStream status.dockerImageRepository value as empty (no internal registry enabled) vs non-empty (internal registry present)? This seem to me as a more proper approach than to check with what the Notebook CR was created 🤔

Copy link

@shalberd shalberd May 29, 2024

Choose a reason for hiding this comment

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

agreed, that is what the image content change trigger admission plugin, which we are not using up to now (part of PR 800) is doing on the openshift side as well, openshift-internally via golang.

log.Info("Internal registry found. Will pickup the default value from image field.")
return nil
} else {
// Split the imageSelection to imagestream and tag
parts := strings.Split(imageSelection, ":")
if len(parts) != 2 {
log.Error(nil, "Invalid image selection format")
return fmt.Errorf("invalid image selection format")
}

imagestreamName := parts[0]
tag := parts[1]

// Specify the namespaces to search in
namespaces := []string{"opendatahub", "redhat-ods-applications"}

imagestreamFound := false

for _, namespace := range namespaces {
// List imagestreams in the specified namespace
imagestreams, err := dynamicClient.Resource(ims).Namespace(namespace).List(ctx, metav1.ListOptions{})
if err != nil {
log.Error(err, "Cannot list imagestreams", "namespace", namespace)
continue
}

// Iterate through the imagestreams to find matches
for _, item := range imagestreams.Items {
metadata := item.Object["metadata"].(map[string]interface{})
name := metadata["name"].(string)

if name == imagestreamName {
status := item.Object["status"].(map[string]interface{})

log.Info("No Internal registry found, pick up imageHash from status.tag.dockerImageReference")

tags := status["tags"].([]interface{})
for _, t := range tags {
tagMap := t.(map[string]interface{})
tagName := tagMap["tag"].(string)
if tagName == tag {
items := tagMap["items"].([]interface{})
if len(items) > 0 {
// Sort items by creationTimestamp to get the most recent one
sort.Slice(items, func(i, j int) bool {
iTime := items[i].(map[string]interface{})["created"].(string)
jTime := items[j].(map[string]interface{})["created"].(string)
return iTime > jTime // Lexicographical comparison of RFC3339 timestamps
})
imageHash := items[0].(map[string]interface{})["dockerImageReference"].(string)
notebook.Spec.Template.Spec.Containers[0].Image = imageHash
// Update the JUPYTER_IMAGE environment variable
Copy link

@shalberd shalberd May 28, 2024

Choose a reason for hiding this comment

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

Hi all, definitely, putting an sha256 style name in for env var JUPYTER_IMAGE, especially for an image (docker.from.name origin), not an imagestream, is not ok.
What I can say for sure is, that, during my work on PR-800, I found out that JUPYTER_IMAGE env var is used during commercial RHOIA / upstream CI testing
https://github.com/red-hat-data-services/ods-ci/blob/master/ods_ci/tests/Resources/Page/ODH/JupyterHub/JupyterHubSpawner.robot#L374
to determine imagestream name and tag.

Meaning I checked with someone back then and also with @VaishnaviHire that it is ok to newly put in imagestreamname:imagestreamtag into that env var, as I did in my changes to dashboard PR-800.

Copy link

@shalberd shalberd May 28, 2024

Choose a reason for hiding this comment

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

Also, as I mentioned before, ODH notebook controller is the wrong place to handle this sort of stuff.
Much better in my view to start declaratively with odh dashboard and the image change trigger annotation.
There are ways to handle long-running notebooks, avoid container restarts, even with that, but we have to put in a slider or checkmark in dashboard GUI to make notebook long-running, set annotation field to paused: true after initial startup.

for i, envVar := range notebook.Spec.Template.Spec.Containers[0].Env {
if envVar.Name == "JUPYTER_IMAGE" {
notebook.Spec.Template.Spec.Containers[0].Env[i].Value = imageHash
break
}
}
imagestreamFound = true
break
}
}
}
}
}
if imagestreamFound {
break
}
}

if !imagestreamFound {
log.Info("Imagestream not found in any of the specified namespaces", "imagestreamName", imagestreamName, "tag", tag)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice from us to provide some log/warning for the else branches in these 2 cases (ifs on lines 473 and 474).

}

return nil
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
/*

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
Expand Down Expand Up @@ -141,6 +140,7 @@ var _ = BeforeSuite(func() {
Handler: &NotebookWebhook{
Log: ctrl.Log.WithName("controllers").WithName("notebook-controller"),
Client: mgr.GetClient(),
Config: mgr.GetConfig(),
OAuthConfig: OAuthConfig{
ProxyImage: OAuthProxyImage,
},
Expand Down
1 change: 1 addition & 0 deletions components/odh-notebook-controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ func main() {
Handler: &controllers.NotebookWebhook{
Log: ctrl.Log.WithName("controllers").WithName("Notebook"),
Client: mgr.GetClient(),
Config: mgr.GetConfig(),
OAuthConfig: controllers.OAuthConfig{
ProxyImage: oauthProxyImage,
},
Expand Down