Skip to content

improve missing service handling for targetGroupBinding #1888

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

Merged
merged 1 commit into from
Mar 18, 2021
Merged

improve missing service handling for targetGroupBinding #1888

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
2 changes: 1 addition & 1 deletion main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func main() {
sgReconciler := networking.NewDefaultSecurityGroupReconciler(sgManager, ctrl.Log)
subnetResolver := networking.NewDefaultSubnetsResolver(cloud.EC2(), cloud.VpcID(), controllerCFG.ClusterName, ctrl.Log.WithName("subnets-resolver"))
tgbResManager := targetgroupbinding.NewDefaultResourceManager(mgr.GetClient(), cloud.ELBV2(),
podInfoRepo, podENIResolver, nodeENIResolver, sgManager, sgReconciler, cloud.VpcID(), controllerCFG.ClusterName, ctrl.Log)
podInfoRepo, podENIResolver, nodeENIResolver, sgManager, sgReconciler, cloud.VpcID(), controllerCFG.ClusterName, mgr.GetEventRecorderFor("targetGroupBinding"), ctrl.Log)
ingGroupReconciler := ingress.NewGroupReconciler(cloud, mgr.GetClient(), mgr.GetEventRecorderFor("ingress"),
finalizerManager, sgManager, sgReconciler, subnetResolver,
controllerCFG, ctrl.Log.WithName("controllers").WithName("ingress"))
Expand Down
13 changes: 12 additions & 1 deletion pkg/backend/endpoint_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,19 @@ package backend

import (
"context"
"fmt"
"github.com/go-logr/logr"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
"sigs.k8s.io/aws-load-balancer-controller/pkg/k8s"
"sigs.k8s.io/controller-runtime/pkg/client"
)

var ErrNotFound = errors.New("backend not found")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:
The error message contains redundant "not found" phrase . For example,
"backend not found: services "svc-1" not found"

I am more inclined to have something like -
"backend: services "svc-1" not found"
or
"backend error: services "svc-1" not found"


