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

setup configmap with certs and mount them in required workbenches #270

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

harshad16
Copy link
Member

@harshad16 harshad16 commented Mar 11, 2024

setup configmap with certs and mount them in required workbenches

Description

The current approach is to mount all the odh-trusted certs to the workbench on custom location /etc/pki/tls/certs/custom-* , this path is not recognized directly by pods, either the pods needs to be run with
update-ca-trust which is only possible in root mode.

based on that, this is new approach, which notices the odh-trusted-ca-bundle configmap and also self-signed certs available on the kube-root-ca.cert confimap, combines these two into a new configmap and mounts it into in notebook
which satisfies the pattern that both odh-trusted-ca-bundle and workbench-ca-bundle exists.

The concatenation of the ca-bundle provided in odh-trusted-ca-bundle into a new configmap is inspired by the work done on data-science-pipeline-operator: opendatahub-io/data-science-pipelines-operator#575

The edges-case, yet to work on:

  1. what happens if the odh-trusted-ca-bundle is deleted?
  2. what happens if the data of odh-trusted-ca-bundle is empty?

How Has This Been Tested?

  1. setup a 2.8.0 operator.
  2. include the odh-notebook-controller with this pr images
  3. test out all the scenarios.
  • on setup of odh 2.8 + operator, NBC should be creating a workbench configmap : workbench-trusted-ca-bundle
  • creation of new workbench should include the env var and volume mount
    • these workbenches should be able to content to URL, submit pipeline, connect db without SSL errors.
  • deletion of the configmap: should reconcile the configmap.
  • deletion of both configmap odh-trusted-ca-bundle and workbench-trusted-ca-bundle should reconcile the notebook which have the volume mount attached.
  • whole setup should disrupt any long-running notebook.

These were scenarios tested on.

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

@openshift-ci openshift-ci bot requested review from LaVLaS and VaishnaviHire March 11, 2024 02:37
@harshad16 harshad16 force-pushed the ssl-path-inclusion branch from 00db350 to 6b587a1 Compare March 11, 2024 04:42
@harshad16 harshad16 requested review from rkpattnaik780 and atheo89 and removed request for LaVLaS and VaishnaviHire March 11, 2024 18:57
@openshift-merge-robot openshift-merge-robot added the needs-rebase The PR needs a rebase or there are conflicts label Mar 12, 2024
Copy link

@rkpattnaik780 rkpattnaik780 left a comment

Choose a reason for hiding this comment

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

LGTM. Verified the scenarios listed in the PR description.

Copy link
Member

@atheo89 atheo89 left a comment

Choose a reason for hiding this comment

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

Great work!! 👍
Tested locally and it works as expected
/lgtm

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
…gmap

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
@harshad16 harshad16 force-pushed the ssl-path-inclusion branch from 32b944a to 66533d2 Compare March 14, 2024 06:27
@openshift-ci openshift-ci bot removed the lgtm label Mar 14, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase The PR needs a rebase or there are conflicts label Mar 14, 2024
@harshad16
Copy link
Member Author

Rebased.
@atheo89 and @rkpattnaik780 can you please add lgtm again.

Copy link

@rkpattnaik780 rkpattnaik780 left a comment

Choose a reason for hiding this comment

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

/lgtm

Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
Co-authored-by: Ramakrishna Pattnaik <rkpattnaik780@gmail.com>
@harshad16 harshad16 force-pushed the ssl-path-inclusion branch from c6d6c0f to f44055f Compare March 14, 2024 13:30
Copy link

@rkpattnaik780 rkpattnaik780 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 14, 2024
Copy link

openshift-ci bot commented Mar 14, 2024

@harshad16: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/odh-notebook-controller-e2e f44055f link true /test odh-notebook-controller-e2e

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

@harshad16
Copy link
Member Author

/approve

Merging to get this in release.

Copy link

openshift-ci bot commented Mar 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16, rkpattnaik780

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:

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

@harshad16 harshad16 merged commit 7cef808 into opendatahub-io:v1.7-branch Mar 14, 2024
4 of 6 checks passed
@harshad16
Copy link
Member Author

/cherrypick stable

@openshift-cherrypick-robot

@harshad16: new pull request created: #279

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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants