Adding support of ingressClassParams in helm charts #2478

haouc · 2022-01-27T02:18:53Z

Issue

Description

We want to add support for ingressClassParams in ingressClass spec parameters in helm charts.

The ingressClassParams was added into values and ingressClass chart. Charts version and README were also updated. Local tests were done from the charts by helm install. The namespaceSelector and group.name were tested with 2048 game ingress.

Update:
Tested for the use cases:

User create ingressClassParams with different spec settings
User doesn't create new ingressClassParams but referring to their existing ingressClassParams.
User create neither new ingressClassParams nor referring to their own resources.

--set ingressClassParams.spec.group.name=test
NAME   CONTROLLER            PARAMETERS                                                      AGE
alb    ingress.k8s.aws/alb   IngressClassParams.elbv2.k8s.aws/aws-load-balancer-controller   8s

--set ingressClassParams.name=dev-test --set ingressClassParams.spec.namespaceSelector.matchLabels.team=team-a
NAME   CONTROLLER            PARAMETERS                                  AGE
alb    ingress.k8s.aws/alb   IngressClassParams.elbv2.k8s.aws/dev-test   8s

Spec:
  Namespace Selector:
    Match Labels:
      Team:  team-a

--set ingressClassParams.name=dev-test --set ingressClassParams.spec.loadBalancerAttributes[0].key=deletion_protection.enabled --set-string ingressClassParams.spec.loadBalancerAttributes[0].value=true --set ingressClassParams.spec.loadBalancerAttributes[1].key=idle_timeout.timeout_seconds --set-string ingressClassParams.spec.loadBalancerAttributes[1].value=120

Spec:
  Load Balancer Attributes:
    Key:    deletion_protection.enabled
    Value:  true
    Key:    idle_timeout.timeout_seconds
    Value:  120


--set ingressClassParams.name=awesome-class --set ingressClassParams.create=false

NAME   CONTROLLER            PARAMETERS                                       AGE
alb    ingress.k8s.aws/alb   IngressClassParams.elbv2.k8s.aws/awesome-class   4s

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 🌟

k8s-ci-robot · 2022-01-27T02:19:01Z

Hi @haouc. 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.

codecov-commenter · 2022-01-27T02:24:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.38%. Comparing base (2e8cf3f) to head (449b5e6).
Report is 460 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2478      +/-   ##
==========================================
- Coverage   53.38%   53.38%   -0.01%     
==========================================
  Files         140      140              
  Lines        7985     7988       +3     
==========================================
+ Hits         4263     4264       +1     
- Misses       3405     3406       +1     
- Partials      317      318       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kishorj · 2022-01-27T19:13:37Z

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

@@ -173,6 +173,7 @@ The default values set by the application itself can be confirmed [here](https:/
 | `serviceAccount.name`                       | Service account to be used                                                                               | None                                                                               |
 | `terminationGracePeriodSeconds`             | Time period for controller pod to do a graceful shutdown                                                 | 10                                                                                 |
 | `ingressClass`                              | The ingress class to satisfy                                                                             | alb                                                                                |
+| `ingressClassParams.name`                   | The created ingressClassParams object name (referred in ingressClass)                                    | ""                                                                                |


nit: "" not needed

kishorj · 2022-01-27T19:17:00Z

helm/aws-load-balancer-controller/values.yaml

@@ -87,6 +87,11 @@ enableCertManager: false
 # ingresses without ingress class annotation and ingresses of type alb
 ingressClass: alb

+# The ingressClassParams the ingress class refers to and enforce settings for a set of Ingresses when using with ingress Controller.
+ingressClassParams:


why not create IngressClassParams resource and refer from the IngressClass instead? users will be able to specify the ingress class params spec during chart installation

since the params spec can be user specific and vary, would that be better to let users create ingress class params resources and just refer their name in charts during install/upgrade? That could be easier to maintain and less error-prone?

kishorj · 2022-01-27T19:17:56Z

helm/aws-load-balancer-controller/values.yaml

@@ -87,6 +87,11 @@ enableCertManager: false
 # ingresses without ingress class annotation and ingresses of type alb
 ingressClass: alb

+# The ingressClassParams the ingress class refers to and enforce settings for a set of Ingresses when using with ingress Controller.


nit: please use the following format for comments

# ingressClassParams specifies the IngressClassParams ...

kishorj · 2022-02-03T17:18:22Z

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

@@ -7,4 +7,7 @@ metadata:
    {{- include "aws-load-balancer-controller.labels" . | nindent 4 }}
 spec:
  controller: ingress.k8s.aws/alb
+  {{- if or .Values.ingressClassParams.create .Values.ingressClassParams.name }}
+  {{ include "aws-load-balancer-controller.ingressClassParameters" . }}


lets include the spec directly instead of using a template function

kishorj · 2022-02-03T18:03:40Z

/ok-to-test

kishorj · 2022-02-03T21:49:31Z

/retest

…block.

kishorj

Thanks for the contribution.

kishorj · 2022-02-07T17:36:25Z

/lgtm

k8s-ci-robot · 2022-02-07T17:36:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haouc, kishorj

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~~ [kishorj]

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

…2478) * Adding support of ingressClassParams in helm charts * Update charts to let users to create ingressClassParams with specs * Simplying the spec in values yaml * Adding support for customer's ingressClassParams * Using values fields directly instead of function * eks1.19 doesn't allow null spec and need move if check out from spec block.

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 Jan 27, 2022

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 27, 2022

