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

RFD 0171: Database session playback #42082

Merged
merged 5 commits into from
Jun 26, 2024

Conversation

gabrielcorado
Copy link
Contributor

Related to #5799 and #9019.

Rendered version

@gabrielcorado gabrielcorado added rfd Request for Discussion no-changelog Indicates that a PR does not require a changelog entry labels May 28, 2024
@gabrielcorado gabrielcorado self-assigned this May 28, 2024
Copy link
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

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

First quick around. Thanks for drafting this!

rfd/0171-database-session-playback.md Outdated Show resolved Hide resolved
rfd/0171-database-session-playback.md Show resolved Hide resolved
Comment on lines 194 to 202
- `on`: Enables session recording, but only queries are recorded. This will
mimic the current behavior.
- `full`: Enables session recording. Queries and responses are recorded.
- `off`: Disables session recording. Audit events are kept unchanged (start,
end, and query events emitted).
- `recording_only`: Enables recording, but events will not be emitted, they
will only be present on recordings. This mode will be the same format as SSH
sessions, where the commands and their results are only present on the
recording.
Copy link
Contributor

@greedy52 greedy52 May 28, 2024

Choose a reason for hiding this comment

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

what mode is preferred in case of mode conflict?

For recording_only, I assume the "start" and "end" events will still emit? Should a toggle on audit-events event be part of record_session?

Copy link
Contributor Author

@gabrielcorado gabrielcorado May 29, 2024

Choose a reason for hiding this comment

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

what mode is preferred in case of mode conflict?

Most verbose one. full > recording_only > on > off

I assume the "start" and "end" events will still emit?

