Adding support for rotating session recording encryption keys

[eriktate]

This PR adds new methods to the recordingencryption.Manager for handling key rotations in a way that ensures the RecordingEncryption and SessionRecordingConfig resources remain in sync. The necessary changes for tctl will be in a future PR, but the RPC endpoints making the functionality available are included here.

[github-actions]

Amplify deployment status

Branch	Commit	Job ID	Status	Preview	Updated (UTC)
eriktate/rotating-encryption-keys	e125ded	44	✅SUCCEED	eriktate-rotating-encryption-keys	2025-08-26 20:51:59

[rosstimothy]

I find this function a bit hard to grok. There's a lot going on, callers can modify it's behavior by passing in different resolveSessionRecordingInput fields. It seems like this exists to have a unified location for holding the backend lock and acting accordingly for each use case, but I'm wondering if the unification is causing things to be more complex than it needs to be.

[eriktate]

It's partly to keep everything in the same backend lock and partly because there's a lot of overlap between the different cases. I did notice I was never providing both a SessionRecordingConfig and a RecordingEncryption to a single call of resolveAllSessionRecordingConfigs, so I split it into two functions. modifySessionRecordingConfig handles explicit modifications of the SessionRecordingConfig and modifyRecordingEncryption handles explicit modifications of the RecordingEncryption. It's not a huge change, but I think it helps a bit with the readability

[rosstimothy]

Much better! Thank you for taking another look at this.

[rosstimothy]

What happens in this state? Maybe add a comment explaining why it's okay to proceed and possibly update the log message to indicate what needs to be done if any action is required, and what this means for the health of the cluster?

[eriktate]

Added some comments here, but this would prevent replay of historical recordings that were encrypted using any of the missing rotated keys. I don't treat this as a catastrophic error because we've already confirmed there's a valid active key at this point, and so there's no destructive harm in allowing the auth service to continue. Assuming the issue is fixed, the impacted recordings would just become playable again.

[rosstimothy]

Is return nil,nil safe? If this is expected we should at least document the behavior. However, I'm fairly certain that this would result in a panic here, if not in the other places this function is called as well.

[eriktate]

It is expected and it should be safe because GetSpec()and GetActiveKeyPairs both have nil checks. I can return a pointer to a zero-value RecordingEncryption instead, but I think the nil is a better signal that there's no RecordingEncryption to work with.

[rosstimothy]

Let's clarify in the function docs the expected contract of the function since nil, nil is not a common return in Go.

[eriktate]

Updated the comment and also opted to return the given encryption argument instead of just nil. It's still possible for the return to be nil, nil, but only if the function was called with a nil to begin with