Enable setting custom objectSelector for webhook #2407

Shreya027 · 2021-12-08T19:01:11Z

Issue

Description

Currently when we install the aws load balancer controller via Helm chart, the objectSelector for Pod Readiness Gate is hard coded without any option to extend it. I have added code to allow custom additions using a template format in webhook.yaml with additional custom values fetched from values.yaml

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 🌟

I have Tested 4 sample scenarios:

Test Scenario 1:
One entry added for matchLabels besides matchExpressions default entry.

webhook config snippet:

  namespaceSelector:
    matchExpressions:
    - key: elbv2.k8s.aws/pod-readiness-gate-inject
      operator: In
      values:
      - enabled
  objectSelector:
    matchExpressions:
    - key: app.kubernetes.io/name
      operator: NotIn
      values:
      - aws-load-balancer-controller
    matchLabels:
      testlabel: testvalue

Sample deployment 2048-pods.yaml snippet

apiVersion: apps/v1
kind: Deployment
...
spec:
  selector:
    matchLabels:
      app.kubernetes.io/name: app-2048
  replicas: 5
  template:
    metadata:
      labels:
        app.kubernetes.io/name: app-2048
        ...
...

Corresponding output with no readiness gates configured:

% kubectl apply  -f 2048-pods.yaml        
deployment.apps/deployment-2048 created
% kubectl get pods -owide -A -w                
NAMESPACE     NAME                                            READY   STATUS    RESTARTS   AGE     IP               NODE                                           NOMINATED NODE   READINESS GATES
game-2048     deployment-2048-xxx             1/1     Running   0          9s      xxx  ip-xxx.us-west-2.compute.internal    <none>           <none>
game-2048     deployment-2048-xxx             1/1     Running   0          9s      xxx  ip-xxx.us-west-2.compute.internal    <none>           <none>
game-2048     deployment-2048-xxx             1/1     Running   0          9s      xxx  ip-xxx.us-west-2.compute.internal    <none>           <none>
game-2048     deployment-2048-xxx             1/1     Running   0          9s      xxx  ip-xxx.us-west-2.compute.internal    <none>           <none>
game-2048     deployment-2048-xxx             1/1     Running   0          9s      xxx  ip-xxx.us-west-2.compute.internal    <none>           <none>

Sample deployment 2048-pods-labels.yaml snippet

apiVersion: apps/v1
kind: Deployment
...
spec:
  selector:
    matchLabels:
      app.kubernetes.io/name: app-2048
  replicas: 5
  template:
    metadata:
      labels:
        app.kubernetes.io/name: app-2048
        testlabel: testvalue
        ...
...

Corresponding output with readiness gates configured:

% kubectl apply  -f 2048-pods-labels.yaml        
deployment.apps/deployment-2048 created
% kubectl get pods -owide -A -w                  
NAMESPACE     NAME                                            READY   STATUS    RESTARTS   AGE    IP               NODE                                           NOMINATED NODE   READINESS GATES
game-2048     deployment-xxx                1/1     Running   0          9s     xxx  ip-xxx.us-west-2.compute.internal   <none>           0/1
game-2048     deployment-xxx                1/1     Running   0          9s     xxx  ip-xxx.us-west-2.compute.internal   <none>           0/1
game-2048     deployment-xxx                1/1     Running   0          9s     xxx  ip-xxx.us-west-2.compute.internal   <none>           0/1
game-2048     deployment-xxx                1/1     Running   0          9s     xxx  ip-xxx.us-west-2.compute.internal   <none>           0/1
game-2048     deployment-xxx                1/1     Running   0          9s     xxx  ip-xxx.us-west-2.compute.internal   <none>           0/1

Test Scenario 2:
One entry added for matchLabels and additional entry added to matchExpressions besides default entry.

webhook config snippet :

  namespaceSelector:
    matchExpressions:
    - key: elbv2.k8s.aws/pod-readiness-gate-inject
      operator: In
      values:
      - enabled
  objectSelector:
    matchExpressions:
    - key: app.kubernetes.io/name
      operator: NotIn
      values:
      - aws-load-balancer-controller
    - key: app.kubernetes.io/name
      operator: In
      values:
      - app-2048
    matchLabels:
      testlabel: testvalue

