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

Refactor ingress key function and finalizer into separate package #883

Merged
merged 1 commit into from
Oct 3, 2019
Merged

Refactor ingress key function and finalizer into separate package #883

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
18 changes: 9 additions & 9 deletions cmd/e2e-test/finalizer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
"k8s.io/ingress-gce/pkg/e2e"
"k8s.io/ingress-gce/pkg/fuzz"
"k8s.io/ingress-gce/pkg/fuzz/features"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/common"
)

// TestFinalizer asserts that finalizer is added/ removed appropriately during the life cycle
Expand Down Expand Up @@ -58,8 +58,8 @@ func TestFinalizer(t *testing.T) {
ing = waitForStableIngress(true, ing, s, t)

ingFinalizers := ing.GetFinalizers()
if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, utils.FinalizerKey)
if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, common.FinalizerKey)
}

gclb := checkGCLB(t, s, ing, numForwardingRules, numBackendServices)
Expand Down Expand Up @@ -103,8 +103,8 @@ func TestFinalizerIngressClassChange(t *testing.T) {
ing = waitForStableIngress(true, ing, s, t)

ingFinalizers := ing.GetFinalizers()
if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, utils.FinalizerKey)
if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, common.FinalizerKey)
}

gclb := checkGCLB(t, s, ing, numForwardingRules, numBackendServices)
Expand Down Expand Up @@ -170,12 +170,12 @@ func TestFinalizerIngressesWithSharedResources(t *testing.T) {
otherIng = waitForStableIngress(true, otherIng, s, t)

ingFinalizers := ing.GetFinalizers()
if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, utils.FinalizerKey)
if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, common.FinalizerKey)
}
otherIngFinalizers := otherIng.GetFinalizers()
if len(otherIngFinalizers) != 1 || otherIngFinalizers[0] != utils.FinalizerKey {
t.Fatalf("GetFinalizers() = %+v, want [%q]", otherIngFinalizers, utils.FinalizerKey)
if len(otherIngFinalizers) != 1 || otherIngFinalizers[0] != common.FinalizerKey {
t.Fatalf("GetFinalizers() = %+v, want [%q]", otherIngFinalizers, common.FinalizerKey)
}

gclb := checkGCLB(t, s, ing, numForwardingRules, numBackendServices)
Expand Down
60 changes: 21 additions & 39 deletions pkg/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import (
ingsync "k8s.io/ingress-gce/pkg/sync"
"k8s.io/ingress-gce/pkg/tls"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/common"
"k8s.io/klog"
)

Expand Down Expand Up @@ -127,12 +128,12 @@ func NewLoadBalancerController(
AddFunc: func(obj interface{}) {
addIng := obj.(*v1beta1.Ingress)
if !utils.IsGLBCIngress(addIng) {
klog.V(4).Infof("Ignoring add for ingress %v based on annotation %v", utils.IngressKeyFunc(addIng), annotations.IngressClassKey)
klog.V(4).Infof("Ignoring add for ingress %v based on annotation %v", common.ToString(addIng), annotations.IngressClassKey)
return
}

klog.V(3).Infof("Ingress %v added, enqueuing", utils.IngressKeyFunc(addIng))
lbc.ctx.Recorder(addIng.Namespace).Eventf(addIng, apiv1.EventTypeNormal, "ADD", utils.IngressKeyFunc(addIng))
klog.V(3).Infof("Ingress %v added, enqueuing", common.ToString(addIng))
lbc.ctx.Recorder(addIng.Namespace).Eventf(addIng, apiv1.EventTypeNormal, "ADD", common.ToString(addIng))
lbc.ingQueue.Enqueue(obj)
},
DeleteFunc: func(obj interface{}) {
Expand All @@ -142,16 +143,16 @@ func NewLoadBalancerController(
return
}
if delIng.ObjectMeta.DeletionTimestamp != nil {
klog.V(2).Infof("Ignoring delete event for Ingress %v, deletion will be handled via the finalizer", utils.IngressKeyFunc(delIng))
klog.V(2).Infof("Ignoring delete event for Ingress %v, deletion will be handled via the finalizer", common.ToString(delIng))
return
}

if !utils.IsGLBCIngress(delIng) {
klog.V(4).Infof("Ignoring delete for ingress %v based on annotation %v", utils.IngressKeyFunc(delIng), annotations.IngressClassKey)
klog.V(4).Infof("Ignoring delete for ingress %v based on annotation %v", common.ToString(delIng), annotations.IngressClassKey)
return
}

klog.V(3).Infof("Ingress %v deleted, enqueueing", utils.IngressKeyFunc(delIng))
klog.V(3).Infof("Ingress %v deleted, enqueueing", common.ToString(delIng))
lbc.ingQueue.Enqueue(obj)
},
UpdateFunc: func(old, cur interface{}) {
Expand All @@ -161,16 +162,16 @@ func NewLoadBalancerController(
// If ingress was GLBC Ingress, we need to track ingress class change
// and run GC to delete LB resources.
if utils.IsGLBCIngress(oldIng) {
klog.V(4).Infof("Ingress %v class was changed, enqueuing", utils.IngressKeyFunc(curIng))
klog.V(4).Infof("Ingress %v class was changed, enqueuing", common.ToString(curIng))
lbc.ingQueue.Enqueue(cur)
return
}
return
}
if reflect.DeepEqual(old, cur) {
klog.V(3).Infof("Periodic enqueueing of %v", utils.IngressKeyFunc(curIng))
klog.V(3).Infof("Periodic enqueueing of %v", common.ToString(curIng))
} else {
klog.V(3).Infof("Ingress %v changed, enqueuing", utils.IngressKeyFunc(curIng))
klog.V(3).Infof("Ingress %v changed, enqueuing", common.ToString(curIng))
}

lbc.ingQueue.Enqueue(cur)
Expand Down Expand Up @@ -376,11 +377,14 @@ func (lbc *LoadBalancerController) SyncBackends(state interface{}) error {

// GCBackends implements Controller.
func (lbc *LoadBalancerController) GCBackends(toKeep []*v1beta1.Ingress) error {
svcPortsToKeep := lbc.ToSvcPorts(toKeep)
// Only GCE ingress associated resources are managed by this controller.
GCEIngresses := operator.Ingresses(toKeep).Filter(utils.IsGCEIngress).AsList()
svcPortsToKeep := lbc.ToSvcPorts(GCEIngresses)
if err := lbc.backendSyncer.GC(svcPortsToKeep); err != nil {
return err
}
// TODO(ingress#120): Move this to the backend pool so it mirrors creation
// Do not delete instance group if there exists a GLBC ingress.
if len(toKeep) == 0 {
igName := lbc.ctx.ClusterNamer.InstanceGroup()
klog.Infof("Deleting instance group %v", igName)
Expand Down Expand Up @@ -416,7 +420,9 @@ func (lbc *LoadBalancerController) SyncLoadBalancer(state interface{}) error {

// GCLoadBalancers implements Controller.
func (lbc *LoadBalancerController) GCLoadBalancers(toKeep []*v1beta1.Ingress) error {
return lbc.l7Pool.GC(toLbNames(toKeep))
// Only GCE ingress associated resources are managed by this controller.
GCEIngresses := operator.Ingresses(toKeep).Filter(utils.IsGCEIngress).AsList()
return lbc.l7Pool.GC(common.ToIngressKeys(GCEIngresses))
}

// MaybeRemoveFinalizers cleans up Finalizers if needed.
Expand All @@ -427,7 +433,7 @@ func (lbc *LoadBalancerController) MaybeRemoveFinalizers(toCleanup []*v1beta1.In
}
for _, ing := range toCleanup {
ingClient := lbc.ctx.KubeClient.NetworkingV1beta1().Ingresses(ing.Namespace)
if err := utils.RemoveFinalizer(ing, ingClient); err != nil {
if err := common.RemoveFinalizer(ing, ingClient); err != nil {
klog.Errorf("Failed to remove Finalizer from Ingress %s/%s: %v", ing.Namespace, ing.Name, err)
return err
}
Expand Down Expand Up @@ -472,7 +478,7 @@ func (lbc *LoadBalancerController) sync(key string) error {
ing = ing.DeepCopy()
ingClient := lbc.ctx.KubeClient.NetworkingV1beta1().Ingresses(ing.Namespace)
if flags.F.FinalizerAdd {
if err := utils.AddFinalizer(ing, ingClient); err != nil {
if err := common.AddFinalizer(ing, ingClient); err != nil {
klog.Errorf("Failed to add Finalizer to Ingress %q: %v", key, err)
return err
}
Expand Down Expand Up @@ -547,7 +553,7 @@ func (lbc *LoadBalancerController) updateIngressStatus(l7 *loadbalancers.L7, ing

// toRuntimeInfo returns L7RuntimeInfo for the given ingress.
func (lbc *LoadBalancerController) toRuntimeInfo(ing *v1beta1.Ingress, urlMap *utils.GCEURLMap) (*loadbalancers.L7RuntimeInfo, error) {
k, err := utils.KeyFunc(ing)
k, err := common.KeyFunc(ing)
if err != nil {
return nil, fmt.Errorf("cannot get key for Ingress %v/%v: %v", ing.Namespace, ing.Name, err)
}
Expand Down Expand Up @@ -606,37 +612,13 @@ func updateAnnotations(client kubernetes.Interface, name, namespace string, anno
return nil
}

// ToSvcPorts returns a list of SVC ports owned by this controller given a list of ingresses.
// ToSvcPorts returns a list of SVC ports given a list of ingresses.
// Note: This method is used for GC.
func (lbc *LoadBalancerController) ToSvcPorts(ings []*v1beta1.Ingress) []utils.ServicePort {
var knownPorts []utils.ServicePort
for _, ing := range ings {
// Only resources associated with GCE Ingress are managed by this controller.
if !utils.IsGCEIngress(ing) {
continue
}
urlMap, _ := lbc.Translator.TranslateIngress(ing, lbc.ctx.DefaultBackendSvcPort.ID)
knownPorts = append(knownPorts, urlMap.AllServicePorts()...)
}
return knownPorts
}

// toLbNames returns a list of load balancers owned by this controller given a list of ingresses.
func toLbNames(ings []*v1beta1.Ingress) []string {
lbNames := make([]string, 0, len(ings))
for _, ing := range ings {
// Only resources associated with GCE Ingress are managed by this controller.
if !utils.IsGCEIngress(ing) {
continue
}
ingKey, err := utils.KeyFunc(ing)
// An error is returned only if ingress object does not have a valid meta.
// So, logging the error would be sufficient here.
if err != nil {
klog.Errorf("Cannot get key for Ingress %v/%v: %v", ing.Namespace, ing.Name, err)
continue
}
lbNames = append(lbNames, ingKey)
}
return lbNames
}
11 changes: 6 additions & 5 deletions pkg/controller/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (
"k8s.io/ingress-gce/pkg/test"
"k8s.io/ingress-gce/pkg/tls"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/common"
namer_util "k8s.io/ingress-gce/pkg/utils/namer"
"k8s.io/legacy-cloud-providers/gce"
)
Expand Down Expand Up @@ -143,7 +144,7 @@ func deleteIngress(lbc *LoadBalancerController, ing *v1beta1.Ingress) {

// getKey returns the key for an ingress.
func getKey(ing *v1beta1.Ingress, t *testing.T) string {
key, err := utils.KeyFunc(ing)
key, err := common.KeyFunc(ing)
if err != nil {
t.Fatalf("Unexpected error getting key for Ingress %v: %v", ing.Name, err)
}
Expand Down Expand Up @@ -336,8 +337,8 @@ func TestIngressClassChangeWithFinalizer(t *testing.T) {
t.Fatalf("Get(%v) = %v, want nil", ingStoreKey, err)
}
ingFinalizers := updatedIng.GetFinalizers()
if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
t.Fatalf("updatedIng.GetFinalizers() = %+v, want [%s]; failed to add finalizer, updatedIng = %+v", ingFinalizers, utils.FinalizerKey, updatedIng)
if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
t.Fatalf("updatedIng.GetFinalizers() = %+v, want [%s]; failed to add finalizer, updatedIng = %+v", ingFinalizers, common.FinalizerKey, updatedIng)
}

anns[annotations.IngressClassKey] = "new-class"
Expand Down Expand Up @@ -470,8 +471,8 @@ func TestEnableFinalizer(t *testing.T) {
t.Fatalf("Get(%v) = %v, want nil", ingStoreKey, err)
}
ingFinalizers := updatedIng.GetFinalizers()
if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
t.Fatalf("updatedIng.GetFinalizers() = %+v, want [%s]; failed to add finalizer, updatedIng = %+v", ingFinalizers, utils.FinalizerKey, updatedIng)
if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
t.Fatalf("updatedIng.GetFinalizers() = %+v, want [%s]; failed to add finalizer, updatedIng = %+v", ingFinalizers, common.FinalizerKey, updatedIng)
}
}

Expand Down
11 changes: 5 additions & 6 deletions pkg/e2e/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,13 @@ package e2e

import (
"context"
"fmt"
"net"
"time"

"encoding/json"
"fmt"
"io/ioutil"
"net"
"net/http"
"strings"
"time"

"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
Expand All @@ -41,7 +40,7 @@ import (
"k8s.io/ingress-gce/pkg/fuzz"
"k8s.io/ingress-gce/pkg/fuzz/features"
"k8s.io/ingress-gce/pkg/fuzz/whitebox"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/common"
"k8s.io/klog"
)

Expand Down Expand Up @@ -131,7 +130,7 @@ func WaitForFinalizer(s *Sandbox, ingName string) error {
return false, nil
}
ingFinalizers := ing.GetFinalizers()
if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
klog.Infof("WaitForFinalizer(%s/%s) = %v, finalizer not added for Ingress %v", s.Namespace, ingName, ingFinalizers, ing)
return false, nil
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/firewalls/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
backendconfigclient "k8s.io/ingress-gce/pkg/backendconfig/client/clientset/versioned/fake"
test "k8s.io/ingress-gce/pkg/test"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/common"
"k8s.io/legacy-cloud-providers/gce"

meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -77,7 +78,7 @@ func TestFirewallCreateDelete(t *testing.T) {
fwc.ctx.KubeClient.NetworkingV1beta1().Ingresses(ing.Namespace).Create(ing)
fwc.ctx.IngressInformer.GetIndexer().Add(ing)

key, _ := utils.KeyFunc(queueKey)
key, _ := common.KeyFunc(queueKey)
if err := fwc.sync(key); err != nil {
t.Fatalf("fwc.sync() = %v, want nil", err)
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/fuzz/features/neg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"k8s.io/ingress-gce/pkg/annotations"
"k8s.io/ingress-gce/pkg/fuzz"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/common"
)

// NEG is a feature in GCP to support pod as Loadbalancer backends
Expand Down Expand Up @@ -86,7 +87,7 @@ func (v *negValidator) CheckResponse(host, path string, resp *http.Response, bod
return fuzz.CheckResponseContinue, err
}

key, err := utils.KeyFunc(v.ing)
key, err := common.KeyFunc(v.ing)
if err != nil {
return fuzz.CheckResponseContinue, err
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/neg/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
"k8s.io/ingress-gce/pkg/neg/readiness"
negtypes "k8s.io/ingress-gce/pkg/neg/types"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/common"
"k8s.io/klog"
)

Expand Down Expand Up @@ -143,15 +144,15 @@ func NewController(
AddFunc: func(obj interface{}) {
addIng := obj.(*v1beta1.Ingress)
if !utils.IsGLBCIngress(addIng) {
klog.V(4).Infof("Ignoring add for ingress %v based on annotation %v", utils.IngressKeyFunc(addIng), annotations.IngressClassKey)
klog.V(4).Infof("Ignoring add for ingress %v based on annotation %v", common.ToString(addIng), annotations.IngressClassKey)
return
}
negController.enqueueIngressServices(addIng)
},
DeleteFunc: func(obj interface{}) {
delIng := obj.(*v1beta1.Ingress)
if !utils.IsGLBCIngress(delIng) {
klog.V(4).Infof("Ignoring delete for ingress %v based on annotation %v", utils.IngressKeyFunc(delIng), annotations.IngressClassKey)
klog.V(4).Infof("Ignoring delete for ingress %v based on annotation %v", common.ToString(delIng), annotations.IngressClassKey)
return
}
negController.enqueueIngressServices(delIng)
Expand All @@ -160,7 +161,7 @@ func NewController(
oldIng := cur.(*v1beta1.Ingress)
curIng := cur.(*v1beta1.Ingress)
if !utils.IsGLBCIngress(curIng) {
klog.V(4).Infof("Ignoring update for ingress %v based on annotation %v", utils.IngressKeyFunc(curIng), annotations.IngressClassKey)
klog.V(4).Infof("Ignoring update for ingress %v based on annotation %v", common.ToString(curIng), annotations.IngressClassKey)
return
}
keys := gatherIngressServiceKeys(oldIng)
Expand Down
Loading