Skip to content
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

Use TLS when Cloning Taxonomy Tree if CACert provided #260

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

gmfrasca
Copy link
Member

TLS-enabled git operations for Taxonomy

Description

If a CA Cert is provided in the form of a 'taxonomy-ca.crt' in the teacher-server ConfigMap, use TLS and said CA Cert to clone and fetch the taxonomy tree.

How Has This Been Tested?

To test, using a git repo that is accessed using a self-signed CA cert:

  1. Import pipeline.yaml
  2. For now, ensure teacher-server ConfigMap does NOT have a key named taxonomy-ca.crt
  3. Run pipeline with default taxonomy repo pointer [Expected Result: Git Clone task should succeed]
  4. Run with the taxonomy_repo parameter pointed to a git repo that can only be accessed using a self-signed CA cert. [Expected Result: Fails to clone]
  5. Add the CA Certificate to teacher-server ConfigMap under the key taxonomy-ca.crt
  6. Repeat Step 3. [Expected Result: Git Clone task should succeed]
  7. Repeat Step 4. [Expected Result: Git Clone task should succeed]

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

@jgarciao
Copy link

jgarciao commented Jan 29, 2025

As the recommended way to add CA bundles in RHOAI is to add them in the DSCInitialization (in .spec.trustedCABundle.customCABundle) and those bundles are already mounted in the pod running the task, I think the ilab pipeline should follow the same pattern and not use a different configmap to store the certificates

Update: discussed on slack: there are other pipeline steps that are also using the same mechanism. For this reason, I think it's fine the approach proposed by this PR for consistency.

)

if use_tls:
full_ca_path = os.path.abspath(f"{taxonomy_ca_cert_path}")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional:

Suggested change
full_ca_path = os.path.abspath(f"{taxonomy_ca_cert_path}")
full_ca_path = os.path.abspath(taxonomy_ca_cert_path)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That variable is actually not a string - i believe its an InputParameterChannel or some other Pipelines construct, which abspath doesnt accept as a string-like type. Because of that, we need to expand it, as casting this specific type if it is None actually expands to the string "None" which would incorrectly pass this check

@gmfrasca
Copy link
Member Author

/hold - container_component does not behave similarly to dsl.components when reading pod environments, need to update implementation

@gmfrasca gmfrasca force-pushed the taxtls branch 2 times, most recently from d076d30 to 1246868 Compare January 29, 2025 21:04
- Mount teacher-server ConfigMap as volume to git clone task
- Provide mount path as env var to git clone task
- Check if 'taxonomy-ca.crt' exists in mounted vol
- Use TLS if cert exists, otherwise use standard git operations

Signed-off-by: Giulio Frasca <[email protected]>
@gmfrasca
Copy link
Member Author

/unhold

@HumairAK
Copy link
Contributor

cc @tumido / @Shreyanand / @leseb / @MichaelClifford / et al

@HumairAK HumairAK merged commit 859cb59 into opendatahub-io:main Jan 30, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants