Validate Ingress condition annotations #2735

r-erema · 2022-07-24T13:41:22Z

Issue

#2042

Description

Earlier if at least 1 ingress in the group has an invalid annotation the rest of the ingresses in the group aren't took into account during building listener rules.

I've handled the condition validation error separately to prevent the termination of the process of the building listener rules for valid ingresses if such an error occurs.

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 · 2022-07-24T13:41:25Z

The committers listed above are authorized under a signed CLA.

✅ login: r-erema / name: Roma Erema (c3cf11f973f16c81703cda5549dcc766c484f7d0)

k8s-ci-robot · 2022-07-24T13:41:29Z

Welcome @r-erema!

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 · 2022-07-24T13:41:30Z

Hi @r-erema. 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.

r-erema · 2022-07-26T06:01:16Z

/assign @M00nF1sh

r-erema · 2022-09-08T19:12:16Z

@M00nF1sh @kishorj Hi, kindly reminder about PR. Please, estimate when it can get to the review. Thanks!

k8s-triage-robot · 2022-12-07T19:38:36Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2023-01-06T20:38:27Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

r-erema · 2023-01-07T03:48:54Z

/remove-lifecycle

johngmyers

/remove-lifecycle rotten

johngmyers · 2023-01-10T06:34:45Z

pkg/ingress/config_types.go

