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

add helm config option to mount ca certs to cost model container #3760

Merged
merged 4 commits into from
Dec 4, 2024

Conversation

mittal-ishaan
Copy link
Contributor

@mittal-ishaan mittal-ishaan commented Dec 4, 2024

What does this PR change?

Adds help config option to mount custom cacerts config map to /etc/pki/ca-trust/source/anchors inside the cost model container.
reference
https://docs.fedoraproject.org/en-US/quick-docs/using-shared-system-certificates/#_adding_new_certificates
https://jfrog.com/help/r/artifactory-how-to-add-custom-ssl-certificates-to-an-artifactory-pod-using-helm-charts/general-steps-to-fix-the-ssl-certificate-error

The user will need to have a secret in the release namespace before using this configuration.
you might want to create a secret with a command like:

kubectl create secret generic ca-certs-secret --from-file=./cert-trust-test-ca.pem -n kubecost

Does this PR rely on any other PRs?

No

How does this PR impact users? (This is the kind of thing that goes in release notes!)

Users will now be able to add their custom ca certs to the cost model container

Links to Issues or tickets this PR addresses or fixes

https://kubecost.atlassian.net/browse/SUP-6432?focusedCommentId=136102

What risks are associated with merging this PR? What is required to fully test this PR?

No

How was this PR tested?

Added a ca cert secret and configured the added helm values. ca cert secret was mounted inside the cost-model container.
image

Have you made an update to documentation? If so, please provide the corresponding PR.

@mittal-ishaan mittal-ishaan marked this pull request as ready for review December 4, 2024 19:05
Copy link
Collaborator

@jessegoodier jessegoodier left a comment

Choose a reason for hiding this comment

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

Thank you!

@jessegoodier jessegoodier merged commit ff74071 into develop Dec 4, 2024
37 checks passed
@jessegoodier jessegoodier deleted the ca-cert-secret-config branch December 4, 2024 20:41
jessegoodier pushed a commit that referenced this pull request Dec 4, 2024
* add helm config option to mount ca certs to cost model container

* update it to be configmap

* shift from config map tp secret

* nit fix
@thomasvn
Copy link
Member

thomasvn commented Dec 4, 2024

@mittal-ishaan Nice work! Because we are not creating a k8s secret on behalf of the user, can you please document this PR description with what the k8s secret needs to look like for ca-certs-secret? Eventually we will need to fully document this as well.

@@ -615,6 +621,10 @@ spec:
mountPath: /var/configs/etl/federated
readOnly: true
{{- end }}
{{- if .Values.kubecostModel.caCertsSecret }}
- name: ca-certs-secret
mountPath: /etc/pki/ca-trust/source/anchors
Copy link
Member

Choose a reason for hiding this comment

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

This is a good start, but are we sure this should be hard-coded? This mountPath should probably be a helm config? I think that different OS's will look at different paths for its trust store. For example /etc/ssl/certs, or /etc/pki/tls/certs, or /usr/local/share/ca-certificates/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the information from the Fedora documentation and the user's request, for now, we only need to mount the certificate to /etc/pki/ca-trust/source/anchors. While other scenarios might require different mount paths in the future, this setup works well for our current needs. We can include it in the Helm config as the default for now but it might remain unused. thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's leave it hard-coded for now due to user-specific request. Let's make a task to update it in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure 👍

mittal-ishaan added a commit that referenced this pull request Dec 5, 2024
* add helm config option to mount ca certs to cost model container

* update it to be configmap

* shift from config map tp secret

* nit fix
@jessegoodier
Copy link
Collaborator

/cherry-pick v2.5

Copy link

Cherry-pick failed with Merge error ff74071167649a58a75d5878fe380770d0d0771a into temp-cherry-pick-f8c64d-v2.5

@jessegoodier
Copy link
Collaborator

CP #3770

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.

3 participants