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 init container for updating ca trust and shift getting ca cert from secret to config map #3763

Merged
merged 18 commits into from
Dec 11, 2024

Conversation

mittal-ishaan
Copy link
Contributor

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

What does this PR change?

Does this PR rely on any other PRs?

#3760

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

Users will now not need to run update-ca-trust command inside the container. The init container will do it.

Links to Issues or tickets this PR addresses or fixes

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

How was this PR tested?

Deployed the kubecost deployment with caCertsSecret helm value set and ca cert as Secret created.
reading through /etc/pki/ca-trust/extracted/openssl/ca-bundle.trust.crt we see it updated with the given ca cert in Secret.

helm values:

global:
  updateCaTrust:
    enabled: false  # Set to true to enable the init container for updating CA trust
    # Security context settings for the init container.
    securityContext:
      runAsUser: 0
      runAsGroup: 0
      runAsNonRoot: false
      allowPrivilegeEscalation: false
      readOnlyRootFilesystem: true
      seccompProfile:
        type: RuntimeDefault
    caCertsSecret: ca-certs-secret  # The name of the Secret containing custom CA certificates to mount to the cost-model container.
    # caCertsConfig: ca-certs-config  # The name of the ConfigMap containing the CA trust configuration.
    resources: {}  # Resource requests and limits for the init container.
    caCertsMountPath: /etc/pki/ca-trust/source/anchors  # The path where the custom CA certificates will be mounted in the init container

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 5, 2024 20:33
@jessegoodier
Copy link
Collaborator

This is looking good to me. Tested with custom secret name.

Will ping customer and merge after we get feedback.

@chipzoller
Copy link
Collaborator

But why a secret? Root certificates aren't sensitive.

@mittal-ishaan
Copy link
Contributor Author

@chipzoller The user requested it to be a secret as they store/retrieve certs as secrets internally.

@chipzoller
Copy link
Collaborator

Then maybe make this so it accepts a Secret or ConfigMap?

@mittal-ishaan
Copy link
Contributor Author

sure, we should do that. let me get that in.

@thomasvn
Copy link
Member

Future TODO. This updates the CA trust and certificates for the cost-analyzer pod. But it's likely that users would need to perform the same updates across other pods like the aggregator and cloudcost pod.

@jessegoodier
Copy link
Collaborator

jessegoodier commented Dec 10, 2024

@mittal-ishaan great work here. Thanks for all of the iterations while we figured out the best approach.

Agree with Thomas that we should add to aggregator and cloud cost when you have a moment.

I'm now questioning if we should have put this under a more general section instead of nesting under kubecostModel ?

@chipzoller and @thomasvn what are your thoughts?

@thomasvn
Copy link
Member

Yes! I like the idea of doing .Values.updateCaTrust instead of .Values.kubecostModel.updateCaTrust.

@chipzoller
Copy link
Collaborator

Also agree that I'd try and pull this out and generalize it, but I'm not sure the root level is the best place.

You guys have any other suggestions for where these values may best go?

@thomasvn
Copy link
Member

Only other place that seems suitable is under .Values.global. I'm open to other ideas though?

@jessegoodier
Copy link
Collaborator

Only other place that seems suitable is under .Values.global. I'm open to other ideas though?

I'm thinking global. is better than productConfigs

cost-analyzer/values.yaml Outdated Show resolved Hide resolved
Copy link
Member

@thomasvn thomasvn left a comment

Choose a reason for hiding this comment

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

This looks good to me. These configs are now available under .Values.global.updateCaTrust.

Approving for immediate merge so fixes are available to customer. However we now need a follow-up PR to make similar changes to the Kubecost Aggregator and Kubecost CloudCost pods, then test those changes.

@jessegoodier
Copy link
Collaborator

Thanks Thomas. great work here team.

Co-authored-by: Thomas Nguyen <[email protected]>
@jessegoodier
Copy link
Collaborator

/cherry-pick v2.5

@jessegoodier jessegoodier enabled auto-merge (squash) December 11, 2024 20:10
@jessegoodier jessegoodier merged commit 3eaddf1 into develop Dec 11, 2024
20 checks passed
@jessegoodier jessegoodier deleted the ca-certs-config branch December 11, 2024 20:19
Copy link

Cherry-pick failed with Merge error 3eaddf172a83f530b881317b6945173e77751715 into temp-cherry-pick-debe5d-v2.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants