Make Ingress validating webhook ignore ingresses not managed by AWS LBC #3272
Conversation
k8s-ci-robot
added
cncf-cla: yes
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3272 +/- ##
==========================================
+ Coverage 54.70% 54.71% +0.01%
==========================================
Files 148 148
Lines 8590 8604 +14
==========================================
+ Hits 4699 4708 +9
- Misses 3559 3564 +5
Partials 332 332
☔ View full report in Codecov by Sentry. |
|
/retest |
kishorj
added
the
tide/merge-method-squash
|
/retest |
2 similar comments
|
/retest |
|
/retest |
| @@ -88,6 +98,21 @@ func (v *ingressValidator) ValidateDelete(ctx context.Context, obj runtime.Objec | |||
| return nil | |||
| } | |||
|
|
|||
| // checkIngressClass checks to see if this ingress is handled by this controller. | |||
| func (v *ingressValidator) checkIngressClass(ctx context.Context, ing *networking.Ingress) (bool, error) { | |||
There was a problem hiding this comment.
nit: the checkIngressClass name is confusing. Since the purpose here is to check if the webhook should skip validating further, we could rename to either of checkIngressSkipValidation, shouldSkipFurtherValidation, checkIngressUnmanaged. Let me know if you think the current name is good enough.
Changes look good otherwise.
There was a problem hiding this comment.
This function is validating the kubernetes.io/ingress.class annotation, the IngressClassName field, and the spec.controller of the IngressClass referenced by the IngressClassName field. Skipping further validation is only one possible outcome; it can fail validation if the IngressClassName references a nonexistent IngressClass.
For naming these sub-validation methods, I tend to prefer using the name of the field or subsection that is being validated. The existing naming pattern isn't entirely consistent; this could be fixed in follow-up.
It looks like checkIngressClassUsage() is being made redundant; it could be removed in this or the followup PR.
There was a problem hiding this comment.
lets refactor in a follow up PR
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johngmyers, 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
…to 2.6.0 Merge in DEL/aws-load-balancer-controller-fork from merge-up to main * commit '195e896b0efbd467694bb9a19de7c5a12c5dde8c': (71 commits) check the canary test result and exit if it failed Apply suggestions from code review Update docs/guide/service/annotations.md Addressing the comment Remove dependency on aws-sdk-go-v2 (kubernetes-sigs#3320) Update live docs for NLB-SG feature release cut v2.6.0 release refactor targetGroupBinding network builder Add support for NLB security groups Allow TLS 1.2 with restricted ciphers for webhooks Update the RSA filter for Cert discovery Doc: Add note for rename behavior of IngressGroup (kubernetes-sigs#3283) Make Ingress validating webhook ignore ingresses not managed by AWS LBC (kubernetes-sigs#3272) add oliviassss as reviewer fix the race condition in pod cache and endpoint resolver Bump github.com/onsi/ginkgo/v2 from 2.6.0 to 2.11.0 Bump github.com/aws/aws-sdk-go from 1.44.184 to 1.44.294 (kubernetes-sigs#3271) Provide better explanation of failure to find a subnet (kubernetes-sigs#3292) test/framework: replace deprecated ioutil.ReadAll (kubernetes-sigs#3256) Add warning in doc for ServiceMutatorWebhook (kubernetes-sigs#3180) ...
Issue
None
Description
Makes the validating admission webhook for Ingress permit creation and modification all ingresses for which it is able to determine that it is not going to manage.
Also fixes a bug where LBC would incorrectly manage Ingresses with an IngressClassName specifying a different controller when LBC was configured to manage Ingresses without either annotation or IngressClassName.
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