Optionally supplement Dex configuration with information required for embedded ui auth #504

richard-cox · 2023-10-18T13:03:20Z

Required by Intergrate Authorisation into embedded ui ui#311
Epinio UI, hosted in Rancher, will need to auth via dex
To support this the dex config needs updating
- Allow the rancher domain to contact Dex
- Add a new client specifically for requests from rancher's ui
These changes are only applied via new chart value rancher.url
- This has been added to questions.yaml

andreas-kupries

one nit (missing end-of-line at end of file).
one big question (about a possible mis-committed file)
the rest looks to be ok.

chart/epinio/get_helm.sh

chart/epinio/templates/dex.yaml

andreas-kupries

It looks ok to me.
I would like to have approval from @enrichman also, as the main dev for the dex/oidc parts.

enrichman

Yes, it looks good to me.

The Dex config update is probably something that we need to test.
I'm not sure about the behavior across Epinio upgrades.

/cc @mmartin24 @thehejik

richard-cox · 2023-10-23T08:34:16Z

I had a think, from memory helm managed resources aren't something that the user should be manually changing. We show a warning in the rancher ui for resources managed by helm saying something similar. So I'm guessing any changes made by the user will get overwritten on upgrade.

thehejik · 2023-10-23T12:19:30Z

chart/epinio/questions.yml

@@ -173,3 +173,9 @@ questions:
  required: false
  type: string
  group: "Debugging"
+- variable: rancher.url


Would be possible to replace the existing one server.accessControlAllowOrigin by this new?

server: # Domain which serves the Rancher UI (to access the API) accessControlAllowOrigin: ""

Done. server.accessControlAllowOrigin has been replaced with server.url.

@enrichman Can we add something that might stand out in release notes, to ensure people who upgrade can switch over?

thehejik · 2023-10-23T15:07:53Z

Helm upgrade after appending --set rancher.url=https://rancher.somewhere.io produces following new entries.

--- dex-config.yaml	2023-10-23 17:01:21.850655068 +0200
+++ dex-config-rancher.yaml	2023-10-23 17:01:36.402797082 +0200
@@ -1,4 +1,6 @@
 issuer: "https://auth.1.2.3.4.nip.io"
+web:
+  allowedOrigins: ['https://rancher.somewhere.io']
 storage:
   type: kubernetes
   config:
@@ -24,6 +26,7 @@
     trustedPeers:
       - epinio-cli
       - epinio-ui
+      - rancher-dashboard
   - id: epinio-cli
     name: 'Epinio cli'
     public: true
@@ -33,3 +36,8 @@
     # Shouldn't be public, https://dexidp.io/docs/custom-scopes-claims-clients/#public-clients
     redirectURIs:
       - "https://epinio.1.2.3.4.nip.io/auth/verify/"
+  - id: rancher-dashboard
+    name: 'Rancher Dashboard'
+    public: true
+    redirectURIs:
+      - "https://rancher.somewhere.io/epinio/auth/verify/"

The dex-config can be obtained by:

$ kubectl get secrets/dex-config -n epinio -o=go-template='{{index .data "config.yaml" | base64decode }}'

thehejik · 2023-11-14T15:33:13Z

It works as expected and the dex-config has the same fields related to rancher-dashboard as in report above. Also questions.yml modification is working properly (@mmartin24 the CORS input field has been removed completely ~~moved from General settings under a new Rancher section~~):

@richard-cox But today I found out that when the chart is installed over rancher:v2.8.0-rc3, there is already value global.cattle.url value automagically set by rancher so maybe we could just reuse the value and remove rancher.url completely, wdyt?

… embedded ui auth - Allows the rancher domain to contact Dex - Adds a new client specifically for requests from rancher's ui (where the embedded ui lives) - Optionally applied via new chart value `rancher.url` (also added to questions.yaml)

- get_helm sneaked in, that should not be included in PR - added new line at end of file

- This is the same content, so only require one. Keep rancher.url as more generic - Also added rancher.url to values.schema - We should add this change to the release notes, specificaly to warn when upgrading

…isting `rancher.url` - as questions.yaml is only applicable in rancher contexts, also remove rancher.url from there

thehejik

Test report:

OK - deployed out of rancher without rancher.url set - the secret data NOT modified
OK - deployed out if rancher with rancher.url set - the secret data modified
OK - deployed in rancher - the CORS field not present anymore - the secret data modified
OK - prev. released E version deployed in rancher -> upgrade from CLI by using the values.yaml from rancher installation extended by rancher.url - the secret data modified
OK - prev. released E version deployed in rancher -> upgrade from CLI by using the values.yaml from rancher installation without rancher.url - the secret data modified
OK - prev. released E version deployed in rancher -> upgrade from CLI by using the values.yaml from rancher installation without rancher.url and global.cattle.url - the secret data NOT modified
OK - upgraded over CLI to the chart under test with removed global.cattle.url - the secret data doesn't contain the rancher-dashboard client anymore

All good including questions.yml, the CORS field is gone but the URL is rendered correctly when installed over rancher.

richard-cox · 2023-11-15T13:50:08Z

OK - deployed in rancher - the CORS field not present anymore - the secret data modified
@thehejik Just to confirm... in this test case the ACCESS_CONTROL_ALLOW_ORIGIN value in the epinio-server Deployment is populated?

thehejik · 2023-11-15T15:52:24Z

@thehejik Just to confirm... in this test case the ACCESS_CONTROL_ALLOW_ORIGIN value in the epinio-server Deployment is populated?

@richard-cox yes, it is:

richard-cox · 2023-11-15T17:05:41Z

@andreas-kupries , @enrichman unless there are objections i'll merge this tomorrow AM

enrichman · 2023-11-15T17:25:14Z

I will be very happy so see this merged! Thanks @richard-cox

richard-cox added this to the v1.11.0 milestone Oct 18, 2023

richard-cox requested a review from andreas-kupries October 18, 2023 13:03

richard-cox self-assigned this Oct 18, 2023

richard-cox mentioned this pull request Oct 18, 2023

Intergrate Authorisation into embedded ui epinio/ui#311

Merged

richard-cox force-pushed the ui-embedded-dex-config branch 2 times, most recently from a80e212 to 255486e Compare October 18, 2023 13:06

andreas-kupries requested changes Oct 18, 2023

View reviewed changes

chart/epinio/get_helm.sh Outdated Show resolved Hide resolved

chart/epinio/templates/dex.yaml Outdated Show resolved Hide resolved

andreas-kupries previously approved these changes Oct 18, 2023

View reviewed changes

richard-cox requested a review from enrichman October 18, 2023 14:46

enrichman previously approved these changes Oct 20, 2023

View reviewed changes

thehejik reviewed Oct 23, 2023

View reviewed changes

richard-cox dismissed stale reviews from enrichman and andreas-kupries via 35ff6d0 November 13, 2023 16:13

richard-cox requested review from andreas-kupries and enrichman November 13, 2023 16:25

richard-cox requested a review from thehejik November 14, 2023 17:21

richard-cox added 6 commits November 15, 2023 09:37

Changes following review

f3e6bd3

- get_helm sneaked in, that should not be included in PR - added new line at end of file

Remove server.accessControlAllowOrigin, use rancher.url instead

1a17866

- This is the same content, so only require one. Keep rancher.url as more generic - Also added rancher.url to values.schema - We should add this change to the release notes, specificaly to warn when upgrading

Use global.cattle.url provided in rancher contexts, fall back on ex…

d6793b7

…isting `rancher.url` - as questions.yaml is only applicable in rancher contexts, also remove rancher.url from there

Attempted lint fix

de415d4

remove dashes, move location

f62c3f8

richard-cox force-pushed the ui-embedded-dex-config branch from f5c0612 to f62c3f8 Compare November 15, 2023 09:38

thehejik approved these changes Nov 15, 2023

View reviewed changes

richard-cox merged commit 04fd986 into epinio:main Nov 16, 2023
2 checks passed

richard-cox deleted the ui-embedded-dex-config branch November 16, 2023 08:05

mmartin24 mentioned this pull request Nov 16, 2023

Cypress / removing manual entering of CORS epinio/epinio-end-to-end-tests#393

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally supplement Dex configuration with information required for embedded ui auth #504

Optionally supplement Dex configuration with information required for embedded ui auth #504

richard-cox commented Oct 18, 2023

andreas-kupries left a comment

andreas-kupries left a comment

enrichman left a comment

richard-cox commented Oct 23, 2023

thehejik Oct 23, 2023

richard-cox Nov 13, 2023

thehejik commented Oct 23, 2023 •

edited

Loading

thehejik commented Nov 14, 2023 •

edited

Loading

thehejik left a comment

richard-cox commented Nov 15, 2023

thehejik commented Nov 15, 2023

richard-cox commented Nov 15, 2023

enrichman commented Nov 15, 2023

Optionally supplement Dex configuration with information required for embedded ui auth #504

Optionally supplement Dex configuration with information required for embedded ui auth #504

Conversation

richard-cox commented Oct 18, 2023

andreas-kupries left a comment

Choose a reason for hiding this comment

andreas-kupries left a comment

Choose a reason for hiding this comment

enrichman left a comment

Choose a reason for hiding this comment

richard-cox commented Oct 23, 2023

thehejik Oct 23, 2023

Choose a reason for hiding this comment

richard-cox Nov 13, 2023

Choose a reason for hiding this comment

thehejik commented Oct 23, 2023 • edited Loading

thehejik commented Nov 14, 2023 • edited Loading

thehejik left a comment

Choose a reason for hiding this comment

richard-cox commented Nov 15, 2023

thehejik commented Nov 15, 2023

richard-cox commented Nov 15, 2023

enrichman commented Nov 15, 2023

thehejik commented Oct 23, 2023 •

edited

Loading

thehejik commented Nov 14, 2023 •

edited

Loading