Skip to content

fix: add support for dynamic config url#1565

Merged
wtrocki merged 10 commits intomainfrom
dynamic-sso-config
May 17, 2022
Merged

fix: add support for dynamic config url#1565
wtrocki merged 10 commits intomainfrom
dynamic-sso-config

Conversation

@wtrocki
Copy link
Collaborator

@wtrocki wtrocki commented May 12, 2022

We are migrating to the single auth provider (removing infamous mas-sso). This PR targets to enable us to do it.

This PR:

  • Adds ability to detect if mas-sso should be used for authentication
  • Adds ability to use dynamic tokenURL

What this does

  • We are using dynamic url for tokenUrl from Kas-API. Previously that API were hardcoded so it is huge change.
  • temp.go is very ugly hack to enable us have dynamic switch once migration happens. What this does is call our api which is unprotected to know if mas-sso is used. If not it uses the sso.redhat.com token.

Assumptions

Both are based on following assumptions:

  • MAS.SSO will be running after migration for some time as CLI will still login to mas-sso (I can use the same flag to disable login if needed).
  • During migration url on that endpoint will be changed to sso.redhat.com
  • The same client and token can be used for both data plane and control plane after migration

@wtrocki wtrocki marked this pull request as draft May 12, 2022 16:26
@wtrocki
Copy link
Collaborator Author

wtrocki commented May 13, 2022

@akoserwal @Rajagopalan-Ranganathan I think this PR is going to solve all issues we would have and it can be merged instantly once we get ssoProviders api public into production.

@wtrocki
Copy link
Collaborator Author

wtrocki commented May 13, 2022

I actually added ability to do conditional login considering that users do not update CLI that often as we can update UI

@wtrocki wtrocki requested a review from rkpattnaik780 May 13, 2022 15:39
@wtrocki
Copy link
Collaborator Author

wtrocki commented May 13, 2022

I have verified this change with mock to test all cases.

@wtrocki wtrocki marked this pull request as ready for review May 13, 2022 15:40
@machi1990
Copy link
Contributor

@akoserwal @Rajagopalan-Ranganathan I think this PR is going to solve all issues we would have and it can be merged instantly once we get ssoProviders api public into production.

The sso provider api is there in prod already: https://api.openshift.com/?urls.primaryName=kafka%20service%20fleet%20manager%20service#/security/getSsoProviders

return true
}

return provider.GetBaseUrl() == "https://identity.api.redhat.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the https://identity.api.redhat.com the same for both environments?
@wtrocki @akoserwal

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think the sso url will be something like https://sso.redhat.com... and not https://identity.api.redhat.com. @akoserwal is this correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Amazing question. I do not know.

Copy link
Collaborator Author

@wtrocki wtrocki May 13, 2022

Choose a reason for hiding this comment

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

I would love to have some flag on that API.
Like useLegacyAuth: true.
While it might look like pouting API, but it would in fact make things
That will be much easier for us to pick up dynamic behaviour rather than assert urls

My ideas for that flag:

  useLegacyAuth: true
  usePluralAuth: true
  multiSSOLogin: true
  customSSO: true

Choose a reason for hiding this comment

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

SsoProvider: MAS SSO/RED Hat SSO (Enum) could it be used?

Copy link
Contributor

Choose a reason for hiding this comment

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

For this change I think it is fine because the values are not going to change e.g a rename or a removal of one etc
Extending an enum is fine and should never ever be a breaking change IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a bool flag-like useLegacyAuth: true would be confusing to other clients that are consuming this end-point. I am planning to add a name: mas_sso or name: redhat_sso.

@wtrocki @Rajagopalan-Ranganathan @carlesarnal @machi1990 wdty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not have strong opinion. I need value for if statement that we can rely on in CLI and UI and this proposal meets it.

Choose a reason for hiding this comment

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

About the name makes sense. I don't have a strong opinion about the flag, but I would like to use whatever value is returned by the sso_providers call.

Choose a reason for hiding this comment

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

the name is absolutely fine and looks to be extensible in the future.

@wtrocki
Copy link
Collaborator Author

wtrocki commented May 13, 2022

I realized that we need support for calling stage API as well - Current PR does call production only

@wtrocki
Copy link
Collaborator Author

wtrocki commented May 13, 2022

Ideally it will be good to test migration 3-4 times on stage before we go to prod with already released UI and CLI that can switch dynamically

@wtrocki
Copy link
Collaborator Author

wtrocki commented May 13, 2022

Fixed problem with not using stage/prod env. Looking to test this with real API :D

@rkpattnaik780
Copy link
Contributor

LGTM!

@wtrocki wtrocki force-pushed the dynamic-sso-config branch from af096a4 to cce629f Compare May 17, 2022 15:12
@wtrocki wtrocki merged commit 9b50176 into main May 17, 2022
@wtrocki wtrocki deleted the dynamic-sso-config branch May 17, 2022 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants