-
Notifications
You must be signed in to change notification settings - Fork 999
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
feat: CA bundle mount options for storage initializer #3250
Conversation
Signed-off-by: Danilo Peixoto <danilopeixoto@outlook.com>
Signed-off-by: Danilo Peixoto <danilopeixoto@outlook.com>
Signed-off-by: Danilo Peixoto <danilopeixoto@outlook.com>
Signed-off-by: Danilo Peixoto <danilopeixoto@outlook.com>
Signed-off-by: Danilo Peixoto <danilopeixoto@outlook.com>
Signed-off-by: Danilo Peixoto <danilopeixoto@outlook.com>
Signed-off-by: Danilo Peixoto <danilopeixoto@outlook.com>
Signed-off-by: Danilo Peixoto <danilopeixoto@outlook.com>
Signed-off-by: Danilo Peixoto <danilopeixoto@outlook.com>
Signed-off-by: Danilo Peixoto <danilopeixoto@outlook.com>
Signed-off-by: Danilo Peixoto <danilopeixoto@outlook.com>
Signed-off-by: Dan Sun <dsun20@bloomberg.net>
Signed-off-by: jooho <jlee@redhat.com>
Signed-off-by: jooho <jlee@redhat.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jooho I left a few additional comments. I think nothing is a blocker. I'm happy to approve if you think it is all OK
...v1beta1/inferenceservice/reconcilers/cabundleconfigmap/cabundle_configmap_reconciler_test.go
Show resolved
Hide resolved
Signed-off-by: jooho <jlee@redhat.com>
970a190
to
dcf8975
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is now good for me.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: israel-hdez, Jooho, spolti The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@yuzisun any update on this ticket? |
kubectl get configmap cabundle -o yaml | ||
apiVersion: v1 | ||
data: | ||
cabundle.crt: XXXXX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Kubernetes uses key ca.crt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's right bit I tried to use a slightly different name so as not to overlap with anything else.
...ller/v1beta1/inferenceservice/reconcilers/cabundleconfigmap/cabundle_configmap_reconciler.go
Outdated
Show resolved
Hide resolved
|
||
# If verify_ssl is true, then check there is custom ca bundle cert | ||
if verify_ssl: | ||
global_ca_bundle_configmap = os.getenv("CA_BUNDLE_CONFIGMAP_NAME") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this env? seems like the presence of AWS_CA_BUNDLE
is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWS_CABUNDLE
represents the path to the CA bundle certificate, while CA_BUNDLE_CONFIGMAP_NAME
represents the configmap name to attach to the storage initialization container.
For backward compatibility reasons, I did not change the semantics of AWS_CABUNDLE
.
Signed-off-by: jooho <jlee@redhat.com>
Thanks for the awesome work!! please help create an issue on website repo to update the doc. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: israel-hdez, Jooho, spolti, yuzisun 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 |
doc issue : kserve/website#324 |
What this PR does / why we need it:
This PR is a collaboration between @Jooho and @danilopeixoto based on the work done by @danilopeixoto in PR #2906 and some additional fixes contributed by @Jooho
It addresses #2897
Related issues listed in #3171
This PR is for self signed certificate for storage-initializer and this feature provides 2 ways
If you set global configuration, the cabundle secret in a namespace where kserve controller is running will be copied to a user namespace in where ISVC object created.
However, if you want to set the cabundle secret for a specific infereceService, you can set an annotation 'serving.kserve.io/s3-cabundle-secret'
You can find the detail information from here
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Type of changes
Please delete options that are not relevant.
Feature/Issue validation/testing:
Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.
Notice
The test model runtime becomes CrashLoopBackOff after running status but it is ok. The most important thing from this test is whether of not to pull a model from SSL Minio pod. So please ignore when the run pod failed at the end.
Test cluster:
Test environment setup:
TEST
cabundle_configmap
in StorageSpecserving.kserve.io/s3-cabundle-configmap
in storage-configSpecial notes for your reviewer:
Checklist:
Release note: