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.

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

Abort Kubernetes Ingress update if Kubernetes API call fails #1295

Merged
20 changes: 16 additions & 4 deletions provider/kubernetes.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,13 @@ func (provider *Kubernetes) loadIngresses(k8sClient k8s.Client) (*types.Configur
}
}
service, exists, err := k8sClient.GetService(i.ObjectMeta.Namespace, pa.Backend.ServiceName)
if err != nil || !exists {
log.Warnf("Error retrieving service %s/%s: %v", i.ObjectMeta.Namespace, pa.Backend.ServiceName, err)
if err != nil {
log.Errorf("Error while retrieving service information from k8s API %s/%s: %v", service.ObjectMeta.Namespace, pa.Backend.ServiceName, err)
return nil, err
}

if !exists {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a test for this case. I suppose it's already covered by a pre-existing test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the does not exist part? No that was on my todo list, it is actually why the ingress part of the new test has sample data. Was thinking of moving it to a separate test though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Separate test sounds good. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests for this and the !exists endpoints

log.Errorf("Service not found for %s/%s", service.ObjectMeta.Namespace, pa.Backend.ServiceName)
delete(templateObjects.Frontends, r.Host+pa.Path)
continue
}
Expand All @@ -184,13 +189,20 @@ func (provider *Kubernetes) loadIngresses(k8sClient k8s.Client) (*types.Configur
if port.Port == 443 {
protocol = "https"
}

endpoints, exists, err := k8sClient.GetEndpoints(service.ObjectMeta.Namespace, service.ObjectMeta.Name)
if err != nil || !exists {
if err != nil {
log.Errorf("Error retrieving endpoints %s/%s: %v", service.ObjectMeta.Namespace, service.ObjectMeta.Name, err)
return nil, err
}

if !exists {
log.Errorf("Endpoints not found for %s/%s", service.ObjectMeta.Namespace, service.ObjectMeta.Name)
continue
}

if len(endpoints.Subsets) == 0 {
log.Warnf("Endpoints not found for %s/%s, falling back to Service ClusterIP", service.ObjectMeta.Namespace, service.ObjectMeta.Name)
log.Warnf("Service endpoints not found for %s/%s, falling back to Service ClusterIP", service.ObjectMeta.Namespace, service.ObjectMeta.Name)
templateObjects.Backends[r.Host+pa.Path].Servers[string(service.UID)] = types.Server{
URL: protocol + "://" + service.Spec.ClusterIP + ":" + strconv.Itoa(int(port.Port)),
Weight: 1,
Expand Down
281 changes: 281 additions & 0 deletions provider/kubernetes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package provider

import (
"encoding/json"
"errors"
"reflect"
"testing"

Expand Down Expand Up @@ -1524,11 +1525,283 @@ func TestServiceAnnotations(t *testing.T) {
}
}

func TestKubeAPIErrors(t *testing.T) {
ingresses := []*v1beta1.Ingress{{
ObjectMeta: v1.ObjectMeta{
Namespace: "testing",
},
Spec: v1beta1.IngressSpec{
Rules: []v1beta1.IngressRule{
{
Host: "foo",
IngressRuleValue: v1beta1.IngressRuleValue{
HTTP: &v1beta1.HTTPIngressRuleValue{
Paths: []v1beta1.HTTPIngressPath{
{
Path: "/bar",
Backend: v1beta1.IngressBackend{
ServiceName: "service1",
ServicePort: intstr.FromInt(80),
},
},
},
},
},
},
},
},
}}

services := []*v1.Service{{
ObjectMeta: v1.ObjectMeta{
Name: "service1",
UID: "1",
Namespace: "testing",
},
Spec: v1.ServiceSpec{
ClusterIP: "10.0.0.1",
Ports: []v1.ServicePort{
{
Port: 80,
},
},
},
}}

endpoints := []*v1.Endpoints{}
watchChan := make(chan interface{})
apiErr := errors.New("failed kube api call")

testCases := []struct {
desc string
apiServiceErr error
apiEndpointsErr error
}{
{
desc: "failed service call",
apiServiceErr: apiErr,
},
{
desc: "failed endpoints call",
apiEndpointsErr: apiErr,
},
}

for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to capture the loop variable by an assignment tc := tc since we run the tests in parallel (as documented). Otherwise, chances are we will be testing a single case only.

t.Parallel()

client := clientMock{
ingresses: ingresses,
services: services,
endpoints: endpoints,
watchChan: watchChan,
apiServiceError: tc.apiServiceErr,
apiEndpointsError: tc.apiEndpointsErr,
}

provider := Kubernetes{}
if _, err := provider.loadIngresses(client); err != apiErr {
t.Errorf("Got error %v, wanted error %v", err, apiErr)
}
})
}
}

func TestMissingResources(t *testing.T) {
ingresses := []*v1beta1.Ingress{{
ObjectMeta: v1.ObjectMeta{
Namespace: "testing",
},
Spec: v1beta1.IngressSpec{
Rules: []v1beta1.IngressRule{
{
Host: "foo",
IngressRuleValue: v1beta1.IngressRuleValue{
HTTP: &v1beta1.HTTPIngressRuleValue{
Paths: []v1beta1.HTTPIngressPath{
{
Backend: v1beta1.IngressBackend{
ServiceName: "service1",
ServicePort: intstr.FromInt(80),
},
},
},
},
},
},
{
Host: "bar",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Could we change the host name to something like "host_with_missing_service" or place an according comment to indicate that the purpose of this Ingress resource test-wise is to not be found because of that reason?

It takes a bit of back of forth scrolling to understand the purpose of the individual objects this test is setting up. I think purposeful names / comments could alleviate that.

IngressRuleValue: v1beta1.IngressRuleValue{
HTTP: &v1beta1.HTTPIngressRuleValue{
Paths: []v1beta1.HTTPIngressPath{
{
Backend: v1beta1.IngressBackend{
ServiceName: "service2",
ServicePort: intstr.FromInt(80),
},
},
},
},
},
},
{
Host: "tar",
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, something that indicates this Ingress' Service's Endpoints are missing?

IngressRuleValue: v1beta1.IngressRuleValue{
HTTP: &v1beta1.HTTPIngressRuleValue{
Paths: []v1beta1.HTTPIngressPath{
{
Backend: v1beta1.IngressBackend{
ServiceName: "service3",
ServicePort: intstr.FromInt(80),
},
},
},
},
},
},
},
},
}}
services := []*v1.Service{
{
ObjectMeta: v1.ObjectMeta{
Name: "service1",
UID: "1",
Namespace: "testing",
},
Spec: v1.ServiceSpec{
ClusterIP: "10.0.0.1",
Ports: []v1.ServicePort{
{
Port: 80,
},
},
},
},
{
ObjectMeta: v1.ObjectMeta{
Name: "service3",
UID: "3",
Namespace: "testing",
},
Spec: v1.ServiceSpec{
ClusterIP: "10.0.0.3",
Ports: []v1.ServicePort{
{
Port: 80,
},
},
},
},
}
endpoints := []*v1.Endpoints{
{
ObjectMeta: v1.ObjectMeta{
Name: "service1",
UID: "1",
Namespace: "testing",
},
Subsets: []v1.EndpointSubset{
{
Addresses: []v1.EndpointAddress{
{
IP: "10.10.0.1",
},
},
Ports: []v1.EndpointPort{
{
Port: 8080,
},
},
},
{
Addresses: []v1.EndpointAddress{
{
IP: "10.20.0.1",
},
},
Ports: []v1.EndpointPort{
{
Port: 8080,
},
},
},
},
},
}

watchChan := make(chan interface{})
client := clientMock{
ingresses: ingresses,
services: services,
endpoints: endpoints,
watchChan: watchChan,
}
provider := Kubernetes{}
actual, err := provider.loadIngresses(client)
if err != nil {
t.Fatalf("error %+v", err)
}

expected := &types.Configuration{
Backends: map[string]*types.Backend{
"foo": {
Servers: map[string]types.Server{
"http://10.10.0.1:8080": {
URL: "http://10.10.0.1:8080",
Weight: 1,
},
"http://10.20.0.1:8080": {
URL: "http://10.20.0.1:8080",
Weight: 1,
},
},
CircuitBreaker: nil,
LoadBalancer: &types.LoadBalancer{
Method: "wrr",
Sticky: false,
},
},
},
Frontends: map[string]*types.Frontend{
"foo": {
Backend: "foo",
PassHostHeader: true,
Routes: map[string]types.Route{
"foo": {
Rule: "Host:foo",
},
},
},
"tar": {
Backend: "tar",
PassHostHeader: true,
Routes: map[string]types.Route{
"tar": {
Rule: "Host:tar",
},
},
},
},
}

actualJSON, _ := json.Marshal(actual)
expectedJSON, _ := json.Marshal(expected)

if !reflect.DeepEqual(actual, expected) {
t.Fatalf("expected %+v, got %+v", string(expectedJSON), string(actualJSON))
}
}

type clientMock struct {
ingresses []*v1beta1.Ingress
services []*v1.Service
endpoints []*v1.Endpoints
watchChan chan interface{}

apiServiceError error
apiEndpointsError error
}

func (c clientMock) GetIngresses(namespaces k8s.Namespaces) []*v1beta1.Ingress {
Expand All @@ -1543,6 +1816,10 @@ func (c clientMock) GetIngresses(namespaces k8s.Namespaces) []*v1beta1.Ingress {
}

func (c clientMock) GetService(namespace, name string) (*v1.Service, bool, error) {
if c.apiServiceError != nil {
return &v1.Service{}, false, c.apiServiceError
}

for _, service := range c.services {
if service.Namespace == namespace && service.Name == name {
return service, true, nil
Expand All @@ -1552,6 +1829,10 @@ func (c clientMock) GetService(namespace, name string) (*v1.Service, bool, error
}

func (c clientMock) GetEndpoints(namespace, name string) (*v1.Endpoints, bool, error) {
if c.apiEndpointsError != nil {
return &v1.Endpoints{}, false, c.apiEndpointsError
}

for _, endpoints := range c.endpoints {
if endpoints.Namespace == namespace && endpoints.Name == name {
return endpoints, true, nil
Expand Down