Abort Kubernetes Ingress update if Kubernetes API call fails #1295

regner · 2017-03-14T20:33:40Z

Currently if a Kubernetes API call fails we potentially remove a working service from Traefik. This changes it so if a Kubernetes API call fails we abort out of the ingress update and use the current working config. Github issue: #1240

Tests are currently failing but wanted to get the PR up for a few reasons:

Confirm this is how we want to solve this issue (abort the whole ingress update if a single API call fails)
That there isn't a better way to do the tests
I would like the tests to catch a specific error, but don't know what error to expect being raised from the API call

Fixes #1240

timoreimann

IMHO that's the right approach. See my comment though on making an error/missing distinction when we get the endpoints.
Generally the test approach is fine. I made a few suggestions though on how to improve the test implementation.
Does my code comment answer this question?

timoreimann · 2017-03-14T21:13:06Z

provider/kubernetes_test.go

+		apiEndpointsError: true,
+	}
+	provider = Kubernetes{}
+	_, err = provider.loadIngresses(client)


I suggest we convert the test into a table-driven one using sub-tests. Among several benefits, this should improve readability and reduce the amount of test code needed.

timoreimann · 2017-03-14T21:18:47Z

provider/kubernetes_test.go

+	_, err := provider.loadIngresses(client)
+
+	if err == nil {
+		t.Fatal("Should have raised error when loading services but did not.")


This can be t.Error in my view.

timoreimann · 2017-03-14T21:21:55Z

provider/kubernetes_test.go

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

 func (c clientMock) GetService(namespace, name string) (*v1.Service, bool, error) {
+	if c.apiServiceError {
+		return &v1.Service{}, true, c.err


Conventionally, if an error occurs in a function returning multiple values, the non-error parameters are set to their default values. Thus, I'd use false for the boolean return value.

Slightly related/unrelated: I don't know why we return true in line 1624 when we seemingly can't find the service.

timoreimann · 2017-03-14T21:23:51Z

provider/kubernetes_test.go

@@ -1552,6 +1625,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 {
+		return &v1.Endpoints{}, true, c.err


Also false here.

timoreimann · 2017-03-14T21:25:36Z

provider/kubernetes_test.go

+			ObjectMeta: v1.ObjectMeta{
+				Namespace: "testing",
+				Annotations: map[string]string{
+					"traefik.frontend.passHostHeader": "true",


Do we need this annotation for the tests at hand?

I'm always in favor of keeping test fixtures as minimized as possible.

Going to move it to a separate test specifically for checking the !exists cases.

timoreimann · 2017-03-14T21:26:15Z

provider/kubernetes_test.go

+			Spec: v1beta1.IngressSpec{
+				Rules: []v1beta1.IngressRule{
+					{
+						Host: "foo",


timoreimann · 2017-03-14T21:27:05Z

provider/kubernetes.go

+					return nil, err
+				}
+
+				if !exists {


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

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.

Separate test sounds good. 👍

Added tests for this and the !exists endpoints

timoreimann · 2017-03-14T21:29:19Z

provider/kubernetes.go

@@ -196,10 +199,10 @@ func (provider *Kubernetes) loadIngresses(k8sClient k8s.Client) (*types.Configur
 						endpoints, exists, err := k8sClient.GetEndpoints(service.ObjectMeta.Namespace, service.ObjectMeta.Name)
 						if err != nil || !exists {


Any particular reason we not splitting up the two cases here as well?

A bad rebase is why actually. Specifically wrote some test code for it. Will fix,

timoreimann · 2017-03-14T21:32:10Z

provider/kubernetes_test.go

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


You can get rid of this variable by changing the type of api{Service,Endpoints}Error to error and returning the error if it's non-nil in the stubbed getter methods.

timoreimann · 2017-03-14T21:40:51Z

provider/kubernetes_test.go

+	_, err := provider.loadIngresses(client)
+
+	if err == nil {
+		t.Fatal("Should have raised error when loading services but did not.")


Instead of just checking for the error being nil, what I think we can do here is pass a custom error variable to the mock (e.g., errors.New("failed")) and check against this very error. It shouldn't matter what error value we inject since our code doesn't depend on any specific error -- we just want to verify that we return the error to the caller that the (mocked) API gives us.
Similar for the endpoint case.

I hope this addresses your third question.

That should work great. Will make the change.

regner · 2017-03-15T00:03:23Z

Tests SHOULD be passing now. Still need to move the tests over to using the table driven test method, need to add tests for the !exists code paths, and am not sure how we want to handle when things don't exist. For example when a service doesn't exist do we still add the frontend? The two cases of this right now are when services or endpoints don't exist. Both cases can easily be someone putting a typo in the service name or port name.

timoreimann · 2017-03-15T10:50:10Z

AFAICS, the only alternative we have to skipping frontends with missing services/endpoints is to revert the entire configuration (i.e., apply the error case resolution). IMHO, we shouldn't do that since such misses are usually due to a user mistake (as you said) and would consequently "punish" all other frontends that did everything correct.
There's also the case where a service or endpoint isn't available because the k8s reconciliation loops haven't completed yet. That's another situation where skipping and hoping for the next watch API event to trigger a benevolent update seems like the better option to me.

I believe the focus of this PR should be to fix #1240, which is to protect against the Kube API temporarily being unavailable. By handling that case gracefully (through reverting to the previous configuration) and maintaining the "missing" behavior as-is, I think we have satisfied the goal at hand. Potential improvements to the missing case, especially if the direction isn't super clear, can be handled by follow-up issues/PRs.

Just my 2 cents. :)

regner · 2017-03-15T19:10:32Z

Was reviewing this PR this morning and noticed that the condition in my new test to check for failure was actually wrong and would be passing even if it shouldn't.

Fixed that and added test data back in. Without the test data the loadIngress logic doesn't get far enough along to trigger the errors we want to test for. So added the test data back in.

Very sorry about that! The logic in loadIngress was fine and I didn't have to make any changes there. It was just the tests not actually doing anything useful.

timoreimann

One more comment.

Also, anxiously waiting for that table-driven version. :-)

timoreimann · 2017-03-15T19:55:57Z

provider/kubernetes_test.go

+	provider := Kubernetes{}
+	_, err := provider.loadIngresses(client)
+
+	if err == nil || err.Error() != "failed service call" {


We can simplify the assertion by comparing error values:

if err != client.apiServiceError { ... }

Once you turn the test into a table-driven one, define and inject an error variable var apiErr = errors.New("error") and compare against that in each test case.

timoreimann · 2017-03-15T19:56:14Z

provider/kubernetes_test.go

+	provider = Kubernetes{}
+	_, err = provider.loadIngresses(client)
+
+	if err == nil || err.Error() != "failed endpoints call" {


Same pattern as above.

emilevauge · 2017-03-15T23:00:14Z

provider/kubernetes_test.go

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


Minor comment, but I would use something more random than failed here.

emilevauge

Thanks a lot @regner :)
Could you squash your commits?
Apart from minor comments, LGTM.

Currently if a Kubernetes API call fails we potentially remove a working service from Traefik. This changes it so if a Kubernetes API call fails we abort out of the ingress update and use the current working config. Github issue: traefik#1240 Also added a test to cover when requested resources (services and endpoints) that the user has specified don’t exist.

regner · 2017-03-16T00:52:58Z

Commits all squashed and that final test case added.

@timoreimann be aware, as this isn't obvious with the squash, that I renamed the test from TestServiceErrors to TestKubeAPIErrors as it isn't just related to services but endpoints as well.

I also changed apiErr := errors.New("failed") to apiErr := errors.New("failed kube api call") as per the comment from @emilevauge.

Other then that I only added the TestMissingResources test case and nothing else should have changed.

timoreimann

Found one tiny test coding bug and filed two suggestions. :-)

timoreimann · 2017-03-16T05:21:47Z

provider/kubernetes_test.go

+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {


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.

timoreimann · 2017-03-16T08:28:04Z

provider/kubernetes_test.go

+					},
+				},
+				{
+					Host: "bar",


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.

timoreimann · 2017-03-16T08:28:31Z

provider/kubernetes_test.go

+					},
+				},
+				{
+					Host: "tar",


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

timoreimann · 2017-03-16T10:33:48Z

The new TestMissingResources test is still failing according to CI.

Tests are failing

…golang.org/subtests

…hat instead of nil.

regner · 2017-03-16T20:43:32Z

My most recent commit where I add a blank Servers attribute the "missing_service" expected output is required to pass the diff. @timoreimann helped me figure this out, but essentially without it we end up comparing a nil to a blank Servers definition.

-          (string) (len=15) "missing_service": (*types.Backend)(0xc4201b0450)({
-           Servers: (map[string]types.Server) <nil>,
+          (string) (len=15) "missing_service": (*types.Backend)(0xc4201b0270)({
+           Servers: (map[string]types.Server) {
+           },

One of the unfortunate parts of that is the normal logging we do for the failed comparison doesn't show this difference. The above output was retrieved when @timoreimann made the following change:

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

Something to keep in mind in the future.

… test to help ensure we have useful output incase of failures

…verride the return value for if the endpoints exist. After the 1.2 release the use of properExists should be removed and the GetEndpoints function should return false for the second value indicating the endpoint doesn’t exist. However at this time that would break a lot of the tests.

timoreimann

LGTM -- we did it, @regner. :-)

Major thanks for bearing with me for so long and helping to discover a few issues with the existing tests. This should prove valuable in improving the correcting the tests in the future, which is vital for our code base to stay healthy.

Again: Great job! 🎉

timoreimann · 2017-03-17T13:15:22Z

@regner I pushed a small commit that includes a link to #1307.

The tests went green before, so I think that we can squash and merge as soon as we have @emilevauge's approval. 🎉

regner · 2017-03-17T14:54:17Z

Good call on adding the link

emilevauge

LGTM!
Thanks a lot @regner & @timoreimann :)

* Abort Kubernetes Ingress update if Kubernetes API call fails Currently if a Kubernetes API call fails we potentially remove a working service from Traefik. This changes it so if a Kubernetes API call fails we abort out of the ingress update and use the current working config. Github issue: #1240 Also added a test to cover when requested resources (services and endpoints) that the user has specified don’t exist. * Specifically capturing the tc range as documented here: https://blog.golang.org/subtests * Updating service names in the mock data to be more clear * Updated expected data to match what currently happens in the loadIngress * Adding a blank Servers to the expected output so we compare against that instead of nil. * Replacing the JSON test output with spew for the TestMissingResources test to help ensure we have useful output incase of failures * Adding a temporary fix to the GetEndoints mocked function so we can override the return value for if the endpoints exist. After the 1.2 release the use of properExists should be removed and the GetEndpoints function should return false for the second value indicating the endpoint doesn’t exist. However at this time that would break a lot of the tests. * Adding quick TODO line about removing the properExists property * Link to issue 1307 re: properExists flag.

timoreimann self-requested a review March 14, 2017 20:35

timoreimann suggested changes Mar 14, 2017

View reviewed changes

emilevauge added this to the 1.2 milestone Mar 14, 2017

emilevauge added status/1-needs-design-review status/2-needs-review labels Mar 14, 2017

timoreimann suggested changes Mar 15, 2017

View reviewed changes

emilevauge reviewed Mar 15, 2017

View reviewed changes

emilevauge previously approved these changes Mar 15, 2017

View reviewed changes

regner force-pushed the 1240-skip-kubernetes-update-if-api-fails branch from c96f69f to fab3cb3 Compare March 16, 2017 00:29

Merge branch 'v1.2' into 1240-skip-kubernetes-update-if-api-fails

e85abc3

timoreimann suggested changes Mar 16, 2017

View reviewed changes

Regner Blok-Andersen added 4 commits March 16, 2017 11:54

Specifically capturing the tc range as documented here: https://blog.…

e806930

…golang.org/subtests

Updating service names in the mock data to be more clear

1f3e5b5

Updated expected data to match what currently happens in the loadIngress

e06235b

Adding a blank Servers to the expected output so we compare against t…

4814013

…hat instead of nil.

Regner Blok-Andersen added 3 commits March 16, 2017 13:49

Replacing the JSON test output with spew for the TestMissingResources…

2b2674a

… test to help ensure we have useful output incase of failures

Adding quick TODO line about removing the properExists property

69442cb

regner mentioned this pull request Mar 16, 2017

Update Kubernetes tests to properly handle endpoints not existing #1307

Closed

timoreimann approved these changes Mar 16, 2017

View reviewed changes

Link to issue 1307 re: properExists flag.

f57e7a5

emilevauge approved these changes Mar 17, 2017

View reviewed changes

emilevauge merged commit 75d92c6 into traefik:v1.2 Mar 17, 2017

timoreimann mentioned this pull request Mar 19, 2017

When Kubernetes API Fails, Traefik starts returning 404s #1240

Closed

jonaz mentioned this pull request Mar 22, 2017

Kubernetes: should not remove configured frontends and backends when we loose contact with apiservers #912

Closed

regner mentioned this pull request Mar 22, 2017

Referencing service variable which is nil in log message #1330

Closed

ldez added kind/bug/fix a bug fix kind/enhancement a new or improved feature. area/provider/k8s/ingress and removed status/2-needs-review status/1-needs-design-review kind/bug/fix a bug fix labels Apr 29, 2017

wu0407 mentioned this pull request Apr 14, 2021

When Kubernetes apiserver down , Traefik 2 starts returning 404 #8066

Closed

		@@ -196,10 +199,10 @@ func (provider Kubernetes) loadIngresses(k8sClient k8s.Client) (types.Configur
		endpoints, exists, err := k8sClient.GetEndpoints(service.ObjectMeta.Namespace, service.ObjectMeta.Name)
		if err != nil \|\| !exists {

Abort Kubernetes Ingress update if Kubernetes API call fails #1295

Abort Kubernetes Ingress update if Kubernetes API call fails #1295

Conversation

regner commented Mar 14, 2017 • edited by emilevauge

timoreimann left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regner commented Mar 15, 2017

timoreimann commented Mar 15, 2017

regner commented Mar 15, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

regner commented Mar 16, 2017

timoreimann left a comment

Choose a reason for hiding this comment

timoreimann Mar 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann commented Mar 16, 2017

regner commented Mar 16, 2017

timoreimann left a comment

Choose a reason for hiding this comment

timoreimann commented Mar 17, 2017 • edited

regner commented Mar 17, 2017

emilevauge left a comment

Choose a reason for hiding this comment

regner commented Mar 14, 2017 •

edited by emilevauge

timoreimann left a comment •

edited

timoreimann Mar 16, 2017 •

edited

timoreimann commented Mar 17, 2017 •

edited