They will be included, like in the SSH sessions (I'll update the text to clarify that).

Should a toggle on audit-events event be part of record_session?

I'm still unsure about this too, because it doesn't look like a recording option. But moving this around (for example, to a cluster-level config) would reduce the ability to more dynamic configurations (for example, per protocol).

Copy link
Contributor

@Tener Tener Jun 4, 2024

Choose a reason for hiding this comment

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

Configuring this at role level leads to some awkwardness. The fact that user has multiple roles forces you to perform merging on this, and few people can track the end result. You end up with a situation where more finicky configurations are unstable (people will be surprised that the recording was made/was not made), and ultimately disable or enable it for everyone.

Instead of roles, how about we scope it around individual databases? The default values may be put in a global settings object, but individual database objects can contain overrides. For auto-discovered databases we can use labels to guide these.

Ultimately, I think this setting is less about the people accessing the data and more about the nature of data itself.

Moving on, do we need these many levels for the configuration? I'd propose we keep the current capabilities without any changes and only treat the full session recording as an optional add-on. That collapses the full / recording / on / off enum down to session_recording_enable: true|false.

However, I do think we could use a more complex configuration, perhaps something like this:

database_session_recording:
    enable: true | false
    response_max_rows: 100 # only meaningful for row-based protocols
    response_max_bytes: 2M # meaningful for all protocols, including row-based
    anonymise: disabled | ellipsis | hash | stats_only (default)

With a strong default anonymisation we can make the feature enabled by default, which both prevents surprise capture of sensitive data as well as promotes the feature among the users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the full (as we're no longer recording the data returned) and recording_only (considered out of this RFD scope as it is not directly related to session recording) options. We're left only with on|off.

the database name.
- Data with fields metadata: Format the data into an ASCII table, where each
field is a column.
- Data without field metadata: Print each row as a single line.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the player show when there is no data result but only queries?

Web UI. We're going to convert database recording events into `SessionPrint`
events, which can be rendered by the players.

This conversion will be agnostic to the database protocol. This will make it
Copy link
Contributor

Choose a reason for hiding this comment

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

Will older db sessions before this feature become playable? What happens when the db sessions are not playable (e.g. protocol opensearch)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will older db sessions before this feature become playable?

That's the initial ideal.

What happens when the db sessions are not playable (e.g. protocol opensearch)?

Protocol-specific events will require additional implementation (to convert them into SessionPrint). So, the recording will not be available if all the events can be converted. We could always try to render them based on the generic events, but some protocols might never emit them (opensearch that you mentioned is one of them).

I'll investigate better how we can improve this UX-wise so we don't show the play button for sessions that we cannot convert properly.

(I'll add all this info to the RFD)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to add non-textual playback option too. The database responses are inherently more structured than SSH terminal events. We can exploit that, for example to show the query response as a actual HTML table (sortable and with client-side filters!) or properly convey the request-response nature of some protocols, like opensearch.

Obviously this would require some UI work, but in the end I think it would be a great alternative to purely text-based reply, which has numerous inherent limitations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be nice to add non-textual playback option too.

This can be implemented outside the session player (currently focused on text-based sessions). All the information will still be present on the recordings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll investigate better how we can improve this UX-wise so we don't show the play button for sessions that we cannot convert properly.

(I'll add all this info to the RFD)

Did I miss this? I don't find a section on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a section describing the general necessary changes on the WebUI. I haven't included the full details, as we might discuss them on the implementation PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added a section describing the general necessary changes on the WebUI. I haven't included the full details, as we might discuss them on the implementation PR.

I am more looking for where users can find the recordings. Before this feature, users have to grab an ID from an audit event. So it's safe to assume that now we can see the database sessions in "Session Recordings" tab and tsh recordings ls right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, we'll bring the database recordings to those listings.

rfd/0171-database-session-playback.md Outdated Show resolved Hide resolved
rfd/0171-database-session-playback.md Show resolved Hide resolved
rfd/0171-database-session-playback.md Outdated Show resolved Hide resolved
oneof Result {
// Status of the query execution. It is used when the command doesn't return
// any data.
Status status = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to rename it to DatabseResultStatus or similar. What will be the contents of the status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a message already present on our events package. I've copied it to the RFD to make it easier to understand.

{
...
"status": {
"success": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

An optional error_code might be nice: these are often present for both relational databases as well as for those based on HTTP requests.

Comment on lines 194 to 202
- `on`: Enables session recording, but only queries are recorded. This will
mimic the current behavior.
- `full`: Enables session recording. Queries and responses are recorded.
- `off`: Disables session recording. Audit events are kept unchanged (start,
end, and query events emitted).
- `recording_only`: Enables recording, but events will not be emitted, they
will only be present on recordings. This mode will be the same format as SSH
sessions, where the commands and their results are only present on the
recording.
Copy link
Contributor

@Tener Tener Jun 4, 2024

Choose a reason for hiding this comment

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

Configuring this at role level leads to some awkwardness. The fact that user has multiple roles forces you to perform merging on this, and few people can track the end result. You end up with a situation where more finicky configurations are unstable (people will be surprised that the recording was made/was not made), and ultimately disable or enable it for everyone.

Instead of roles, how about we scope it around individual databases? The default values may be put in a global settings object, but individual database objects can contain overrides. For auto-discovered databases we can use labels to guide these.

Ultimately, I think this setting is less about the people accessing the data and more about the nature of data itself.

Moving on, do we need these many levels for the configuration? I'd propose we keep the current capabilities without any changes and only treat the full session recording as an optional add-on. That collapses the full / recording / on / off enum down to session_recording_enable: true|false.

However, I do think we could use a more complex configuration, perhaps something like this:

database_session_recording:
    enable: true | false
    response_max_rows: 100 # only meaningful for row-based protocols
    response_max_bytes: 2M # meaningful for all protocols, including row-based
    anonymise: disabled | ellipsis | hash | stats_only (default)

With a strong default anonymisation we can make the feature enabled by default, which both prevents surprise capture of sensitive data as well as promotes the feature among the users.

Web UI. We're going to convert database recording events into `SessionPrint`
events, which can be rendered by the players.

This conversion will be agnostic to the database protocol. This will make it
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to add non-textual playback option too. The database responses are inherently more structured than SSH terminal events. We can exploit that, for example to show the query response as a actual HTML table (sortable and with client-side filters!) or properly convey the request-response nature of some protocols, like opensearch.

Obviously this would require some UI work, but in the end I think it would be a great alternative to purely text-based reply, which has numerous inherent limitations.

rfd/0171-database-session-playback.md Outdated Show resolved Hide resolved
rfd/0171-database-session-playback.md Outdated Show resolved Hide resolved
@gabrielcorado
Copy link
Contributor Author

For now, we're removing the data recording from this RFD scope. I've updated all the sections and will resolve some related comments.

Copy link
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

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

Could you also add some info on how the user can find these recordings? like how recordings are listed, in both Web UI and tsh.

rfd/0171-database-session-playback.md Outdated Show resolved Hide resolved
}
```

### Recording options
Copy link
Contributor

Choose a reason for hiding this comment

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

The recording mode is optional for the initial MVP. And it might depend on how we proceed with #40170

Comment on lines +206 to +209
- The player will define if there is need to display the number of affected
records. Protocol-specific variations can be added for this. For example, if
the query was a `SELECT` the player might display it as "Returned X rows"
after the status.
Copy link
Contributor

Choose a reason for hiding this comment

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

The DatabaseSessionCommandResult only captures a number affected_records. How does the player known how to render Returned xxx vs xxx inserted? If the logic is based on the query, how reliable is our logic?

Copy link
Contributor Author

@gabrielcorado gabrielcorado Jun 12, 2024

Choose a reason for hiding this comment

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

It is the player translator's job to generate those descriptions. For example, tt can be implemented similarly to our PoC as a state machine. So, it knows the type of the command that generated the result. Databases that generate concurrent executions could be used; in that case, we would need a field to make the relationship of command -> result.

Web UI. We're going to convert database recording events into `SessionPrint`
events, which can be rendered by the players.

This conversion will be agnostic to the database protocol. This will make it
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll investigate better how we can improve this UX-wise so we don't show the play button for sessions that we cannot convert properly.

(I'll add all this info to the RFD)

Did I miss this? I don't find a section on this.

rfd/0171-database-session-playback.md Outdated Show resolved Hide resolved
Comment on lines +171 to +173
- `on`: Enables session recording.
- `off`: Disables session recording. Audit events are kept unchanged (start,
end, and query events emitted).
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I recall, all database access events currently go to both audit log and session recording. So we're just adding an ability to replay them (and an extra event for query result). Is that correct? Will off option basically mean that we stop emitting events to session recordings?

Copy link
Contributor Author

@gabrielcorado gabrielcorado Jun 12, 2024

Choose a reason for hiding this comment

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

As far as I recall, all database access events currently go to both audit log and session recording. So we're just adding an ability to replay them (and an extra event for query result). Is that correct?

Yes.

Will off option basically mean that we stop emitting events to session recordings?

Yes. This change will not affect audit events (similar to SSH enhanced session recordings). Currently, there is no option to do this other than completely disabling the session recordings on the cluster level (auth_server.session_recording: "off").

@gabrielcorado gabrielcorado added this pull request to the merge queue Jun 26, 2024
Merged via the queue into master with commit 8d102a3 Jun 26, 2024
37 checks passed
@gabrielcorado gabrielcorado deleted the rfd/0171-database-session-playback branch June 26, 2024 23:36
@gabrielcorado gabrielcorado restored the rfd/0171-database-session-playback branch June 27, 2024 15:15
@gabrielcorado gabrielcorado deleted the rfd/0171-database-session-playback branch June 27, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry rfd Request for Discussion size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants