cert-manager: add trusted internal ca when configured

[infra-monkey]

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
In order to use an acme issuer with an internal authority, the CA of the authority must be trusted by cert-manager.
If not, the issuer with throw errors saying the certificate of the acme uri is unknown.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
I tried to keep it simple.
One additional variable in the addon inventory to specify the trusted internal ca.
A configmap is create with the content of the ca.
This configmap is mounted in the cert-manager container in /etc/ssl/certs in order to be trusted.
As not everyone will use acme with an internal authority, the cm and mount are not defined if the variable is not defined.

Does this PR introduce a user-facing change?:

Add a new option `cert_manager_trusted_internal_ca` to specify trusted internal ca of cert_manager.

[k8s-ci-robot]

Hi @infra-monkey. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[oomichi]

Hi @infra-monkey
Could you provide some link to know the configuration is valid on cert-manager?

[infra-monkey]

@oomichi Hi,
So I inspired this change from https://smallstep.com/docs/tutorials/kubernetes-acme-ca
I ran a cluster update on my k8s cluster from this branch and it works with the latest change.
Here are some outputs of the state of my cluster after applying this change:
describe-cm.txt
describe-deployment.txt
describe-clusterissuer.txt

Extracted from the issuer crd definition (https://github.com/jetstack/cert-manager/blob/master/deploy/crds/crd-issuers.yaml#L141):

skipTLSVerify:
  description: Enables or disables validation of the ACME server TLS certificate. If true, requests to the ACME server will not have their TLS certificate validated (i.e. insecure connections will be allowed). Only enable this option in development environments. The cert-manager system installed roots will be used to verify connections to the ACME server if this is false. Defaults to false.
  type: boolean

This is the only "official" reference I could find about this feature.

[oomichi]

@infra-monkey Thank you so much for your explanation, I got it.
This additional option doesn't break the existing features by default.
So I feel it is fine to add it and get feedback if any problem.

/ok-to-test
/lgtm

[infra-monkey]

@oomichi thank you for the approval.
Indeed my initial description was light.

I put "none" for the user facing change. As there is a new option in the inventory, should I specifiy it there?

[oomichi]

@oomichi thank you for the approval. Indeed my initial description was light.

I put "none" for the user facing change. As there is a new option in the inventory, should I specifiy it there?

That is a nice point.
It would be better to write a release note instead of NONE like

Add a new option cert_manager_trusted_internal_ca to specify internal ca of cert_manager

or something.

[infra-monkey]

@oomichi a vagrant job failed in the pipeline. How can I restart the tests?

[cristicalin]

@infra-monkey would you mid updating https://github.com/kubernetes-sigs/kubespray/blob/master/docs/cert_manager.md with some details on the new variable and the intended purpose?

[infra-monkey]

@infra-monkey would you mid updating https://github.com/kubernetes-sigs/kubespray/blob/master/docs/cert_manager.md with some details on the new variable and the intended purpose?

Great point. I'll do that. I hope my english will be good enough.

[cristicalin]

It might be a good idea to rebase your branch on latest master since the CI job is complaining about failing to merge your doc changes.

[infra-monkey]

@cristicalin Rebased from master. It bring a lot of changes from master that have nothing to do with this change.
Sorry I'm not a git guru.

CI fails, but because it can't pull containers.

[cristicalin]

The relevant failure on the CI is this:

Using index info to reconstruct a base tree...
M	docs/cert_manager.md
Falling back to patching base and 3-way merge...
Auto-merging docs/cert_manager.md
CONFLICT (content): Merge conflict in docs/cert_manager.md
Patch failed at 0004 Update documentation
Use 'git am --show-current-patch' to see the failed patch
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
error: Failed to merge in the changes.
Uploading artifacts for failed job 00:00
Uploading artifacts...
WARNING: cluster-dump/: no matching files          
ERROR: No files to upload                          

https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/jobs/1735187854#L40

Your PR now brings is some changes from the master branch which means the rebase was somehow broken. Usually, the way I do this is like this:

git remote set-url origin https://github.com/cristicalin/kubespray.git
git pull origin
git checkout <my_local_branch>
git rebase -i origin master  # ensure only my commits show up

[cristicalin]

Unfortunately it seems our CI python was pushed to 3.10 and is now breaking in interesting ways. I pushed a fix #8153 but there may be other breakage.

[infra-monkey]

Unfortunately it seems our CI python was pushed to 3.10 and is now breaking in interesting ways. I pushed a fix #8153 but there may be other breakage.

I just saw that. I'll wait for you PR to be merged in master and include it here.
Thank you for looking into it

[floryut]

Unfortunately it seems our CI python was pushed to 3.10 and is now breaking in interesting ways. I pushed a fix #8153 but there may be other breakage.

I just saw that. I'll wait for you PR to be merged in master and include it here. Thank you for looking into it

Merged now you can rebase 👍

[infra-monkey]

I cherry picked the CI fix.

[infra-monkey]

@oomichi @cristicalin @floryut Seems the pipeline fails on a timeout downloading a file.
I don't see how my changes could influence this part.
How should I proceed?

[floryut]

@oomichi @cristicalin @floryut Seems the pipeline fails on a timeout downloading a file. I don't see how my changes could influence this part. How should I proceed?

yup there is an issue with calico release, I've submitted a PR and I'm trying to get in touch with Calico maintainers to understands what the f** happened

[infra-monkey]

yup there is an issue with calico release, I've submitted a PR and I'm trying to get in touch with Calico maintainers to understands what the f** happened

I have seen your PR. Indeed it is strange that the hash would change without a version change.
I'll wait for your go before rebasing the fix.
Bad timing for me but at least the ci failures are caught early

[cristicalin]

@infra-monkey #8157 just merged, please rebase and lets try this run one more time.

[infra-monkey]

YEAH ! It finally passed!

[oomichi]

YEAH ! It finally passed!

Current pull request is not rebased yet, it just contains necessary commits in the pull request.
Please rebase this pull request like

git checkout master
git remote add upstream https://github.com/kubernetes-sigs/kubespray.git
git fetch upstream
git merge upstream/master
git checkout <your branch>
git rebase master
git push -f origin <your branch>

[infra-monkey]

@oomichi I didn't think you wanted a full rebase before merge. I misunderstood the request to rebase.
It is done now. CI running

[cristicalin]

@infra-monkey it is the norm to have only your commits in a PR unless you are doing backports to older branches. This is needed to keep the git commit history clean and also to avoid issues when github squashes the commits in the final merge.

[cristicalin]

/approve

[floryut]

@infra-monkey Thank you for the PR 👍

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, floryut, infra-monkey

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:

• OWNERS [floryut]

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

[infra-monkey]

@infra-monkey Thank you for the PR +1

Thank you all for the guidance and patience :)

[cristicalin]

/lgtm