Add workflow update-options identity to history event #8576
Conversation
| requestID, | ||
| completionCallbacks, | ||
| links, | ||
| "", |
There was a problem hiding this comment.
start workflow request does not provide identity, so we don't have anything to put here
There was a problem hiding this comment.
this is for nexus callbacks and not versioning override anyway
| requestID, | ||
| callbacks, | ||
| event.Links, | ||
| attr.GetIdentity(), |
There was a problem hiding this comment.
reapply the identity that was already on the update-options event
| switch op := operation.GetVariant().(type) { | ||
| case *workflowpb.PostResetOperation_UpdateWorkflowOptions_: | ||
| _, _, err := updateworkflowoptions.MergeAndApply(resetMS, op.UpdateWorkflowOptions.GetWorkflowExecutionOptions(), op.UpdateWorkflowOptions.GetUpdateMask()) | ||
| _, _, err := updateworkflowoptions.MergeAndApply(resetMS, op.UpdateWorkflowOptions.GetWorkflowExecutionOptions(), op.UpdateWorkflowOptions.GetUpdateMask(), "") |
There was a problem hiding this comment.
don't apply an identity for post-reset-operations update-options, because even if we applied the identity of the reset request, the reset code paths are used in a lot of places and I don't want to accidentally have inconsistencies on the passive cluster or something. Better to just be consistent and not have an identity if the update-options was part of a reset operation (for now at least)
There was a problem hiding this comment.
with state-based replication this code might not run on the passive side.
Ideally we should put the reset requester in the event but I'm fine doing it later. But please add this explanation in the code as a comment so the future contributor can understand the reasoning.
carlydf
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
_**READ BEFORE MERGING:** All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted._ <!-- Describe what has changed in this PR --> Ddd identity to workflow update-options <!-- Tell your future self why have you made these changes --> So that the history event can serve as an audit log as to who set versioning overrides on the workflow <!-- Are there any breaking changes on binary or code level? --> None <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> temporalio/temporal#8576
_**READ BEFORE MERGING:** All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted._ <!-- Describe what has changed in this PR --> Ddd identity to workflow update-options <!-- Tell your future self why have you made these changes --> So that the history event can serve as an audit log as to who set versioning overrides on the workflow <!-- Are there any breaking changes on binary or code level? --> None <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> temporalio/temporal#8576
| requestID, | ||
| completionCallbacks, | ||
| links, | ||
| "", |
There was a problem hiding this comment.
Bug: Audit Gap: Workflow Option Identity Missing
The AddWorkflowExecutionOptionsUpdatedEvent call passes an empty string for the identity parameter when attaching options to an existing workflow during conflict resolution. However, s.request.StartRequest.GetIdentity() is available and used elsewhere in the same file to access the workflow starter's identity. This results in missing audit log information about who set the workflow options when using OnConflictOptions during start workflow conflicts.
| switch op := operation.GetVariant().(type) { | ||
| case *workflowpb.PostResetOperation_UpdateWorkflowOptions_: | ||
| _, _, err := updateworkflowoptions.MergeAndApply(resetMS, op.UpdateWorkflowOptions.GetWorkflowExecutionOptions(), op.UpdateWorkflowOptions.GetUpdateMask()) | ||
| _, _, err := updateworkflowoptions.MergeAndApply(resetMS, op.UpdateWorkflowOptions.GetWorkflowExecutionOptions(), op.UpdateWorkflowOptions.GetUpdateMask(), "") |
There was a problem hiding this comment.
with state-based replication this code might not run on the passive side.
Ideally we should put the reset requester in the event but I'm fine doing it later. But please add this explanation in the code as a comment so the future contributor can understand the reasoning.
What changed?
Add workflow update-options identity to history event
Why?
So that users can use the event as an audit log to find out who set an override
How did you test it?
Note
Add identity to
WORKFLOW_EXECUTION_OPTIONS_UPDATEDhistory event and propagate through APIs, batcher, NDC, and tests.identitytoWorkflowExecutionOptionsUpdatedattributes and wire throughEventFactory,HistoryBuilder, andMutableState(AddWorkflowExecutionOptionsUpdatedEvent).UpdateWorkflowOptions.MergeAndApplynow acceptsidentityand passes it to mutable state.updateworkflowoptionshandler usesreq.GetIdentity();startworkflowpasses empty identity for attach-only updates.identity.Identityin requests.identityin expectations.identityin history for direct and batch updates.Written by Cursor Bugbot for commit 8958b2f. This will update automatically on new commits. Configure here.