oidc-authservice: add authservice to manifests #529

yanniszark · 2019-10-15T16:23:39Z

Which issue is resolved by this Pull Request:
Resolves #267
Resolves kubeflow/kubeflow#3839

Description of your changes:
This PR adds the AuthService to manifests.

It also adds those new capabilities to the existing_arrikto config.

Some small changes that were made to the Dex application:

Add VirtualService in istio overlay.
Use the latest image built from master, so as to include server: templates: use relative URLs to refer to assets dexidp/dex#1554

Checklist:

Unit tests have been rebuilt:
1. cd manifests/tests
2. make generate
3. make test

/cc @krishnadurai @elikatsis

This change is

krishnadurai · 2019-10-15T17:55:43Z

@yanniszark this is awesome! Thanks for being prompt!
I'll review this soon.

krishnadurai

@yanniszark great work here!

* Use the latest image built from master, so as to include [dexidp/dex#1554](https://github.com/dexidp/dex/pull/1554)

Especially for this cool contribution to dex!

I have had an initial look at the code and have a few comments. I would like to further deploy and test this out for my next pass.

krishnadurai · 2019-10-16T10:13:00Z

dex-auth/dex-crds/overlays/istio/virtual-service.yaml

+        prefix: /dex/
+    route:
+    - destination:
+        host: dex.auth.svc.cluster.local


Can we parameterize dex.auth.svc.cluster.local to dex.$(namespace).svc.cluster.local?

krishnadurai · 2019-10-16T10:26:20Z

dex-auth/dex-crds/overlays/istio/virtual-service.yaml

@@ -0,0 +1,22 @@
+# This config is gated on kiali upgrade to 0.21 from 0.16 in istio 1.1.6:
+# https://github.com/kiali/kiali/issues/1154
+# https://github.com/istio/istio/issues/11131


These comments were added because when this virtual-service is deployed through kubectl apply -f, kiali rejects this configuration as invalid for Istio 1.1.6.
I'll test this by deploying once again to see if it persists when we deploy through KfDef and kfctl. Otherwise, we could mention that only kubectl apply -f has this issue and KfDef works fine.

I can confirm that this issue is not surfacing through KfDef.

Great! What are the actionable items for this?

I think we can just change the comment to say that this was observed while deploying through kubectl CLI and it works fine with KfDef.

@krishnadurai I'm not too familiar with the issue and it seems you have experience with it.
Could you open an issue explaining what happens in order to track this?

krishnadurai · 2019-10-16T10:29:19Z

dex-auth/dex-crds/base/kustomization.yaml

@@ -80,5 +80,5 @@ configurations:
 - params.yaml
 images:
 - name: quay.io/coreos/dex
-  newName: quay.io/coreos/dex
-  newTag: v2.9.0
+  newName: gcr.io/arrikto/dexidp/dex


nit: We could move this image to gcr.io/kubeflow-public-images. This can be done after this PR gets merged in.
Can we add a TODO here to replace this with a dex upstream image whenever they release next?

Agreed.
I don't personally have access to gcr.io/kubeflow-images-public, so it has to be done by someone with the right permissions.

krishnadurai · 2019-10-16T10:30:00Z

istio/oidc-authservice/base/deployment.yaml

+    spec:
+      containers:
+      - name: authservice
+        image: gcr.io/arrikto/kubeflow/oidc-authservice:6ac9400


nit: We could move this image to gcr.io/kubeflow-public-images, after this PR is in.

krishnadurai · 2019-10-16T10:30:59Z

istio/oidc-authservice/base/envoy-filter.yaml

+kind: EnvoyFilter
+metadata:
+  name: authn-filter
+  namespace: istio-system


nit: Let's strip the namespace off metadata fields.

krishnadurai · 2019-10-16T10:38:43Z

istio/oidc-authservice/base/kustomization.yaml

+    apiVersion: v1
+  fieldref:
+    fieldpath: data.skip_auth_uri
+- name: userid-header


nit: We could use a convention to have these variables named in a consistent manner within this configMap.

Throughout manifests we seem to have different styles camel case, snake case, capitalized names for environment vars. I would be great to have a convention. This is not in relation to this just this PR, we could address this separately.

I agree, it is unfortunate.
I used snake case for the userid related parameters because that's how they're defined in other applications as well.

krishnadurai · 2019-10-16T14:47:40Z

istio/oidc-authservice/base/deployment.yaml

+          - name: CLIENT_SECRET
+            value: $(application_secret)
+          - name: STORE_PATH
+            value: /var/lib/authservice/data.db


@yanniszark, I'll need help from you to understand this part. I'm trying to figure out how this DB is getting used by the authservice. The latest code seems to be hosted here: https://github.com/arrikto/ambassador-auth-oidc/commits/feature-kubeflow-rc

Likewise, in the code for authservice, I can see that redis is being used for blacklisting tokens and is an optional component: ajmyyra/ambassador-auth-oidc@a07b7bd

It would be great if you can point me to where and how this DB is getting used by the authservice. Secondly, is this being used for blacklisting tokens?

@krishnadurai the latest code is in the arrikto/oidc-authservice repo.
Pleaso also see: #267 (comment)

The default DB is BoltDB, an embedded key-value store. Its data lives on a PVC.
The DB is used for 2 things:

Store state of the OIDC flow (see state parameter of OAuth).

Store sessions.

krishnadurai · 2019-10-16T14:51:57Z

tests/dex-crds-overlays-istio_test.go

+    spec:
+      serviceAccountName: dex
+      containers:
+      - image: quay.io/coreos/dex:v2.9.0


We should change this to the newer dex image for consistency.

This should be fixed by test generation.

I think the source file for dex deployment has this older value.

True, but this seems like an issue of the test generation, unrelated to this PR.
The image is changed using the images field of the kustomization file, which is the standard way provided by kustomize.

krishnadurai · 2019-10-16T14:56:16Z

istio/oidc-authservice/base/envoy-filter.yaml

+      httpService:
+        serverUri:
+          uri: http://authservice.istio-system.svc.cluster.local
+          cluster: outbound|8080||authservice.istio-system.svc.cluster.local


We should parameterize this to authservice.$(namespace).svc.cluster.local in case istio-system isn't istio deployment's namespace.

krishnadurai · 2019-10-16T15:06:20Z

tests/dex-crds-overlays-istio_test.go

+subjects:
+- kind: ServiceAccount
+  name: dex                 # Service account assigned to the dex pod.
+  namespace: auth           # The namespace dex is running in.


I'll remove these and replace them with $(namespace) in a follow-up PR.

yanniszark · 2019-10-16T17:58:13Z

Thanks @krishnadurai, great comments.
I rebased and addressed the proposed fixes in a new commit.
PTAL

krishnadurai

@yanniszark I have tested this on a fresh GCP cluster with the modified existing KfDef. Few points to note:

Istio ingressgateway is NodePort
Certificate creation was done manually - will be taken care by the CertManger changes
The platform existing.go code didn't seem to execute from the latest kfctl

I think these above 3 configurations can be tackled with follow on PRs.

There are a few comments which I've made and need to be addressed in this PR.

krishnadurai · 2019-10-16T18:22:05Z

tests/dex-crds-overlays-istio_test.go

+    spec:
+      serviceAccountName: dex
+      containers:
+      - image: quay.io/coreos/dex:v2.9.0


I think the source file for dex deployment has this older value.

krishnadurai · 2019-10-16T20:05:59Z

kfdef/kfctl_existing_arrikto.yaml

+    name: istio
+  - kustomizeConfig:
+      overlays:
+        - application


kustomizeConfig for application and application-crds needs to be on-top here so that we avoid this error:

error: unable to recognize "/tmp/kout": no matches for kind "Application" in version "app.k8s.io/v1beta1"

Interesting, I didn't get this error.
I wonder why.
Will fix according to your suggestion.

krishnadurai · 2019-10-16T20:08:30Z

kfdef/kfctl_existing_arrikto.yaml

+          value: istio-system
+      repoRef:
+        name: manifests
+        path: istio/istio-install


In istio-install istio-ingressgateway service is by default set to NodePort. I think the earlier platform config for this config assumes it to be LoadBalancer type.

We can address this later.

Yes, let's address this later.

krishnadurai · 2019-10-17T11:30:30Z

dex-auth/dex-crds/overlays/istio/virtual-service.yaml

@@ -0,0 +1,22 @@
+# This config is gated on kiali upgrade to 0.21 from 0.16 in istio 1.1.6:
+# https://github.com/kiali/kiali/issues/1154
+# https://github.com/istio/istio/issues/11131


I can confirm that this issue is not surfacing through KfDef.

yanniszark · 2019-10-17T17:46:49Z

@krishnadurai thanks again for the review.
Gave it a final pass and fixed the application ordering.

For the comment:

I can confirm that this issue is not surfacing through KfDef.

I'm not sure if there's an actionable item there.
Is there and is it necessary for now, or can we leave it for a subsequent PR?

krishnadurai

@yanniszark thanks.

Some of the comments on how we handle namespaces need to be addressed:

I'm not sure if there's an actionable item there.
Is there and is it necessary for now, or can we leave it for a subsequent PR?

We could just note that this was observed while deploying through kubectl CLI and KfDef works fine.

krishnadurai · 2019-10-18T09:40:57Z

dex-auth/dex-crds/overlays/istio/virtual-service.yaml

@@ -0,0 +1,22 @@
+# This config is gated on kiali upgrade to 0.21 from 0.16 in istio 1.1.6:
+# https://github.com/kiali/kiali/issues/1154
+# https://github.com/istio/istio/issues/11131


I think we can just change the comment to say that this was observed while deploying through kubectl CLI and it works fine with KfDef.

Signed-off-by: Yannis Zarkadas <[email protected]>

yanniszark · 2019-10-18T12:53:20Z

@krishnadurai thanks, I made the changes but forgot to commit.
All done now!

Signed-off-by: Yannis Zarkadas <[email protected]>

krishnadurai · 2019-10-18T13:33:11Z

@yanniszark great work with this PR!

/lgtm

yanniszark · 2019-10-18T14:10:14Z

/approve

k8s-ci-robot · 2019-10-18T14:10:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yanniszark

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yanniszark]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* istio: oidc-authservice Signed-off-by: Yannis Zarkadas <[email protected]> * dex: add virtualservice Signed-off-by: Yannis Zarkadas <[email protected]> * existing_arrikto kfdef update Signed-off-by: Yannis Zarkadas <[email protected]> * dex: custom image for relative urls Signed-off-by: Yannis Zarkadas <[email protected]> * istio: oidc-authservice: add application overlay Signed-off-by: Yannis Zarkadas <[email protected]> * reorder applications Signed-off-by: Yannis Zarkadas <[email protected]> * parameterize namespace Signed-off-by: Yannis Zarkadas <[email protected]> * regenerate tests Signed-off-by: Yannis Zarkadas <[email protected]> # Conflicts: # kfdef/kfctl_aws.0.7.0.yaml

…529 on v0.7-branch. #529: istio: oidc-authservice (#615) * istio: oidc-authservice Signed-off-by: Yannis Zarkadas <[email protected]> * dex: add virtualservice Signed-off-by: Yannis Zarkadas <[email protected]> * dex: custom image for relative urls Signed-off-by: Yannis Zarkadas <[email protected]> * istio: oidc-authservice: add application overlay Signed-off-by: Yannis Zarkadas <[email protected]> * parameterize namespace Signed-off-by: Yannis Zarkadas <[email protected]> * regenerate tests Signed-off-by: Yannis Zarkadas <[email protected]>

k8s-ci-robot requested review from elikatsis and krishnadurai October 15, 2019 16:23

googlebot added the cla: yes label Oct 15, 2019

k8s-ci-robot added the size/XXL label Oct 15, 2019

krishnadurai suggested changes Oct 16, 2019

View reviewed changes

yanniszark force-pushed the feature-existing-arrikto-internal-relative-urls-v3 branch from 3d6e278 to 52aa3e5 Compare October 16, 2019 17:56

krishnadurai suggested changes Oct 17, 2019

View reviewed changes

yanniszark force-pushed the feature-existing-arrikto-internal-relative-urls-v3 branch from 52aa3e5 to dd0b728 Compare October 17, 2019 17:37

krishnadurai suggested changes Oct 18, 2019

View reviewed changes

yanniszark added 6 commits October 18, 2019 14:21

istio: oidc-authservice

73cec63

Signed-off-by: Yannis Zarkadas <[email protected]>

istio: oidc-authservice: add application overlay

2cf5749

Signed-off-by: Yannis Zarkadas <[email protected]>

dex: add virtualservice

9651566

Signed-off-by: Yannis Zarkadas <[email protected]>

existing_arrikto kfdef update

9cbe48b

Signed-off-by: Yannis Zarkadas <[email protected]>

dex: custom image for relative urls

c7a090a

Signed-off-by: Yannis Zarkadas <[email protected]>

reorder applications

1dec326

Signed-off-by: Yannis Zarkadas <[email protected]>

yanniszark force-pushed the feature-existing-arrikto-internal-relative-urls-v3 branch from dd0b728 to 001e4fa Compare October 18, 2019 12:54

yanniszark added 2 commits October 18, 2019 16:29

parameterize namespace

b3e06fa

Signed-off-by: Yannis Zarkadas <[email protected]>

regenerate tests

acc18c8

Signed-off-by: Yannis Zarkadas <[email protected]>

yanniszark force-pushed the feature-existing-arrikto-internal-relative-urls-v3 branch from 001e4fa to acc18c8 Compare October 18, 2019 13:29

k8s-ci-robot assigned krishnadurai Oct 18, 2019

k8s-ci-robot added the lgtm label Oct 18, 2019

k8s-ci-robot added the approved label Oct 18, 2019

k8s-ci-robot merged commit 7f53ac9 into kubeflow:master Oct 18, 2019

krishnadurai mentioned this pull request Oct 23, 2019

Integrate Dex Auth with Multi User Story #267

Closed

yanniszark mentioned this pull request Nov 14, 2019

v0.7-branch: cherry-pick oidc-authservice and cert-manager PRs for kfctl_existing_arrikto #616

Closed

oidc-authservice: add authservice to manifests #529

oidc-authservice: add authservice to manifests #529

Conversation

yanniszark commented Oct 15, 2019 • edited by jlewi Loading

krishnadurai commented Oct 15, 2019

krishnadurai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishnadurai Oct 16, 2019 • edited Loading

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

Choose a reason for hiding this comment

yanniszark commented Oct 16, 2019

krishnadurai 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

krishnadurai Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanniszark commented Oct 17, 2019

krishnadurai left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanniszark commented Oct 18, 2019

krishnadurai commented Oct 18, 2019

yanniszark commented Oct 18, 2019

k8s-ci-robot commented Oct 18, 2019

yanniszark commented Oct 15, 2019 •

edited by jlewi

Loading

krishnadurai Oct 16, 2019 •

edited

Loading

krishnadurai Oct 16, 2019 •

edited

Loading

krishnadurai left a comment •

edited

Loading