Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support namespaced ingress without accessing the IngressClass #11223

Closed
wants to merge 17 commits into from
Closed

Support namespaced ingress without accessing the IngressClass #11223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
61343bb
Support namespaced ingressClass without accessing the IngresClass obj…
Apr 6, 2024
24c4ec9
fix indent issue
Apr 6, 2024
5ff4253
Fix indent issue
Apr 6, 2024
d14ca86
Rollback go.work.sum
Apr 6, 2024
43354dd
Rollback go.work.sum
Apr 6, 2024
f7556a3
Fix unitest issue
Apr 7, 2024
8ef4db3
fix unitest issue
Apr 8, 2024
cae5005
set ingressClassName and fix unitest issue
Apr 8, 2024
51c58c3
update unittest
Apr 8, 2024
c97e62a
skip IngressClassName validation when no permission on IngressClass
Apr 8, 2024
a436a32
fix indent issue
Apr 8, 2024
dadd1c0
indent-error-flow: if block ends with a return statement, so drop thi…
Apr 8, 2024
bd7f2e8
typo erro
Apr 8, 2024
a4a5367
fixed unitest error
Apr 8, 2024
2653b8e
fix unitest
Apr 8, 2024
7c2b047
fix unitest issue
Apr 8, 2024
9d3afe4
Merge branch 'main' into fix_namepaced_ingress_issue
yong-jie-gong Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion internal/ingress/controller/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1049,7 +1049,14 @@ func (s *k8sStore) GetService(key string) (*corev1.Service, error) {

func (s *k8sStore) GetIngressClass(ing *networkingv1.Ingress, icConfig *ingressclass.Configuration) (string, error) {
// First we try ingressClassName
if !icConfig.IgnoreIngressClass && ing.Spec.IngressClassName != nil {
if ing.Spec.IngressClassName != nil {
if icConfig.IgnoreIngressClass {
if icConfig.AnnotationValue == *ing.Spec.IngressClassName {
return *ing.Spec.IngressClassName, nil
}
Comment on lines +1053 to +1056
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so you ignore IngressClassName for one thing but use it for another thing?

This is not right IMHO, you are changing the semantics of two options, one public API of Ingress and one config of ingress-nginx ... this is a -1 as it is a breaking change for two things

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

==Before change, if IgnoreIngressClass is false and ing.Spec.IngressClassName is not empty, ingress-controller mandate the IngressClass validation. it mean fails directly if no read permission on IngressClass resource

==After change
if IgnoreIngressClass is false and ing.Spec.IngressClassName is not empty, stay as before. if IgnoreIngressClass is true && ( ing.Spec.IngressClassName == <ingress class defined by cli "--ingress-class" ), ignore the IngressClass resource matchness check

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to set IgnoreIngressClass to true or false ? Is there a helm chart related values-file key-value pair ?

Copy link
Member

@Gacko Gacko Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to set IgnoreIngressClass to true or false ? Is there a helm chart related values-file key-value pair ?

You can always pass it as controller.extraArgs. I never got why users would add new values to the chart for stuff that's just being passed directly in to the arguments. But that's not to be discussed here, just my 2 cents. 😅

if IgnoreIngressClass is false and ing.Spec.IngressClassName is not empty, stay as before. if IgnoreIngressClass is true && ( ing.Spec.IngressClassName == <ingress class defined by cli "--ingress-class" ), ignore the IngressClass resource matchness check

And that's what @aojea means: .spec.ingressClassName and what is should be used for is defined by a public API. We are not going to break this API.

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#ingressspec-v1-networking-k8s-io

ingressClassName is the name of an IngressClass cluster resource. Ingress controller implementations use this field to know whether they should be serving this Ingress resource, by a transitive connection (controller -> IngressClass -> Ingress resource). Although the kubernetes.io/ingress.class annotation (simple constant name) was never formally defined, it was widely supported by Ingress controllers to create a direct binding between Ingress controller and Ingress resources. Newly created Ingress resources should prefer using the field. However, even though the annotation is officially deprecated, for backwards compatibility reasons, ingress controllers should still honor that annotation if present.


return "", errors.Errorf("lack of permission on cluster IngressClass: %s, %s", *ing.Spec.IngressClassName, icConfig.AnnotationValue)
}
iclass, err := s.listers.IngressClass.ByKey(*ing.Spec.IngressClassName)
if err != nil {
return "", err
Expand Down
23 changes: 22 additions & 1 deletion test/e2e/settings/ingress_class.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,14 +645,35 @@ var _ = framework.IngressNginxDescribe("[Flag] ingress-class", func() {
Status(http.StatusOK)
})

ginkgo.It("should watch Ingress with matched Ingress.Spec.IngressClassName and CLI parameter --ingress-class", func() {
validHost := "validhostnameforingressclassname"
testIngressClassName := "testclass"

ing := framework.NewSingleIngress(validHost, "/", validHost, f.Namespace, framework.EchoService, 80, nil)
ing.Spec.IngressClassName = &testIngressClassName
f.EnsureIngress(ing)

f.WaitForNginxConfiguration(func(cfg string) bool {
return strings.Contains(cfg, "server_name "+validHost)
})

f.HTTPTestClient().
GET("/").
WithHeader("Host", validHost).
Expect().
Status(http.StatusOK)
})

ginkgo.It("should ignore Ingress with only IngressClassName", func() {
invalidHost := "noclassforyou"
unmatchedIngressClassName := "testclass-unmatched"

ing := framework.NewSingleIngress(invalidHost, "/", invalidHost, f.Namespace, framework.EchoService, 80, nil)
ing.Spec.IngressClassName = &unmatchedIngressClassName
f.EnsureIngress(ing)

f.WaitForNginxConfiguration(func(cfg string) bool {
return !strings.Contains(cfg, "server_name noclassforyou")
return !strings.Contains(cfg, "server_name "+invalidHost)
Comment on lines -655 to +676
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is the same thing as variable "invalidHost := "noclassforyou""

Uploading image.png…

})

f.HTTPTestClient().
Expand Down