Proposal/Feature add flag --skip-empty-annotation to honor only annotated resources. #664

nutellinoit · 2020-05-14T20:31:33Z

What this PR does / why we need it:
This PR addresses the need to limit kong ingress controller on only the annotated resources. Mainly when using multiple ingress controller in a Kubernetes cluster.

Special notes for your reviewer:
I added only a flag on the ingress controller --skip-empty-annotation that when set to true skips the evaluation of all non annotated resources.

I tried the setup on minikube with some ingresses and kong CRDs and everything is working. Also make test-all is passing and I added some new test case on internal/ingress/annotations/annotations_test.go test suite.

Thank you!

CLAassistant · 2020-05-14T20:31:37Z

All committers have signed the CLA.

hbagdi · 2020-05-19T20:58:46Z

We are discussing this internally if this should be added or not. This is something that has been asked for quite a bit. We need to figure out what would be a good default value and what would be the recommendation going forward, especially with Ingress V1 in Kubernetes 1.18 which changes how class should be interpreted. We will look into this post 0.9 release which is scheduled for next week.

nutellinoit · 2020-05-20T07:08:38Z

Thank you @hbagdi !

hbagdi

Thanks for the PR. We need to polish this up a little bit but this is a step in the right direction.

hbagdi · 2020-06-02T20:24:36Z

docs/concepts/deployment.md

@@ -264,6 +264,7 @@ There are a few different ways of accomplishing this:
  where any ingress resource that is not annotated is picked up.
  Therefore with different ingress class then `kong`, you have to use that
  ingress class with every Kong CRD object (plugin, consumer) which you use.
+  You can force the honoring of only `kong` ingress class using the flag `--skip-empty-annotation=true`.


Please remove the doc update from this PR. We will do doc updates later.

hbagdi · 2020-06-02T20:24:54Z

go.mod

@@ -26,7 +26,9 @@ require (
 	github.com/tidwall/gjson v1.2.1
 	github.com/tidwall/match v1.0.1 // indirect
 	github.com/tidwall/pretty v0.0.0-20190325153808-1166b9ac2b65 // indirect
+	golang.org/x/lint v0.0.0-20200302205851-738671d3881b // indirect


These dependencies shouldn't be added. Please clean go.mod and go.sum files.

hbagdi · 2020-06-02T20:26:31Z

internal/ingress/annotations/annotations.go

@@ -46,15 +46,15 @@ const (
 	DefaultIngressClass = "kong"
 )

-func validIngress(ingressAnnotationValue, ingressClass string) bool {
+func validIngress(ingressAnnotationValue, ingressClass string, skipEmptyAnnotation bool) bool {


Can you rename the variable to skipClasslessIngress?
Ingress class is a formalized concept in Ingress v1 and we would like to reuse parts of this PR for that as well.

hbagdi · 2020-06-02T20:27:20Z

cli/ingress-controller/flags.go

@@ -158,6 +159,8 @@ Kong's Admin SSL certificate.`)
 	// Resource filtering
 	flags.String("watch-namespace", apiv1.NamespaceAll,
 		`Namespace to watch for Ingress. Default is to watch all namespaces`)
+	flags.Bool("skip-empty-annotation", false,


Please rename this flag to skip-classless-ingress-v1beta1.
We plan to introduce another flag to control this behavior for Ingress v1 as well.

hbagdi · 2020-06-02T20:28:47Z

cli/ingress-controller/flags.go

-	IngressClass   string
-	ElectionID     string
+	WatchNamespace      string
+	SkipEmptyAnnotation bool


Please rename this field to SkipClasslessIngressV1beta1. Please rename other fields accordingly.

hbagdi · 2020-06-02T20:30:13Z

internal/ingress/annotations/annotations.go

 	// we have 2 valid combinations
 	// 1 - ingress with default class | blank annotation on ingress
 	// 2 - ingress with specific class | same annotation on ingress
 	//
 	// and 2 invalid combinations
 	// 3 - ingress with default class | fixed annotation on ingress
 	// 4 - ingress with specific class | different annotation on ingress
-	if ingressAnnotationValue == "" && ingressClass == DefaultIngressClass {
+	if ingressAnnotationValue == "" && !skipEmptyAnnotation && ingressClass == DefaultIngressClass {


Can you please update the code comments according to this change?
This is a tricky logic and we should document it for future-selves.

hbagdi · 2020-06-02T20:31:06Z

internal/ingress/annotations/annotations_test.go

-		controller string
-		isValid    bool
+		ingress      string
+		forceIngress bool


Please rename forceIngress to something meaningful.
How about SkipClasslessIngress?

hbagdi · 2020-06-02T20:33:36Z

internal/ingress/annotations/annotations_test.go

+		{"custom", false, "custom", true},
+		{"", false, "killer", false},
+		{"custom", false, "kong", false},
+		{"custom", true, "kong", false},


Can we add {"", true, "custom", false}?

hbagdi · 2020-06-02T20:34:05Z

internal/ingress/store/store.go

@@ -102,11 +104,12 @@ type CacheStores struct {
 }

 // New creates a new object store to be used in the ingress controller
-func New(cs CacheStores, ingressClass string) Storer {
+func New(cs CacheStores, ingressClass string, skipEmptyAnnotation bool) Storer {


This needs to be refactored away but let's keep that out of scope for this PR.

hbagdi · 2020-06-02T20:35:48Z

Also, please rebase on latest next branch.

nutellinoit · 2020-06-04T10:18:02Z

Hi @hbagdi , i will make the requested changes as soon as possible, thank you for the feedback!

…o/ingress.class: 'kong' annotation.

…/skip-empty-annotation/skip-classless-ingress-v1beta1/g, s/skipEmptyAnnotation/skipClasslessIngress/g, s/forceIngress/skipClasslessIngress/g, go fmt, added one more test

nutellinoit · 2020-06-05T08:23:58Z

Hi @hbagdi , i did all the requested changes!

hbagdi

This needs an integration test. Please look in parser_test.go to add that and then this should be good to go.

hbagdi · 2020-06-08T17:51:07Z

internal/ingress/store/fake_store.go

@@ -140,7 +140,7 @@ func NewFakeStore(

 			KnativeIngress: knativeIngressStore,
 		},
-		isValidIngresClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"),
+		isValidIngresClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong", false),


Can you make this configurable and tweak NewFakeStore to pass this in?

hbagdi · 2020-06-24T20:40:38Z

Friendly ping on an update on this.

nutellinoit · 2020-06-25T16:17:38Z

Friendly ping on an update on this.

I will look into it this week! Thanks for the ping

nutellinoit · 2020-06-26T07:04:15Z

Hi @hbagdi , i did the requested changes!

hbagdi

This a good step towards where we want to be but it is not the final state. We will do a few changes before this is actually released. The updated plan is documented in a Google doc in #690.

hbagdi · 2020-07-08T21:20:38Z

Thank you for your contribution. Please fill out the following form to claim your contributor swag:
https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributor-t-shirt.

hbagdi added please review labels May 18, 2020

hbagdi suggested changes Jun 2, 2020

View reviewed changes

hbagdi changed the base branch from master to next June 2, 2020 20:35

hbagdi added changes requested and removed please review labels Jun 2, 2020

nutellinoit added 5 commits June 5, 2020 09:45

Adding flag flags.go

56e9022

Added flag --skip-empty-annotation to force usage of the kubernetes.i…

71667dc

…o/ingress.class: 'kong' annotation.

Fixing docs

f2a7a7e

Reverted go.mod and go.sum

3f42d7b

Reverted docs

8e5a39b

nutellinoit force-pushed the feature/force-ingress-class branch from 296673c to 8e5a39b Compare June 5, 2020 07:52

nutellinoit added 3 commits June 5, 2020 10:09

Renamed flags, s/SkipEmptyAnnotation/SkipClasslessIngressV1beta1/g, s…

3341d8e

…/skip-empty-annotation/skip-classless-ingress-v1beta1/g, s/skipEmptyAnnotation/skipClasslessIngress/g, s/forceIngress/skipClasslessIngress/g, go fmt, added one more test

Fixing long line lint error

32ba2e1

One more time linting fix

97629d8

hbagdi suggested changes Jun 8, 2020

View reviewed changes

nutellinoit added 2 commits June 26, 2020 08:15

Adding parameter SkipClasslessIngress on FakeObjects in fake_store.go

823eaac

Added integration test on parser_test.go : TestKongSkipClasslessIngress

90edd8e

hbagdi approved these changes Jul 7, 2020

View reviewed changes

hbagdi merged commit 4bf2405 into Kong:next Jul 7, 2020

rainest mentioned this pull request Jul 10, 2020

Rework handling of ingress.class annotation #767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal/Feature add flag --skip-empty-annotation to honor only annotated resources. #664

Proposal/Feature add flag --skip-empty-annotation to honor only annotated resources. #664

nutellinoit commented May 14, 2020

CLAassistant commented May 14, 2020 •

edited

Loading

hbagdi commented May 19, 2020

nutellinoit commented May 20, 2020

hbagdi left a comment

hbagdi Jun 2, 2020

hbagdi Jun 2, 2020

hbagdi Jun 2, 2020

hbagdi Jun 2, 2020

hbagdi Jun 2, 2020

hbagdi Jun 2, 2020

hbagdi Jun 2, 2020

hbagdi Jun 2, 2020

hbagdi Jun 2, 2020

hbagdi commented Jun 2, 2020

nutellinoit commented Jun 4, 2020

nutellinoit commented Jun 5, 2020

hbagdi left a comment

hbagdi Jun 8, 2020

nutellinoit Jun 9, 2020

hbagdi commented Jun 24, 2020

nutellinoit commented Jun 25, 2020

nutellinoit commented Jun 26, 2020

hbagdi left a comment

hbagdi commented Jul 8, 2020

Proposal/Feature add flag --skip-empty-annotation to honor only annotated resources. #664

Proposal/Feature add flag --skip-empty-annotation to honor only annotated resources. #664

Conversation

nutellinoit commented May 14, 2020

CLAassistant commented May 14, 2020 • edited Loading

hbagdi commented May 19, 2020

nutellinoit commented May 20, 2020

hbagdi left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Jun 2, 2020

nutellinoit commented Jun 4, 2020

nutellinoit commented Jun 5, 2020

hbagdi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Jun 24, 2020

nutellinoit commented Jun 25, 2020

nutellinoit commented Jun 26, 2020

hbagdi left a comment

Choose a reason for hiding this comment

hbagdi commented Jul 8, 2020

CLAassistant commented May 14, 2020 •

edited

Loading