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

feat(controller,api): adds a cabundle option to the values.yaml file. #2382

Closed
wants to merge 8 commits into from
Closed

feat(controller,api): adds a cabundle option to the values.yaml file. #2382

wants to merge 8 commits into from

Conversation

lknite
Copy link
Contributor

@lknite lknite commented Aug 5, 2024

If enabled, it adds an initContainer to the api/controller deployment which mounts certs provided via either a configMap or secret.

Uses an emptyDir, or optionally a pvc, for the actual certs mount within the controller; this allows the controller to write to the certs folder if needed.

If enabled, it adds an initContainer to the controller deployment which mounts certs provided via either a configMap or secret.

Uses an emptyDir, or optionally a pvc, for the actual certs mount within the controller; this allows the controller to write to the certs folder if needed.

Signed-off-by: Leland Knight <[email protected]>
@lknite lknite requested a review from a team as a code owner August 5, 2024 02:33
Copy link

netlify bot commented Aug 5, 2024

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit f4b0eae
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/66b7c5cd546498000878ac55
😎 Deploy Preview https://deploy-preview-2382.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@lknite lknite changed the title Adds a controller.ca-bundle option to the values.yaml file. feat: adds a controller.ca-bundle option to the values.yaml file. Aug 5, 2024
@krancour
Copy link
Member

krancour commented Aug 5, 2024

What is the point of the init container and the volumes here? I'm not clear about what would be insufficient about just mounting a user-provided secret into the container that contains the trust store.

@krancour
Copy link
Member

krancour commented Aug 5, 2024

Following up on my previous comment:

The original issue was #2271 and it seemed to be about CA for the OIDC IDP, so in the back of my mind, that was the context/scope. That issue can be solved by adding more CA certs directly to the http client that the Kargo API server uses to communicate with the IDP and that doesn't require anything more than those certs to be mounted somewhere where the API server will look for them. e.g. Secrets do the trick. No init container needed. Very small code change needed.

This PR seems to address the issue of custom CA certs more-broadly, at the system level, which is nice, but also seems to do it for the controller and not the API server, meaning it doesn't solve the original issue.

Perhaps what is required here is some clarification on the scope of what you are trying to solve for.

@lknite lknite changed the title feat: adds a controller.ca-bundle option to the values.yaml file. feat(controller): adds a controller.ca-bundle option to the values.yaml file. Aug 5, 2024
@lknite
Copy link
Contributor Author

lknite commented Aug 5, 2024

If we liked this approach I was going to submit a pr for the api also doing the same thing, maybe for all the services. I started with the controller because the controller also wants to write to the /etc/ssl/certs folder and (unless i'm mistaken) just mounting a secret will make the path read-only, so we at the least for the controller need to use an 'emptyDir' instead.

The initContainer approach is nice because it can take a ca-bundle with multiple certs and split them up into individual certs and add each one. This is a little easier on the SRE because in general update-ca-trust and update-ca-certificates expects a single cert per file, though its common to have many in one, e.g. trustmanager , such as the root ca & int cert in one file.

... the initContainer would only be added if someone specifically enabled the ca-bundle option

@krancour
Copy link
Member

krancour commented Aug 5, 2024

@lknite, ok I get where you were going. It was the scope / area where you implemented the change that confused me a bit. With greater clarity on that now, I will take another look.

lknite added 2 commits August 10, 2024 12:28
- added to 'or' before the 'volumes' section to ensure cabundle related volumes are added

Signed-off-by: Leland Knight <[email protected]>
Signed-off-by: Leland Knight <[email protected]>
@lknite lknite changed the title feat(controller): adds a controller.ca-bundle option to the values.yaml file. feat(controller,api): adds a controller.ca-bundle option to the values.yaml file. Aug 10, 2024
@lknite lknite changed the title feat(controller,api): adds a controller.ca-bundle option to the values.yaml file. feat(controller,api): adds a cabundle option to the values.yaml file. Aug 10, 2024
@lknite
Copy link
Contributor Author

lknite commented Aug 10, 2024

added to kargo-api as well

will now resolve issue #2271

lknite and others added 5 commits August 10, 2024 13:25
…t.yaml to ensure cabundle is mounted if enabled

Signed-off-by: Leland Knight <[email protected]>
Merge branch 'feature-add-ca-bundle-to-controller' of https://github.com/lknite/kargo into feature-add-ca-bundle-to-controller
If enabled, it adds an initContainer to the controller deployment which mounts certs provided via either a configMap or secret.

Uses an emptyDir, or optionally a pvc, for the actual certs mount within the controller; this allows the controller to write to the certs folder if needed.

Signed-off-by: Leland Knight <[email protected]>
@lknite
Copy link
Contributor Author

lknite commented Aug 10, 2024

tried to update my fork, might have messed up things will redo with a new pull request

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.

2 participants