Improve parsing of annotations and use of Ingress wrapper #3474

aledbf · 2018-11-28T00:42:19Z

What this PR does / why we need it:

Continues the work started in #3437 and introduce a new local store that contains the original Ingress and the parsed annotations.

Which issue this PR fixes:

fixes #2494
fixes #3473
fixes #3468
fixes #3489

ElvinEfendi · 2018-11-28T06:57:23Z

internal/ingress/controller/store/ingress_annotation.go

-// IngressAnnotationsLister makes a Store that lists annotations in Ingress rules.
-type IngressAnnotationsLister struct {
+// IngressWithAnnotationsLister makes a Store that lists Ingress rules with annotations already parsed
+type IngressWithAnnotationsLister struct {
 	cache.Store
 }

 // ByKey returns the Ingress annotations matching key in the local Ingress annotations Store.


can you adjust this comment as well

ElvinEfendi · 2018-11-28T07:04:11Z

Why are you making all those internal/ingress/annotations/* changes? Can you give high level overview of what's the source of problem (and why #3437 was not enough) and how are you fixing it?

aledbf · 2018-11-28T10:11:48Z

Why are you making all those internal/ingress/annotations/* changes?

Consistency in the parsing method and also simplify the validations to set the defaults

Can you give high level overview of what's the source of problem (and why #3437 was not enough) and how are you fixing it?

The change in #3437 is not enough because at the end we are still joining the ingress and annotations when we build the model. The change here ensures we only extract the annotations when there's an event and we always use a single store, without any logic to get the ingresses

aledbf · 2018-12-01T00:57:08Z

~~@ElvinEfendi I used #3479 for the last commit~~

ElvinEfendi · 2018-12-02T17:20:47Z

internal/ingress/controller/store/store.go

-	var ingresses []*ingress.Ingress
-	for _, item := range s.listers.Ingress.List() {
-		ing := item.(*extensions.Ingress)
-		if !class.IsValid(ing) {


How's this case handled now? I don't see this function being used in new changes.

hmm looks like this is being handled in ingress event handlers and this was redundant

ElvinEfendi · 2018-12-02T17:30:52Z

internal/ingress/controller/store/store.go

@@ -597,9 +594,26 @@ func (s *k8sStore) extractAnnotations(ing *extensions.Ingress) {
 	key := k8s.MetaNamespaceKey(ing)


The comment seems to be outdated - it does not describe what the function does.

Also should we call this function syncIngress now?

ElvinEfendi · 2018-12-02T17:38:03Z

internal/ingress/annotations/modsecurity/main_test.go

@@ -39,24 +39,24 @@ func TestParse(t *testing.T) {

 	testCases := []struct {
 		annotations map[string]string
-		expected    Config


why are you switching this to pointer?

ElvinEfendi · 2018-12-02T17:40:47Z

internal/ingress/annotations/proxy/main.go

 	}

-	pb, err := parser.GetStringAnnotation("proxy-buffering", ing)
-	if err != nil || pb == "" {


why did you change all these to not check for == ""?

I just added a new commit that avoids empty annotations

ElvinEfendi · 2018-12-02T17:44:38Z

internal/ingress/controller/store/ingress_annotation.go

 	i, exists, err := il.GetByKey(key)
 	if err != nil {
 		return nil, err
 	}
 	if !exists {
 		return nil, NotExistsError(key)
 	}
-	return i.(*annotations.Ingress), nil


Is annotations.Ingress being used anywhere after this PR?

Only in the Ingress struct (ParsedAnnotations *annotations.Ingress)

ElvinEfendi · 2018-12-02T17:47:54Z

internal/ingress/controller/store/store.go

@@ -132,13 +129,13 @@ type Informer struct {

 // Lister contains object listers (stores).
 type Lister struct {
-	Ingress           IngressLister


Why do we still need this?

We cannot get rid of this because the informer still needs this type.

Does this mean we will be storing extensions.Ingress objects in both here and IngressWithAnnotation? That sounds like unnecessary duplication.

I'm not too familiar with how k8s go client works, but it does not make sense to not be able to configure informer without storing the objects. We should not need IngressLister, since all the ingress objects are already going to be stored in IngressWithAnnotation.

Does this mean we will be storing extensions.Ingress objects in both here and IngressWithAnnotation? That sounds like unnecessary duplication.

I know but there is no way to have a custom type (IngressWithAnnotation) in the informer.

I'm not too familiar with how k8s go client works, but it does not make sense to not be able to configure informer without storing the objects

You need to use a store to be able to handle deltas.

ElvinEfendi · 2018-12-05T11:31:05Z

/lgtm

https://github.com/kubernetes/ingress-nginx/pull/3474/files#r239025364 can be addressed later if it's an actual concern.

k8s-ci-robot · 2018-12-05T11:31:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

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~~ [ElvinEfendi,aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ElvinEfendi · 2018-12-05T11:48:22Z

How's this PR fixing #3489?

ElvinEfendi · 2018-12-11T11:00:26Z

How's this PR fixing #3489?

#3489 was reopened, this PR isn't fixing it.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 28, 2018

k8s-ci-robot requested review from antoineco and ElvinEfendi November 28, 2018 00:42

aledbf assigned ElvinEfendi Nov 28, 2018

aledbf force-pushed the improve-parsing branch 3 times, most recently from e2517c0 to 08ace5a Compare November 28, 2018 01:53

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 28, 2018

aledbf force-pushed the improve-parsing branch from 08ace5a to 17f99ab Compare November 28, 2018 02:15

ElvinEfendi reviewed Nov 28, 2018

View reviewed changes

aledbf force-pushed the improve-parsing branch from 17f99ab to 36d19f2 Compare November 28, 2018 10:08

aledbf force-pushed the improve-parsing branch 4 times, most recently from 37d992f to 8cefb74 Compare November 30, 2018 22:56

aledbf force-pushed the improve-parsing branch 3 times, most recently from 664a1d9 to f8ae100 Compare December 1, 2018 01:49

ElvinEfendi reviewed Dec 2, 2018

View reviewed changes

aledbf added 2 commits December 2, 2018 15:35

Improve parsing of annotations and use of Ingress wrapper

67808c0

Only copy fields being used

f78e2e3

aledbf force-pushed the improve-parsing branch 2 times, most recently from 7b205a8 to 733f9a7 Compare December 2, 2018 18:52

Annotations cannot being empty

497246f

aledbf force-pushed the improve-parsing branch from 733f9a7 to 497246f Compare December 2, 2018 19:08

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2018

k8s-ci-robot merged commit c4ba238 into kubernetes:master Dec 5, 2018

aledbf deleted the improve-parsing branch December 5, 2018 12:02

ElvinEfendi mentioned this pull request Dec 17, 2018

fix status updated: make sure ingress.status is copied #3569

Merged

wayt mentioned this pull request Jan 7, 2019

Fix a bug in Ingress update handler #3633

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve parsing of annotations and use of Ingress wrapper #3474

Improve parsing of annotations and use of Ingress wrapper #3474

aledbf commented Nov 28, 2018 •

edited

Loading

ElvinEfendi Nov 28, 2018

ElvinEfendi commented Nov 28, 2018 •

edited

Loading

aledbf commented Nov 28, 2018

aledbf commented Dec 1, 2018 •

edited

Loading

ElvinEfendi Dec 2, 2018

ElvinEfendi Dec 2, 2018

ElvinEfendi Dec 2, 2018

aledbf Dec 2, 2018

ElvinEfendi Dec 2, 2018

aledbf Dec 2, 2018

ElvinEfendi Dec 2, 2018

aledbf Dec 2, 2018

ElvinEfendi Dec 2, 2018

aledbf Dec 2, 2018

ElvinEfendi Dec 2, 2018

aledbf Dec 2, 2018

ElvinEfendi Dec 5, 2018

aledbf Dec 5, 2018

ElvinEfendi commented Dec 5, 2018

k8s-ci-robot commented Dec 5, 2018

ElvinEfendi commented Dec 5, 2018

ElvinEfendi commented Dec 11, 2018

		@@ -597,9 +594,26 @@ func (s k8sStore) extractAnnotations(ing extensions.Ingress) {
		key := k8s.MetaNamespaceKey(ing)

Improve parsing of annotations and use of Ingress wrapper #3474

Improve parsing of annotations and use of Ingress wrapper #3474

Conversation

aledbf commented Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

ElvinEfendi commented Nov 28, 2018 • edited Loading

aledbf commented Nov 28, 2018

aledbf commented Dec 1, 2018 • 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

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

ElvinEfendi commented Dec 5, 2018

k8s-ci-robot commented Dec 5, 2018

ElvinEfendi commented Dec 5, 2018

ElvinEfendi commented Dec 11, 2018

aledbf commented Nov 28, 2018 •

edited

Loading

ElvinEfendi commented Nov 28, 2018 •

edited

Loading

aledbf commented Dec 1, 2018 •

edited

Loading