Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-enable the dev dashboard #3477

Merged
merged 5 commits into from
Jul 3, 2023

Conversation

dbutenhof
Copy link
Member

@dbutenhof dbutenhof commented Jul 3, 2023

PBENCH-1203

The shift to HTTPS and Keycloak has broken our dashboard local dev mode hack in two ways:

  1. The axios.get call in the express mirror server needs to either load certificates or disable validation. This adds the CI private CA key.
  2. Login requires that the Keycloak be configured with the address of the dashboard code, which in this mode is http://localhost:3000. This adds that redirect to the Keycloak configuration

@dbutenhof dbutenhof added the Dashboard Of and relating to the Dashboard GUI label Jul 3, 2023
@dbutenhof dbutenhof self-assigned this Jul 3, 2023
@webbnh
Copy link
Member

webbnh commented Jul 3, 2023

  1. The axios.get call in the express mirror server needs to either load certificates or disable validation. (The latter seems easier since it's only for local dev mode.)

I can make my peace with disabling the TLS validation, but you should know that you have another option when running out of a Git checkout: the required CA is available from ./server/pbenchinacan/etc/pki/tls/certs/pbench_CA.crt...if it's obvious how to access that file from the Axios context (I know basically nothing about Axios...).

  1. Login requires that the Keycloak be configured with the address of the dashboard code, which in this mode is http://localhost:3000. We need to figure out how to get Keycloak and React on the same page here

[...]

  1. We could hardcode the additional address (in three places) in setting up the Keycloak pbench client (valid redirect, valid post logout, web origins) unconditionally... ugly, and we don't want it for staging
  2. We could add another option to the functional test deployment to add them only for a runlocal
  3. We could just document how to do it manually ...

I'm OK with hardcoding the additional address. You won't see the ugliness once we commit it to code. 😁 Using localhost seems restrictive enough that I'm not worried about it in the general case; nevertheless, I don't think we'll be using the Keycloak Pbench Client for Staging, so I don't think that plays into it.

Adding more options to deployment, on the other hand, is unappealing to me. (And, I think requiring it to be configured manually is a Bad Idea™...that's gonna end up scripted (I hope), and so we might as well bake it in somewhere upstream.)

So, let's just add it to our canned Keycloak.

@dbutenhof
Copy link
Member Author

  1. The axios.get call in the express mirror server needs to either load certificates or disable validation. (The latter seems easier since it's only for local dev mode.)

I can make my peace with disabling the TLS validation, but you should know that you have another option when running out of a Git checkout: the required CA is available from ./server/pbenchinacan/etc/pki/tls/certs/pbench_CA.crt...if it's obvious how to access that file from the Axios context (I know basically nothing about Axios...).

Which is the problem: it's also really (incredibly) poorly documented. But that cliff had a surprisingly crumbly edge, and you pushed me over... and, shockingly, it seems to work.

So, let's just add it to our canned Keycloak.

Sure; you're right, it's really only used for local runs anyway now.

webbnh
webbnh previously approved these changes Jul 3, 2023
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK. (Modulo a couple of nits.)

Comment on lines 21 to 22
KEYCLOAK_REDIRECT_URI=${KEYCLOAK_REDIRECT_URI:-"https://localhost:8443/*"}
KEYCLOAK_DEV_ORIGIN=${KEYCLOAK_DEV_ORIGIN:-"http://localhost:3000"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For KEYCLOAK_DEV_ORIGIN we're adding the wildcard at the reference, while for KEYCLOAK_REDIRECT_URI we're adding it in the definition. Can we treat them both the same way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the things I was trying to clean up when the rebase on your change broke my universe. 😆

Originally I thought I'd have to set the "web origin" separately, but Keycloak derives that from the redirects. So, yeah, I'd already put the /* back up top but I'm having trouble testing it.

Comment on lines 121 to 122
"attributes": {"post.logout.redirect.uris": "'${KEYCLOAK_REDIRECT_URI}'"},
"redirectUris": ["'${KEYCLOAK_REDIRECT_URI}'"]}')
"redirectUris": ["'${KEYCLOAK_REDIRECT_URI}'", "'${KEYCLOAK_DEV_ORIGIN}/*'" ]}')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need/want KEYCLOAK_DEV_ORIGIN in the "post.logout.redirect.uris" list?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in theory, yeah; I deferred it because I couldn't figure out how because despite the plural in the key it doesn't actually take a list. And then I pushed an update before I remembered that. 😦

I believe I've discovered that using "+" actually isn't in this case taken as an undesired "universal" but rather as a deferral to the normal redirectUris list; and as such ought to work.

If only I could test it ...

dbutenhof added 4 commits July 3, 2023 17:41
PBENCH-1203

The shift to HTTPS and Keycloak has broken our dashboard local dev mode hack
in two ways:

1. The `axios.get` call in the express mirror server needs to either load
certificates or disable validation. (The latter seems easier since it's only
for local dev mode.)
2. Login requires that the Keycloak be configured with the address of the
dashboard code, which in this mode is `http://localhost:3000`. We need to
figure out how to get Keycloak and React on the same page here

I'm less sure how to accomplish the second, though.

1. We could hardcode the additional address (in three places) in setting up
the Keycloak pbench client (valid redirect, valid post logout, web origins)
unconditionally... ugly, and we don't want it for staging
2. We could add another option to the functional test deployment to add them
only for a `runlocal`
3. We could just document how to do it manually ...

   NOTE: log in to `https://localhost:8090` with `admin`/`admin`, select the
   `pbench-server` realm, `Clients` in the sidebar, select the `pbench-client`,
   then `+ Add valid redirect URIs` and `+Add valid post logout redirect URIs`
   and on each add `http://localhost:3000/*`, then `+Add web origins` and add
   `http://localhost:3000`, then click `Save` at the bottom.)

I don't really like any of these options much, so feedback is welcome.
Well, blind, but not really faith. Maybe it'll work. Who knows. If not, I'll
hack more on Wednesday...

This is trying to add the multiple logout redirects...
@dbutenhof dbutenhof changed the title Simple first step to re-enable the dev dashboard Re-enable the dev dashboard Jul 3, 2023
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good!

@dbutenhof dbutenhof merged commit 75d4d91 into distributed-system-analysis:main Jul 3, 2023
@dbutenhof dbutenhof deleted the devhttps branch July 3, 2023 22:37
@dbutenhof
Copy link
Member Author

Merging with one approval to get Varshini unblocked!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dashboard Of and relating to the Dashboard GUI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants