Add basic auth to kubernetes provider #1488

alpe · 2017-04-24T14:40:30Z

This will add the kubernetes provider for basic authentication. Thanks to @SantoDE for #1147.

Some constraints:

Basic authentication only
Realm not configurable; only traefikdefault
Secret is in same namespace as ingress rule
Secret must contain only single file (*)

(*) otherwise we can have some conventions here like key name (pattern)

Example

Based on https://docs.traefik.io/user-guide/kubernetes/

Encode credentials
htpasswd -c ./auth jane
Deploy a secret to k8s
kubectl --namespace=kube-system create secret generic mysecret --from-file auth
Deploy ingress

apiVersion: v1
kind: Service
metadata:
  name: traefik-web-ui
  namespace: kube-system
spec:
  selector:
    k8s-app: traefik-ingress-lb
  ports:
  - port: 80
    targetPort: 8080
---
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: traefik-web-ui
  namespace: kube-system
  annotations:
    ingress.kubernetes.io/auth-type: "basic"
    ingress.kubernetes.io/auth-secret: "mysecret"
spec:
  rules:
  - host: traefik-ui.local
    http:
      paths:
      - backend:
          serviceName: traefik-web-ui
          servicePort: 80

Note the two annotations to enable basic auth and link the secret

errm

LGTM apart from the one small spelling error

errm · 2017-04-24T14:49:01Z

provider/kubernetes/kubernetes.go

+		return nil, fmt.Errorf("secret %q/%q not found", namespace, secretName)
+	}
+	if secret == nil {
+		return nil, errors.New("secret data must nost be nil")


SantoDE

LGTM 👼

@alpe squash and rebase please ;-)

timoreimann

Left a few comments.

I notice that most of the unhappy paths are not test-covered. Can we improve here? You already added the necessary secret stub.

timoreimann · 2017-04-24T16:21:47Z

provider/kubernetes/kubernetes.go

+						return nil, err
+					}
+					if len(basicAuthCreds) == 0 {
+						return nil, errors.New("secet file without credentials")


Typo (secet -> secret)

timoreimann · 2017-04-24T16:22:28Z

provider/kubernetes/kubernetes.go

+					}
+				} else if authType != "" {
+					return nil, fmt.Errorf("unsupported auth-type: %q", authType)
+				}


Could we please put the entire block in a separate function?

loadIngresses is already pretty big.

timoreimann · 2017-04-24T16:24:22Z

provider/kubernetes/kubernetes.go

+					if len(basicAuthCreds) == 0 {
+						return nil, errors.New("secet file without credentials")
+					}
+				} else if authType != "" {


Can we do this check before the happy path so that we don't need the else?

The condition was authType != "" && strings.ToLower(authType) == "basic" for the error. The else block made sense but it's better in the extracted method.

timoreimann · 2017-04-24T16:26:02Z

provider/kubernetes/kubernetes.go

+	}
+	if len(secret.Data) != 1 {
+		return nil, errors.New("secret must contain single element only")
+	}


This block lends itself to a switch statement IMHO.

The order of the conditions matter. The switch works top down when multiple conditions are true. So that's fine but it feels more fragile that I'll add note to keep the order.

timoreimann · 2017-04-24T16:27:00Z

provider/kubernetes/kubernetes.go

+		return nil, fmt.Errorf("secret %q/%q not found", namespace, secretName)
+	}
+	if secret == nil {
+		return nil, errors.New("secret data must not be nil")


Can we rephrase this without the nil for the user? What does a nil secret imply?

Why can this be nil anyway when ok is true? I was wondering a lot when I ran into this but I did not spent time investigating.

My guess is it won't ever be nil unless ok is false or err is non-nil. The pointer is probably for performance / in-place-update reasons.

timoreimann · 2017-04-24T16:27:41Z

provider/kubernetes/kubernetes.go

+		return nil, errors.New("secret data must not be nil")
+	}
+	if len(secret.Data) != 1 {
+		return nil, errors.New("secret must contain single element only")


Improvement suggestion: "multiple secrets are not supported".

Data can have 0 elements, too. I think this is a temporary solution until we have some convention for a key name or pattern.

timoreimann · 2017-04-24T16:28:48Z

provider/kubernetes/kubernetes.go

+	for _, v := range secret.Data {
+		firstElement = v
+		break
+	}


Why not firstElement := secret.Data[0]? We have already asserted there's only a single element.

Data is a map

Ah true, missed that. 👍

timoreimann · 2017-04-24T16:29:27Z

provider/kubernetes/kubernetes.go

+	if len(secret.Data) != 1 {
+		return nil, errors.New("secret must contain single element only")
+	}
+	var firstElement []byte


Variable name is not ideal. Maybe firstSecret instead?

timoreimann · 2017-04-24T16:32:31Z

provider/kubernetes/kubernetes_test.go

+	actual = provider.loadConfig(*actual)
+	got := actual.Frontends["basic/auth"].BasicAuth
+	if !reflect.DeepEqual(got, []string{"myUser:myEncodedPW"}) {
+		t.Fatalf("unexpected creadentials: %+v", got)


timoreimann · 2017-04-24T16:33:42Z

provider/kubernetes/kubernetes_test.go

@@ -2121,6 +2278,19 @@ func (c clientMock) GetService(namespace, name string) (*v1.Service, bool, error
 	return nil, false, nil
 }

+func (c clientMock) GetSecret(namespace, name string) (*v1.Secret, bool, error) {
+	if c.apiServiceError != nil {


This should not reuse apiServiceError but get its own field.

alpe · 2017-04-24T20:12:16Z

I have applied most of the review comments @timoreimann requested. Thanks for the feedback! The test coverage and documentation can be better. I may be able to do a little bit more end of the week.

timoreimann · 2017-04-24T20:33:11Z

Thanks @alpe, looking pretty good now.

A bit of documentation (especially with regards to the limitations mentioned in the PR) and coverage of the important unhappy paths would be great to have. 👍

emilevauge · 2017-05-01T16:36:22Z

Ping @alpe

ldez · 2017-05-02T13:15:33Z

Documentation added

documentation added

kekoav · 2017-06-02T05:11:58Z

@alpe @timoreimann I know this is done but I was reviewing due to #1596 and saw this:
https://github.com/containous/traefik/blob/master/provider/kubernetes/client.go#L266

// fireEvent checks if all controllers have synced before firing
// Used after startup or a reconnect
func (c *clientImpl) fireEvent(event interface{}, eventCh chan interface{}) {
	if !c.ingController.HasSynced() || !c.svcController.HasSynced() || !c.epController.HasSynced() {
		return
	}
	eventHandlerFunc(eventCh, event)
}

Was excluding the new secController from this check for "all controllers" intentional?

timoreimann · 2017-06-02T06:15:57Z

Side question: What do we need the secController for exactly?

I'm really not too deep into that part of the code. What I know though is that we use various events as a trigger to poll the k8s APIs. Does the secController make sure we do that for Secrets events as well? If so, can such events ever indicate a state change relevant for Traefik?

The traefik controller shall have access to secrets for the k8s basic authentication (traefik#1488) to work

The traefik controller shall have access to secrets for the k8s basic authentication (#1488) to work

shadycuz · 2017-11-07T14:08:44Z

@alpe These instuctions

Encode credentials
htpasswd -c ./auth jane
Deploy a secret to k8s
kubectl --namespace=kube-system create secret generic mysecret --from-file auth

are not on the traefik website and it was very hard to eventually track down this PR and find the example for creating the secret.

alpe · 2017-11-07T14:41:34Z

@shadycuz Apologies, this could have been better. I had very limited time to work on this when I contributed the code. I'm happy that you find it useful. I am very sure the community will appreciate it when you could help with some doc so that it is easier for others to setup the basic auth.

shadycuz · 2017-11-07T14:46:32Z

@alpe I can do that from what I know and what I see in this PR, but I have other questions, for example can we generate the secret according to instructions for generating the web dashboard auth?

Passwords can be encoded in MD5, SHA1 and BCrypt: you can use htpasswd to generate those ones.

as long as its USERNAME:ENCRYPTEDPASSWORD ?

ldez · 2017-11-07T15:02:04Z

https://docs.traefik.io/configuration/backends/kubernetes/#authentication

erkolson · 2017-11-10T21:37:36Z

Probably not the right place to post this, please tell me somewhere else that is more appropriate...

I think it is problematic to grant permission to get every secret in the kube-system namespace. It seems like the RBAC constraints should be limited to a specific secret resource or pattern for the secret name, for example:

rules:
  - apiGroups:
      - ""
    resources:
      - secrets
    verbs:
      - list
      - watch
  - apiGroups:
      - ""
    resources:
      - secrets
    verbs:
      - get
    resourceNames:
      - "traefik-*"

A small constraint on the user's secret name would make it a lot safer to run in the kube-system namespace.

ldez · 2017-11-10T22:44:35Z

@erkolson Could ask your question on the Traefik community Slack channel or Stack Overflow using the "traefik" tag

ldez added kind/enhancement a new or improved feature. area/provider/k8s/ingress labels Apr 24, 2017

errm suggested changes Apr 24, 2017

View reviewed changes

alpe force-pushed the k8s-auth branch from 1f7d95b to f929161 Compare April 24, 2017 14:56

errm approved these changes Apr 24, 2017

View reviewed changes

SantoDE approved these changes Apr 24, 2017

View reviewed changes

alpe force-pushed the k8s-auth branch from f929161 to 70b599b Compare April 24, 2017 15:32

timoreimann self-requested a review April 24, 2017 16:23

timoreimann previously requested changes Apr 24, 2017

View reviewed changes

alpe force-pushed the k8s-auth branch from 70b599b to b434e3c Compare April 24, 2017 20:00

alpe force-pushed the k8s-auth branch from b434e3c to d344b95 Compare April 24, 2017 20:13

ldez added the contributor/waiting-for-corrections label Apr 24, 2017

ldez added this to the 1.3 milestone Apr 25, 2017

ldez added status/3-docs-review contributor/waiting-for-documentation and removed contributor/waiting-for-corrections labels Apr 28, 2017

ldez force-pushed the k8s-auth branch from d344b95 to 5f0836a Compare May 2, 2017 13:08

ldez removed the contributor/waiting-for-documentation label May 2, 2017

ldez force-pushed the k8s-auth branch from bdb0c60 to 3f68012 Compare May 2, 2017 19:50

zuc mentioned this pull request May 2, 2017

Add basic auth to kubernetes provider #1532

Closed

ldez force-pushed the k8s-auth branch 2 times, most recently from 5f0836a to 97e0be3 Compare May 3, 2017 09:47

alpe and others added 2 commits May 3, 2017 13:18

Add basic auth to kubernetes provider

89da3b1

doc: add k8s basic auth.

420a6db

ldez force-pushed the k8s-auth branch from 97e0be3 to 420a6db Compare May 3, 2017 11:18

ldez merged commit b528339 into traefik:master May 3, 2017

ldez removed the status/3-needs-docs-review label May 11, 2017

Eriner mentioned this pull request May 13, 2017

basic auth on kubernetes - unexpected watch event object type #1596

Closed

spinto mentioned this pull request Jun 3, 2017

traefik controller access to secrets #1707

Merged

timoreimann mentioned this pull request Jul 4, 2017

traefik stops processing Ingresses if RBAC permissions for Secrets are not granted #1826

Closed

ldez pushed a commit to spinto/traefik that referenced this pull request Jul 7, 2017

traefik controller access to secrets

5d52587

The traefik controller shall have access to secrets for the k8s basic authentication (traefik#1488) to work

ldez pushed a commit that referenced this pull request Jul 7, 2017

traefik controller access to secrets

ca5bbab

The traefik controller shall have access to secrets for the k8s basic authentication (#1488) to work

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic auth to kubernetes provider #1488

Add basic auth to kubernetes provider #1488

alpe commented Apr 24, 2017

errm left a comment

errm Apr 24, 2017

SantoDE left a comment

timoreimann left a comment

timoreimann Apr 24, 2017

timoreimann Apr 24, 2017

timoreimann Apr 24, 2017

alpe Apr 24, 2017

timoreimann Apr 24, 2017

alpe Apr 24, 2017

timoreimann Apr 24, 2017

alpe Apr 24, 2017

timoreimann Apr 24, 2017

timoreimann Apr 24, 2017

alpe Apr 24, 2017

timoreimann Apr 24, 2017

timoreimann Apr 24, 2017

alpe Apr 24, 2017

timoreimann Apr 24, 2017

timoreimann Apr 24, 2017

timoreimann Apr 24, 2017

timoreimann Apr 24, 2017

alpe commented Apr 24, 2017

timoreimann commented Apr 24, 2017

emilevauge commented May 1, 2017

ldez commented May 2, 2017

kekoav commented Jun 2, 2017

timoreimann commented Jun 2, 2017

shadycuz commented Nov 7, 2017

alpe commented Nov 7, 2017

shadycuz commented Nov 7, 2017

ldez commented Nov 7, 2017

erkolson commented Nov 10, 2017 •

edited

ldez commented Nov 10, 2017

Add basic auth to kubernetes provider #1488

Add basic auth to kubernetes provider #1488

Conversation

alpe commented Apr 24, 2017

Some constraints:

Example

errm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SantoDE left a comment

Choose a reason for hiding this comment

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

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

alpe commented Apr 24, 2017

timoreimann commented Apr 24, 2017

emilevauge commented May 1, 2017

ldez commented May 2, 2017

kekoav commented Jun 2, 2017

timoreimann commented Jun 2, 2017

shadycuz commented Nov 7, 2017

alpe commented Nov 7, 2017

shadycuz commented Nov 7, 2017

ldez commented Nov 7, 2017

erkolson commented Nov 10, 2017 • edited

ldez commented Nov 10, 2017

erkolson commented Nov 10, 2017 •

edited