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

Tenants will work only with embedded console #751

Merged
merged 7 commits into from
Aug 9, 2021

Conversation

dvaldivia
Copy link
Collaborator

@dvaldivia dvaldivia commented Aug 5, 2021

This PR builds on top of @nitisht PR but has some extra changes, such as:

  • Complete removal of the Console Configuration .spec.console from the tenant
  • kubectl minio tenant create now creates a single user for the tenant
  • Fixed a problem that was preventing the old console deployment from being deleted
  • fixed a problem that was preventing the hot server update to work

To Test this make sure to build a new logsearch container image and use that either as the default image or in the tenant's .spec.log.image field

nitisht and others added 3 commits August 5, 2021 09:58
Existing console fields are now deprecated and will be removed
moving forward.

Also upgrade to latest MinIO release.
Signed-off-by: Daniel Valdivia <18384552+dvaldivia@users.noreply.github.com>
Signed-off-by: Daniel Valdivia <18384552+dvaldivia@users.noreply.github.com>
Signed-off-by: Daniel Valdivia <18384552+dvaldivia@users.noreply.github.com>
Signed-off-by: Daniel Valdivia <18384552+dvaldivia@users.noreply.github.com>
@ravindk89
Copy link
Contributor

@dvaldivia minor question - for setting up SSO via Console, I think we'd expect to have to also set MINIO_BROWSER_REDIRECT_URL right? Would that be handled in this PR, or would the user be responsible for altering the envvars in the tenant to include that redirect based on the external ingress address?

@harshavardhana
Copy link
Member

@dvaldivia minor question - for setting up SSO via Console, I think we'd expect to have to also set MINIO_BROWSER_REDIRECT_URL right? Would that be handled in this PR, or would the user be responsible for altering the envvars in the tenant to include that redirect based on the external ingress address?

everything should be handle in this PR - AFAICS @ravindk89

@ravindk89
Copy link
Contributor

@dvaldivia minor question - for setting up SSO via Console, I think we'd expect to have to also set MINIO_BROWSER_REDIRECT_URL right? Would that be handled in this PR, or would the user be responsible for altering the envvars in the tenant to include that redirect based on the external ingress address?

everything should be handle in this PR - AFAICS @ravindk89

I'll take your word for it, I didn't see the var being set at a glance, but I also don't know all the places to look. I did see we're setting MINIO_SERVER_URL so that's good.

Signed-off-by: Daniel Valdivia <18384552+dvaldivia@users.noreply.github.com>
Signed-off-by: Daniel Valdivia <18384552+dvaldivia@users.noreply.github.com>
Alevsk
Alevsk previously requested changes Aug 5, 2021
Copy link
Contributor

@Alevsk Alevsk left a comment

Choose a reason for hiding this comment

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

I need to test this branch in detail, deploying tenants with oidc, ldap, encryption options, etc, hows is gonna work with console/operator-ui? we will need a new version correct?, ill test all this tonight and tomorrow

@dvaldivia
Copy link
Collaborator Author

@ravindk89 @harshavardhana we could have the OIDC callback be set by the browser sine we start the request from there, this would ensure we always can infer the domain regardless of the configuration

@harshavardhana
Copy link
Member

@ravindk89 @harshavardhana we could have the OIDC callback be set by the browser sine we start the request from there, this would ensure we always can infer the domain regardless of the configuration

If it's possible that would be great.

@ravindk89
Copy link
Contributor

It would certainly simplify a recent pain point if that were the case.

Copy link
Contributor

@nitisht nitisht left a comment

Choose a reason for hiding this comment

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

Tried updating the Operator image. Here are the steps:

  1. Create Operator v4.1.3 and Tenant with Console enabled.
  2. Upgrade Operator to image based on this PR.

Issues:

  1. Old console pods are not getting deleted/removed.
  2. Getting this warning in the Service.
 Warning  FailedToUpdateEndpointSlices  9m11s  endpoint-slice-controller  Error updating Endpoint Slices for Service default/minio-console: failed to update minio-console-l9vrs EndpointSlice for Service default/minio-console: Operation cannot be fulfilled on endpointslices.discovery.k8s.io "minio-console-l9vrs": the object has been modified; please apply your changes to the latest version and try again

@dvaldivia
Copy link
Collaborator Author

@nitisht you need to perform a MinIO update to get rid of the console pods

@nitisht
Copy link
Contributor

nitisht commented Aug 6, 2021

@nitisht you need to perform a MinIO update to get rid of the console pods

ah right. Let me try that

Copy link
Contributor

@nitisht nitisht left a comment

Choose a reason for hiding this comment

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

LGTM & basic migration scenarios working fine

@harshavardhana harshavardhana requested a review from Alevsk August 9, 2021 07:31
@dvaldivia dvaldivia merged commit 2981556 into minio:master Aug 9, 2021
@dvaldivia dvaldivia deleted the unify-console branch August 9, 2021 07: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.

5 participants