Substitute hardcoded "<namespace>/<name>" with k8s ListerGetter #3470

yue9944882 · 2018-06-11T08:50:55Z

What does this PR do?

Small refactor for kubernetes provider.

Remove those hardcoded <namespace>/<name> index and use common functions of ListerGetter provided by kubernetes informers. Or developers might get confused by previous code. 🤔

Note that according to current implementations of kubernetes informers on master branch which haven't been changed for a long time, the error returned by ListerGetter can only be "Not Found" error. So, with this PR, the return value by GetEndpoints,GetService,GetSecret will only be:

Endpoints/Service/Secret, true, nil
nil, false, "Not Found Error"

Motivation

For better human readability

More

Added/updated tests
Added/updated documentation

Additional Notes

dtomcej

LGTM

timoreimann · 2018-06-19T22:52:53Z

provider/kubernetes/client.go

-		endpoint = item.(*corev1.Endpoints)
+	endpoint, err := c.factories[c.lookupNamespace(namespace)].Core().V1().Endpoints().Lister().Endpoints(namespace).Get(name)
+	if err != nil {
+		return nil, !kubeerror.IsNotFound(err), err


IIUC, we will now return a non-nil error even if this is just about a missing object. However, our call site still considers non-nil errors as triggers to end Ingress parsing prematurely and back off as there is some presumed issue with the Kubernetes API connection.

I think we still need to translate not found errors into a nil, false, nil triple (or move the distinction to the call site).

Similar for other changes you have made.

(or move the distinction to the call site)

In order to narrow the scope of changes, I'd prefer the former solution. 😀

timoreimann · 2018-06-19T22:58:09Z

Even though upstream returns not found errors only right now, I'd still implement processing of the return value such that any other, additional error values introduced in the future are handled properly.

yue9944882 · 2018-06-22T09:24:34Z

Even though upstream returns not found errors only right now, I'd still implement processing of the return value such that any other, additional error values introduced in the future are handled properly.

Agree with that. IMHO, handling error values would be better to be done in the call site. Or just returning double (object, error) would be even better to me🧐

timoreimann · 2018-06-22T09:43:16Z

Even though upstream returns not found errors only right now, I'd still implement processing of the return value such that any other, additional error values introduced in the future are handled properly.

Agree with that. IMHO, handling error values would be better to be done in the call site. Or just returning double (object, error) would be even better to me.

Agree to differentiating at the call site as long as we are still able to tell the "not found" and "other error" cases apart where we need to (which is at the point where we decide to either log a warning and continue processing, or log an error and back of).

timoreimann · 2018-06-22T09:46:01Z

provider/kubernetes/client.go

+		if kubeerror.IsNotFound(err) {
+			return nil, false, nil
+		}
+		return nil, false, err


You only did the distinction for the endpoints now, but we need it for services and all other objects as well where we eventually treat absence and general errors differently with regards to Ingress processing.

timoreimann · 2018-07-02T17:12:58Z

provider/kubernetes/client.go

@@ -171,15 +172,11 @@ func (c *clientImpl) GetIngresses() []*extensionsv1beta1.Ingress {
 func (c *clientImpl) UpdateIngressStatus(namespace, name, ip, hostname string) error {
 	keyName := namespace + "/" + name

-	item, exists, err := c.factories[c.lookupNamespace(namespace)].Extensions().V1beta1().Ingresses().Informer().GetStore().GetByKey(keyName)
+	ing, err := c.factories[c.lookupNamespace(namespace)].Extensions().V1beta1().Ingresses().Lister().Ingresses(namespace).Get(name)
 	if err != nil {
 		return fmt.Errorf("failed to get ingress %s with error: %v", keyName, err)


I think we should omit the with error part now. It used to be necessary to distinguish between the "missing" and "true error" cases, but now the upstream error will include this information.

timoreimann · 2018-07-02T17:14:24Z

provider/kubernetes/client.go

-	return service, exists, err
+	service, err := c.factories[c.lookupNamespace(namespace)].Core().V1().Services().Lister().Services(namespace).Get(name)
+	exist, err := translateNotFoundError(err)
+	return service, exist, err


What do you think about just doing

return service, translateNotFoundError(err)

here and elsewhere too?

@timoreimann I'm afraid my IDE is not allowing me to do so. Is this syntax supported by golang?

it's not supported

true, my bad.

timoreimann · 2018-07-02T17:19:17Z

provider/kubernetes/client.go

+		return false, err
+	}
+	return true, nil
+}


I wonder if this is more or less readable:

func translateNotFoundError(err error) (bool, error) { if kubeerror.IsNotFound(err) { return false, nil } return err == nil, err }

WDYT?

timoreimann · 2018-07-02T17:20:32Z

provider/kubernetes/client.go

+		return false, err
+	}
+	return true, nil
+}