k8s-ci-robot requested review from kishorj and M00nF1sh January 27, 2022 02:19

kishorj reviewed Jan 27, 2022

View reviewed changes

kishorj added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 31, 2022

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 31, 2022

haouc force-pushed the main branch from e5e1b04 to 7d10017 Compare January 31, 2022 23:20

haouc requested a review from kishorj January 31, 2022 23:21

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2022

haouc added 2 commits February 2, 2022 17:14

Adding support of ingressClassParams in helm charts

b801b4d

Update charts to let users to create ingressClassParams with specs

3369daa

haouc force-pushed the main branch from 7d10017 to cc8a154 Compare February 3, 2022 01:21

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 3, 2022

haouc force-pushed the main branch from cc8a154 to ad732c6 Compare February 3, 2022 01:27

Simplying the spec in values yaml

0dfe2a5

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 3, 2022

haouc force-pushed the main branch from abef0da to da0374a Compare February 3, 2022 02:19

Adding support for customer's ingressClassParams

7e394e5

haouc force-pushed the main branch from da0374a to 7e394e5 Compare February 3, 2022 02:20

kishorj reviewed Feb 3, 2022

View reviewed changes

Using values fields directly instead of function

a19df6f

haouc requested a review from kishorj February 3, 2022 18:02

kishorj approved these changes Feb 3, 2022

View reviewed changes

kishorj removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2022

eks1.19 doesn't allow null spec and need move if check out from spec …

449b5e6

…block.

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2022

haouc requested a review from kishorj February 7, 2022 17:13

kishorj approved these changes Feb 7, 2022

View reviewed changes

k8s-ci-robot assigned kishorj Feb 7, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2022

k8s-ci-robot merged commit 8b7f864 into kubernetes-sigs:main Feb 7, 2022

This was referenced Feb 16, 2022

🥳 aws-load-balancer-controller v2.4.0 Automated Release! 🥑 oliviassss/eks-charts#19

Closed

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

Merged

M00nF1sh mentioned this pull request Jul 22, 2022

[BUG] ALBs for Ingress might be recreated while updating corresponding IngressClass's spec.parameters #2731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support of ingressClassParams in helm charts #2478

Adding support of ingressClassParams in helm charts #2478

haouc commented Jan 27, 2022 •

edited

Loading

k8s-ci-robot commented Jan 27, 2022

codecov-commenter commented Jan 27, 2022 •

edited

Loading

kishorj Jan 27, 2022

kishorj Jan 27, 2022

haouc Jan 27, 2022

kishorj Jan 27, 2022 •

edited

Loading

kishorj Feb 3, 2022

kishorj commented Feb 3, 2022

kishorj commented Feb 3, 2022

kishorj left a comment

kishorj commented Feb 7, 2022

k8s-ci-robot commented Feb 7, 2022

Adding support of ingressClassParams in helm charts #2478

Adding support of ingressClassParams in helm charts #2478

Conversation

haouc commented Jan 27, 2022 • edited Loading

Issue

Description

Checklist

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

k8s-ci-robot commented Jan 27, 2022

codecov-commenter commented Jan 27, 2022 • edited Loading

Codecov Report

kishorj Jan 27, 2022

Choose a reason for hiding this comment

kishorj Jan 27, 2022

Choose a reason for hiding this comment

haouc Jan 27, 2022

Choose a reason for hiding this comment

kishorj Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

kishorj Feb 3, 2022

Choose a reason for hiding this comment

kishorj commented Feb 3, 2022

kishorj commented Feb 3, 2022

kishorj left a comment

Choose a reason for hiding this comment

kishorj commented Feb 7, 2022

k8s-ci-robot commented Feb 7, 2022

haouc commented Jan 27, 2022 •

edited

Loading

codecov-commenter commented Jan 27, 2022 •

edited

Loading

kishorj Jan 27, 2022 •

edited

Loading