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

Jump to bottom

RHOAIENG-7327: Set certs on both creation and update of notebooks #373

Merged
merged 2 commits into from
Aug 26, 2024
Merged

RHOAIENG-7327: Set certs on both creation and update of notebooks #373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7fd72c7
Set certs on both creation and update of notebooks
harshad16 Jul 30, 2024
35263e2
Included Test case, testing certs update on update of notebook
harshad16 Aug 21, 2024
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
146 changes: 108 additions & 38 deletions components/odh-notebook-controller/controllers/notebook_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,31 +159,35 @@ var _ = Describe("The Openshift Notebook controller", func() {
}, duration, interval).Should(HaveOccurred())
})

It("Should mount a trusted-ca if exists on the given namespace", func() {
})

// New test case for notebook creation
When("Creating a Notebook, test certificate is mounted", func() {
const (
Name = "test-notebook"
Namespace = "default"
)

It("Should mount a trusted-ca when it exists on the given namespace", func() {
ctx := context.Background()
logger := logr.Discard()

By("By simulating the existence of odh-trusted-ca-bundle ConfigMap")
// Create a ConfigMap similar to odh-trusted-ca-bundle for simulation
workbenchTrustedCACertBundle := "workbench-trusted-ca-bundle"
trustedCACertBundle := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "odh-trusted-ca-bundle",
Namespace: "default",
Labels: map[string]string{
"config.openshift.io/inject-trusted-cabundle": "true",
},
trustedCACertBundle := createOAuthConfigmap(
"odh-trusted-ca-bundle",
"default",
map[string]string{
"config.openshift.io/inject-trusted-cabundle": "true",
},
Data: map[string]string{
"ca-bundle.crt": "-----BEGIN CERTIFICATE-----\n<base64-encoded-cert-data>\n-----END CERTIFICATE-----",
"odh-ca-bundle.crt": "-----BEGIN CERTIFICATE-----\n<base64-encoded-cert-data>\n-----END CERTIFICATE-----",
},
}
map[string]string{
"ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIIB8jCCAXigAwIBAgITBmyf18G7EEwpQ+Vxe3ssyBrBDjAKBggqhkjOPQQDAzA5MQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRkwFwYDVQQDExBBbWF6b24gUm9vdCBDQSA0MB4XDTE1MDUyNjAwMDAwMFoXDTQwMDUyNjAwMDAwMFowOTELMAkGA1UEBhMCVVMxDzANBgNVBAoTBkFtYXpvbjEZMBcGA1UEAxMQQW1hem9uIFJvb3QgQ0EgNDB2MBAGByqGSM49AgEGBSuBBAAiA2IABNKrijdPo1MN/sGKe0uoe0ZLY7Bi9i0b2whxIdIA6GO9mif78DluXeo9pcmBqqNbIJhFXRbb/egQbeOc4OO9X4Ri83BkM6DLJC9wuoihKqB1+IGuYgbEgds5bimwHvouXKNCMEAwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAYYwHQYDVR0OBBYEFNPsxzplbszh2naaVvuc84ZtV+WBMAoGCCqGSM49BAMDA2gAMGUCMDqLIfG9fhGt0O9Yli/W651+kI0rz2ZVwyzjKKlwCkcO8DdZEv8tmZQoTipPNU0zWgIxAOp1AE47xDqUEpHJWEadIRNyp4iciuRMStuW1KyLa2tJElMzrdfkviT8tQp21KW8EA==\n-----END CERTIFICATE-----",
"odh-ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIIB8jCCAXigAwIBAgITBmyf18G7EEwpQ+Vxe3ssyBrBDjAKBggqhkjOPQQDAzA5MQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRkwFwYDVQQDExBBbWF6b24gUm9vdCBDQSA0MB4XDTE1MDUyNjAwMDAwMFoXDTQwMDUyNjAwMDAwMFowOTELMAkGA1UEBhMCVVMxDzANBgNVBAoTBkFtYXpvbjEZMBcGA1UEAxMQQW1hem9uIFJvb3QgQ0EgNDB2MBAGByqGSM49AgEGBSuBBAAiA2IABNKrijdPo1MN/sGKe0uoe0ZLY7Bi9i0b2whxIdIA6GO9mif78DluXeo9pcmBqqNbIJhFXRbb/egQbeOc4OO9X4Ri83BkM6DLJC9wuoihKqB1+IGuYgbEgds5bimwHvouXKNCMEAwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAYYwHQYDVR0OBBYEFNPsxzplbszh2naaVvuc84ZtV+WBMAoGCCqGSM49BAMDA2gAMGUCMDqLIfG9fhGt0O9Yli/W651+kI0rz2ZVwyzjKKlwCkcO8DdZEv8tmZQoTipPNU0zWgIxAOp1AE47xDqUEpHJWEadIRNyp4iciuRMStuW1KyLa2tJElMzrdfkviT8tQp21KW8EA==\n-----END CERTIFICATE-----",
})

// Create the ConfigMap
if err := cli.Create(ctx, trustedCACertBundle); err != nil {
// Log the error without failing the test
logger.Info("Error occurred during creation of ConfigMap: %v", err)
}
Expect(cli.Create(ctx, trustedCACertBundle)).Should(Succeed())
defer func() {
// Clean up the ConfigMap after the test
if err := cli.Delete(ctx, trustedCACertBundle); err != nil {
Expand All @@ -192,14 +196,12 @@ var _ = Describe("The Openshift Notebook controller", func() {
}
}()

By("By checking and mounting the trusted-ca bundle")
// Invoke the function to mount the CA certificate bundle
err := CheckAndMountCACertBundle(ctx, cli, notebook, logger)
if err != nil {
// Log the error without failing the test
logger.Info("Error occurred during mounting CA certificate bundle: %v", err)
}
By("By creating a new Notebook")
notebook := createNotebook(Name, Namespace)
Expect(cli.Create(ctx, notebook)).Should(Succeed())
time.Sleep(interval)

By("By checking that trusted-ca bundle is mounted")
// Assert that the volume mount and volume are added correctly
volumeMountPath := "/etc/pki/tls/custom-certs/ca-bundle.crt"
expectedVolumeMount := corev1.VolumeMount{
Expand All @@ -208,13 +210,8 @@ var _ = Describe("The Openshift Notebook controller", func() {
SubPath: "ca-bundle.crt",
ReadOnly: true,
}
if len(notebook.Spec.Template.Spec.Containers[0].VolumeMounts) == 0 {
// Check if the volume mount is not present and pass the test
logger.Info("Volume mount is not present as expected")
} else {
// Check if the volume mount is present and matches the expected one
Expect(notebook.Spec.Template.Spec.Containers[0].VolumeMounts).To(ContainElement(expectedVolumeMount))
}
// Check if the volume mount is present and matches the expected one
Expect(notebook.Spec.Template.Spec.Containers[0].VolumeMounts).To(ContainElement(expectedVolumeMount))

expectedVolume := corev1.Volume{
Name: "trusted-ca",
Expand All @@ -231,13 +228,8 @@ var _ = Describe("The Openshift Notebook controller", func() {
},
},
}
if len(notebook.Spec.Template.Spec.Volumes) == 0 {
// Check if the volume is not present and pass the test
logger.Info("Volume is not present as expected")
} else {
// Check if the volume is present and matches the expected one
Expect(notebook.Spec.Template.Spec.Volumes).To(ContainElement(expectedVolume))
}
// Check if the volume is present and matches the expected one
Expect(notebook.Spec.Template.Spec.Volumes).To(ContainElement(expectedVolume))
})

})
Expand Down Expand Up @@ -273,6 +265,71 @@ var _ = Describe("The Openshift Notebook controller", func() {
return notebook.Spec.Template.Spec.Containers[0].Image
}, duration, interval).Should(Equal(updatedImage))
})

It("When notebook CR is updated, should mount a trusted-ca if it exists on the given namespace", func() {
ctx := context.Background()
logger := logr.Discard()

By("By simulating the existence of odh-trusted-ca-bundle ConfigMap")
// Create a ConfigMap similar to odh-trusted-ca-bundle for simulation
workbenchTrustedCACertBundle := "workbench-trusted-ca-bundle"
trustedCACertBundle := createOAuthConfigmap(
"odh-trusted-ca-bundle",
"default",
map[string]string{
"config.openshift.io/inject-trusted-cabundle": "true",
},
map[string]string{
"ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIIB8jCCAXigAwIBAgITBmyf18G7EEwpQ+Vxe3ssyBrBDjAKBggqhkjOPQQDAzA5MQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRkwFwYDVQQDExBBbWF6b24gUm9vdCBDQSA0MB4XDTE1MDUyNjAwMDAwMFoXDTQwMDUyNjAwMDAwMFowOTELMAkGA1UEBhMCVVMxDzANBgNVBAoTBkFtYXpvbjEZMBcGA1UEAxMQQW1hem9uIFJvb3QgQ0EgNDB2MBAGByqGSM49AgEGBSuBBAAiA2IABNKrijdPo1MN/sGKe0uoe0ZLY7Bi9i0b2whxIdIA6GO9mif78DluXeo9pcmBqqNbIJhFXRbb/egQbeOc4OO9X4Ri83BkM6DLJC9wuoihKqB1+IGuYgbEgds5bimwHvouXKNCMEAwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAYYwHQYDVR0OBBYEFNPsxzplbszh2naaVvuc84ZtV+WBMAoGCCqGSM49BAMDA2gAMGUCMDqLIfG9fhGt0O9Yli/W651+kI0rz2ZVwyzjKKlwCkcO8DdZEv8tmZQoTipPNU0zWgIxAOp1AE47xDqUEpHJWEadIRNyp4iciuRMStuW1KyLa2tJElMzrdfkviT8tQp21KW8EA==\n-----END CERTIFICATE-----",
"odh-ca-bundle.crt": "-----BEGIN CERTIFICATE-----\nMIIB8jCCAXigAwIBAgITBmyf18G7EEwpQ+Vxe3ssyBrBDjAKBggqhkjOPQQDAzA5MQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRkwFwYDVQQDExBBbWF6b24gUm9vdCBDQSA0MB4XDTE1MDUyNjAwMDAwMFoXDTQwMDUyNjAwMDAwMFowOTELMAkGA1UEBhMCVVMxDzANBgNVBAoTBkFtYXpvbjEZMBcGA1UEAxMQQW1hem9uIFJvb3QgQ0EgNDB2MBAGByqGSM49AgEGBSuBBAAiA2IABNKrijdPo1MN/sGKe0uoe0ZLY7Bi9i0b2whxIdIA6GO9mif78DluXeo9pcmBqqNbIJhFXRbb/egQbeOc4OO9X4Ri83BkM6DLJC9wuoihKqB1+IGuYgbEgds5bimwHvouXKNCMEAwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAYYwHQYDVR0OBBYEFNPsxzplbszh2naaVvuc84ZtV+WBMAoGCCqGSM49BAMDA2gAMGUCMDqLIfG9fhGt0O9Yli/W651+kI0rz2ZVwyzjKKlwCkcO8DdZEv8tmZQoTipPNU0zWgIxAOp1AE47xDqUEpHJWEadIRNyp4iciuRMStuW1KyLa2tJElMzrdfkviT8tQp21KW8EA==\n-----END CERTIFICATE-----",
})
// Create the ConfigMap
Expect(cli.Create(ctx, trustedCACertBundle)).Should(Succeed())
defer func() {
// Clean up the ConfigMap after the test
if err := cli.Delete(ctx, trustedCACertBundle); err != nil {
// Log the error without failing the test
logger.Info("Error occurred during deletion of ConfigMap: %v", err)
}
}()

By("By updating the Notebook's image")
key := types.NamespacedName{Name: Name, Namespace: Namespace}
Expect(cli.Get(ctx, key, notebook)).Should(Succeed())

updatedImage := "registry.redhat.io/ubi8/ubi:updated"
notebook.Spec.Template.Spec.Containers[0].Image = updatedImage
Expect(cli.Update(ctx, notebook)).Should(Succeed())
time.Sleep(interval)
harshad16 marked this conversation as resolved.
Show resolved Hide resolved

By("By checking that trusted-ca bundle is mounted")
// Assert that the volume mount and volume are added correctly
volumeMountPath := "/etc/pki/tls/custom-certs/ca-bundle.crt"
expectedVolumeMount := corev1.VolumeMount{
Name: "trusted-ca",
MountPath: volumeMountPath,
SubPath: "ca-bundle.crt",
ReadOnly: true,
}
Expect(notebook.Spec.Template.Spec.Containers[0].VolumeMounts).To(ContainElement(expectedVolumeMount))

expectedVolume := corev1.Volume{
Name: "trusted-ca",
VolumeSource: corev1.VolumeSource{
ConfigMap: &corev1.ConfigMapVolumeSource{
LocalObjectReference: corev1.LocalObjectReference{Name: workbenchTrustedCACertBundle},
Optional: pointer.Bool(true),
Items: []corev1.KeyToPath{
{
Key: "ca-bundle.crt",
Path: "ca-bundle.crt",
},
},
},
},
}
Expect(notebook.Spec.Template.Spec.Volumes).To(ContainElement(expectedVolume))
})
})

