chore: Add ability to mount self-signed certs to kfp #10849
Conversation
DharmitD
force-pushed
the
mount-cert
branch
3 times, most recently
from
6058013
to
7037050
Compare
DharmitD
force-pushed
the
mount-cert
branch
2 times, most recently
from
59bc1fe
to
87ad759
Compare
| caBundleCfgMapName := os.Getenv("ARTIFACT_COPY_STEP_CABUNDLE_CONFIGMAP_NAME") | ||
| caBundleCfgMapKey := os.Getenv("ARTIFACT_COPY_STEP_CABUNDLE_CONFIGMAP_KEY") | ||
| caBundleMountPath := os.Getenv("ARTIFACT_COPY_STEP_CABUNDLE_MOUNTPATH") |
There was a problem hiding this comment.
We initially picked these based on the kfp-tekton implementation, but I think we can pick better env vars now that are more generic, I propose:
EXECUTOR_CABUNDLE_CONFIGMAP_NAME
EXECUTOR_CABUNDLE_CONFIGMAP_KEY
EXECUTOR_CABUNDLE_MOUNTPATH
since these are utilized in the launcher/executor
There was a problem hiding this comment.
Done, updated to have these new env vars.
|
@chensun from the
If these scripts run with no errors, can we consider approve? |
DharmitD
force-pushed
the
mount-cert
branch
3 times, most recently
from
a71963f
to
b65656f
Compare
This update allows CA bundles to be mounted to the launcher/executor pods since those make external connections to object store, which can be behind self signed certs. Detailed Changes: - Added `REQUESTS_CA_BUNDLE` to the environment variables. This is necessary because many Python-based libraries (e.g., requests) utilize this environment variable for SSL/TLS certificate verification. Notably, even though Boto3 is documented to use `AWS_CA_BUNDLE`, tests have shown that it only respects `REQUESTS_CA_BUNDLE`. Reference: https://requests.readthedocs.io/en/latest/user/advanced/#ssl-cert-verification and aws/aws-cli#3425. - Configured `AWS_CA_BUNDLE` for AWS CLI and related utilities to ensure AWS services utilize our custom CA bundle for SSL/TLS. - Set up `SSL_CERT_FILE` environment variable for OpenSSL's default certificate file. This setting is important as the `SSL_CERT_DIR` path adjustments had inconsistent results across different environments, as discussed in OpenSSL documentation: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_default_verify_paths.html Signed-off-by: ddalvi <ddalvi@redhat.com> Co-authored-by: Vani Haripriya <vmudadla@redhat.com> Co-authored-by: Humair Khan <HumairAK@users.noreply.github.com> Signed-off-by: ddalvi <ddalvi@redhat.com>
|
/hold |
DharmitD
force-pushed
the
mount-cert
branch
3 times, most recently
from
34945bb
to
76a1429
Compare
| defer func() { | ||
| // Clean up environment variables | ||
| os.Unsetenv("EXECUTOR_CABUNDLE_CONFIGMAP_NAME") | ||
| os.Unsetenv("EXECUTOR_CABUNDLE_CONFIGMAP_KEY") | ||
| os.Unsetenv("EXECUTOR_CABUNDLE_MOUNTPATH") | ||
| }() |
There was a problem hiding this comment.
you probably don't need to call this every iteration, and can just call it at the end of the function
There was a problem hiding this comment.
done, updated to have the defer func in the end of the function.
| } | ||
|
|
||
| // Call the function with a reference name | ||
| templateName := c.addContainerExecutorTemplate("test-ref") |
There was a problem hiding this comment.
this should be the container-impl template name "system-container-impl" not executor template name which is what the function returns
There was a problem hiding this comment.
Done, updated to have the container-impl template name.
DharmitD
force-pushed
the
mount-cert
branch
2 times, most recently
from
83d212e
to
4aaaaf2
Compare
|
/unhold |
|
Added a unit test to test the feature, unit test is passing: https://github.com/kubeflow/pipelines/actions/runs/9604504756/job/26490130091?pr=10849 |
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // Setup the environment variables for testing |
There was a problem hiding this comment.
I would remove all the comments in this testing function they are stating what is obvious in the code that follows them
// Setup the environment variables for testing
// Creating an instance of workflowCompiler with properly initialized members
// Call the function with a reference name
// Fetch the template and check existence
// Check Volumes
// Check VolumeMounts in the container
// Clean up environment variables
There was a problem hiding this comment.
Done, removed these comments.
Signed-off-by: ddalvi <ddalvi@redhat.com>
|
@DharmitD: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HumairAK, rimolive 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 |
Description of your changes:
Resolves #10848
This update allows CA bundles to be mounted to the launcher/executor pods since those make external connections to object store, which can be behind self signed certs.
users can already mount ca bundles to kfp pods themselves should they wish where applicable
This feature has been developed by @HumairAK and @VaniHaripriya, I'm merely proposing the PR and presenting it.
Checklist: