Fix ingress class

[gianrubio]

Description

When args ingress-class is empty, the default class is nginx so all the ingress without the annotation "kubernetes.io/ingress.class" must be builded. The current controller build all the ingress including ingress with anotation.

Ex. empty arg

Controller

containers:
      - args:
        - /nginx-ingress-controller    
....

Empty class annotation (controller must build this)

kind: Ingress
metadata:
  annotations:    
    kubernetes.io/tls-acme: "true"
....

Fixed class annotation (controller must NOT build this)

kind: Ingress

metadata:
  annotations:    
    kubernetes.io/tls-acme: "true"
    kubernetes.io/ingress.class: "my-fake-class"
....

On the other side when args ingress-class is configured, just the ingress with the same class must be builded.

Ex. with ingress args

Controller

containers:
      - args:
        - /nginx-ingress-controller    
        - --ingress-class=internal-nginx
....

Empty class annotation (controller must NOT build this)

kind: Ingress
metadata:
  annotations:    
    kubernetes.io/tls-acme: "true"
....

Fixed class annotation (controller must build this)

kind: Ingress

metadata:
  annotations:    
    kubernetes.io/tls-acme: "true"
    kubernetes.io/ingress.class: "internal-nginx
....

Fix #282

[k8s-reviewable]

This change is 

[coveralls]

Coverage decreased (-0.007%) to 45.343% when pulling 2ddba72 on gianrubio:fix-ingress-class into f5f9f5e on kubernetes:master.

[aledbf]

@gianrubio why the case "Empty class annotation (controller must NOT build this)" when a custom class is used? This should be allowed (empty class means all the ingress controllers will use the ingress rule)

[gianrubio]

... Empty class means all the ingress controllers will use the ingress rule.

When I'm specifying the ingress-class it's because I want to restrict this ingress to only run specific ingress. It's the same behaviour for the service/pods selector.

If I want to all the controllers use the same rule (blank anotation), I'll start a ingress without ingress-class.

What do you think?

[aledbf]

this should be done only when the value of the flag != ""

[gianrubio]

This is the same as current behaviour, I just moved to a variable.
Ingress-class will be configured on https://github.com/gianrubio/ingress/blob/2ddba72baa451abffd036de4306a91062e82a146/core/pkg/ingress/controller/launch.go#L146

[aledbf]

@gianrubio add a warning if flag != "" or flag != "nginx" saying that only ingress rules with the class flag value will be processed

[aledbf]

What do you think?

I think we need more input from other users :)

To that end I think we need to merge this PR (so is included in the next beta) and add in the log a warning when the user specifies a different class indicating that only ingress rules with the same class will be evaluated.

[whereisaaron]

I've often wondered why the 'class' is an annotation and not a label? It it were a label, the controller could use a label selector to watch namespaces for only changes to its labelled Ingress resources, instead of every Ingress Controller watching a namespace having to be notified, parse and check every single Ingress for the annotation. On a larger cluster labels would seem more efficient/scaleable?

[aledbf]

On a larger cluster labels would seem more efficient/scaleable?

No because the ingress controller uses a local store where we already have all the ingress rules (we do not query the api server). This store uses event notifications to reduce the traffic between the controller and the api server.

[aledbf]

/lgtm

[k8s-ci-robot]

@aledbf: you can't LGTM a PR unless you are an assignee.

In response to this comment:

/lgtm

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. I understand the commands that are listed here.

[aledbf]

/lgtm