When("Creating a Notebook, test Networkpolicies", func() {
Expand Down Expand Up @@ -969,3 +1026,16 @@ func createOAuthNetworkPolicy(name, namespace string, npProtocol corev1.Protocol
},
}
}

// createOAuthConfigmap creates a ConfigMap
// this function can be used to create any kinda of ConfigMap
func createOAuthConfigmap(name, namespace string, label map[string]string, configMapData map[string]string) *corev1.ConfigMap {
return &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
Labels: label,
},
Data: configMapData,
}
}
11 changes: 6 additions & 5 deletions components/odh-notebook-controller/controllers/notebook_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,6 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm
return admission.Errored(http.StatusInternalServerError, err)
}

// Only Mount ca bundle on new notebook creation
err = CheckAndMountCACertBundle(ctx, w.Client, notebook, log)
if err != nil {
return admission.Errored(http.StatusInternalServerError, err)
}
}

// Check Imagestream Info both on create and update operations
harshad16 marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -261,6 +256,12 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm
if err != nil {
return admission.Errored(http.StatusInternalServerError, err)
}

// Mount ca bundle on notebook creation and update
err = CheckAndMountCACertBundle(ctx, w.Client, notebook, log)
if err != nil {
return admission.Errored(http.StatusInternalServerError, err)
}
Copy link
Member

@jstourac jstourac Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My first impression of this is that this is triggered only when the Notebook CR is changed somehow. But the original issue is talking also about the plain workbench restart (https://issues.redhat.com/browse/RHOAIENG-7327) - so this change covers also this case?

Copy link
Member

@jiridanek jiridanek Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks ok to me, starting the notebook updates it, because starting/stopping (and apparently restarting!) is done through setting annotations on the CR

kubeflow/components/notebook-controller/controllers/notebook_controller.go

Line 410 in 5ad9c05

if metav1.HasAnnotation(instance.ObjectMeta, "kubeflow-resource-stopped") {

(I did not know the CR supports restarting, that is not exposed in the dashboard ui)

Copy link

Choose a reason for hiding this comment

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

Correct, currently, I have an ose-cli cronjob running every evening that scales down the statefulsets / stops the notebooks in all namespaces (cause users forget and we don't have long-running ones):

currentdatetimenotebook=$(date '+%Y-%m-%dT%H:%M:%SZ');
oc patch notebook $notebook -n "$ds_ns" --type="json" -p="[{\"op\": \"add\", \"path\": \"/metadata/annotations/kubeflow-resource-stopped\", \"value\":\"$currentdatetimenotebook\"}]";

Got that hint from dashboard workbench slider GUI ...

and yes, restarting is not part of dashboard logic currently on the part of notebooks, only start / stop i.e. omitting / deleting kubeflow-resource-stopped leads to start.

https://github.com/search?q=repo%3Aopendatahub-io%2Fodh-dashboard%20%22kubeflow-resource-stopped%22&type=code

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, correct the goal is to update certs, in both cases
Notebook CR change or Restart (which is toggle start/stop).
This would take care of all the scenarios.

}

// Inject the OAuth proxy if the annotation is present but only if Service Mesh is disabled
Expand Down
Loading