I think we should add a quick unit test for this function... it's easy to get some boolean wrong while the test should be easy to write.

ldez · 2018-07-03T08:24:30Z

provider/kubernetes/client_test.go

+
+func TestTranslateNotFoundError(t *testing.T) {
+	testCases := []struct {
+		err            error


could you add desc and put a description of each case.

ldez · 2018-07-03T08:27:34Z

provider/kubernetes/client_test.go

+		},
+	}
+	for _, testCase := range testCases {
+		exists, err := translateNotFoundError(testCase.err)


for _, test := range testCases { test := test t.Run(test.desc, func(t *testing.T) { t.Parallel() exists, err := translateNotFoundError(test.err) assert.Equal(t, test.expectedExists, exists) assert.Equal(t, test.expectedError, err) }) }

ldez · 2018-07-03T08:28:36Z

provider/kubernetes/client.go

 	}
-	log.Infof("Updated status on ingress %s", keyName)
+	log.Infof("Updated status on ingress %s", namespace+"/"+name)


log.Infof("Updated status on ingress %s/%s", namespace,name)

ldez · 2018-07-03T08:29:05Z

provider/kubernetes/client.go

@@ -192,40 +187,31 @@ func (c *clientImpl) UpdateIngressStatus(namespace, name, ip, hostname string) e

 	_, err = c.clientset.ExtensionsV1beta1().Ingresses(ingCopy.Namespace).UpdateStatus(ingCopy)
 	if err != nil {
-		return fmt.Errorf("failed to update ingress status %s with error: %v", keyName, err)
+		return fmt.Errorf("failed to update ingress status %s: %v", namespace+"/"+name, err)


return fmt.Errorf("failed to update ingress status %s/%s: %v", namespace, name, err)

ldez · 2018-07-03T08:29:28Z

provider/kubernetes/client.go

-	}
-	if !exists {
-		return fmt.Errorf("failed to update ingress %s because it does not exist", keyName)
+		return fmt.Errorf("failed to get ingress %s: %v", namespace+"/"+name, err)


return fmt.Errorf("failed to get ingress %s/%s: %v", namespace, name, err)

ldez · 2018-07-03T09:11:39Z

provider/kubernetes/client_test.go

+	"github.com/stretchr/testify/assert"
+	kubeerror "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"testing"


import ( "fmt" "testing" "github.com/stretchr/testify/assert" kubeerror "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" )

ldez

LGTM

yue9944882 requested a review from a team as a code owner June 11, 2018 08:50

traefiker added area/provider/k8s/ingress size/S status/0-needs-triage labels Jun 11, 2018

ldez added status/2-needs-review kind/enhancement a new or improved feature. and removed status/0-needs-triage labels Jun 11, 2018

dtomcej approved these changes Jun 11, 2018

View reviewed changes

yue9944882 force-pushed the chore-listergetter-funcs-to-get-resource branch from f0a249a to 402ef53 Compare June 11, 2018 18:12

ldez added the bot/no-merge label Jun 12, 2018

timoreimann suggested changes Jun 19, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jun 19, 2018

yue9944882 force-pushed the chore-listergetter-funcs-to-get-resource branch from 9399736 to c8b0256 Compare June 22, 2018 09:17

timoreimann suggested changes Jun 22, 2018

View reviewed changes

yue9944882 force-pushed the chore-listergetter-funcs-to-get-resource branch from c8b0256 to 0724af9 Compare June 24, 2018 13:13

dtomcej removed the contributor/waiting-for-corrections label Jun 28, 2018

mmatur added this to the 1.7 milestone Jul 2, 2018

timoreimann reviewed Jul 2, 2018

View reviewed changes

ldez requested changes Jul 3, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jul 3, 2018

ldez requested changes Jul 3, 2018

View reviewed changes

timoreimann approved these changes Jul 3, 2018

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label Jul 3, 2018

ldez removed the bot/no-merge label Jul 4, 2018

ldez approved these changes Jul 4, 2018

View reviewed changes

ldez added status/3-needs-merge and removed status/2-needs-review labels Jul 4, 2018

traefiker added the status/4-merge-in-progress label Jul 4, 2018

yue9944882 and others added 5 commits July 4, 2018 14:46

cleanup hardcoded "<namespace>/<name>" and use listergetter

3859fd6

translate not found error

77c60f4

review: update error msg & add unit test

1b6ee3f

review: minor changes

aaa37c5

review: order fields.

37b9698

traefiker force-pushed the chore-listergetter-funcs-to-get-resource branch from e918bf7 to 37b9698 Compare July 4, 2018 14:46

traefiker removed the status/4-merge-in-progress label Jul 4, 2018

traefiker merged commit b1f1a5b into traefik:master Jul 4, 2018

traefiker removed the status/3-needs-merge label Jul 4, 2018

ldez changed the title ~~Substitude hardcoded "<namespace>/<name>" with k8s ListerGetter~~ Substitute hardcoded "<namespace>/<name>" with k8s ListerGetter Mar 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Substitute hardcoded "<namespace>/<name>" with k8s ListerGetter #3470

Substitute hardcoded "<namespace>/<name>" with k8s ListerGetter #3470

yue9944882 commented Jun 11, 2018 •

edited by ldez

dtomcej left a comment

timoreimann Jun 19, 2018 •

edited

yue9944882 Jun 22, 2018

timoreimann commented Jun 19, 2018

yue9944882 commented Jun 22, 2018 •

edited

timoreimann commented Jun 22, 2018 •

edited

timoreimann Jun 22, 2018

timoreimann Jul 2, 2018

timoreimann Jul 2, 2018

yue9944882 Jul 3, 2018

ldez Jul 3, 2018

timoreimann Jul 3, 2018

timoreimann Jul 2, 2018

timoreimann Jul 2, 2018

ldez Jul 3, 2018

ldez Jul 3, 2018

ldez Jul 3, 2018

ldez Jul 3, 2018

ldez Jul 3, 2018

ldez Jul 3, 2018

ldez left a comment

Substitute hardcoded "<namespace>/<name>" with k8s ListerGetter #3470

Substitute hardcoded "<namespace>/<name>" with k8s ListerGetter #3470

Conversation

yue9944882 commented Jun 11, 2018 • edited by ldez

What does this PR do?

Motivation

More

Additional Notes

dtomcej left a comment

Choose a reason for hiding this comment

timoreimann Jun 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann commented Jun 19, 2018

yue9944882 commented Jun 22, 2018 • edited

timoreimann commented Jun 22, 2018 • 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

ldez left a comment

Choose a reason for hiding this comment

yue9944882 commented Jun 11, 2018 •

edited by ldez

timoreimann Jun 19, 2018 •

edited

yue9944882 commented Jun 22, 2018 •

edited

timoreimann commented Jun 22, 2018 •

edited