Allow override of Certificate resource fields for duration of webhook certs #4105

usamaahmadkhan · 2025-03-24T10:16:20Z

Issue

closes #4102

Description

Allow override default values of webhook certificates by templating fields duration and renewBefore

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the docs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

linux-foundation-easycla · 2025-03-24T10:16:27Z

The committers listed above are authorized under a signed CLA.

✅ login: usamaahmadkhan / name: Usama Ahmad (cb663ec, 39cdd0d, a422689, f38c6c6)

k8s-ci-robot · 2025-03-24T10:16:29Z

Welcome @usamaahmadkhan!

It looks like this is your first PR to kubernetes-sigs/aws-load-balancer-controller 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-load-balancer-controller has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-03-24T10:16:30Z

Hi @usamaahmadkhan. 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-sigs/prow repository.

usamaahmadkhan · 2025-03-25T08:03:58Z

/assign @wweiwei-li

wweiwei-li · 2025-03-26T19:36:11Z

/ok-to-test

usamaahmadkhan · 2025-03-27T00:31:54Z

@wweiwei-li seems like govulncheck is failing. Let me know how to proceed. I would like to merge this ASAP.

oliviassss · 2025-03-27T05:46:30Z

helm/aws-load-balancer-controller/README.md

@@ -257,6 +257,8 @@ The default values set by the application itself can be confirmed [here](https:/
 | `podDisruptionBudget`                          | Limit the disruption for controller pods. Require at least 2 controller replicas and 3 worker nodes                                                                                                                                                                                                                                          | `{}`                                              |
 | `updateStrategy`                               | Defines the update strategy for the deployment                                                                                                                                                                                                                                                                                               | `{}`                                              |
 | `enableCertManager`                            | If enabled, cert-manager issues the webhook certificates instead of the helm template, requires cert-manager and it's CRDs to be installed                                                                                                                                                                                                   | `false`                                           |
+| `certManager.duration`                            | Set the expiry duration for the webhook certificates. Used only when `enableCertManager: true`                                                                                                                                                                                                  | `""`                                           |
+| `certManager.renewBefore`                            | Set the renewal time period duration for the webhook certificates. Used only when `enableCertManager: true`.  If this value is greater than `certManager.duration`, it will be automatically renewed 2/3rds of the way through the certificate’s duration.                                                                                                                                                                                                  | `""`                                           |


Is this automatically renewal handled from cert-manager side?

oliviassss · 2025-03-27T05:52:10Z

helm/aws-load-balancer-controller/README.md

@@ -257,6 +257,8 @@ The default values set by the application itself can be confirmed [here](https:/
 | `podDisruptionBudget`                          | Limit the disruption for controller pods. Require at least 2 controller replicas and 3 worker nodes                                                                                                                                                                                                                                          | `{}`                                              |
 | `updateStrategy`                               | Defines the update strategy for the deployment                                                                                                                                                                                                                                                                                               | `{}`                                              |
 | `enableCertManager`                            | If enabled, cert-manager issues the webhook certificates instead of the helm template, requires cert-manager and it's CRDs to be installed                                                                                                                                                                                                   | `false`                                           |
+| `certManager.duration`                            | Set the expiry duration for the webhook certificates. Used only when `enableCertManager: true`                                                                                                                                                                                                  | `""`                                           |


This doesn't look correct to me, if enableCertManager is set to true, the webhook is issued by the cert-manager instead of supplied by helm chart. so the value you try to add in helm chart won't take effect.

oliviassss · 2025-03-27T05:53:05Z

helm/aws-load-balancer-controller/templates/webhook.yaml

@@ -237,6 +237,14 @@ spec:
    kind: Issuer
    name: {{ template "aws-load-balancer-controller.namePrefix" . }}-selfsigned-issuer
  secretName: {{ template "aws-load-balancer-controller.webhookCertSecret" . }}
+  {{- with .Values.certManager -}}


This manifest will only be rendered when

{{- if not $.Values.enableCertManager }}

it conflicts with your statement in README. Please clarify

@oliviassss current behaviour is that If enableCertManager is true, duration and renewBefore fields are not specified in the Certificate resource and the implicit behaviour of the certmanager controller is that it will issue for 90d and renew after 60d.

This change allows to override this behaviour by explicitly mentioning duration and renewBefore fields optionally if enableCertManager is true

usamaahmadkhan · 2025-03-27T12:07:21Z

@oliviassss I have updated the description to be more precise. Let me know if it needs further improvement

oliviassss · 2025-03-27T22:49:21Z

/lgtm
/approve

k8s-ci-robot · 2025-03-28T06:06:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oliviassss, surenraju-careem, usamaahmadkhan, wweiwei-li

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~~ [oliviassss,wweiwei-li]

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

wweiwei-li · 2025-03-28T21:11:33Z

/lgtm

… certs (kubernetes-sigs#4105) * Allow override Certificate fields for duration * Add docs for new fields * Update documentation text to be more precise * fix typo

k8s-ci-robot requested review from oliviassss and wweiwei-li March 24, 2025 10:16

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 24, 2025

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 24, 2025

usamaahmadkhan added 2 commits March 24, 2025 15:36

Allow override Certificate fields for duration

cb663ec

Add docs for new fields

f38c6c6

usamaahmadkhan force-pushed the enhancement/certmanager-duration branch from ff3f83f to f38c6c6 Compare March 24, 2025 10:37

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 24, 2025

usamaahmadkhan mentioned this pull request Mar 24, 2025

[Feature Enhancement] Allow overriding of default webhook certificate values #4102

Closed

2 tasks

k8s-ci-robot assigned wweiwei-li Mar 25, 2025

surenraju-careem approved these changes Mar 25, 2025

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 26, 2025

oliviassss reviewed Mar 27, 2025

View reviewed changes

Update documentation text to be more precise

39cdd0d

fix typo

a422689

k8s-ci-robot assigned oliviassss Mar 27, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2025

oliviassss added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 27, 2025

wweiwei-li approved these changes Mar 28, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2025

k8s-ci-robot merged commit de50bdd into kubernetes-sigs:main Mar 29, 2025
9 checks passed

zac-nixon mentioned this pull request May 6, 2025

🥳 aws-load-balancer-controller v2.13.0 Automated Release! 🥑 aws/eks-charts#1214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow override of Certificate resource fields for duration of webhook certs #4105

Allow override of Certificate resource fields for duration of webhook certs #4105

Uh oh!

usamaahmadkhan commented Mar 24, 2025

Uh oh!

linux-foundation-easycla bot commented Mar 24, 2025 •

edited

Loading

Uh oh!

k8s-ci-robot commented Mar 24, 2025

Uh oh!

k8s-ci-robot commented Mar 24, 2025

Uh oh!

usamaahmadkhan commented Mar 25, 2025

Uh oh!

wweiwei-li commented Mar 26, 2025

Uh oh!

usamaahmadkhan commented Mar 27, 2025

Uh oh!

oliviassss Mar 27, 2025

Uh oh!

oliviassss Mar 27, 2025

Uh oh!

oliviassss Mar 27, 2025

Uh oh!

usamaahmadkhan Mar 27, 2025

Uh oh!

usamaahmadkhan commented Mar 27, 2025 •

edited

Loading

Uh oh!

oliviassss commented Mar 27, 2025

Uh oh!

k8s-ci-robot commented Mar 28, 2025

Uh oh!

wweiwei-li commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

Allow override of Certificate resource fields for duration of webhook certs #4105

Allow override of Certificate resource fields for duration of webhook certs #4105

Uh oh!

Conversation

usamaahmadkhan commented Mar 24, 2025

Issue

Description

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Uh oh!

linux-foundation-easycla bot commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 24, 2025

Uh oh!

k8s-ci-robot commented Mar 24, 2025

Uh oh!

usamaahmadkhan commented Mar 25, 2025

Uh oh!

wweiwei-li commented Mar 26, 2025

Uh oh!

usamaahmadkhan commented Mar 27, 2025

Uh oh!

oliviassss Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

oliviassss Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

oliviassss Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

usamaahmadkhan Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

usamaahmadkhan commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviassss commented Mar 27, 2025

Uh oh!

k8s-ci-robot commented Mar 28, 2025

Uh oh!

wweiwei-li commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Mar 24, 2025 •

edited

Loading

usamaahmadkhan commented Mar 27, 2025 •

edited

Loading