fix: check default ingclass when ingclass is nill #2963

yasinlachiny · 2023-01-09T00:05:12Z

Issue

Description

Per the documentation, the default ingress class is the one with the ingressclass.kubernetes.io/is-default-class annotation
So if the ingressClassName is nil we should check for the default ingress class.

To reproduce the issue:
1- Set alb ingressClass as a default
2- Create an ingress without any ingressClassName and kubernetes.io/ingress.class
3- It won't create an alb in AWS but by this PR it will create because the default is alb

I check the default ingress with GetDefaultIngressClass function.

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 · 2023-01-09T00:05:21Z

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

johngmyers · 2023-01-09T16:40:26Z

/ok-to-test

codecov-commenter · 2023-01-09T22:51:11Z

Codecov Report

Base: 54.09% // Head: 54.13% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (cd7bf2c) compared to base (9c3f5a2).
Patch coverage: 92.59% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2963      +/-   ##
==========================================
+ Coverage   54.09%   54.13%   +0.04%     
==========================================
  Files         144      144              
  Lines        8291     8303      +12     
==========================================
+ Hits         4485     4495      +10     
- Misses       3479     3480       +1     
- Partials      327      328       +1

Impacted Files	Coverage Δ
pkg/ingress/class_loader.go	`87.09% <88.88%> (+0.43%)`	⬆️
pkg/ingress/group_loader.go	`86.23% <100.00%> (-0.49%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

johngmyers · 2023-01-10T06:18:39Z

pkg/ingress/class_loader.go

+	var defaultClassFound bool
+	ingClassList := &networking.IngressClassList{}
+	if err := l.client.List(ctx, ingClassList); err != nil {
+		return "", fmt.Errorf("failed to fetch ingressClasses, %v", err.Error())


Suggested change

return "", fmt.Errorf("failed to fetch ingressClasses, %v", err.Error())

return "", fmt.Errorf("fetching ingressClasses: %w", err.Error())

err.Error() is string. I changed the error format based on other errors

johngmyers · 2023-01-10T06:23:46Z

pkg/ingress/class_loader_test.go

@@ -59,7 +60,7 @@ func Test_defaultClassLoader_Load(t *testing.T) {
 					},
 				},
 			},
-			wantErr: errors.New("invalid ingress class: spec.ingressClassName is nil"),
+			wantErr: nil,


Should specify want instead of wantErr.

Need to add a test where there is a default IngressClass. It should assert that the default IngressClass was used.

Need to add a test where there are two IngressClasses annotated as being the default.

I added multiple test cases

when IngressClass is ALB - Multiple default classes specified

when IngressClassName unspecified - Multiple default classes specified

when IngressClassName unspecified - Default class specified

when IngressClassName unspecified - Default class unspecified

johngmyers · 2023-01-10T06:26:54Z

pkg/ingress/class_loader.go

@@ -43,9 +46,40 @@ type defaultClassLoader struct {
 	client client.Client
 }

+// GetDefaultIngressClass returns the default IngressClass from the list of IngressClasses.
+// If multiple IngressClasses are marked as the default, it returns an error.


Document the behavior for when there is no default IngressClass.

Hi @johngmyers
Thank you for reviewing my code.

I added the document about it.

yasinlachiny · 2023-01-17T11:39:14Z

Hi @johngmyers
Could you please review my new code if you have time?
I also have another PR that is blocked by this one.

johngmyers · 2023-01-18T01:21:17Z

Thanks for doing this!

/lgtm
/assign @M00nF1sh

yasinlachiny · 2023-01-24T12:09:03Z

Hi @kishorj @M00nF1sh ,
Could you please review my code if you have time?

yasinlachiny · 2023-02-03T14:35:57Z

Hi @kishorj @M00nF1sh ,
Could you please review my code if you have time?🙏
I also have another #2942 that is blocked by this one.

kishorj

Thanks for your contribution.

kishorj · 2023-02-03T23:23:13Z

/lgtm

k8s-ci-robot · 2023-02-03T23:23:24Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

* fix: check default ingclass when ingclass is nill * fix: change the error when the defaul class and ingClassName are nill * test: multiple default class specified * test: when IngressClassName unspecified - Default class unspecified

M00nF1sh · 2024-10-27T06:15:21Z

@yasinlachiny sorry for overlooked this PR, i think we don't need this handling given k8s have a admission webhook to inject the default ingress class, not sure why would we need to check for the annotation in our code
ref: https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/admission/network/defaultingressclass/admission.go

k8s-ci-robot requested review from kishorj and M00nF1sh January 9, 2023 00:05

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

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 9, 2023

fix: check default ingclass when ingclass is nill

b8641e1

yasinlachiny force-pushed the add-ing-class branch from 2486c5f to b8641e1 Compare January 9, 2023 00:14

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 9, 2023

yasinlachiny force-pushed the add-ing-class branch from 26b2186 to e21ebc5 Compare January 9, 2023 22:43

fix: change the error when the defaul class and ingClassName are nill

f0b82e3

yasinlachiny force-pushed the add-ing-class branch from e21ebc5 to f0b82e3 Compare January 9, 2023 22:44

johngmyers suggested changes Jan 10, 2023

View reviewed changes

test: multiple default class specified

cd7bf2c

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

test: when IngressClassName unspecified - Default class unspecified

eb9f773

k8s-ci-robot assigned M00nF1sh and johngmyers Jan 18, 2023

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

johngmyers mentioned this pull request Jan 24, 2023

Add johngmyers as reviewer #2999

Merged

kishorj added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 3, 2023

kishorj approved these changes Feb 3, 2023

View reviewed changes

k8s-ci-robot assigned kishorj Feb 3, 2023

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

k8s-ci-robot merged commit fb96e58 into kubernetes-sigs:main Feb 4, 2023

This was referenced Apr 14, 2023

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

Closed

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

Merged

johngmyers mentioned this pull request Aug 23, 2023

Add johngmyers as approver #3356

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check default ingclass when ingclass is nill #2963

fix: check default ingclass when ingclass is nill #2963

yasinlachiny commented Jan 9, 2023

k8s-ci-robot commented Jan 9, 2023

johngmyers commented Jan 9, 2023

codecov-commenter commented Jan 9, 2023 •

edited

Loading

johngmyers Jan 10, 2023

yasinlachiny Jan 10, 2023

johngmyers Jan 10, 2023

yasinlachiny Jan 10, 2023 •

edited

Loading

johngmyers Jan 10, 2023

yasinlachiny Jan 10, 2023

yasinlachiny commented Jan 17, 2023

johngmyers commented Jan 18, 2023

yasinlachiny commented Jan 24, 2023

yasinlachiny commented Feb 3, 2023

kishorj left a comment

kishorj commented Feb 3, 2023

k8s-ci-robot commented Feb 3, 2023

M00nF1sh commented Oct 27, 2024

	return "", fmt.Errorf("failed to fetch ingressClasses, %v", err.Error())
	return "", fmt.Errorf("fetching ingressClasses: %w", err.Error())

fix: check default ingclass when ingclass is nill #2963

fix: check default ingclass when ingclass is nill #2963

Conversation

yasinlachiny commented Jan 9, 2023

Issue

Description

Checklist

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

k8s-ci-robot commented Jan 9, 2023

johngmyers commented Jan 9, 2023

codecov-commenter commented Jan 9, 2023 • edited Loading

Codecov Report

johngmyers Jan 10, 2023

Choose a reason for hiding this comment

yasinlachiny Jan 10, 2023

Choose a reason for hiding this comment

johngmyers Jan 10, 2023

Choose a reason for hiding this comment

yasinlachiny Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

johngmyers Jan 10, 2023

Choose a reason for hiding this comment

yasinlachiny Jan 10, 2023

Choose a reason for hiding this comment

yasinlachiny commented Jan 17, 2023

johngmyers commented Jan 18, 2023

yasinlachiny commented Jan 24, 2023

yasinlachiny commented Feb 3, 2023

kishorj left a comment

Choose a reason for hiding this comment

kishorj commented Feb 3, 2023

k8s-ci-robot commented Feb 3, 2023

M00nF1sh commented Oct 27, 2024

codecov-commenter commented Jan 9, 2023 •

edited

Loading

yasinlachiny Jan 10, 2023 •

edited

Loading