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

The new plan still can be shown and started on plans page when plan creation is failed #405

Open
qiyuann opened this issue Feb 7, 2023 · 13 comments
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@qiyuann
Copy link

qiyuann commented Feb 7, 2023

When creating a new plan is failed because of following error:
Screenshot from 2023-02-07 11-49-30

The new plan still can be shown on the plans page, migration can be started, VM could be imported successfully.

If creating a new plan is failed, the plan shouldn't be shown on the plans page.

Steps to reproduce:

  1. Create a non cluster admin user

  2. Create a role with following rules:
    Screenshot from 2023-02-07 12-03-28
    Note that there is no 'delete' verb on networkmaps

  3. Bind the user with the role

  4. Log into cluster with the user, and create a new plan

@qiyuann qiyuann changed the title The new plan still can be shown and started on plans page when plan creation failed The new plan still can be shown and started on plans page when plan creation is failed Feb 7, 2023
@yaacov yaacov added the kind/bug Categorizes issue or PR as related to a bug. label Feb 7, 2023
@yaacov
Copy link
Member

yaacov commented Feb 7, 2023

@rszwajko it looks like it's connected to the flow of creating a copy of the network mappings,

@qiyuann is this fixed by giving the user all permissions on networkmaps ?

@qiyuann
Copy link
Author

qiyuann commented Feb 7, 2023

yes, it could be fixed by giving the user all permissions on networkmaps.

@ahadas
Copy link
Member

ahadas commented Feb 7, 2023

@yaacov shall we move it to kubev2v/forklift then?

@yaacov
Copy link
Member

yaacov commented Feb 7, 2023

yes, it could be fixed by giving the user all permissions on networkmaps.

it looks like the UI trying to copy the mapping

a. moving target to UI 2.4.1
b. @ahadas thanks, yes, kubev2v/forklift should give the permissions so the UI will work in 2.4.0

cc:// @rszwajko @sjd78 @sgratch

@ahadas
Copy link
Member

ahadas commented Feb 7, 2023

@yaacov I lost you - I meant to move this issue to kubev2v/forklift so the plan would be set with some condition that prevents it from starting when the mappings are missing
but if you think it should be addressed in the UI - can't we create the mappings before creating the plan (as a workaround)?

@yaacov
Copy link
Member

yaacov commented Feb 7, 2023

can't we create the mappings before creating the plan

@ahadas . in this test, the user can't create mappings.
even if you try to create use an existing mapping, since the UI try to copy it, the UI will not be able to create a new copy of the mappings and the wizard will show an error.

@yaacov yaacov self-assigned this Feb 28, 2023
@yaacov
Copy link
Member

yaacov commented Jul 17, 2023

yes, it could be fixed by giving the user all permissions on networkmaps.

The workaround is to grant "All" permissions to networkMaps

moving to next because of capacity and easyworkaround.

@ahadas what is the default permissions to networkMaps , i see that for storageMap it's 'All' ?

cc:// @qiyuann (for moving to next)

@ahadas
Copy link
Member

ahadas commented Jul 17, 2023

@ahadas what is the default permissions to networkMaps , i see that for storageMap it's 'All' ?

it depends on the entity - for forklift-controller it's * (all), for networkmaps.forklift.konveyor.io-v1beta1-admin it's also *, for networkmaps.forklift.konveyor.io-v1beta1-crdview it's get, for networkmaps.forklift.konveyor.io-v1beta1-edit it's create, update, patch, delete, and for networkmaps.forklift.konveyor.io-v1beta1-viewit'sget, list, watch`.

we have a task to document the required permissions for non-admin users - that's ok to document that we currently require permissions to delete the networkmap in order to create a plan (if that's the current state)

@yaacov
Copy link
Member

yaacov commented Jul 30, 2023

closing, this will be fixed on the operator / docs side, by documenting the requirements and adjusting the operator to create the needed permissions

@yaacov yaacov closed this as completed Jul 30, 2023
@ahadas ahadas reopened this Jul 30, 2023
@ahadas ahadas transferred this issue from kubev2v/forklift-console-plugin Jul 30, 2023
@RichardHoch
Copy link
Collaborator

@ahadas @yaacov Are we saying that non-admins can do everything except delete migration plans? If so, that's easy to document. Even not being able to delete any entity is easy to document.
Documenting exactly which permissions non-admins have is less easy, especially if we expect to add new permissions, which will have to be added. Let me know what's needed.

@yaacov
Copy link
Member

yaacov commented Jul 31, 2023

note:
submitted upstream: kubev2v/forklift-console-plugin#634
patch 634 prevents non admins from accessing the "overview/settings" page (kubev2v/forklift-console-plugin#630)

note I:
the console defined admin as users that can list namespaces

note II:
kubev2v/forklift-console-plugin#631 - an open bug to hide the create/edit/delete options from users that don't have permissions to create/edit/delete providers/plans and mappings

@RichardHoch
Copy link
Collaborator

@ahadas @yaacov I understand that the settings page is only for admins. But what about the rest of the Migrations pages -- are there any that non-admins cannot work with, completely, including deleting items?

@yaacov
Copy link
Member

yaacov commented Jul 31, 2023

I. a "user" will have all the options except "overview / settings" page ( admins only)

II. a "read only user" will not be able to
create
edit
delete
duplicate ( it's like create )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

4 participants