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

refactor ingress backend pod detection #11

Merged
merged 1 commit into from
Dec 13, 2024
Merged

refactor ingress backend pod detection #11

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
87 changes: 39 additions & 48 deletions pkg/controllers/ingress_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"k8s.io/apimachinery/pkg/types"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/source"
Expand Down Expand Up @@ -100,16 +99,6 @@ func (r *IngressReconciler) filterIngressRulesByHost(rules []netv1.IngressRule)
return rulesToBind
}

func (r *IngressReconciler) getTargetFromIngress(ingress netv1.Ingress) (string, error) {
if r.DevMode {
return "devmode", nil
}
if len(ingress.Status.LoadBalancer.Ingress) == 0 {
return "", fmt.Errorf("Ingress has no status")
}
return ingress.Status.LoadBalancer.Ingress[0].Hostname, nil
}

func (r *IngressReconciler) isIngressWeighted(ingress netv1.Ingress) bool {
return r.ingressHasAnnotationKey(ingress, r.annotationKey("traffic-weight"))
}
Expand Down Expand Up @@ -168,7 +157,19 @@ func setGlobalHealthCheckID(endpoint *externaldnsk8siov1alpha1.Endpoint) {
}
}

func (r *IngressReconciler) newDnsEndpoint(ctx context.Context, dnsEndpoint *externaldnsk8siov1alpha1.DNSEndpoint, target string, ingress netv1.Ingress) {
func (r *IngressReconciler) addIngressTargetsToEndpoint(endpoint *externaldnsk8siov1alpha1.Endpoint, ingress netv1.Ingress) {
for _, lb := range ingress.Status.LoadBalancer.Ingress {
if lb.Hostname != "" {
hostName := lb.Hostname
if r.DevMode {
hostName = "devmode"
}
endpoint.Targets = append(endpoint.Targets, hostName)
}
}
}

func (r *IngressReconciler) newDnsEndpoint(ctx context.Context, dnsEndpoint *externaldnsk8siov1alpha1.DNSEndpoint, ingress netv1.Ingress) {
var desiredWeight uint
var err error

Expand All @@ -188,21 +189,19 @@ func (r *IngressReconciler) newDnsEndpoint(ctx context.Context, dnsEndpoint *ext
dnsEndpoint.ObjectMeta.Annotations = make(map[string]string)
}
}

if !r.allIngressBackendsHavePods(ctx, &ingress) {
desiredWeight = 0
}
dnsEndpoint.Spec = externaldnsk8siov1alpha1.DNSEndpointSpec{Endpoints: []*externaldnsk8siov1alpha1.Endpoint{}}
for _, rule := range r.filterIngressRulesByHost(ingress.Spec.Rules) {

if !r.ingressRuleHasPods(ctx, ingress.ObjectMeta.Namespace, &rule) {
desiredWeight = 0
}

ep := &externaldnsk8siov1alpha1.Endpoint{
DNSName: rule.Host,
Targets: externaldnsk8siov1alpha1.Targets{
target,
},
DNSName: rule.Host,
RecordType: "CNAME",
SetIdentifier: r.ClusterName,
}
r.addIngressTargetsToEndpoint(ep, ingress)

setEndpointProviderSpecificProperty(ep, WeightProperty, strconv.FormatUint(uint64(desiredWeight), 10))
setGlobalHealthCheckID(ep)
Expand All @@ -218,12 +217,6 @@ func (r *IngressReconciler) reconcileDNSEntries(ctx context.Context, ingress net
return nil
}

target, err := r.getTargetFromIngress(ingress)
if err != nil {
log.Info("Ingress object doesn't have target assigned. Skipping")
return nil
}

// Reconcile uses this property that an ingress has a single matching dnsendpoint
// with the same name. Shall this be changed, we should also change the Reconcile code
var dnsEndpoint = &externaldnsk8siov1alpha1.DNSEndpoint{
Expand All @@ -232,11 +225,10 @@ func (r *IngressReconciler) reconcileDNSEntries(ctx context.Context, ingress net
Namespace: ingress.GetNamespace(),
},
}
var f controllerutil.MutateFn = func() error {
r.newDnsEndpoint(ctx, dnsEndpoint, target, ingress)
_, err := ctrl.CreateOrUpdate(ctx, r.Client, dnsEndpoint, func() error {
r.newDnsEndpoint(ctx, dnsEndpoint, ingress)
return nil
}
_, err = ctrl.CreateOrUpdate(ctx, r.Client, dnsEndpoint, f)
})
return err
}

Expand All @@ -255,23 +247,23 @@ func (r *IngressReconciler) endpointBeingDeleted(ctx context.Context, obj types.
in DNSEndpoint Object we set its weight based on hostname level. in case a host in an ingress has more than 1 service for different paths,
we will only set the weight to 0 if all services do not have backing pods.
*/
func (r *IngressReconciler) ingressRuleHasPods(ctx context.Context, namespace string, rule *netv1.IngressRule) bool {

// there are some edge cases that the rules does not have HTTP property defined
// keeping the same behavior
if rule.HTTP == nil {
return false
}
paths := rule.HTTP.Paths

func (r *IngressReconciler) allIngressBackendsHavePods(ctx context.Context, ingress *netv1.Ingress) bool {
var servicesName = make(map[string]types.NamespacedName)

for _, path := range paths {
servicesName[path.Backend.Service.Name] = types.NamespacedName{Namespace: namespace, Name: path.Backend.Service.Name}
for _, rule := range ingress.Spec.Rules {
// there are some edge cases that the rules does not have HTTP property defined
// keeping the same behavior
if rule.HTTP == nil {
continue
}
paths := rule.HTTP.Paths

for _, path := range paths {
servicesName[path.Backend.Service.Name] = types.NamespacedName{Namespace: ingress.Namespace, Name: path.Backend.Service.Name}
}
}

for _, svc := range servicesName {

var endpoints v1.Endpoints

if err := r.Get(ctx, svc, &endpoints); err != nil {
Expand All @@ -281,12 +273,6 @@ func (r *IngressReconciler) ingressRuleHasPods(ctx context.Context, namespace st
if apierrors.IsNotFound(err) {
return false
}
return true
}

if !endpoints.DeletionTimestamp.IsZero() {
// If the endpoint is being deleted, it will eventually disappear, and ingresses will not have pods pretty soon.
// Handling this case here helps reduce downtime in some cases
return false
}

Expand All @@ -299,6 +285,11 @@ func (r *IngressReconciler) ingressRuleHasPods(ctx context.Context, namespace st
}

func (r *IngressReconciler) endpointsHasPods(endpoints *v1.Endpoints) bool {
if !endpoints.DeletionTimestamp.IsZero() {
// If the endpoint is being deleted, it will eventually disappear, and ingresses will not have pods pretty soon.
// Handling this case here helps reduce downtime in some cases
return false
}
if len(endpoints.Subsets) == 0 {
return false
}
Expand Down
33 changes: 0 additions & 33 deletions pkg/controllers/ingress_controller_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,39 +122,6 @@ func TestFilterIngressRulesByHost(t *testing.T) {
})
}

func TestGetTargetFromIngress(t *testing.T) {
t.Run("with dev mode, devmode should be returned", func(t *testing.T) {
reconciler := IngressReconciler{
DevMode: true,
}
target, err := reconciler.getTargetFromIngress(netv1.Ingress{})
assert.NoError(t, err)
assert.Equal(t, "devmode", target)
})
t.Run("with a Hostname target, the host name should be returned", func(t *testing.T) {
reconciler := IngressReconciler{}
target, err := reconciler.getTargetFromIngress(netv1.Ingress{
Status: netv1.IngressStatus{
LoadBalancer: netv1.IngressLoadBalancerStatus{
Ingress: []netv1.IngressLoadBalancerIngress{
{
Hostname: "hello.world",
},
},
},
},
})
assert.NoError(t, err)
assert.Equal(t, "hello.world", target)
})
t.Run("no target, an error should be returned", func(t *testing.T) {
reconciler := IngressReconciler{}
target, err := reconciler.getTargetFromIngress(netv1.Ingress{})
assert.Error(t, err)
assert.Equal(t, "", target)
})
}

func TestReconcileIngressShouldCreateDNSEndpointsWithCorrectWeight(t *testing.T) {
extendedScheme := NewScheme()

Expand Down
123 changes: 120 additions & 3 deletions pkg/controllers/ingress_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
v1 "k8s.io/api/core/v1"
netv1 "k8s.io/api/networking/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand All @@ -31,6 +32,9 @@ func TestIngressController(t *testing.T) {
testutils.IntegrationTest(t)
scheme := NewScheme()

trafficweight.Store.AWSHealthCheckID = ""
trafficweight.Store.DesiredWeight = 0

kubeconfig, err := k8s.NewClientConfigBuilder().WithKubeConfigPath(testEnvConf.KubeconfigFile()).Build()
require.NoError(t, err)

Expand Down Expand Up @@ -127,7 +131,7 @@ func TestIngressController(t *testing.T) {

t.Run("Should forge DNS Endpoints", func(t *testing.T) {
forged := &externaldnsk8siov1alpha1.DNSEndpoint{}
reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing)
reconciler.newDnsEndpoint(context.Background(), forged, ing)
assert.Equal(t, expected, *forged)
})

Expand Down Expand Up @@ -191,7 +195,7 @@ func TestIngressController(t *testing.T) {
t.Run("if we dont set --aws-health-check-id ingress shouldnt have health property", func(t *testing.T) {
trafficweight.Store.AWSHealthCheckID = ""
forged := &externaldnsk8siov1alpha1.DNSEndpoint{}
reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing)
reconciler.newDnsEndpoint(context.Background(), forged, ing)
assert.Equal(t, expected, *forged)
})

Expand Down Expand Up @@ -226,7 +230,7 @@ func TestIngressController(t *testing.T) {
}

forged := &externaldnsk8siov1alpha1.DNSEndpoint{}
reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing)
reconciler.newDnsEndpoint(context.Background(), forged, ing)
assert.Equal(t, expected, *forged)
ing.Spec.Rules = oldRules
})
Expand Down Expand Up @@ -378,3 +382,116 @@ func TestSetGlobalHealthcheckID(t *testing.T) {
assert.Len(t, ep.Spec.Endpoints[0].ProviderSpecific, 0)
})
}

func TestAddIngressTargets(t *testing.T) {
t.Run("When not in devmode", func(t *testing.T) {
ingress := netv1.Ingress{
Status: netv1.IngressStatus{
LoadBalancer: netv1.IngressLoadBalancerStatus{
Ingress: []netv1.IngressLoadBalancerIngress{
{Hostname: "test-hostname"},
{Hostname: "test-hostname-2"},
},
},
},
}
ep := externaldnsk8siov1alpha1.Endpoint{}

r := IngressReconciler{}

r.addIngressTargetsToEndpoint(&ep, ingress)

require.Len(t, ep.Targets, 2)
assert.Contains(t, ep.Targets, "test-hostname")
assert.Contains(t, ep.Targets, "test-hostname-2")
})
t.Run("When in devmode", func(t *testing.T) {
ingress := netv1.Ingress{
Status: netv1.IngressStatus{
LoadBalancer: netv1.IngressLoadBalancerStatus{
Ingress: []netv1.IngressLoadBalancerIngress{
{Hostname: "test-hostname"},
{Hostname: "test-hostname-2"},
},
},
},
}
ep := externaldnsk8siov1alpha1.Endpoint{}

r := IngressReconciler{DevMode: true}

r.addIngressTargetsToEndpoint(&ep, ingress)

require.Len(t, ep.Targets, 2)
assert.Contains(t, ep.Targets, "devmode")
assert.Contains(t, ep.Targets, "devmode")
})
}

func TestAllIngressBackendsHavePods(t *testing.T) {
t.Run("When all backends have pods", func(t *testing.T) {
ingress := MockIngress(
WithObjectNamespace[*netv1.Ingress]("test-namespace"),
IngressWithRules(
NewRule(RuleWithHTTPPaths(
NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-1")), NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-2")),
)),
NewRule(RuleWithHTTPPaths(
NewHTTPIngressPath(PathWithBackendServiceName("rule-2-service-1")),
)),
),
)
client := fake.NewClientBuilder().WithObjects(
MockEndpoints(EndpointsWithName("rule-1-service-1"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.1"))),
MockEndpoints(EndpointsWithName("rule-1-service-2"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.2"))),
MockEndpoints(EndpointsWithName("rule-2-service-1"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.3"))),
).Build()
r := IngressReconciler{Client: client}
assert.True(t, r.allIngressBackendsHavePods(context.Background(), ingress))
})
t.Run("When a backend does not have pods", func(t *testing.T) {
ingress := MockIngress(
WithObjectNamespace[*netv1.Ingress]("test-namespace"),
IngressWithRules(
NewRule(RuleWithHTTPPaths(
NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-1")), NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-2")),
)),
NewRule(RuleWithHTTPPaths(
NewHTTPIngressPath(PathWithBackendServiceName("rule-2-service-1")),
)),
),
)
client := fake.NewClientBuilder().WithObjects(
MockEndpoints(EndpointsWithName("rule-1-service-1"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.1"))),
MockEndpoints(EndpointsWithName("rule-1-service-2"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithoutSubset()),
MockEndpoints(EndpointsWithName("rule-2-service-1"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.3"))),
).Build()
r := IngressReconciler{Client: client}
assert.False(t, r.allIngressBackendsHavePods(context.Background(), ingress))
})
t.Run("When a backend service is missing", func(t *testing.T) {
ingress := MockIngress(
WithObjectNamespace[*netv1.Ingress]("test-namespace"),
IngressWithRules(
NewRule(RuleWithHTTPPaths(
NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-1")), NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-2")),
)),
),
)
client := fake.NewClientBuilder().WithObjects(
MockEndpoints(EndpointsWithName("rule-1-service-1"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.1"))),
).Build()
r := IngressReconciler{Client: client}
assert.False(t, r.allIngressBackendsHavePods(context.Background(), ingress))
})
}

func TestEndpointsHasPods(t *testing.T) {
r := IngressReconciler{}

assert.True(t, r.endpointsHasPods(MockEndpoints()))
assert.False(t, r.endpointsHasPods(MockEndpoints(EndpointsWithSubsets(NewSubsetWithAddressIPs()))))
assert.False(t, r.endpointsHasPods(MockEndpoints(EndpointsWithoutSubset())))

assert.False(t, r.endpointsHasPods(MockEndpoints(WithObjectFinalizers[*v1.Endpoints]("test.adevinta.com"), WithObjectDeletionTimestamp[*v1.Endpoints](metav1.Now()))))
}
Loading