// TODO: for pod endpoints, we currently rely on endpoints events, we might change to use pod events directly in the future.
// under current implementation with pod readinessGate enabled, an unready endpoint but not match our inclusionCriteria won't be registered,
// and it won't turn ready due to blocked by readinessGate, and no future endpoint events will trigger.
Expand Down Expand Up @@ -55,9 +59,13 @@ func (r *defaultEndpointResolver) ResolvePodEndpoints(ctx context.Context, svcKe
if err != nil {
return nil, false, err
}

epsKey := k8s.NamespacedName(svc) // k8s Endpoints have same name as k8s Service
eps := &corev1.Endpoints{}
if err := r.k8sClient.Get(ctx, epsKey, eps); err != nil {
if apierrors.IsNotFound(err) {
return nil, false, fmt.Errorf("%w: %v", ErrNotFound, err.Error())
}
return nil, false, err
}

Expand Down Expand Up @@ -149,11 +157,14 @@ func (r *defaultEndpointResolver) ResolveNodePortEndpoints(ctx context.Context,
func (r *defaultEndpointResolver) findServiceAndServicePort(ctx context.Context, svcKey types.NamespacedName, port intstr.IntOrString) (*corev1.Service, corev1.ServicePort, error) {
svc := &corev1.Service{}
if err := r.k8sClient.Get(ctx, svcKey, svc); err != nil {
if apierrors.IsNotFound(err) {
return nil, corev1.ServicePort{}, fmt.Errorf("%w: %v", ErrNotFound, err.Error())
}
return nil, corev1.ServicePort{}, err
}
svcPort, err := k8s.LookupServicePort(svc, port)
if err != nil {
return nil, corev1.ServicePort{}, err
return nil, corev1.ServicePort{}, fmt.Errorf("%w: %v", ErrNotFound, err.Error())
}

return svc, svcPort, nil
Expand Down
107 changes: 107 additions & 0 deletions pkg/backend/endpoint_resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package backend
import (
"context"
"errors"
"fmt"
"github.com/golang/mock/gomock"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
Expand Down Expand Up @@ -131,6 +132,21 @@ func Test_defaultEndpointResolver_ResolvePodEndpoints(t *testing.T) {
},
},
}
svc1WithoutHTTPPort := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNS,
Name: "svc-1",
},
Spec: corev1.ServiceSpec{
Type: corev1.ServiceTypeClusterIP,
Ports: []corev1.ServicePort{
{
Name: "https",
Port: 443,
},
},
},
}
ep1A := &corev1.Endpoints{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNS,
Expand Down Expand Up @@ -704,6 +720,56 @@ func Test_defaultEndpointResolver_ResolvePodEndpoints(t *testing.T) {
},
wantContainsPotentialReadyEndpoints: true,
},
{
name: "service not found",
env: env{
services: []*corev1.Service{},
endpointsList: []*corev1.Endpoints{},
},
fields: fields{
podInfoRepoGetCalls: []podInfoRepoGetCall{},
},
args: args{
svcKey: k8s.NamespacedName(svc1),
port: intstr.FromString("http"),
opts: nil,
},
want: []PodEndpoint{},
wantContainsPotentialReadyEndpoints: false,
wantErr: fmt.Errorf("%w: %v", ErrNotFound, "services \"svc-1\" not found"),
},
{
name: "service port not found",
env: env{
services: []*corev1.Service{svc1WithoutHTTPPort},
endpointsList: []*corev1.Endpoints{},
},
fields: fields{
podInfoRepoGetCalls: []podInfoRepoGetCall{},
},
args: args{
svcKey: k8s.NamespacedName(svc1),
port: intstr.FromString("http"),
opts: nil,
},
wantErr: fmt.Errorf("%w: %v", ErrNotFound, "unable to find port http on service test-ns/svc-1"),
},
{
name: "endpoints not found",
env: env{
services: []*corev1.Service{svc1},
endpointsList: []*corev1.Endpoints{},
},
fields: fields{
podInfoRepoGetCalls: []podInfoRepoGetCall{},
},
args: args{
svcKey: k8s.NamespacedName(svc1),
port: intstr.FromString("http"),
opts: nil,
},
wantErr: fmt.Errorf("%w: %v", ErrNotFound, "endpoints \"svc-1\" not found"),
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -850,6 +916,21 @@ func Test_defaultEndpointResolver_ResolveNodePortEndpoints(t *testing.T) {
},
},
}
svc1WithoutHTTPPort := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNS,
Name: "svc-1",
},
Spec: corev1.ServiceSpec{
Type: corev1.ServiceTypeClusterIP,
Ports: []corev1.ServicePort{
{
Name: "https",
Port: 443,
},
},
},
}
svc2 := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNS,
Expand Down Expand Up @@ -957,6 +1038,32 @@ func Test_defaultEndpointResolver_ResolveNodePortEndpoints(t *testing.T) {
},
wantErr: errors.New("service type must be either 'NodePort' or 'LoadBalancer': test-ns/svc-2"),
},
{
name: "service not found",
env: env{
nodes: []*corev1.Node{node1, node2, node3, node4},
services: []*corev1.Service{},
},
args: args{
svcKey: k8s.NamespacedName(svc1),
port: intstr.FromString("http"),
opts: []EndpointResolveOption{WithNodeSelector(labels.Everything())},
},
wantErr: fmt.Errorf("%w: %v", ErrNotFound, "services \"svc-1\" not found"),
},
{
name: "service port not found",
env: env{
nodes: []*corev1.Node{node1, node2, node3, node4},
services: []*corev1.Service{svc1WithoutHTTPPort},
},
args: args{
svcKey: k8s.NamespacedName(svc1),
port: intstr.FromString("http"),
opts: []EndpointResolveOption{WithNodeSelector(labels.Everything())},
},
wantErr: fmt.Errorf("%w: %v", ErrNotFound, "unable to find port http on service test-ns/svc-1"),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
1 change: 1 addition & 0 deletions pkg/k8s/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,6 @@ const (
TargetGroupBindingEventReasonFailedRemoveFinalizer = "FailedRemoveFinalizer"
TargetGroupBindingEventReasonFailedUpdateStatus = "FailedUpdateStatus"
TargetGroupBindingEventReasonFailedCleanup = "FailedCleanup"
TargetGroupBindingEventReasonBackendNotFound = "BackendNotFound"
TargetGroupBindingEventReasonSuccessfullyReconciled = "SuccessfullyReconciled"
)
13 changes: 12 additions & 1 deletion pkg/targetgroupbinding/resource_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"fmt"
"k8s.io/client-go/tools/record"
"time"

awssdk "github.com/aws/aws-sdk-go/aws"
Expand Down Expand Up @@ -38,7 +39,7 @@ type ResourceManager interface {
func NewDefaultResourceManager(k8sClient client.Client, elbv2Client services.ELBV2,
podInfoRepo k8s.PodInfoRepo, podENIResolver networking.PodENIInfoResolver, nodeENIResolver networking.NodeENIInfoResolver,
sgManager networking.SecurityGroupManager, sgReconciler networking.SecurityGroupReconciler,
vpcID string, clusterName string, logger logr.Logger) *defaultResourceManager {
vpcID string, clusterName string, eventRecorder record.EventRecorder, logger logr.Logger) *defaultResourceManager {
targetsManager := NewCachedTargetsManager(elbv2Client, logger)
endpointResolver := backend.NewDefaultEndpointResolver(k8sClient, podInfoRepo, logger)
networkingManager := NewDefaultNetworkingManager(k8sClient, podENIResolver, nodeENIResolver, sgManager, sgReconciler, vpcID, clusterName, logger)
Expand All @@ -47,6 +48,7 @@ func NewDefaultResourceManager(k8sClient client.Client, elbv2Client services.ELB
targetsManager: targetsManager,
endpointResolver: endpointResolver,
networkingManager: networkingManager,
eventRecorder: eventRecorder,
logger: logger,

targetHealthRequeueDuration: defaultTargetHealthRequeueDuration,
Expand All @@ -61,6 +63,7 @@ type defaultResourceManager struct {
targetsManager TargetsManager
endpointResolver backend.EndpointResolver
networkingManager NetworkingManager
eventRecorder record.EventRecorder
logger logr.Logger

targetHealthRequeueDuration time.Duration
Expand Down Expand Up @@ -95,6 +98,10 @@ func (m *defaultResourceManager) reconcileWithIPTargetType(ctx context.Context,
}
endpoints, containsPotentialReadyEndpoints, err := m.endpointResolver.ResolvePodEndpoints(ctx, svcKey, tgb.Spec.ServiceRef.Port, resolveOpts...)
if err != nil {
if errors.Is(err, backend.ErrNotFound) {
m.eventRecorder.Event(tgb, corev1.EventTypeWarning, k8s.TargetGroupBindingEventReasonBackendNotFound, err.Error())
return m.Cleanup(ctx, tgb)
}
return err
}

Expand Down Expand Up @@ -146,6 +153,10 @@ func (m *defaultResourceManager) reconcileWithInstanceTargetType(ctx context.Con
resolveOpts := []backend.EndpointResolveOption{backend.WithNodeSelector(nodeSelector)}
endpoints, err := m.endpointResolver.ResolveNodePortEndpoints(ctx, svcKey, tgb.Spec.ServiceRef.Port, resolveOpts...)
if err != nil {
if errors.Is(err, backend.ErrNotFound) {
m.eventRecorder.Event(tgb, corev1.EventTypeWarning, k8s.TargetGroupBindingEventReasonBackendNotFound, err.Error())
return m.Cleanup(ctx, tgb)
}
return err
}
tgARN := tgb.Spec.TargetGroupARN
Expand Down