-
Notifications
You must be signed in to change notification settings - Fork 2
Add OIDC to bootstrap CLI #3399
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if feels that we are closer here. I have added some more comments on the code.
I would like to test it to understand the user experience. However, i could not find indications in the PR on how to do that (please see here https://github.com/weaveworks/weave-gitops-enterprise/blob/main/.github/pull_request_template.md?plain=1#L36)
I could not see higher level testing for the scenario of bootstrapping wge with oidc as flow nor as a single. please let me know your expectations here.
Regarding testing: please, let me know how should i verify that it works and the strategy for having automated testing for both bootstrap
command and bootstrap auth
. i could not find them in the PR.
cmd/gitops/app/bootstrap/cmd.go
Outdated
WithVersion(flags.version). | ||
WithDomainType(flags.domainType). | ||
WithDomain(flags.domain). | ||
WithPrivateKey(flags.privateKeyPath, flags.privateKeyPassword). | ||
WithOIDCConfig(steps.AuthOIDC, flags.discoveryURL, flags.clientID, flags.clientSecret, true). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that you are configuring oidc, i think that you dont need passing steps.AuthOIDC,
cmd/gitops/app/bootstrap/cmd.go
Outdated
WithVersion(flags.version). | ||
WithDomainType(flags.domainType). | ||
WithDomain(flags.domain). | ||
WithPrivateKey(flags.privateKeyPath, flags.privateKeyPassword). | ||
WithOIDCConfig(steps.AuthOIDC, flags.discoveryURL, flags.clientID, flags.clientSecret, true). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that you are passing flags.discoveryURL
let me know why PromptedForDiscoveryURL
is required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PromptedForDiscoveryURL
is to differentiate for the step method where it's called, from the separate command or from within the setup flow. it's responsibe for asking Do you want to setup OIDC to access Weave GitOps Dashboard
pkg/bootstrap/steps/oidc.go
Outdated
if err != nil { | ||
return []StepOutput{}, err | ||
} | ||
c.Logger.Actionf("retrieved issure url: %s", issuerUrl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.Logger.Actionf("retrieved issure url: %s", issuerUrl) | |
c.Logger.Actionf("retrieved issuer url: %s", issuerUrl) |
c.IssuerURL = issuerUrl | ||
|
||
if c.DomainType == domainTypeLocalhost { | ||
c.RedirectURL = "http://localhost:8000/oauth2/callback" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would happen if we are exposing SSL server on 8000 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the localhost installation is only HTTP, otherwise it'll require setting up local certificate and a lot of things could go wrong here
if c.DomainType == domainTypeLocalhost { | ||
c.RedirectURL = "http://localhost:8000/oauth2/callback" | ||
} else { | ||
c.RedirectURL = fmt.Sprintf("https://%s/oauth2/callback", c.UserDomain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about if the user exposes non https domain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for majority of users exposing https is a must ?, what do you think ?
return []StepOutput{}, err | ||
} | ||
c.Logger.Actionf("rendered HelmRelease file") | ||
c.Logger.Successf(oidcConfirmationMsg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.Logger.Successf(oidcConfirmationMsg)
given this message if I was a user, I would understand that oidc is ready to use at this stage but actually will be after the reconciliation happens, correct? if that is the case, we could revisit this msg
cmd/gitops/app/bootstrap/cmd.go
Outdated
WithVersion(flags.version). | ||
WithDomainType(flags.domainType). | ||
WithDomain(flags.domain). | ||
WithPrivateKey(flags.privateKeyPath, flags.privateKeyPassword). | ||
WithOIDCConfig(steps.AuthOIDC, flags.discoveryURL, flags.clientID, flags.clientSecret, true). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OIDC can be tested by using the following parameters:-
bootstrap --discovery-url https://dex-01.wge.dev.weave.works/.well-known/openid-configu0ration --client-id weave-gitops-enterprise
- These configs are configured on WW dex setup here
- We have added two redirect URLs under weave-gitops-enterprise static-client, one to be used if we choose the domain type to be local here and another if external DNS has been chosen here, so if you need to test with a different external DNS you need to add it under one of the static-clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding this to the PR description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed client-secret from the description to do not leak it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acceptance scenarios based on the configuration
scenarios: happy path without previous
- As user without existing oidc, I could setup odic with valid arguments via interactive session
- As user without existing oidc, I could setup odic with valid arguments via non-interactive flags
scenarios: happy path with previous oidc configuration
- As user with existing oidc, I could overwrite odic with valid arguments via interactive session
- As user with existing oidc, I could overwrite odic with valid arguments via non-interactive session
scenarios: happy path with previous oidc configuration
- As user with existing oidc, I could reuse odic via interactive session
- As user with existing oidc, I could reuse odic via non-interactive session
scenarios: unhappy path
- As user with existing oidc, I could not setup odic with invalid arguments via interactive session
- As user with existing oidc, I could not setup odic with invalid arguments via non-interactive session
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing Scenario:
As user without existing oidc, I could setup odic with valid arguments via non-interactive flags
Run with the following args
cmd_acceptance_test.go:152: "level"=0 "msg"="execute cmd" "args"=["bootstrap","--kubeconfig=/var/folders/9b/bkrspzws5xgd7x_ldtc880pr0000gn/T/kubeconfig1556281884","--version=0.33.0","--private-key=/Users/enekofb/.ssh/id_ed25519","--private-key-password=""","--username=admin","--password=admin123","--domain-type=localhost","--discovery-url=https://dex-01.wge.dev.weave.works/.well-known/openid-configu0ration --client-id=weave-gitops-enterprise --client-secret="]
and got the following output
◎ reconciling changes
✔ changes are reconciled successfully!
◎ OIDC Configuration
? Do you want to setup OIDC to access Weave GitOps Dashboards? [y/N] █
✗ Please enter OIDC clientID: █
cmd_acceptance_test.go:159:
Expected
<*errors.errorString | 0x14000c63510>:
cannot execute bootstrap: cannot process input 'OIDC Configuration': ^D
{
s: "cannot execute bootstrap: cannot process input 'OIDC Configuration': ^D",
}
to be nil
cmd_acceptance_test.go:260: "level"=0 "msg"="deleted object" "name"="weave-gitops-enterprise-credentials" "ns"="flux-system" "kind"="Secret"
cmd_acceptance_test.go:260: "level"=0 "msg"="deleted object" "name"="cluster-user-auth" "ns"="flux-system" "kind"="Secret"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing Scenario: As user without existing oidc, I could setup odic with valid arguments via interactive session
◎ reconciling changes
✔ changes are reconciled successfully!
◎ OIDC Configuration
Do you want to setup OIDC to access Weave GitOps Dashboards: y
Please enter OIDC clientSecret: ******************************
► retrieved issuer url: https://dex-01.wge.dev.weave.works
► setting redirect url: http://localhost:8000/oauth2/callback
◎ Configuring OIDC
► configuring oidc values
► rendered HelmRelease file
✔ OIDC has been configured successfully! It will be ready to use after reconcillation
► updating HelmRelease file
► creating secret: flux-system/oidc-auth
✔ created secret flux-system/oidc-auth
► write file to repo: wge-hrelease.yaml
► cloning flux git repo: flux-system/flux-system
✔ cloned flux git repo: flux-system/flux-system
✔ file committed to repo: wge-hrelease.yaml
◎ reconciling changes
✔ changes are reconciled successfully!
◎ preparing dashboard domain
✔ WGE v0.33.0 is installed successfully. To access the dashboard, run the following command to create portforward to the dasboard local domain http://localhost:8000
kubectl -n flux-system port-forward svc/clusters-service 8000:8000
after that i could login:

but could not see anything:

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scenarios: happy path with previous oidc configuration
As user with existing oidc, I could overwrite odic with valid arguments via interactive session
does not finish with both yes
◎ reconciling changes
✔ changes are reconciled successfully!
◎ OIDC Configuration
⚠️ OIDC is already configured on the cluster. To reset configurations please remove secret 'oidc-auth' in namespace 'flux-system' and run 'bootstrap auth --type=oidc' command again
Do you want to continue using existing OIDC configurations: y
Do you want to setup OIDC to access Weave GitOps Dashboards: y
◎ preparing dashboard domain
does not finish with yes/no-selection
◎ reconciling changes
✔ changes are reconciled successfully!
◎ OIDC Configuration
⚠️ OIDC is already configured on the cluster. To reset configurations please remove secret 'oidc-auth' in namespace 'flux-system' and run 'bootstrap auth --type=oidc' command again
Do you want to continue using existing OIDC configurations: y
? Do you want to setup OIDC to access Weave GitOps Dashboards? [y/N] █
◎ preparing dashboard domain
it finishes with yes and no
✔ file committed to repo: wge-hrelease.yaml
◎ reconciling changes
✔ changes are reconciled successfully!
◎ OIDC Configuration
⚠️ OIDC is already configured on the cluster. To reset configurations please remove secret 'oidc-auth' in namespace 'flux-system' and run 'bootstrap auth --type=oidc' command again
Do you want to continue using existing OIDC configurations: y
✗ Do you want to setup OIDC to access Weave GitOps Dashboards:
◎ preparing dashboard domain
✔ WGE v0.33.0 is installed successfully. To access the dashboard, run the following command to create portforward to the dasboard local domain http://localhost:8000
kubectl -n flux-system port-forward svc/clusters-service 8000:8000
➜
As user with existing oidc, I could overwrite odic with valid arguments via non-interactive session
we cannot
✔ selected version 0.33.0
◎ user authentication
⚠️ admin login credentials already exist on the cluster. To reset admin credentials please remove secret 'cluster-user-auth' in namespace 'flux-system', then try again
do you want to continue using existing credentials: y
◎ dashboard access
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested the journeys here: lets discuss it with our session
to add:
- support for oidc with acceptance test started here
to follow up:
- rbac for oidc (see comment)
- story for existing resources for non-interactive user https://weaveworks.slack.com/archives/C05HC9R4DUG/p1698222391488089
|
||
func OIDCConfigStep(config Config) BootstrapStep { | ||
|
||
inputs := []StepInput{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is the case
in case of non-interactive. he should see that anyway
it breaks the contract, a non-interactive user wont be able to use it.
cmd/gitops/app/bootstrap/cmd.go
Outdated
WithVersion(flags.version). | ||
WithDomainType(flags.domainType). | ||
WithDomain(flags.domain). | ||
WithPrivateKey(flags.privateKeyPath, flags.privateKeyPassword). | ||
WithOIDCConfig(steps.AuthOIDC, flags.discoveryURL, flags.clientID, flags.clientSecret, true). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acceptance scenarios based on the configuration
scenarios: happy path without previous
- As user without existing oidc, I could setup odic with valid arguments via interactive session
- As user without existing oidc, I could setup odic with valid arguments via non-interactive flags
scenarios: happy path with previous oidc configuration
- As user with existing oidc, I could overwrite odic with valid arguments via interactive session
- As user with existing oidc, I could overwrite odic with valid arguments via non-interactive session
scenarios: happy path with previous oidc configuration
- As user with existing oidc, I could reuse odic via interactive session
- As user with existing oidc, I could reuse odic via non-interactive session
scenarios: unhappy path
- As user with existing oidc, I could not setup odic with invalid arguments via interactive session
- As user with existing oidc, I could not setup odic with invalid arguments via non-interactive session
cmd/gitops/app/bootstrap/cmd.go
Outdated
WithVersion(flags.version). | ||
WithDomainType(flags.domainType). | ||
WithDomain(flags.domain). | ||
WithPrivateKey(flags.privateKeyPath, flags.privateKeyPassword). | ||
WithOIDCConfig(steps.AuthOIDC, flags.discoveryURL, flags.clientID, flags.clientSecret, true). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing Scenario:
As user without existing oidc, I could setup odic with valid arguments via non-interactive flags
Run with the following args
cmd_acceptance_test.go:152: "level"=0 "msg"="execute cmd" "args"=["bootstrap","--kubeconfig=/var/folders/9b/bkrspzws5xgd7x_ldtc880pr0000gn/T/kubeconfig1556281884","--version=0.33.0","--private-key=/Users/enekofb/.ssh/id_ed25519","--private-key-password=""","--username=admin","--password=admin123","--domain-type=localhost","--discovery-url=https://dex-01.wge.dev.weave.works/.well-known/openid-configu0ration --client-id=weave-gitops-enterprise --client-secret="]
and got the following output
◎ reconciling changes
✔ changes are reconciled successfully!
◎ OIDC Configuration
? Do you want to setup OIDC to access Weave GitOps Dashboards? [y/N] █
✗ Please enter OIDC clientID: █
cmd_acceptance_test.go:159:
Expected
<*errors.errorString | 0x14000c63510>:
cannot execute bootstrap: cannot process input 'OIDC Configuration': ^D
{
s: "cannot execute bootstrap: cannot process input 'OIDC Configuration': ^D",
}
to be nil
cmd_acceptance_test.go:260: "level"=0 "msg"="deleted object" "name"="weave-gitops-enterprise-credentials" "ns"="flux-system" "kind"="Secret"
cmd_acceptance_test.go:260: "level"=0 "msg"="deleted object" "name"="cluster-user-auth" "ns"="flux-system" "kind"="Secret"
cmd/gitops/app/bootstrap/cmd.go
Outdated
WithVersion(flags.version). | ||
WithDomainType(flags.domainType). | ||
WithDomain(flags.domain). | ||
WithPrivateKey(flags.privateKeyPath, flags.privateKeyPassword). | ||
WithOIDCConfig(steps.AuthOIDC, flags.discoveryURL, flags.clientID, flags.clientSecret, true). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing Scenario: As user without existing oidc, I could setup odic with valid arguments via interactive session
◎ reconciling changes
✔ changes are reconciled successfully!
◎ OIDC Configuration
Do you want to setup OIDC to access Weave GitOps Dashboards: y
Please enter OIDC clientSecret: ******************************
► retrieved issuer url: https://dex-01.wge.dev.weave.works
► setting redirect url: http://localhost:8000/oauth2/callback
◎ Configuring OIDC
► configuring oidc values
► rendered HelmRelease file
✔ OIDC has been configured successfully! It will be ready to use after reconcillation
► updating HelmRelease file
► creating secret: flux-system/oidc-auth
✔ created secret flux-system/oidc-auth
► write file to repo: wge-hrelease.yaml
► cloning flux git repo: flux-system/flux-system
✔ cloned flux git repo: flux-system/flux-system
✔ file committed to repo: wge-hrelease.yaml
◎ reconciling changes
✔ changes are reconciled successfully!
◎ preparing dashboard domain
✔ WGE v0.33.0 is installed successfully. To access the dashboard, run the following command to create portforward to the dasboard local domain http://localhost:8000
kubectl -n flux-system port-forward svc/clusters-service 8000:8000
after that i could login:

but could not see anything:

cmd/gitops/app/bootstrap/cmd.go
Outdated
WithVersion(flags.version). | ||
WithDomainType(flags.domainType). | ||
WithDomain(flags.domain). | ||
WithPrivateKey(flags.privateKeyPath, flags.privateKeyPassword). | ||
WithOIDCConfig(steps.AuthOIDC, flags.discoveryURL, flags.clientID, flags.clientSecret, true). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scenarios: happy path with previous oidc configuration
As user with existing oidc, I could overwrite odic with valid arguments via interactive session
does not finish with both yes
◎ reconciling changes
✔ changes are reconciled successfully!
◎ OIDC Configuration
⚠️ OIDC is already configured on the cluster. To reset configurations please remove secret 'oidc-auth' in namespace 'flux-system' and run 'bootstrap auth --type=oidc' command again
Do you want to continue using existing OIDC configurations: y
Do you want to setup OIDC to access Weave GitOps Dashboards: y
◎ preparing dashboard domain
does not finish with yes/no-selection
◎ reconciling changes
✔ changes are reconciled successfully!
◎ OIDC Configuration
⚠️ OIDC is already configured on the cluster. To reset configurations please remove secret 'oidc-auth' in namespace 'flux-system' and run 'bootstrap auth --type=oidc' command again
Do you want to continue using existing OIDC configurations: y
? Do you want to setup OIDC to access Weave GitOps Dashboards? [y/N] █
◎ preparing dashboard domain
it finishes with yes and no
✔ file committed to repo: wge-hrelease.yaml
◎ reconciling changes
✔ changes are reconciled successfully!
◎ OIDC Configuration
⚠️ OIDC is already configured on the cluster. To reset configurations please remove secret 'oidc-auth' in namespace 'flux-system' and run 'bootstrap auth --type=oidc' command again
Do you want to continue using existing OIDC configurations: y
✗ Do you want to setup OIDC to access Weave GitOps Dashboards:
◎ preparing dashboard domain
✔ WGE v0.33.0 is installed successfully. To access the dashboard, run the following command to create portforward to the dasboard local domain http://localhost:8000
kubectl -n flux-system port-forward svc/clusters-service 8000:8000
➜
As user with existing oidc, I could overwrite odic with valid arguments via non-interactive session
we cannot
✔ selected version 0.33.0
◎ user authentication
⚠️ admin login credentials already exist on the cluster. To reset admin credentials please remove secret 'cluster-user-auth' in namespace 'flux-system', then try again
do you want to continue using existing credentials: y
◎ dashboard access
fixed the OIDC prompt |
* added oidc scenario * can run acceptance on oidc
3a263c2
to
5088f08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Followups
- https://github.com/weaveworks/weave-gitops-enterprise/pull/3399/files#r1368358114
- user documentation
- testing withOidcConfig
- test for when we are not running localhost:8000 but localhost:9000
- acceptance scenarios for non-happy path #3399 (comment)
- extract domain logic out of oidc step https://github.com/weaveworks/weave-gitops-enterprise/pull/3399/files#r1369984602
- single step for oidc, we have now two
@@ -97,6 +112,18 @@ func (c *ConfigBuilder) WithPrivateKey(privateKeyPath string, privateKeyPassword | |||
return c | |||
} | |||
|
|||
func (c *ConfigBuilder) WithOIDCConfig(discoveryURL string, clientID string, clientSecret string, prompted bool) *ConfigBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
followup: add testing for this logic as we setup expectations that if arguments are passed means that we want to install oidc which is an implicit contract
@@ -97,6 +112,18 @@ func (c *ConfigBuilder) WithPrivateKey(privateKeyPath string, privateKeyPassword | |||
return c | |||
} | |||
|
|||
func (c *ConfigBuilder) WithOIDCConfig(discoveryURL string, clientID string, clientSecret string, prompted bool) *ConfigBuilder { | |||
c.authType = AuthOIDC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
followup: add this as argument
Closes #3275
What changed?
Why was this change made?
How was this change implemented?
How did you validate the change?
How to test: #3399 (comment)
Release notes
Documentation Changes
Other follow ups