-
Notifications
You must be signed in to change notification settings - Fork 38
✨ 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
✨ Add support for specifying TLS config including custom CA certificates #238
Conversation
9bb155e
to
6fd2156
Compare
/test pull-cluster-api-addon-provider-helm-e2e |
6fd2156
to
5d7c5d4
Compare
/cc @Jont828 |
/test pull-cluster-api-addon-provider-helm-e2e |
The e2e tests are failing due to calico images - fix in #239 which needs to be merged before this one. |
/retest |
5d7c5d4
to
5fccfc4
Compare
Rebased to pull in calico fix from #239. |
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, thanks again @jimmidyson! I just had two minor doc comments.
/assign @Jont828
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 |
5fccfc4
to
64bb651
Compare
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
I'd like to get another maintainer's eyes on this as well, ping @jackfrancis @Jont828.
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.
Thanks for the patience, been busy with several projects recently. Overall seems fine, just some nits regarding the API types.
64bb651
to
2507fee
Compare
Thanks @Jont828! Applied review feedback and pushed 🙏 |
@Jont828 can we get a release for this functionality 🙏 |
048d0fa
to
2a97e39
Compare
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
Can you squash? LGTM otherwise. And sure, I can cut a new release after we get the current PRs merged. |
2a97e39
to
9c8993a
Compare
Thanks @Jont828! Squashed and ready to go 🚀 |
9c8993a
to
ffb2dc2
Compare
/lgtm |
/lgtm |
[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 |
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 #