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

Fix ingress class #362

Merged
merged 1 commit into from
Mar 3, 2017
Merged

Fix ingress class #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 10 additions & 4 deletions controllers/nginx/pkg/cmd/controller/nginx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,10 @@ const (
)

var (
tmplPath = "/etc/nginx/template/nginx.tmpl"
cfgPath = "/etc/nginx/nginx.conf"
binary = "/usr/sbin/nginx"
tmplPath = "/etc/nginx/template/nginx.tmpl"
cfgPath = "/etc/nginx/nginx.conf"
binary = "/usr/sbin/nginx"
defIngressClass = "nginx"
)

// newNGINXController creates a new NGINX Ingress controller.
Expand Down Expand Up @@ -256,7 +257,12 @@ func (n NGINXController) Info() *ingress.BackendInfo {

// OverrideFlags customize NGINX controller flags
func (n NGINXController) OverrideFlags(flags *pflag.FlagSet) {
flags.Set("ingress-class", "nginx")
flags.Set("ingress-class", defIngressClass)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}

// DefaultIngressClass just return the default ingress class
func (n NGINXController) DefaultIngressClass() string {
return defIngressClass
}

// testTemplate checks if the NGINX configuration inside the byte array is valid
Expand Down
15 changes: 8 additions & 7 deletions core/pkg/ingress/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ type Configuration struct {
UDPConfigMapName string
DefaultSSLCertificate string
DefaultHealthzURL string
DefaultIngressClass string
// optional
PublishService string
// Backend is the particular implementation to be used.
Expand Down Expand Up @@ -166,7 +167,7 @@ func newIngressController(config *Configuration) *GenericController {
ingEventHandler := cache.ResourceEventHandlerFuncs{
AddFunc: func(obj interface{}) {
addIng := obj.(*extensions.Ingress)
if !IsValidClass(addIng, config.IngressClass) {
if !IsValidClass(addIng, config) {
glog.Infof("ignoring add for ingress %v based on annotation %v", addIng.Name, ingressClassKey)
return
}
Expand All @@ -175,7 +176,7 @@ func newIngressController(config *Configuration) *GenericController {
},
DeleteFunc: func(obj interface{}) {
delIng := obj.(*extensions.Ingress)
if !IsValidClass(delIng, config.IngressClass) {
if !IsValidClass(delIng, config) {
glog.Infof("ignoring delete for ingress %v based on annotation %v", delIng.Name, ingressClassKey)
return
}
Expand All @@ -185,7 +186,7 @@ func newIngressController(config *Configuration) *GenericController {
UpdateFunc: func(old, cur interface{}) {
oldIng := old.(*extensions.Ingress)
curIng := cur.(*extensions.Ingress)
if !IsValidClass(curIng, config.IngressClass) && !IsValidClass(oldIng, config.IngressClass) {
if !IsValidClass(curIng, config) && !IsValidClass(oldIng, config) {
return
}

Expand Down Expand Up @@ -588,7 +589,7 @@ func (ic *GenericController) getBackendServers() ([]*ingress.Backend, []*ingress
for _, ingIf := range ings {
ing := ingIf.(*extensions.Ingress)

if !IsValidClass(ing, ic.cfg.IngressClass) {
if !IsValidClass(ing, ic.cfg) {
continue
}

Expand Down Expand Up @@ -711,7 +712,7 @@ func (ic *GenericController) createUpstreams(data []interface{}) map[string]*ing
for _, ingIf := range data {
ing := ingIf.(*extensions.Ingress)

if !IsValidClass(ing, ic.cfg.IngressClass) {
if !IsValidClass(ing, ic.cfg) {
continue
}

Expand Down Expand Up @@ -872,7 +873,7 @@ func (ic *GenericController) createServers(data []interface{},
// initialize all the servers
for _, ingIf := range data {
ing := ingIf.(*extensions.Ingress)
if !IsValidClass(ing, ic.cfg.IngressClass) {
if !IsValidClass(ing, ic.cfg) {
continue
}

Expand Down Expand Up @@ -912,7 +913,7 @@ func (ic *GenericController) createServers(data []interface{},
// configure default location and SSL
for _, ingIf := range data {
ing := ingIf.(*extensions.Ingress)
if !IsValidClass(ing, ic.cfg.IngressClass) {
if !IsValidClass(ing, ic.cfg) {
continue
}

Expand Down
1 change: 1 addition & 0 deletions core/pkg/ingress/controller/launch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ func NewIngressController(backend ingress.Controller) *GenericController {
ResyncPeriod: *resyncPeriod,
DefaultService: *defaultSvc,
IngressClass: *ingressClass,
DefaultIngressClass: backend.DefaultIngressClass(),
Namespace: *watchNamespace,
ConfigMapName: *configMap,
TCPConfigMapName: *tcpConfigMapName,
Expand Down
18 changes: 12 additions & 6 deletions core/pkg/ingress/controller/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,20 +88,26 @@ func matchHostnames(pattern, host string) bool {
// IsValidClass returns true if the given Ingress either doesn't specify
// the ingress.class annotation, or it's set to the configured in the
// ingress controller.
func IsValidClass(ing *extensions.Ingress, class string) bool {
if class == "" {
return true
}
func IsValidClass(ing *extensions.Ingress, config *Configuration) bool {
currentIngClass := config.IngressClass

cc, err := parser.GetStringAnnotation(ingressClassKey, ing)
if err != nil && !errors.IsMissingAnnotations(err) {
glog.Warningf("unexpected error reading ingress annotation: %v", err)
}
if cc == "" {

// 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 (cc == "" && currentIngClass == "") || (currentIngClass == config.DefaultIngressClass) {
return true
}

return cc == class
return cc == currentIngClass
}

func mergeLocationAnnotations(loc *ingress.Location, anns map[string]interface{}) {
Expand Down
38 changes: 32 additions & 6 deletions core/pkg/ingress/controller/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ func (fe *fakeError) Error() string {
return "fakeError"
}

// just 2 combinations are valid
// 1 - ingress with default class (or no args) | blank annotation on ingress | valid
// 2 - ingress with specified class | same annotation on ingress | valid
//
// this combinations are invalid
// 3 - ingress with default class (or no args) | fixed annotation on ingress | invalid
// 4 - ingress with specified class | different annotation on ingress | invalid
func TestIsValidClass(t *testing.T) {
ing := &extensions.Ingress{
ObjectMeta: api.ObjectMeta{
Expand All @@ -47,27 +54,46 @@ func TestIsValidClass(t *testing.T) {
},
}

b := IsValidClass(ing, "")
config := &Configuration{DefaultIngressClass: "nginx", IngressClass: ""}
b := IsValidClass(ing, config)
if !b {
t.Error("Expected a valid class (missing annotation)")
}

config.IngressClass = "custom"
b = IsValidClass(ing, config)
if b {
t.Error("Expected a invalid class (missing annotation)")
}

data := map[string]string{}
data[ingressClassKey] = "custom"
ing.SetAnnotations(data)

b = IsValidClass(ing, "custom")
b = IsValidClass(ing, config)
if !b {
t.Errorf("Expected valid class but %v returned", b)
}
b = IsValidClass(ing, "nginx")

config.IngressClass = "killer"
b = IsValidClass(ing, config)
if b {
t.Errorf("Expected invalid class but %v returned", b)
}
b = IsValidClass(ing, "")
if !b {

data[ingressClassKey] = ""
ing.SetAnnotations(data)
config.IngressClass = "killer"
b = IsValidClass(ing, config)
if b {
t.Errorf("Expected invalid class but %v returned", b)
}

config.IngressClass = ""
b = IsValidClass(ing, config)
if !b {
t.Errorf("Expected valid class but %v returned", b)
}

}

func TestIsHostValid(t *testing.T) {
Expand Down
2 changes: 2 additions & 0 deletions core/pkg/ingress/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ type Controller interface {
Info() *BackendInfo
// OverrideFlags allow the customization of the flags in the backend
OverrideFlags(*pflag.FlagSet)
// DefaultIngressClass just return the default ingress class
DefaultIngressClass() string
}

// StoreLister returns the configured stores for ingresses, services,
Expand Down