RHOAIENG-4873: Kserve self signed certs

[jackdelahunt]

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

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

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:

• 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

[grdryn]

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

[jackdelahunt]

@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]

@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:

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. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

@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:

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

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

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:

• 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. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[grdryn]

• I think we should prefer the term TLS cert rather than SSL cert.
• 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?

[jackdelahunt]

• 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]

• 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 #241 (comment)
). 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]

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 #241 (comment)
). 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.

[grdryn]

/approve with one little inline comment.

@LaVLaS I think you've tested this, so I'll leave it to you to override the test job and give the lgtm

[grdryn]

Suggested change

- `S3_SELF_SIGNED_CERT` - Self signed cert to be used for S3 when pulling models

^ this looks like a mistake - it's already listed on line 41, and doesn't seem to make sense here inside this code block

[jackdelahunt]

Created a small follow up #248

[LaVLaS]

/approve

Everything work as intended (success & expected failure) when a cert is required. Additionally, existing works flow where a cert is not required and omitted worked as inteded

[openshift-ci]

[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 [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]

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]

/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]

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

In response to this:

/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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.