Add must-gather-service enhancement #36
Conversation
Proposing enhancement adding backend to execute must-gather via API.
This reverts commit 16c7d6c.
|
|
||
| ### Risks and Mitigations | ||
|
|
||
| The must-gather service needs OpenShift cluster admin account token to be able to execute must-gather command. The token should be passed by Operator, in similar way as for other migration-toolkit components. |
There was a problem hiding this comment.
I would really like for us to describe how this is working?
Are we creating a service account with the correct role bindings or are we passing in some kubeconfig that is defined somewhere else?
On top of this, do we know where this service is running (which namespace? how will it be exposed to the clusters network?)
There was a problem hiding this comment.
The kubeconfig mounted as PV should used, but I'm just getting more info on authentication process on this - will provide update later.
Expected namespace is the same as Foklift (or other projects) services are deployed. So e.g. openshift-migration, rhmtv, ..
|
|
||
| The must-gather service needs OpenShift cluster admin account token to be able to execute must-gather command. The token should be passed by Operator, in similar way as for other migration-toolkit components. | ||
|
|
||
| HTTP API should use the same authentication as UI where is should be executed from. Must-gather could provide sensitive information about the cluster so needs to be handled in same way as admin UI is. |
There was a problem hiding this comment.
I would really like this to be explained. AFAIK the admin UI is using the user's OAuth account token to make the calls on their behalf.
Also, if I can pass it an arbitrary image and get cluster-admin access, I can do some pretty awful things I would be very concerned about what RBAC/auth(z) we have it place here to make sure this is locked down.
There was a problem hiding this comment.
All of must-gather functionality requires admin access to the cluster, so the UI should require it too.
I'm investigating deeper on this.
| $ curl http://localhost:8080/must-gather/15/data -o must-gather-archive.tar.gz | ||
| ``` | ||
|
|
||
| List all must-gather executions |
There was a problem hiding this comment.
Are these persisted? do we require a PV for this to work?
There was a problem hiding this comment.
They are not persisted. The temporary storage is on the container filesystem which gets deleted on container reboot (sqlite db & dumped archives), also automatic old-data cleanup is implemented.
There was a problem hiding this comment.
So the UI code could have a must gather per migration(or some ID). but that might be missing. is this something the UI will have to handle?
There was a problem hiding this comment.
If the UI sends query for a not existing must-gather execution (by ID or a custom name), it gets 404 HTTP response, so it needs expect that this could happen.
|
Thanks for feedback on this PR. It has been merged with Forklift UI part into #37 where our discussion can continue. |
* Add must-gather from Forklift UI enhancement Proposing enhancement allowing to execute must-gather from Forklift UI. * Update and merge backend enhancement #36 * add security and resources consumption into risks * Add auth by bearer token * Update Auth SubjectAccessReview check
Proposing enhancement adding backend for must-gather execution via API.
The backend service should be re-usable for different OpenShift-based projects. Just UI will be different (in a follow-up enhancement).
Related issue: aufi/must-gather-rest-wrapper#1