Test Scenario 3:
No entry added for matchLabels and additional entry added to matchExpressions besides default entry.

webhook config snippet:

  namespaceSelector:
    matchExpressions:
    - key: elbv2.k8s.aws/pod-readiness-gate-inject
      operator: In
      values:
      - enabled
  objectSelector:
    matchExpressions:
    - key: app.kubernetes.io/name
      operator: NotIn
      values:
      - aws-load-balancer-controller
    - key: app.kubernetes.io/name
      operator: In
      values:
      - app-2048

Test Scenario 4:
No addition entry added to matchLabels or matchExpressions besides matchExpressions default entry.

webhook config snippet

  namespaceSelector:
    matchExpressions:
    - key: elbv2.k8s.aws/pod-readiness-gate-inject
      operator: In
      values:
      - enabled
  objectSelector:
    matchExpressions:
    - key: app.kubernetes.io/name
      operator: NotIn
      values:
      - aws-load-balancer-controller

I was able to observe the correct behaviour for all the 4 scenarios listed

k8s-ci-robot · 2021-12-08T19:01:20Z

Hi @Shreya027. 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 · 2021-12-08T19:10:01Z

Codecov Report

Merging #2407 (a693256) into main (2f525fd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2407   +/-   ##
=======================================
  Coverage   53.35%   53.35%           
=======================================
  Files         140      140           
  Lines        7962     7962           
=======================================
  Hits         4248     4248           
  Misses       3397     3397           
  Partials      317      317

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f525fd...a693256. Read the comment docs.

kishorj

Need to update the README.md as well documenting objectSelector

kishorj · 2021-12-08T22:23:09Z

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

@@ -232,3 +232,13 @@ backendSecurityGroup:

 # disableRestrictedSecurityGroupRules specifies whether to disable creating port-range restricted security group rules for traffic
 disableRestrictedSecurityGroupRules:
+
+#objectSelector for webhook


nit: space after #

kishorj · 2021-12-08T22:25:59Z

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

+#objectSelector for webhook
+objectSelector:
+  matchExpressions:
+  #- key: <key>


nit: space after #

add proper indentation

Shreya027 · 2021-12-09T18:26:51Z

@kishorj Hi have added section in README for the change. I'm guessing doc will be updated separately.

kishorj · 2021-12-17T00:00:53Z

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

