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

Rework handling of ingress.class annotation #767

Merged
merged 53 commits into from
Aug 26, 2020
Merged

Rework handling of ingress.class annotation #767

Show file tree
Hide file tree
Changes from 50 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
c24b8a2
fix: correct function name typo
Jun 30, 2020
8f1cd94
wip: allow classless always
Jun 30, 2020
1c9ab54
wip: fix tests
Jun 30, 2020
b5d972d
wip: ignore classless KongClusterPlugin
Jun 30, 2020
2f431a3
wip: update comment
Jun 30, 2020
64098fe
wip: add warnings
Jul 2, 2020
cad2b23
wip: hacky hacky logging
Jul 2, 2020
75cc4da
wip: add notes from mtg
Jul 6, 2020
bc0a387
wip: refactor class handling
Jul 6, 2020
dd917ac
wip: remove superfluous check
Jul 7, 2020
513e394
wip: rename class handlers
Jul 7, 2020
dcc319c
wip: rename in prep to split reh
Jul 7, 2020
7c696a6
wip: split REHs
Jul 7, 2020
25e7730
wip: refactor validator calls
Jul 7, 2020
cbb0b2a
wip: merge branch 'next' into feat/follow-classless
Jul 9, 2020
742cd89
wip: merge branch next into feat/follow-classless
Jul 9, 2020
284880f
wip: fix incorrect merge
Jul 9, 2020
cc030d2
wip: fix lint/mod issues
Jul 9, 2020
0b8c7d1
wip: deps?
Jul 9, 2020
fd8eac4
wip: go decide what you want
Jul 9, 2020
baace98
wip: fix misaligned/misconfigured tests
Jul 9, 2020
960900a
wip: wrangle tests more
Jul 10, 2020
9456797
wip: fix fake store handling
Jul 10, 2020
c4265c8
wip: merge origin/next into feat/follow-classless
Jul 16, 2020
4f7c5a3
wip: merge origin/next
Aug 3, 2020
9d70471
wip: tidy go.mod
Aug 3, 2020
b00d7ed
wip: neutralize class filters in REH
Aug 3, 2020
5f621c3
wip: disable credential class checks
Aug 3, 2020
6158c25
pr: remove remaining REH class filters
Aug 4, 2020
8474870
pr: remove additional obsolete code
Aug 4, 2020
1ccd6eb
pr: replace old error handler
Aug 4, 2020
be804bf
chore: remove temporary comment
Aug 4, 2020
109061d
wip: refactor class filter
Aug 12, 2020
e946eb8
wip: remove garbage
Aug 12, 2020
a2997ab
wip: remove garbage
Aug 12, 2020
d869324
wip: clean up trial and error
Aug 12, 2020
daf05f5
pr: fix remaining store issues
Aug 13, 2020
02335b6
pr: strip out added errors
Aug 13, 2020
e361c0a
pr: rearrange constant definition
Aug 18, 2020
33e32ef
pr: rename handling variables
Aug 18, 2020
eca131e
pr: simplify returns and remove WIP comments
Aug 18, 2020
b076e95
pr: remove WIP comments
Aug 18, 2020
d9fbc20
pr: remove superfluous return
Aug 18, 2020
b0bfaa3
pr: add explanatory comments for test table
Aug 19, 2020
fb3ab86
store: establish new classless default behaviors
Aug 17, 2020
2d0d44b
pr: reconcile name changes
Aug 19, 2020
d933951
store: remove placeholder code
Aug 19, 2020
03fefa7
test: check both default and non-default classes
Aug 20, 2020
1bac620
pr: convert annotation test to use matching values
Aug 24, 2020
e737aad
pr: remove matching logic from fake store
Aug 24, 2020
8fc0042
pr: merge next into feat/follow-classless
Aug 26, 2020
3570563
pr: merge 'origin/next' into feat/follow-classless
Aug 26, 2020
1bac3a4
pr: remove superfluous test
Aug 26, 2020
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
12 changes: 7 additions & 5 deletions cli/ingress-controller/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,11 @@ type cliConfig struct {
KongCustomEntitiesSecret string

// Resource filtering
WatchNamespace string
SkipClasslessIngressV1beta1 bool
IngressClass string
ElectionID string
WatchNamespace string
ProcessClasslessIngressV1beta1 bool
ProcessClasslessKongConsumer bool
IngressClass string
ElectionID string

// Ingress Status publish resource
PublishService string
Expand Down Expand Up @@ -325,7 +326,8 @@ func parseFlags() (cliConfig, error) {

// Resource filtering
config.WatchNamespace = viper.GetString("watch-namespace")
config.SkipClasslessIngressV1beta1 = viper.GetBool("skip-classless-ingress-v1beta1")
config.ProcessClasslessIngressV1beta1 = viper.GetBool("process-classless-ingress-v1beta1")
config.ProcessClasslessKongConsumer = viper.GetBool("process-classless-kong-consumer")
config.IngressClass = viper.GetString("ingress-class")
config.ElectionID = viper.GetString("election-id")

Expand Down
9 changes: 3 additions & 6 deletions cli/ingress-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import (
"github.com/hashicorp/go-uuid"
"github.com/hbagdi/go-kong/kong"
"github.com/kong/kubernetes-ingress-controller/internal/admission"
"github.com/kong/kubernetes-ingress-controller/internal/ingress/annotations"
"github.com/kong/kubernetes-ingress-controller/internal/ingress/controller"
"github.com/kong/kubernetes-ingress-controller/internal/ingress/store"
"github.com/kong/kubernetes-ingress-controller/internal/ingress/utils"
Expand Down Expand Up @@ -348,10 +347,8 @@ func main() {
updateChannel := channels.NewRingChannel(1024)
reh := controller.ResourceEventHandler{
UpdateCh: updateChannel,
IsValidIngresClass: annotations.IngressClassValidatorFunc(
cliConfig.IngressClass,
cliConfig.SkipClasslessIngressV1beta1),
}

var informers []cache.SharedIndexInformer
var cacheStores store.CacheStores

Expand Down Expand Up @@ -429,8 +426,8 @@ func main() {
runtime.HandleError(fmt.Errorf("Timed out waiting for caches to sync"))
}

store := store.New(cacheStores, cliConfig.IngressClass, cliConfig.SkipClasslessIngressV1beta1,
log.WithField("component", "store"))
store := store.New(cacheStores, cliConfig.IngressClass, cliConfig.ProcessClasslessIngressV1beta1,
cliConfig.ProcessClasslessKongConsumer, log.WithField("component", "store"))
kong, err := controller.NewKongController(&controllerConfig, updateChannel,
store)
if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion cli/ingress-controller/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ func TestHandleSigterm(t *testing.T) {
KubeClient: kubeClient,
},
channels.NewRingChannel(1024),
store.New(store.CacheStores{}, conf.IngressClass, conf.SkipClasslessIngressV1beta1, logrus.New()),
store.New(store.CacheStores{}, conf.IngressClass, conf.ProcessClasslessIngressV1beta1,
conf.ProcessClasslessKongConsumer, logrus.New()),
)

exitCh := make(chan int, 1)
Expand Down
43 changes: 27 additions & 16 deletions internal/ingress/annotations/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

type ClassMatching int

const (
IgnoreClassMatch ClassMatching = iota
ExactOrEmptyClassMatch ClassMatching = iota
ExactClassMatch ClassMatching = iota
)

const (
ingressClassKey = "kubernetes.io/ingress.class"

Expand Down Expand Up @@ -49,39 +57,42 @@ const (
DefaultIngressClass = "kong"
)

func validIngress(ingressAnnotationValue, ingressClass string, skipClasslessIngress bool) bool {
// 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 == "" && !skipClasslessIngress && ingressClass == DefaultIngressClass {
func validIngress(ingressAnnotationValue, ingressClass string, handling ClassMatching) bool {
switch handling {
case IgnoreClassMatch:
// class is not considered at all. any value, even a mismatch, is valid
return true
case ExactOrEmptyClassMatch:
// aka lazy. exact match desired, but empty permitted
return ingressAnnotationValue == "" || ingressAnnotationValue == ingressClass
case ExactClassMatch:
// what it says on the tin
// this may be another place we want to return a warning, since an empty-class resource will never be valid
return ingressAnnotationValue == ingressClass
default:
panic("invalid ingress class handling option received")
hbagdi marked this conversation as resolved.
Show resolved Hide resolved
}
return ingressAnnotationValue == ingressClass
}

// IngressClassValidatorFunc returns a function which can validate if an Object
// belongs to an the ingressClass or not.
func IngressClassValidatorFunc(
ingressClass string, skipClasslessIngress bool) func(obj metav1.Object) bool {
ingressClass string) func(obj metav1.Object, handling ClassMatching) bool {

return func(obj metav1.Object) bool {
return func(obj metav1.Object, handling ClassMatching) bool {
ingress := obj.GetAnnotations()[ingressClassKey]
return validIngress(ingress, ingressClass, skipClasslessIngress)
return validIngress(ingress, ingressClass, handling)
}
}

// IngressClassValidatorFuncFromObjectMeta returns a function which
// can validate if an ObjectMeta belongs to an the ingressClass or not.
func IngressClassValidatorFuncFromObjectMeta(
ingressClass string, skipClasslessIngress bool) func(obj *metav1.ObjectMeta) bool {
ingressClass string) func(obj *metav1.ObjectMeta, handling ClassMatching) bool {

return func(obj *metav1.ObjectMeta) bool {
return func(obj *metav1.ObjectMeta, handling ClassMatching) bool {
ingress := obj.GetAnnotations()[ingressClassKey]
return validIngress(ingress, ingressClass, skipClasslessIngress)
return validIngress(ingress, ingressClass, handling)
}
}

Expand Down
44 changes: 26 additions & 18 deletions internal/ingress/annotations/annotations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,28 @@ import (

func TestIngressClassValidatorFunc(t *testing.T) {
tests := []struct {
ingress string
skipClasslessIngress bool
controller string
isValid bool
ingress string // the class set on the Ingress resource
classMatching ClassMatching // the "user" classless ingress flag value, translated to its match strategy
controller string // the class set on the controller
isValid bool // the expected verdict
}{
{"", false, "", true},
{"", false, "kong", true},
{"", true, "kong", false},
{"kong", false, "kong", true},
{"kong", true, "kong", true},
{"custom", false, "custom", true},
{"", false, "killer", false},
{"custom", false, "kong", false},
{"custom", true, "kong", false},
{"", true, "custom", false},
{"", ExactOrEmptyClassMatch, "", true},
{"", ExactOrEmptyClassMatch, DefaultIngressClass, true},
{"", ExactClassMatch, DefaultIngressClass, false},
{DefaultIngressClass, ExactOrEmptyClassMatch, DefaultIngressClass, true},
{DefaultIngressClass, ExactClassMatch, DefaultIngressClass, true},
{"custom", ExactOrEmptyClassMatch, "custom", true},
{"", ExactOrEmptyClassMatch, "killer", true},
{"custom", ExactOrEmptyClassMatch, DefaultIngressClass, false},
{"custom", ExactClassMatch, DefaultIngressClass, false},
{"", ExactOrEmptyClassMatch, "custom", true},
{"", ExactOrEmptyClassMatch, "kozel", true},
{"", ExactClassMatch, "kozel", false},
{"kozel", ExactOrEmptyClassMatch, "kozel", true},
{"kozel", ExactClassMatch, "kozel", true},
{"", ExactOrEmptyClassMatch, "killer", true},
{"custom", ExactOrEmptyClassMatch, "kozel", false},
{"custom", ExactClassMatch, "kozel", false},
}

ing := &extensions.Ingress{
Expand All @@ -55,10 +62,11 @@ func TestIngressClassValidatorFunc(t *testing.T) {
ing.SetAnnotations(data)
for _, test := range tests {
ing.Annotations[ingressClassKey] = test.ingress
f := IngressClassValidatorFunc(test.controller, test.skipClasslessIngress)
b := f(&ing.ObjectMeta)
if b != test.isValid {
t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b)
f := IngressClassValidatorFunc(test.controller)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest rewriting this test to have the following values in the table:

  • annotation value
  • kong's ingress class specified in config
  • selected matching strategy
  • expected verdict

This would mean directly putting ClassHandling in the table, as opposed to skipClasslessIngress (which adds apparently unnecessary complexity and makes it easier to miss missing cases or mistakes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is essentially what it has now--I've added comments to clarify what's what.

What we don't have is tests of the strategies independent of their user-facing controls: the latter are boolean flags, and the former are derived from them. Effectively we have integration tests here, and a poor delineation between integration and unit tests throughout the codebase, alongside the poor separation between components (the store/annotation distinction is messy in both the tests and implementation).

Copy link
Contributor

Choose a reason for hiding this comment

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

That is essentially what it has now

My point is that this logic is unnecessary:

	var b bool
		if test.skipClasslessIngress {
			b = f(&ing.ObjectMeta, ExactClassMatch)
		} else {
			b = f(&ing.ObjectMeta, ExactOrEmptyClassMatch)
		}

it could be as simple as

    b = f(&ing.ObjectMeta, test.matchingStrategy)

Effectively we have integration tests here

The problem here is that we have replicated a part of the "system under test" (namely, the if as quoted above) in the test itself. This is not right because it adds a degree of freedom to the actual system under test. Therefore, IMO it's better to remove the if and update the test data to use a ClassMatching, not a bool as input.


result := f(&ing.ObjectMeta, test.classMatching)
if result != test.isValid {
t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, result)
}
}
}
Expand Down
36 changes: 1 addition & 35 deletions internal/ingress/controller/event_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,12 @@ import (

"github.com/eapache/channels"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// ResourceEventHandler is "ingress.class" aware resource
// handler.
type ResourceEventHandler struct {
IsValidIngresClass func(object metav1.Object) bool
UpdateCh *channels.RingChannel
UpdateCh *channels.RingChannel
}

// EventType type of event associated with an informer
Expand All @@ -37,15 +34,7 @@ type Event struct {
Old interface{}
}

// OnAdd is invoked whenever a resource is added.
func (reh ResourceEventHandler) OnAdd(obj interface{}) {
object, err := meta.Accessor(obj)
if err != nil {
return
}
if !reh.IsValidIngresClass(object) {
return
}
reh.UpdateCh.In() <- Event{
Type: CreateEvent,
Obj: obj,
Expand All @@ -54,14 +43,6 @@ func (reh ResourceEventHandler) OnAdd(obj interface{}) {

// OnDelete is invoked whenever a resource is deleted.
func (reh ResourceEventHandler) OnDelete(obj interface{}) {
object, err := meta.Accessor(obj)
if err != nil {
return
}
if !reh.IsValidIngresClass(object) {
return
}

reh.UpdateCh.In() <- Event{
Type: DeleteEvent,
Obj: obj,
Expand All @@ -71,21 +52,6 @@ func (reh ResourceEventHandler) OnDelete(obj interface{}) {
// OnUpdate is invoked whenever a resource is changed. old holds
// the previous resource and cur is the updated resource.
func (reh ResourceEventHandler) OnUpdate(old, cur interface{}) {
oldObj, err := meta.Accessor(old)
if err != nil {
return
}
curObj, err := meta.Accessor(cur)
if err != nil {
return
}
validOld := reh.IsValidIngresClass(oldObj)
validCur := reh.IsValidIngresClass(curObj)

if !validCur && !validOld {
return
}

reh.UpdateCh.In() <- Event{
Type: UpdateEvent,
Obj: cur,
Expand Down
Loading