Skip to content

Commit

Permalink
Merge pull request #418 from aledbf/fix-status-update
Browse files Browse the repository at this point in the history
Only update Ingress status for the configured class
  • Loading branch information
aledbf authored Mar 10, 2017
2 parents db96c9d + ad24784 commit ea7a936
Show file tree
Hide file tree
Showing 6 changed files with 139 additions and 106 deletions.
55 changes: 55 additions & 0 deletions core/pkg/ingress/annotations/class/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
Copyright 2015 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package class

import (
"github.com/golang/glog"
"k8s.io/kubernetes/pkg/apis/extensions"

"k8s.io/ingress/core/pkg/ingress/annotations/parser"
"k8s.io/ingress/core/pkg/ingress/errors"
)

const (
// IngressKey picks a specific "class" for the Ingress.
// The controller only processes Ingresses with this annotation either
// unset, or set to either the configured value or the empty string.
IngressKey = "kubernetes.io/ingress.class"
)

// IsValid 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 IsValid(ing *extensions.Ingress, controller, defClass string) bool {
ingress, err := parser.GetStringAnnotation(IngressKey, ing)
if err != nil && !errors.IsMissingAnnotations(err) {
glog.Warningf("unexpected error reading ingress annotation: %v", err)
}

// 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 ingress == "" && controller == defClass {
return true
}

return ingress == controller
}
58 changes: 58 additions & 0 deletions core/pkg/ingress/annotations/class/main_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
Copyright 2017 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package class

import (
"testing"

"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/apis/extensions"
)

func TestIsValidClass(t *testing.T) {
tests := []struct {
ingress string
controller string
defClass string
isValid bool
}{
{"", "", "nginx", true},
{"", "nginx", "nginx", true},
{"nginx", "nginx", "nginx", true},
{"custom", "custom", "nginx", true},
{"", "killer", "nginx", false},
{"", "", "nginx", true},
{"custom", "nginx", "nginx", false},
}

ing := &extensions.Ingress{
ObjectMeta: api.ObjectMeta{
Name: "foo",
Namespace: api.NamespaceDefault,
},
}

data := map[string]string{}
ing.SetAnnotations(data)
for _, test := range tests {
ing.Annotations[IngressKey] = test.ingress
b := IsValid(ing, test.controller, test.defClass)
if b != test.isValid {
t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b)
}
}
}
35 changes: 17 additions & 18 deletions core/pkg/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (

cache_store "k8s.io/ingress/core/pkg/cache"
"k8s.io/ingress/core/pkg/ingress"
"k8s.io/ingress/core/pkg/ingress/annotations/class"
"k8s.io/ingress/core/pkg/ingress/annotations/healthcheck"
"k8s.io/ingress/core/pkg/ingress/annotations/proxy"
"k8s.io/ingress/core/pkg/ingress/annotations/service"
Expand All @@ -58,11 +59,6 @@ const (
defServerName = "_"
podStoreSyncedPollPeriod = 1 * time.Second
rootLocation = "/"

// ingressClassKey picks a specific "class" for the Ingress. The controller
// only processes Ingresses with this annotation either unset, or set
// to either the configured value or the empty string.
ingressClassKey = "kubernetes.io/ingress.class"
)

var (
Expand Down Expand Up @@ -168,17 +164,17 @@ func newIngressController(config *Configuration) *GenericController {
ingEventHandler := cache.ResourceEventHandlerFuncs{
AddFunc: func(obj interface{}) {
addIng := obj.(*extensions.Ingress)
if !IsValidClass(addIng, config) {
glog.Infof("ignoring add for ingress %v based on annotation %v", addIng.Name, ingressClassKey)
if !class.IsValid(addIng, ic.cfg.IngressClass, ic.cfg.DefaultIngressClass) {
glog.Infof("ignoring add for ingress %v based on annotation %v", addIng.Name, class.IngressKey)
return
}
ic.recorder.Eventf(addIng, api.EventTypeNormal, "CREATE", fmt.Sprintf("Ingress %s/%s", addIng.Namespace, addIng.Name))
ic.syncQueue.Enqueue(obj)
},
DeleteFunc: func(obj interface{}) {
delIng := obj.(*extensions.Ingress)
if !IsValidClass(delIng, config) {
glog.Infof("ignoring delete for ingress %v based on annotation %v", delIng.Name, ingressClassKey)
if !class.IsValid(delIng, ic.cfg.IngressClass, ic.cfg.DefaultIngressClass) {
glog.Infof("ignoring delete for ingress %v based on annotation %v", delIng.Name, class.IngressKey)
return
}
ic.recorder.Eventf(delIng, api.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", delIng.Namespace, delIng.Name))
Expand All @@ -187,7 +183,8 @@ func newIngressController(config *Configuration) *GenericController {
UpdateFunc: func(old, cur interface{}) {
oldIng := old.(*extensions.Ingress)
curIng := cur.(*extensions.Ingress)
if !IsValidClass(curIng, config) && !IsValidClass(oldIng, config) {
if !class.IsValid(curIng, ic.cfg.IngressClass, ic.cfg.DefaultIngressClass) &&
!class.IsValid(oldIng, ic.cfg.IngressClass, ic.cfg.DefaultIngressClass) {
return
}

Expand Down Expand Up @@ -303,10 +300,12 @@ func newIngressController(config *Configuration) *GenericController {

if config.UpdateStatus {
ic.syncStatus = status.NewStatusSyncer(status.Config{
Client: config.Client,
PublishService: ic.cfg.PublishService,
IngressLister: ic.ingLister,
ElectionID: config.ElectionID,
Client: config.Client,
PublishService: ic.cfg.PublishService,
IngressLister: ic.ingLister,
ElectionID: config.ElectionID,
IngressClass: config.IngressClass,
DefaultIngressClass: config.DefaultIngressClass,
})
} else {
glog.Warning("Update of ingress status is disabled (flag --update-status=false was specified)")
Expand Down Expand Up @@ -590,7 +589,7 @@ func (ic *GenericController) getBackendServers() ([]*ingress.Backend, []*ingress
for _, ingIf := range ings {
ing := ingIf.(*extensions.Ingress)

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

Expand Down Expand Up @@ -713,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) {
if !class.IsValid(ing, ic.cfg.IngressClass, ic.cfg.DefaultIngressClass) {
continue
}

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

Expand Down Expand Up @@ -925,7 +924,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) {
if !class.IsValid(ing, ic.cfg.IngressClass, ic.cfg.DefaultIngressClass) {
continue
}

Expand Down
29 changes: 0 additions & 29 deletions core/pkg/ingress/controller/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,7 @@ import (
"github.com/golang/glog"
"github.com/imdario/mergo"

"k8s.io/kubernetes/pkg/apis/extensions"

"k8s.io/ingress/core/pkg/ingress"
"k8s.io/ingress/core/pkg/ingress/annotations/parser"
"k8s.io/ingress/core/pkg/ingress/errors"
)

// DeniedKeyName name of the key that contains the reason to deny a location
Expand Down Expand Up @@ -85,31 +81,6 @@ func matchHostnames(pattern, host string) bool {
return true
}

// 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, 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)
}

// 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 == currentIngClass
}

func mergeLocationAnnotations(loc *ingress.Location, anns map[string]interface{}) {
if _, ok := anns[DeniedKeyName]; ok {
loc.Denied = anns[DeniedKeyName].(error)
Expand Down
59 changes: 0 additions & 59 deletions core/pkg/ingress/controller/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ import (
"k8s.io/ingress/core/pkg/ingress/annotations/proxy"
"k8s.io/ingress/core/pkg/ingress/annotations/ratelimit"
"k8s.io/ingress/core/pkg/ingress/annotations/rewrite"
"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/apis/extensions"
)

type fakeError struct{}
Expand All @@ -39,63 +37,6 @@ 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{
Name: "foo",
Namespace: api.NamespaceDefault,
},
}

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, config)
if !b {
t.Errorf("Expected valid class but %v returned", b)
}

config.IngressClass = "killer"
b = IsValidClass(ing, config)
if b {
t.Errorf("Expected invalid class but %v returned", 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) {
fkCert := &ingress.SSLCert{
CAFileName: "foo",
Expand Down
9 changes: 9 additions & 0 deletions core/pkg/ingress/status/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"k8s.io/kubernetes/pkg/util/wait"

cache_store "k8s.io/ingress/core/pkg/cache"
"k8s.io/ingress/core/pkg/ingress/annotations/class"
"k8s.io/ingress/core/pkg/k8s"
"k8s.io/ingress/core/pkg/strings"
"k8s.io/ingress/core/pkg/task"
Expand All @@ -53,6 +54,9 @@ type Config struct {
PublishService string
IngressLister cache_store.StoreToIngressLister
ElectionID string

DefaultIngressClass string
IngressClass string
}

// statusSync keeps the status IP in each Ingress rule updated executing a periodic check
Expand Down Expand Up @@ -243,6 +247,11 @@ func (s *statusSync) updateStatus(newIPs []api.LoadBalancerIngress) {
wg.Add(len(ings))
for _, cur := range ings {
ing := cur.(*extensions.Ingress)

if !class.IsValid(ing, s.Config.IngressClass, s.Config.DefaultIngressClass) {
continue
}

go func(wg *sync.WaitGroup) {
defer wg.Done()
ingClient := s.Client.Extensions().Ingresses(ing.Namespace)
Expand Down

0 comments on commit ea7a936

Please sign in to comment.