@@ -214,3 +214,5 @@ The default values set by the application itself can be confirmed [here](https:/
 | `enableBackendSecurityGroup`                | If enabled, controller uses shared security group for backend traffic                                    | `true`                                                                             |
 | `backendSecurityGroup`                      | Backend security group to use instead of auto created one if the feature is enabled                      | ``                                                                                 |
 | `disableRestrictedSecurityGroupRules`       | If disabled, controller will not specify port range restriction in the backend security group rules      | `false`                                                                            |
+| `objectSelector.matchExpressions`           | Webhook configuration to select specific pods by specifying the expression to be matched                 | `app.kubernetes.io/name NotIn {{ include "aws-load-balancer-controller.name" . }}` |


The default should be None in this case. The load balancer controller name is currently hardcoded in the template and not configurable.

kishorj · 2021-12-17T00:23:59Z

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

@@ -214,3 +214,5 @@ The default values set by the application itself can be confirmed [here](https:/
 | `enableBackendSecurityGroup`                | If enabled, controller uses shared security group for backend traffic                                    | `true`                                                                             |
 | `backendSecurityGroup`                      | Backend security group to use instead of auto created one if the feature is enabled                      | ``                                                                                 |
 | `disableRestrictedSecurityGroupRules`       | If disabled, controller will not specify port range restriction in the backend security group rules      | `false`                                                                            |
+| `objectSelector.matchExpressions`           | Webhook configuration to select specific pods by specifying the expression to be matched                 | `app.kubernetes.io/name NotIn {{ include "aws-load-balancer-controller.name" . }}` |


Suggested change

| `objectSelector.matchExpressions` | Webhook configuration to select specific pods by specifying the expression to be matched | `app.kubernetes.io/name NotIn {{ include "aws-load-balancer-controller.name" . }}` |

| `objectSelector.matchExpressions` | Webhook configuration to select specific pods by specifying the expression to be matched | None |

k8s-ci-robot · 2021-12-17T16:55:41Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

kishorj · 2021-12-17T16:55:41Z

/ok-to-test

Shreya027 · 2021-12-17T23:12:36Z

/retest

kishorj · 2021-12-17T23:41:49Z

/retest

kishorj · 2021-12-18T01:20:42Z

/lgtm

* Enable setting custom objectSelector for webhook kubernetes-sigs#650 * Add formatting changes * Add webhook changes to README * Read update suggestion * Update READ table header to original

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 8, 2021

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 8, 2021

k8s-ci-robot requested review from kishorj and M00nF1sh December 8, 2021 19:01

kishorj reviewed Dec 8, 2021

View reviewed changes

Shreya027 force-pushed the webhook branch from a0affb4 to 065b0ab Compare December 9, 2021 18:23

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 9, 2021

Shreya027 requested a review from kishorj December 9, 2021 18:25

kishorj added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 16, 2021

kishorj reviewed Dec 17, 2021

View reviewed changes

Shreya Anil Naik added 4 commits December 17, 2021 19:44

Enable setting custom objectSelector for webhook kubernetes-sigs#650

5be5b30

Add formatting changes

e56f56c

Add webhook changes to README

fb55d2d

Read update suggestion

8c6e461

Shreya027 force-pushed the webhook branch from 065b0ab to 8c6e461 Compare December 17, 2021 14:15

Update READ table header to original

a693256

Shreya027 requested a review from kishorj December 17, 2021 14:23

kishorj approved these changes Dec 17, 2021

View reviewed changes

k8s-ci-robot assigned kishorj Dec 18, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2021

k8s-ci-robot merged commit d1a22e0 into kubernetes-sigs:main Dec 18, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable setting custom objectSelector for webhook #2407

Enable setting custom objectSelector for webhook #2407

Shreya027 commented Dec 8, 2021

k8s-ci-robot commented Dec 8, 2021

codecov-commenter commented Dec 8, 2021 •

edited

Loading

kishorj left a comment

kishorj Dec 8, 2021 •

edited

Loading

kishorj Dec 8, 2021

kishorj Dec 8, 2021

Shreya027 commented Dec 9, 2021

kishorj Dec 17, 2021

kishorj Dec 17, 2021

k8s-ci-robot commented Dec 17, 2021

kishorj commented Dec 17, 2021

Shreya027 commented Dec 17, 2021

kishorj commented Dec 17, 2021

kishorj commented Dec 18, 2021

	\| `objectSelector.matchExpressions` \| Webhook configuration to select specific pods by specifying the expression to be matched \| `app.kubernetes.io/name NotIn {{ include "aws-load-balancer-controller.name" . }}` \|
	\| `objectSelector.matchExpressions` \| Webhook configuration to select specific pods by specifying the expression to be matched \| None \|

Enable setting custom objectSelector for webhook #2407

Enable setting custom objectSelector for webhook #2407

Conversation

Shreya027 commented Dec 8, 2021

Issue

Description

Checklist

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

k8s-ci-robot commented Dec 8, 2021

codecov-commenter commented Dec 8, 2021 • edited Loading

Codecov Report

kishorj left a comment

Choose a reason for hiding this comment

kishorj Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

kishorj Dec 8, 2021

Choose a reason for hiding this comment

kishorj Dec 8, 2021

Choose a reason for hiding this comment

Shreya027 commented Dec 9, 2021

kishorj Dec 17, 2021

Choose a reason for hiding this comment

kishorj Dec 17, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 17, 2021

kishorj commented Dec 17, 2021

Shreya027 commented Dec 17, 2021

kishorj commented Dec 17, 2021

kishorj commented Dec 18, 2021

codecov-commenter commented Dec 8, 2021 •

edited

Loading

kishorj Dec 8, 2021 •

edited

Loading