-
Notifications
You must be signed in to change notification settings - Fork 242
V1.0.x fix: Allow update of node using parent org identity #1073
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
Conversation
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Codecov Report
@@ Coverage Diff @@
## release-1.0 #1073 +/- ##
=============================================
Coverage 100.00% 100.00%
=============================================
Files 324 324
Lines 20812 20820 +8
=============================================
+ Hits 20812 20820 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| if err != nil && retryable { | ||
| return HandlerResult{Action: ActionRetry}, err | ||
| } else if err != nil { | ||
| log.L(ctx).Infof("Unable to process identity update (parked) %s: %s", msg.Header.ID, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this one parked? This would be for things like an unknown or unconfirmed parent identity. All other failures here seem to result in outright rejection of the update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, we couldn't resolve something up the parent stack. The code in creation (which this was copied from) did a wait at this point, on the basis there could be something in the parent stack still being resolved.
I believe it's technical also possible when replaying a chain for that to be true here. Or at least I've not done enough analysis of the topics used on the messages in that scenario to rule it out. So I left the logic equivalent in create and update paths.
awrichar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but with one question about error handling

Testing patching Node identity for Cert cycling, I found that in v1.0 we were missing the API from the Swagger, but it did exist
PATCHnamespaces/{ns}/identities/{iid}However, attempt to run it for a node resulted in the following - due to a discrepancy between the checking for the author of the message between identity creation, and identity update:
[2022-09-22T22:39:53.629Z] WARN node_0: Invalid identity update message 2ac79c71-3983-45a4-8942-7f1bc124225b - wrong author: did:firefly:org/org_0 dbtx=reAmSQrS role=aggregatorThis PR is a V1.0 stream fix to correct this. A separate PR will be needed for the V1.1 stream - in #1074