Skip to content
This repository was archived by the owner on Jun 29, 2022. It is now read-only.

Rename Headlamp to Web UI and add OIDC support#1100

Merged
iaguis merged 5 commits intomasterfrom
iaguis/update-headlamp
Oct 22, 2020
Merged

Rename Headlamp to Web UI and add OIDC support#1100
iaguis merged 5 commits intomasterfrom
iaguis/update-headlamp

Conversation

@iaguis
Copy link
Contributor

@iaguis iaguis commented Oct 19, 2020

This PR renames the Headlamp component to Web UI as discussed OOB. The reasoning is that this component is the main UI for Lokomotive clusters and we'll probably add more functionality than what is in upstream Headlamp.

It also switches to a released version of Headlamp (v0.1.1) and exposes OIDC authentication.

@iaguis iaguis requested review from invidian, ipochi and knrt10 October 19, 2020 16:36
@iaguis iaguis force-pushed the iaguis/update-headlamp branch from 922a108 to d1608f4 Compare October 19, 2020 17:00
ipochi
ipochi previously requested changes Oct 20, 2020
View reviewed changes
Copy link
Member

@ipochi ipochi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have two main questions regarding this PR:

  1. If Dex and Gangway are already installed in an existing cluster and then I install web-ui, how will this change affect the existing setup.
  2. On a fresh cluster, if the installation is of the order : dex, web-ui and then gangway, does it ensure that nothing is broken for Gangway ?

knrt10
knrt10 suggested changes Oct 20, 2020
View reviewed changes
knrt10
knrt10 reviewed Oct 20, 2020
View reviewed changes
knrt10
knrt10 reviewed Oct 20, 2020
View reviewed changes
invidian
invidian reviewed Oct 20, 2020
View reviewed changes
@iaguis
Copy link
Contributor Author

iaguis commented Oct 20, 2020

  • If Dex and Gangway are already installed in an existing cluster and then I install web-ui, how will this change affect the existing setup.

If OIDC is disabled it won't do anything.

If OIDC is enabled you'd need to add the Web UI callback URL to the static_client.redirect_uris attribute of the dex component like mentioned in the docs.

  • On a fresh cluster, if the installation is of the order : dex, web-ui and then gangway, does it ensure that nothing is broken for Gangway ?

Yes, that works.

@iaguis iaguis force-pushed the iaguis/update-headlamp branch from d1608f4 to 15abe7b Compare October 20, 2020 18:22
@iaguis
Copy link
Contributor Author

iaguis commented Oct 20, 2020 •
edited by knrt10
Loading

Pushed an update.

Originally I missed changing the default cluster.oidc { cluster_id } from gangway to clusterauth so this push fixes that. This means we need to add something like this to the release notes:

This release changes the default cluster.oidc.client_id parameter from gangway to clusterauth. This setting must match gangway.client_id and dex.static_client.client_id. If you were default settings for oidc you'll need to add client_id = "gangway" or change the client_id parameters to on dex and gangway to clusterauth.

@iaguis iaguis force-pushed the iaguis/update-headlamp branch from 15abe7b to 2e06dd9 Compare October 20, 2020 18:42
knrt10
knrt10 suggested changes Oct 21, 2020
View reviewed changes
invidian
invidian reviewed Oct 21, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

LGTM except the @knrt10 comment 👍

invidian
invidian reviewed Oct 21, 2020
View reviewed changes
@iaguis iaguis force-pushed the iaguis/update-headlamp branch from 2e06dd9 to a2e1fa0 Compare October 21, 2020 12:44
invidian
invidian previously approved these changes Oct 21, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

LGTM

@knrt10
Copy link
Contributor

knrt10 commented Oct 21, 2020

@iaguis linter and build is failing. Can you please check

iaguis added 4 commits October 21, 2020 14:53
@iaguis
*: rename Headlamp component to Web UI
055081d 
The reasoning is that this component is the main UI for Lokomotive
clusters and we'll probably add more functionality than what is in
upstream Headlamp.
@iaguis
web-ui: switch to release v0.1.1
a2171a5
@iaguis
pkg/web-ui: add OIDC authentication support
de4fe65 
Headlamp now supports OIDC authentication. Let's expose the OIDC
configuration so Lokomotive users can use it.
@iaguis
docs/dex,gangway: rename static_client vars
4b015f8 
Now that we have OIDC authentication for the Web UI, this static client
is not Gangway specific anymore so it doesn't make sense using gangway
in the variable names or examples.

Pending work: rename the CI variables too.
@iaguis iaguis force-pushed the iaguis/update-headlamp branch from a2e1fa0 to e600468 Compare October 21, 2020 12:54
@iaguis iaguis force-pushed the iaguis/update-headlamp branch from e600468 to e75c8a9 Compare October 22, 2020 08:02
@iaguis iaguis requested review from invidian, ipochi and knrt10 October 22, 2020 09:33
knrt10
knrt10 approved these changes Oct 22, 2020
View reviewed changes
Copy link
Contributor

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

Looks great. Nice work. +1

@iaguis iaguis dismissed ipochi’s stale review October 22, 2020 10:07

Addressed comment.

@iaguis
Copy link
Contributor Author

iaguis commented Oct 22, 2020

Taking @invidian's LGTM from before and @knrt10's I consider this ready to merge.

@iaguis iaguis merged commit 6bd08f6 into master Oct 22, 2020
@iaguis iaguis deleted the iaguis/update-headlamp branch October 22, 2020 10:07
@iaguis iaguis mentioned this pull request Oct 26, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@invidian invidian Awaiting requested review from invidian

@ipochi ipochi Awaiting requested review from ipochi

1 more reviewer

@knrt10 knrt10 knrt10 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@iaguis @knrt10 @ipochi @invidian