search

topolvm / pvc-autoresizer

Auto-resize PersistentVolumeClaim objects based on Prometheus metrics
Apache License 2.0
249 stars 29 forks source link

fix: cert-manager duration expects XhYmZs format #273

Closed gozer closed 1 month ago

gozer commented 4 months ago

It accepts shorter format like Yh, but the mutating webhook, if installed/enabled will convert it to the full format, causing potential endless reconciliation loops with tools like ArgoCD

Fixes #190

llamerada-jp commented 4 months ago

if installed/enabled will convert it to the full format, causing potential endless reconciliation loops with tools like ArgoCD

The reverse is also true, If there is a mutating webhook that converts to a shorter format, The change o this PR can cause endless reconcilation loop. I have checked and argocd works fine with both 87600h and 87600h0m0s descriptions. According to upstream's comment, the problem was fixed at v2.10 or later. Would you please review the mutating webhook settings first, and if you can't avoid the problem, change it so that it can handle both formats.

gozer commented 4 months ago

The reverse is also true, If there is a mutating webhook that converts to a shorter format, The change to this PR can cause endless reconcilation loop. I have checked and argocd works fine with both 87600h and 87600h0m0s descriptions.

The issue isn't with ArgoCD itself, but rather with cert-manager itself, it requires durations to be in that format, and it's cert-manager's own mutating webhook controller that makes this conversion transparently.

So, technically, a Certificate spec.duration field needs to be in this format to be correct, this this PR

llamerada-jp commented 4 months ago

Thanks for letting me know.

I tried to check the output format of cert-manager for the review, but I could not see the above behavior in my environment. It may be a difference in versions, so please tell me which version you tested. Or do I need any options for cert-manager?

gozer commented 4 months ago

Thanks for letting me know.

No worries!

I tried to check the output format of cert-manager for the review, but I could not see the above behavior in my environment. It may be a difference in versions, so please tell me which version you tested. Or do I need any options for cert-manager?

The validating/mutating webhook portion of cert-manager is an optional component.

I've seen it with cert-manager 1.13.3 as of now.

llamerada-jp commented 4 months ago

I checked my environment, cert-manager is 1.14.4, and the webhook is enabled. Does your environment set additional options for webhooks?

I checked the cert-manager code, but could only find a webhook for the CertificateResuest custom resource and could not found logic for Certificate custom resource. https://github.com/cert-manager/cert-manager/tree/master/internal/webhook/admission/certificaterequest

The validating/mutating webhook portion of cert-manager is an optional component.

Does this mean it is a external product?

iflan7744 commented 3 months ago

@gozer I agree with your change, and I am also experiencing this issue when installing the chart via ArgoCD. One small suggestion: instead of hardcoding, it would be nice if we can call it via values.yaml, allowing people to modify it based on their requirements.

webhook:
  pvcMutatingWebhook:
    enabled: true
  certificate:
    generate: true
    caCertDuration: 87600h  # 10 years
    certDuration: 8760h     # 1 year
github-actions[bot] commented 2 months ago

This pull request has been automatically marked as stale because it has not had any activity for 30 days. It will be closed in a week if no further activity occurs. Thank you for your contributions.

gozer commented 2 months ago

@gozer I agree with your change, and I am also experiencing this issue when installing the chart via ArgoCD. One small suggestion: instead of hardcoding, it would be nice if we can call it via values.yaml, allowing people to modify it based on their requirements.

Thanks! I didn't have time to dig down precisely where it's coming from so I can offer a reproductible test case. At least I am not alone.

And yes, making it a value makes a whole lot more sense. I'll fix the PR.

toshipp commented 2 months ago

@gozer Please check and answer @llamerada-jp's question. We can not approve this unless it is reasonable.

github-actions[bot] commented 1 month ago

This pull request has been automatically marked as stale because it has not had any activity for 30 days. It will be closed in a week if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 1 month ago

This pull request has been automatically closed due to inactivity. Please feel free to reopen this issue (or open a new one) if these changes are still required. Thank you for your contribution.