elyra-ai / elyra

Elyra extends JupyterLab with an AI centric approach.
https://elyra.readthedocs.io/en/stable/
Apache License 2.0
1.86k stars 344 forks source link

Allow users to include ssl_sa_certs via env vars for kfp_tekton client #3150

Closed harshad16 closed 3 months ago

harshad16 commented 1 year ago

What changes were proposed in this pull request?

As the pipeline editor fails to connect to an unsecured pipeline server due to a certs issue, this PR includes an environment variable that can be pointed to the ssl_cert, which would be used for setting up the connection.

KF_PIPELINES_SSL_SA_CERTS can be set with cert path.

and ssl_ca_cert var would be set with the value and passed to kfp SDK.

Fixes: #3149

How was this pull request tested?

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.
lresende commented 1 year ago

This only allows for one global cert path, should we actually add a config on the runtime metadata to allow passing this on the UI per server?

kevin-bates commented 1 year ago

This only allows for one global cert path, should we actually add a config on the runtime metadata to allow passing this on the UI per server?

Is the idea that a site could target multiple KFP clusters (e.g., dev, test, and production) where the "valid" certs vary from target to target?

This makes sense, although I don't recall what the overhead is to extend runtime configurations with an optional property. Does this require UI work or is that portion dynamic?

harshad16 commented 1 year ago

This only allows for one global cert path, should we actually add a config on the runtime metadata to allow passing this on the UI per server?

Is the idea that a site could target multiple KFP clusters (e.g., dev, test, and production) where the "valid" certs vary from target to target?

This makes sense, although I don't recall what the overhead is to extend runtime configurations with an optional property. Does this require UI work or is that portion dynamic?

@lresende @kevin-bates thanks for the comments If we want to provide this option via runtime configurations then it would require UI change.

With this my idea, was the simply provide a way for user with unsecured cluster to utilize the feature of elyra pipeline , with a simple env var set. as i understand from the notes, the insecure cluster support for overall elyra feature is not in avenue of development for now.

shalberd commented 1 year ago

Nice work, I like this current proposed no-frills way

lresende commented 1 year ago

LGTM, could you also provide a little update to the docs mentioning the var so that other users can leverage it.

harshad16 commented 3 months ago

could you also provide a little update to the docs mentioning the var so that other users can leverage it.

@lresende , sorry totally missed this PR. As you instructed, I updated the docs with details. PTAL, at your convenience