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

[harshad16]

Description

Set certs on both creation and update of notebooks
Fixes: https://issues.redhat.com/browse/RHOAIENG-7327

How Has This Been Tested?

For testing, the setup would use the RHOAI operator instance:

• Create a workbench instance in 2.9+ RHOAI version.

• Once the workbench is created check the details
  

• after that set the trustedCABundle setup inside the DSCI instance

spec:
  trustedCABundle:
      customCABundle: ''
      managementState: Managed

• Once the setup is set, update the DSC instance.

spec:
  components:
       workbenches:
          devFlags:
            manifests:
              - contextDir: components/odh-notebook-controller/config
                sourcePath: ''
                uri: 'https://github.com/opendatahub-io/kubeflow/tarball/pull/373/head'
              - contextDir: components/notebook-controller/config
                sourcePath: ''
                uri: 'https://github.com/opendatahub-io/kubeflow/tarball/pull/373/head'
              - contextDir: ''
                sourcePath: base
                uri: 'https://github.com/opendatahub-io/notebooks/tarball/v1.21.0'
          managementState: Managed

• After the setup, Toggle the workbench with changes, either with Notebook CR change or restart(toggle start/stop)
• Notice the change:
  

Merge criteria:

• The commits are squashed in a cohesive manner and have meaningful messages.
• Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has manually tested the changes and verified that the changes work

[harshad16]

/hold

Working in progress

[jstourac]

I haven't tested this in any way yet, but LGTM in general.

I'm trying to recall what were the reasons we didn't do it this way from the start? 🤔
Also, do we have any tracking issue for this? --- update: https://issues.redhat.com/browse/RHOAIENG-7327

[jstourac]

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?

[jiridanek]

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)

[shalberd]

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

[harshad16]

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.

[harshad16]

/unhold

[harshad16]

This is ready for review.

I'm trying to recall what were the reasons we didn't do it this way from the start?

The reason was, initially, as we started with this
#252,
the focus was to only include the certs of the RHOAI global certs.
And we didn't want to disrupt any long running notebook, if the cluster was not set with global certs.
as self-signed certs were not included in logic, only global certs.
with that case, it could have caused issue for long running notebooks.

Later, when we did the combination of both self-signed and global.
#270
with that, now we can implement this change for long running notebook as well.
as this now cover all grounds and would disrupt any flow.

[jiridanek]

It's missing tests, as specified on the Jira ticket; do you want to create new follow-up ticket to add the tests later?

[atheo89]

/lgtm

[harshad16]

It's missing tests, as specified on the Jira ticket; do you want to create new follow-up ticket to add the tests later?

Totally missed it, and missed updating this PR.
the new test is included, also updated the older test with some corrections.

result:

[jiridanek]

few suggestions about the test documentation strings

[harshad16]

Ready of review again

[jiridanek]

/lgtm

[harshad16]

/approve

Thanks for the review.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [harshad16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[harshad16]

/cherrypick stable

[openshift-cherrypick-robot]

@harshad16: #373 failed to apply on top of branch "stable":

Applying: Set certs on both creation and update of notebooks
Using index info to reconstruct a base tree...
M	components/odh-notebook-controller/controllers/notebook_webhook.go
Falling back to patching base and 3-way merge...
Auto-merging components/odh-notebook-controller/controllers/notebook_webhook.go
Applying: Included Test case, testing certs update on update of notebook
Using index info to reconstruct a base tree...
M	components/odh-notebook-controller/controllers/notebook_controller_test.go
Falling back to patching base and 3-way merge...
Auto-merging components/odh-notebook-controller/controllers/notebook_controller_test.go
CONFLICT (content): Merge conflict in components/odh-notebook-controller/controllers/notebook_controller_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Included Test case, testing certs update on update of notebook
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick stable

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.