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

Generating certificates without a cert-manager #226

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

tropnikovvl
Copy link
Contributor

What should the feature do:
Automatically create a certificate for a webhook in the absence of a cert-manager

What is use case behind this feature:
There is no need to install extra applications in the cluster and support them (cert-manager)

#223

Copy link
Contributor

@llamerada-jp llamerada-jp left a comment

Choose a reason for hiding this comment

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

Thank you for your work. There are some requests.

Would you please add a new e2e pattern using this certificate like below?

  • Add init-cluster-with-certificate (or better name) target in e2e/Makefile.
  • Add new test pattern in .github/wokflows/e2e.yaml with new makefile option and latest k8s version.

charts/pvc-autoresizer/values.yaml Show resolved Hide resolved
charts/pvc-autoresizer/templates/controller/issuer.yaml Outdated Show resolved Hide resolved
charts/pvc-autoresizer/templates/controller/secret.yaml Outdated Show resolved Hide resolved
charts/pvc-autoresizer/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/pvc-autoresizer/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/pvc-autoresizer/templates/_helpers.tpl Outdated Show resolved Hide resolved
@satoru-takeuchi
Copy link
Member

@tropnikovvl please fix the following error in both Kiunt and Test Charts and Main

time="2023-11-22T00:13:48Z" level=info msg="Generating README Documentation for chart charts/pvc-autoresizer"

Copy link
Contributor

@llamerada-jp llamerada-jp left a comment

Choose a reason for hiding this comment

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

I added the comment in mistake, I will wait for the CI to pass.

Copy link
Contributor

@llamerada-jp llamerada-jp left a comment

Choose a reason for hiding this comment

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

Deployment requires a secret pvc-autoresizer-controller, but pvc-autoresizer-tls may be created. Would you check and fix it?

@satoru-takeuchi
Copy link
Member

The PR looks good. Please squash commits and add signed-off-by to the merged commit. I'll approve this after that and all tests

BTW, signed-off-by is necessary for all commits. The DCO test fails because the second last commit doesn't have this commit. Squashing will resolve this.

@satoru-takeuchi
Copy link
Member

The PR looks good. Please squash commits and add signed-off-by to the merged commit. I'll approve this after that and all tests

This PR includes unrelated ones like this. Please be careful not to include these commits to squashed single commit.

@tropnikovvl
Copy link
Contributor Author

The PR looks good. Please squash commits and add signed-off-by to the merged commit. I'll approve this after that and all tests

Done

@tropnikovvl
Copy link
Contributor Author

tropnikovvl commented Nov 29, 2023

I'm getting a strange certificate mismatch error.
Although it is placed in the same place where the cert-manager’s certificate was usually placed.

[FAILED] stdout=pod/test-pvc created
  , stderr=Error from server (InternalError): error when creating "STDIN": Internal error occurred: failed calling webhook "mpersistentvolumeclaim.topolvm.io": failed to call webhook: Post "[https://pvc-autoresizer-controller.pvc-autoresizer.svc:443/pvc/mutate?timeout=10s](https://pvc-autoresizer-controller.pvc-autoresizer.svc/pvc/mutate?timeout=10s)": tls: failed to verify certificate: x509: certificate signed by unknown authority (possibly because of "crypto/rsa: verification error" while trying to verify candidate authority certificate "pvc-autoresizer-ca")

I doubt the name discrepancy has anything to do with this.
These lines of code show that the names are generated correctly, just like for the cert-manager.

https://github.com/topolvm/pvc-autoresizer/pull/226/files#diff-5233c6e21a5a8257b2feb24c9cef9e1fd8920f357871d601adc65a4bb254295bR69

{{- $serviceName := printf "%s-controller" (include "pvc-autoresizer.fullname" .) -}}
{{- $altNames := list (printf "%s.%s" $serviceName .Release.Namespace) (printf "%s.%s.svc" $serviceName .Release.Namespace) (printf "%s.%s.svc.%s" $serviceName .Release.Namespace .Values.webhook.certificate.dnsDomain) -}}
{{- $cert := genSignedCert $serviceName nil $altNames 3650 $ca -}}

https://github.com/topolvm/pvc-autoresizer/blob/main/charts/pvc-autoresizer/templates/controller/certificate.yaml#L45

  dnsNames:
    - {{ template "pvc-autoresizer.fullname" . }}-controller
    - {{ template "pvc-autoresizer.fullname" . }}-controller.{{ .Release.Namespace }}
    - {{ template "pvc-autoresizer.fullname" . }}-controller.{{ .Release.Namespace }}.svc

@llamerada-jp
Copy link
Contributor

I ran the e2e test locally and checked some manifests. I found ca.crt in Secret is not equal to caBundle in MutatingWebhookConfiguration. In other systems like TopoLVM, this value is equal. Could you check in your environment and try to get these values to be equal?

@tropnikovvl
Copy link
Contributor Author

I ran the e2e test locally and checked some manifests. I found ca.crt in Secret is not equal to caBundle in MutatingWebhookConfiguration. In other systems like TopoLVM, this value is equal. Could you check in your environment and try to get these values to be equal?

I fixed the problem, it was related to different contexts in helm

Signed-off-by: Vladislav Tropnikov <vladislav.tropnikov@genestack.com>
Signed-off-by: Vladislav Tropnikov <vladislav.tropnikov@genestack.com>
@satoru-takeuchi satoru-takeuchi merged commit 2bbab2c into topolvm:main Dec 1, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants