-
Notifications
You must be signed in to change notification settings - Fork 128
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
Custom CA bundle for build-task #1025
Conversation
206f7e8
to
fd5eac2
Compare
/ok-to-test |
fd5eac2
to
6faaa72
Compare
Quality Gate passedIssues Measures |
- name: caTrustConfigMapName | ||
type: string | ||
description: The name of the ConfigMap to read CA bundle data from. | ||
default: trusted-ca | ||
- name: caTrustConfigMapKey |
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.
I'm conflicted about the consistency of these param names.
On the one hand, the buildah task uses UPPERCASE_NAMES, so this is inconsistent. On the other hand, we already dropped the ball in #942
(And in the context of the whole pipeline, the params should be consistent across all the tasks.)
For this PR, I don't really care what you choose. We really need to make the param casing across all tasks consistent. Breaking changes on that scale will be fun 🥲
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.
LGTM, could you just improve the commit message? Custom CA bundle for prefetch-dependencies for build-task
doesn't really make sense :)
I think we'd need to do something with the |
@yftacherzog maybe it can be done in a followup pr? |
ca_bundle=/mnt/trusted-ca/ca-bundle.crt | ||
if [ -f "$ca_bundle" ]; then | ||
echo "INFO: Using mounted CA bundle: $ca_bundle" | ||
cp -vf $ca_bundle /etc/pki/ca-trust/source/anchors |
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.
Perhaps we can symlink instead of copy to avoid creating another copy of the entire bundle in RAM.
For now we end up with 3 copies at least:
- The config map
- The file in
/etc/pki/ca-trust/source/anchors
- The resulting bundle file after running
update-ca-trust
Maybe we can even mount the config map directly into /etc/pki/ca-trust/source/anchors
?
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.
maybe, but we now that this approach works and I don't think the memory overhead is too big.
I think we should proceed with this PR for unblocking @ashwindasr
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.
How hard is it to test using ln -s
instead of cp
?
I'm ok with that, but does this change provide added functionality without the upload-sbom task? |
Good catch 👍
Now that you mention it, @ashwindasr @gbenhaim what exactly is the purpose of this change? Is it to let the buildah task pull images from / push images to registries with self-signed certs? Or is it to let any instructions in the Containerfile access any servers with self-signed certs? If it's the latter, I'm pretty sure this PR doesn't achieve it |
@yftacherzog, what @ashwindasr needs it to pull images from a registry with self signed certificates. |
c1d360e
to
84d5ecd
Compare
Thanks @yftacherzog
So do you think this PR would help us fix this issue, or do you think additional changes are required? |
As far as I understand, it should. |
/lgtm |
/approve |
@gbenhaim looks like you're not an approver? |
@chmeliik can you please approve the pr? |
The corresponding update to the buildah-oci-ta Task is in #1039 |
Replicates #942, but for build task
Slack convo