search

opendatahub-io / ai-edge

ODH integration with AI at the Edge usecases
Apache License 2.0
8 stars 17 forks source link

RHOAIENG-4873: Kserve self signed certs #241

Closed jackdelahunt closed 4 months ago

jackdelahunt commented 4 months ago

Description

Add support for using self signed certs with kserve to download models from an object store

How Has This Been Tested?

Merge criteria:

grdryn commented 4 months ago

@jackdelahunt I know this is marked as Draft, but just wondering if it's actually needed? What's the overlap with #230?

jackdelahunt commented 4 months ago

@grdryn sorry I didn't update this much, this is for self signed certs with kserve. The other PR is just for git

openshift-ci-robot commented 4 months ago

@jackdelahunt: This pull request references RHOAIENG-4873 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/241): > > >## Description > > >## How Has This Been Tested? > > > > >## 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 > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=opendatahub-io%2Fai-edge). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci-robot commented 4 months ago

@jackdelahunt: This pull request references RHOAIENG-4873 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/241): >## Description >Add support for using self signed certs with kserve to download models from an object store > >## How Has This Been Tested? >- Use the e2e test provided in the repo >- Setup e2e tests as normal >```bash >make AWS_SECRET_ACCESS_KEY=... AWS_ACCESS_KEY_ID=... S3_REGION=... S3_ENDPOINT=... IMAGE_REGISTRY_USERNAME=... IMAGE_REGISTRY_PASSWORD=... S3_BUCKET=... NAMESPACE=... TARGET_IMAGE_TAGS_JSON=... go-test-setup >``` >- Set the new env variable `S3_SELF_SIGNED_CERT` as the path to the cert > >- And then run >```bash >make AWS_SECRET_ACCESS_KEY=... AWS_ACCESS_KEY_ID=... S3_REGION=... S3_ENDPOINT=... IMAGE_REGISTRY_USERNAME=... IMAGE_REGISTRY_PASSWORD=... S3_BUCKET=... NAMESPACE=... TARGET_IMAGE_TAGS_JSON=... S3_SELF_SIGNED_CERT=... go-test >``` > >## Merge criteria: > > > >- [x] The commits are squashed in a cohesive manner and have meaningful messages. >- [x] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious). >- [x] The developer has manually tested the changes and verified that the changes work > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=opendatahub-io%2Fai-edge). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
jackdelahunt commented 4 months ago
  • Where we say "self-signed cert", should we instead be saying something like "certificate authority"?

@grdryn So the configmaps that get mounted to the pipeline runs are called git-self-signed-cert and s3-self-signed-cert. What should these be renamed to? Just git-cert, git-CA-cert or git-signed-cert?

And then of course same thing goes for changing the naming of things with self signed in them

LaVLaS commented 4 months ago
  • I think we should prefer the term TLS cert rather than SSL cert.

While I agree TLS is the correct term there is potential confusion with underlying dependencies that still use the term SSL for their public confgs & apis (See https://github.com/opendatahub-io/ai-edge/pull/241#discussion_r1562663813 ). In documentation, we could enforce the use of SSL/TLS where both apply or just tls based features/configs that we own completely

Where we say "self-signed cert", should we instead be saying something like "certificate authority"? Just because a cert isn't trusted by default, doesn't mean that it's "self-signed", does it? It could be signed by a centralized CA owned by the users organisation, and I guess that's what they'd need to import here in that case, is it?

self-signed cert is relative to whether it's issuer has been included in the trusted Certificate Authority bundle for the environment. Your example is correct and we are generalizing all of these "not trusted by default certificates" under the term self-signed cert. In a perfect environment, this PR only adds support for working with S3 stores in non-production environments

grdryn commented 4 months ago

While I agree TLS is the correct term there is potential confusion with underlying dependencies that still use the term SSL for their public confgs & apis (See https://github.com/opendatahub-io/ai-edge/pull/241#discussion_r1562663813 ). In documentation, we could enforce the use of SSL/TLS where both apply or just tls based features/configs that we own completely

I definitely haven't thought about this as much as you have (and don't necessarily fully understand all of the implications). My suggestion was mainly based on having a quick scan at other projects documentation, and how some appear to prefer "TLS certificate" rather than "SSL certificate" or SSL/TLS certificate". I don't have a strong enough opinion, so fine with it being ssl.

openshift-ci[bot] commented 4 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grdryn, LaVLaS

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](https://github.com/opendatahub-io/ai-edge/blob/main/OWNERS)~~ [LaVLaS,grdryn] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
LaVLaS commented 4 months ago

I definitely haven't thought about this as much as you have (and don't necessarily fully understand all of the implications). My suggestion was mainly based on having a quick scan at other projects documentation, and how some appear to prefer "TLS certificate" rather than "SSL certificate" or SSL/TLS certificate". I don't have a strong enough opinion, so fine with it being ssl.

You are correct on the use of TLS over SSL. I have no object on using the TLS or at minimum SSL/TLS in all documentation. The use in the pipeline was just parity with its use in the git-clone task and kserve/storage library

LaVLaS commented 4 months ago

/override ci/prow/test-ai-edge /lgtm

Adding override due to openshift-ci downtime and offline tests were successful for each use case with/without certs and existing functionality was preserved

openshift-ci[bot] commented 4 months ago

@LaVLaS: Overrode contexts on behalf of LaVLaS: ci/prow/test-ai-edge

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/241#issuecomment-2052909520): >/override ci/prow/test-ai-edge >/lgtm > >Adding override due to openshift-ci downtime and offline tests were successful for each use case with/without certs and existing functionality was preserved Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.