@@ -238,7 +240,7 @@ type HTTPRequestMethodConditionConfig struct {

 func (c *HTTPRequestMethodConditionConfig) validate() error {
 	if len(c.Values) == 0 {
-		return errors.New("values cannot be empty")
+		return errors.Wrap(errConditionValidation, "values cannot be empty")


Prefer fmt.Errorf() and %w for wrapping errors.

But this isn't the use case for wrapping; one should probably use a custom error type and a constructor.

@johngmyers Agree regarding to fmt.Errorf().

But this isn't the use case for wrapping

What are cases for wrapping and what for creation custom type? It will be good to understand for me to make it properly in the future. Thanks!

The canonical case for wrapping is when you received an error from doing a call and you want to add information about the context in which the error occurred before passing it up to your caller. I tend to start the wrapping message with a gerund, for example "writing file %q".

A custom error type is for when the caller needs to programmatically check if a particular error condition occurred. In this case, the caller wants to know if there was an invalid rule condition.

@johngmyers Thanks a lot!

johngmyers · 2023-01-10T06:55:13Z

pkg/ingress/model_build_listener_rules.go

+					t.logger.Error(err, "Ingress has invalid annotation for service, thus will be ignored",
+						"Ingress",
+						ing,
+						"Service name",
+						path.Backend.Service.Name,
+					)


This isn't the clearest error message. And logging an entire ClassifiedIngress is noisy.

Perhaps something like "Ignoring Ingress NAMESPACE/NAME: alb.ingress.kubernetes.io/conditions.SERVICE annotation: ERROR"

@johngmyers Sticking the same style of logging error in that project, where I noticed first go a common error message at the beggining and then entities involved in the error in key-values format, e.g.

aws-load-balancer-controller/controllers/elbv2/eventhandlers/node.go

Line 92 in a92e689

h.logger.Error(err, "failed to get nodeSelector", "TargetGroupBinding", tgb)

aws-load-balancer-controller/pkg/backend/endpoint_resolver.go

Line 187 in a92e689

r.logger.Error(err, "ignore pod Endpoint without non-exist nodeInfo", "podKey", podKey.String())

I'll change your suggested error format a little, but anyway make it less noisy

t.logger.Error(err, "Ignoring Ingress invalid annotation", "Ingress", fmt.Sprintf("%s/%s", ing.Ing.Namespace, ing.Ing.Name), "Service", path.Backend.Service.Name, )

Consider logging the annotation key. It might not be obvious that it's the actions. annotation this is referring to.

It might not be obvious that it's the actions.

@johngmyers Probably you've meant conditions., I'll add that info to error.

Seems I unintentionally proved my point:-)

johngmyers · 2023-01-10T06:57:16Z

/ok-to-test

johngmyers · 2023-01-10T07:07:22Z

Would it be possible to put these checks in the validating admission webhook so such ingresses don't get created in the first place?

johngmyers · 2023-01-20T06:05:16Z

If you want to explore the validating admission webhook path, the code is in webhooks/networking/ingress_validator.go.

Although the existing ingress_validator code uses generic errors, it would be better to return API validation errors from apimachinery. Examples of this can be found in my PR #2945.

johngmyers · 2023-01-28T06:41:58Z

webhooks/networking/ingress_validator_test.go

+				annotationParser:              annotationParser,
+				classAnnotationMatcher:        classAnnotationMatcher,
+				disableIngressGroupAnnotation: tt.fields.disableIngressGroupAnnotation,
+				logger:                        &log.NullLogger{},


Suggested change

logger: &log.NullLogger{},

logger: logr.New(&log.NullLogSink{}),

The issue was in depricated &log.NullLogger{}, need to be used logr.Discard() instead. Thanks anyway!

johngmyers · 2023-01-28T06:44:24Z

You should change the title of the PR. Something like "Validate Ingress condition annotations".

r-erema · 2023-01-28T10:18:49Z

You should change the title of the PR. Something like "Validate Ingress condition annotations".

@johngmyers Done

I have the failed job "pull-aws-load-balancer-controller-unit-test"

Fixed as well

kishorj

Thanks for your contribution. We still need the validation block in buildConditions(). I'm fine with the changes otherwise.

kishorj · 2023-02-04T00:14:05Z

pkg/ingress/enhanced_backend_builder.go

@@ -164,11 +164,7 @@ func (b *defaultEnhancedBackendBuilder) buildConditions(_ context.Context, ingAn
 	if err != nil {
 		return nil, err
 	}
-	for _, condition := range conditions {


Please keep this validation block in place. We've seen feature requests to optionally disable validation webhook for ingress. There would also be a case where the invalid ingress already exists before the controller got updated. We still want to error out from model builder if validation fails.

I do not think we should support disabling our validation webhook for ingress. If someone disables it, they can deal with the fallout of configurations the webhook would have prevented.

We should not add unnecessary complexity.

As for me I'm not a fan of duplication of logic/checkings etc, so I'd prefer to keep validation in 1 place. @kishorj @johngmyers can we meet consolidate decision(maybe by community voting, etc.)?

We discussed further on this matter in the provider-aws meeting. Since there is a possibility of having ingress resources from a prior version (before the webhook validation), we still do these validation for 2.5.0. We can clean up the validation in future releases.

Validation returned back

k8s-ci-robot · 2023-02-20T22:53:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kishorj, r-erema

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 · 2023-02-20T22:53:50Z

/lgtm

Merge in DEL/aws-load-balancer-controller-fork from DELIVERY-8202_pull_upstream to main * commit '49805eac72ec533acdbc2580d6f57c68a9cad45c': update the default base image (kubernetes-sigs#3075) update recommended IAM policy template (kubernetes-sigs#3068) update to discovery.k8s.io/v1 (kubernetes-sigs#3072) validation ingress condition annotation in validation webhook (kubernetes-sigs#2735) Fix conciseLogger's incorrect call to variadic func (kubernetes-sigs#3066) Verify CRDs are up to date in merge check (kubernetes-sigs#3022) Refactor model builder test (kubernetes-sigs#3024)

…etes-sigs#2735)

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 24, 2022

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 Jul 24, 2022

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 24, 2022

k8s-ci-robot requested review from kishorj and M00nF1sh July 24, 2022 13:41

r-erema force-pushed the 2042-stuck-cintroller-ingress-invalid-condition branch from e316b31 to c3cf11f Compare July 25, 2022 06:25

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 Jul 25, 2022

r-erema force-pushed the 2042-stuck-cintroller-ingress-invalid-condition branch from c3cf11f to bb0b89d Compare July 25, 2022 07:46

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

k8s-ci-robot assigned M00nF1sh Jul 26, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 7, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 6, 2023

johngmyers suggested changes Jan 10, 2023

View reviewed changes

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 10, 2023

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 Jan 10, 2023

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

johngmyers reviewed Jan 28, 2023

View reviewed changes

r-erema force-pushed the 2042-stuck-cintroller-ingress-invalid-condition branch from 17a5cc3 to 5841971 Compare January 28, 2023 09:49

r-erema changed the title ~~Ignoring ingresses in the group with invalid conditions in the annotation~~ Validate Ingress condition annotations Jan 28, 2023

r-erema requested review from johngmyers and removed request for kishorj and M00nF1sh January 28, 2023 10:20

kishorj reviewed Feb 4, 2023

View reviewed changes

validation ingress condition annotation in validation webhook

d8ec5c3

r-erema force-pushed the 2042-stuck-cintroller-ingress-invalid-condition branch from 5841971 to d8ec5c3 Compare February 19, 2023 05:44

r-erema requested review from kishorj and removed request for johngmyers February 19, 2023 07:04

kishorj approved these changes Feb 20, 2023

View reviewed changes

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

k8s-ci-robot assigned kishorj Feb 20, 2023

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

k8s-ci-robot merged commit 4530894 into kubernetes-sigs:main Feb 21, 2023

r-erema mentioned this pull request Jul 14, 2023

REQUEST: New membership for r-erema kubernetes/org#4329

Closed

9 tasks

johngmyers mentioned this pull request Aug 23, 2023

Add johngmyers as approver #3356

Merged

12 tasks

Timothy-Dougherty pushed a commit to adammw/aws-load-balancer-controller that referenced this pull request Nov 9, 2023

validation ingress condition annotation in validation webhook (kubern…

f944783

…etes-sigs#2735)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Ingress condition annotations #2735

Validate Ingress condition annotations #2735

r-erema commented Jul 24, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jul 24, 2022 •

edited

Loading

k8s-ci-robot commented Jul 24, 2022

k8s-ci-robot commented Jul 24, 2022

r-erema commented Jul 26, 2022 •

edited

Loading

r-erema commented Sep 8, 2022 •

edited

Loading

k8s-triage-robot commented Dec 7, 2022

k8s-triage-robot commented Jan 6, 2023

r-erema commented Jan 7, 2023

johngmyers left a comment

johngmyers Jan 10, 2023

r-erema Jan 19, 2023 •

edited

Loading

johngmyers Jan 20, 2023 •

edited

Loading

r-erema Jan 20, 2023

johngmyers Jan 10, 2023

r-erema Jan 19, 2023 •

edited

Loading

johngmyers Jan 20, 2023

r-erema Jan 20, 2023

johngmyers Jan 20, 2023

johngmyers commented Jan 10, 2023

johngmyers commented Jan 10, 2023

johngmyers commented Jan 20, 2023 •

edited

Loading

johngmyers Jan 28, 2023

r-erema Jan 28, 2023

johngmyers commented Jan 28, 2023

r-erema commented Jan 28, 2023

kishorj left a comment

kishorj Feb 4, 2023

johngmyers Feb 4, 2023 •

edited

Loading

r-erema Feb 5, 2023 •

edited

Loading

kishorj Feb 17, 2023

r-erema Feb 19, 2023

k8s-ci-robot commented Feb 20, 2023

kishorj commented Feb 20, 2023

	logger: &log.NullLogger{},
	logger: logr.New(&log.NullLogSink{}),

Validate Ingress condition annotations #2735

Validate Ingress condition annotations #2735

Conversation

r-erema commented Jul 24, 2022 • edited Loading

Issue

Description

Checklist

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

linux-foundation-easycla bot commented Jul 24, 2022 • edited Loading

k8s-ci-robot commented Jul 24, 2022

k8s-ci-robot commented Jul 24, 2022

r-erema commented Jul 26, 2022 • edited Loading

r-erema commented Sep 8, 2022 • edited Loading

k8s-triage-robot commented Dec 7, 2022

k8s-triage-robot commented Jan 6, 2023

r-erema commented Jan 7, 2023

johngmyers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-erema Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

johngmyers Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-erema Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers commented Jan 10, 2023

johngmyers commented Jan 10, 2023

johngmyers commented Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers commented Jan 28, 2023

r-erema commented Jan 28, 2023

kishorj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

r-erema Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 20, 2023

kishorj commented Feb 20, 2023

r-erema commented Jul 24, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jul 24, 2022 •

edited

Loading

r-erema commented Jul 26, 2022 •

edited

Loading

r-erema commented Sep 8, 2022 •

edited

Loading

r-erema Jan 19, 2023 •

edited

Loading

johngmyers Jan 20, 2023 •

edited

Loading

r-erema Jan 19, 2023 •

edited

Loading

johngmyers commented Jan 20, 2023 •

edited

Loading

johngmyers Feb 4, 2023 •

edited

Loading

r-erema Feb 5, 2023 •

edited

Loading