Skip to content

✨ Add support for specifying TLS config including custom CA certificates #238

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

Merged

Conversation

jimmidyson
Copy link
Member

What this PR does / why we need it:
Currently it is very difficult to use CAAPH with charts from an OCI registry served by a registry with a custom CA. This PR fixes that by allowing a CA certificate secret to be specified on a HelmChartProxy which will ultimately be used by the helm client to install/upgrade the cgart.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 16, 2024
@jimmidyson jimmidyson force-pushed the jimmi/trusted-ca-certificates branch 2 times, most recently from 9bb155e to 6fd2156 Compare May 16, 2024 13:54
@jimmidyson
Copy link
Member Author

/test pull-cluster-api-addon-provider-helm-e2e

@jimmidyson jimmidyson force-pushed the jimmi/trusted-ca-certificates branch from 6fd2156 to 5d7c5d4 Compare May 16, 2024 15:10
@jimmidyson
Copy link
Member Author

/cc @Jont828

@k8s-ci-robot k8s-ci-robot requested a review from Jont828 May 16, 2024 18:38
@jimmidyson
Copy link
Member Author

/test pull-cluster-api-addon-provider-helm-e2e

@jimmidyson
Copy link
Member Author

The e2e tests are failing due to calico images - fix in #239 which needs to be merged before this one.

@mboersma
Copy link
Contributor

/retest

@jimmidyson jimmidyson force-pushed the jimmi/trusted-ca-certificates branch from 5d7c5d4 to 5fccfc4 Compare May 21, 2024 12:36
@jimmidyson
Copy link
Member Author

Rebased to pull in calico fix from #239.

@jimmidyson
Copy link
Member Author

@mboersma @Jont828 All e2e tests passing now! Would love to get this merged.

Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

lgtm, thanks again @jimmidyson! I just had two minor doc comments.

/assign @Jont828

@faiq
Copy link
Contributor

faiq commented May 21, 2024

Does this require any RBAC permission changes to read a secret across different namespaces?

EDIT nvm looks like it has https://github.com/kubernetes-sigs/cluster-api-addon-provider-helm/blob/main/config/rbac/role.yaml#L16

@jimmidyson jimmidyson force-pushed the jimmi/trusted-ca-certificates branch from 5fccfc4 to 64bb651 Compare May 21, 2024 15:13
Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm

I'd like to get another maintainer's eyes on this as well, ping @jackfrancis @Jont828.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2024
Copy link
Contributor

@Jont828 Jont828 left a comment

Choose a reason for hiding this comment

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

Thanks for the patience, been busy with several projects recently. Overall seems fine, just some nits regarding the API types.

@jimmidyson jimmidyson force-pushed the jimmi/trusted-ca-certificates branch from 64bb651 to 2507fee Compare May 22, 2024 18:01
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2024
@jimmidyson
Copy link
Member Author

Thanks @Jont828! Applied review feedback and pushed 🙏

@faiq
Copy link
Contributor

faiq commented May 22, 2024

@Jont828 can we get a release for this functionality 🙏

@jimmidyson jimmidyson force-pushed the jimmi/trusted-ca-certificates branch from 048d0fa to 2a97e39 Compare May 22, 2024 20:29
Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2024
@Jont828
Copy link
Contributor

Jont828 commented May 23, 2024

Can you squash? LGTM otherwise. And sure, I can cut a new release after we get the current PRs merged.

@jimmidyson jimmidyson force-pushed the jimmi/trusted-ca-certificates branch from 2a97e39 to 9c8993a Compare May 23, 2024 19:34
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2024
@jimmidyson
Copy link
Member Author

Thanks @Jont828! Squashed and ready to go 🚀

@jimmidyson jimmidyson force-pushed the jimmi/trusted-ca-certificates branch from 9c8993a to ffb2dc2 Compare May 23, 2024 19:35
@mboersma
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2024
@Jont828
Copy link
Contributor

Jont828 commented May 24, 2024

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jimmidyson, Jont828

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 24, 2024
@k8s-ci-robot k8s-ci-robot merged commit ee2670b into kubernetes-sigs:main May 24, 2024
13 checks passed
@jimmidyson jimmidyson deleted the jimmi/trusted-ca-certificates branch May 24, 2024 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants