Added session management routes + crd + cli interaction.

[meowjesty]

• Issue: #217
• operator PR: #427

Creates a new cli command to forcefully kill one or more OperatorSessions.

Adds a new CustomResource, as this new command is a a separate route in the operator from the ones that we had.

[Razz4780]

I feel like this also needs another operator feature, like session_management: bool. Otherwise the users will see real ugliness 💀

[meowjesty]

I feel like this also needs another operator feature, like session_management: bool. Otherwise the users will see real ugliness 💀

Ideally we would have some credentials/permissions system in place, so a random user would not be able to just kill-all sessions, instead they could only kill their own. While some admin level user could do whatever.

We could add a route that enables/disables session management, but this seems like extra annoyance for users, due to us not having a system in place to handle this stuff?

[aviramha]

I feel like this also needs another operator feature, like session_management: bool. Otherwise the users will see real ugliness 💀

Ideally we would have some credentials/permissions system in place, so a random user would not be able to just kill-all sessions, instead they could only kill their own. While some admin level user could do whatever.

We could add a route that enables/disables session management, but this seems like extra annoyance for users, due to us not having a system in place to handle this stuff?

That's controlled via k8s RBAC. To use the route users need to have the delete verb on the sessions resource.

[Razz4780]

I feel like this also needs another operator feature, like session_management: bool. Otherwise the users will see real ugliness 💀

Ideally we would have some credentials/permissions system in place, so a random user would not be able to just kill-all sessions, instead they could only kill their own. While some admin level user could do whatever.
We could add a route that enables/disables session management, but this seems like extra annoyance for users, due to us not having a system in place to handle this stuff?

That's controlled via k8s RBAC. To use the route users need to have the delete verb on the sessions resource.

To be honest, I was not thinking about access control at all. Rather compatibility with older operators, which don't have these routes. Like with copy_target, the operator should announce that it supports killing sessions, so that the users don't see generic 404 or 503 (?) when it doesn't

[aviramha]

Ah it could be nice the cli to check for feature existence, yes.

[meowjesty]

Ah it could be nice the cli to check for feature existence, yes.

Hmm we could just convert an 404 for hitting a missing route in an error saying "The operator doesn't seem to support this operation.". Maybe the message could also contain the operator version? I think the whole handling for this can be done on the cli-side?

[meowjesty]

killing all sessions
  x Session operation `kill-all` failed!
    x `operator session kill-all` is not supported by the mirrord-operator found in your cluster, consider updating it!                                                                                             2024-02-26T18:02:45.664986Z ERROR mirrord::operator::session: not_supported="`operator session kill-all` is not supported by the mirrord-operator found in your cluster, consider updating it!"
Error:   × Failed to connect to the operator, probably due to RBAC: getting status failed: ApiError: "": Failed to parse error data (ErrorResponse { status: "404 Not Found", message: "\"\"", reason: "Failed to parse
  │ error data", code: 404 })

(...)

Progress now shows a nicer error if operator version doesn't support the session ops.

[Razz4780]

Looks mostly good, but there are 3 things:

1. Do we really need the response.reason.contains("parse") part when checking operator response? I think checking status code should be enough.
2. Same error handling logic is repeated in all 3 command handlers
3. Using OperatorApiError::KubeError with OperatorOperation::GettingStatus is not a good idea. User sees Failed to connect to the operator, probably due to RBAC: getting status failed in the CLI output :x You can use OperatorApiError::UnsupportedFeature for this. It would require fetching the operator status first (you'd need the operator version), but I think it's even better - we can check if the operator is installed before attempting to kill sessions

[Razz4780]

Wonderful 🐈