feat: Notify local endpoints about certificate rotation #576
Conversation
…ernet && Couriers) and tests
Filmaluco
force-pushed
the
feature/539_notify_local_endpoints_cert_renew
branch
from
b219ac0 to
1cd4244
Compare
Filmaluco
force-pushed
the
feature/539_notify_local_endpoints_cert_renew
branch
from
1cd4244 to
70abe07
Compare
There was a problem hiding this comment.
Obrigado Filipe!
A few general notes, not tied to any specific file/line:
-
Per Notify local endpoints when the gateway's id certificate is renewed #539, even though the current use of this notification is to alert endpoints to the rotation of the gateway's certificate, the implementation should ideally be generic enough to be reused more generally when the gateway's cert changes for any reason. With this in mind, did you consider using Android broadcast messages instead, and if so, why was it ruled out? I'm probably being naive but I'd have thought they'd be easier to implement.
I think they'd be more versatile as well: for example, if an Awala-compatible app is installed before the gateway is installed+registered, a broadcast could be used by that app to register with the gateway.
-
Since we seem to be changing the notification id for incoming parcels (
tech.relaycorp.endpoint.NOTIFY), I wonder if we should take advantage of that to revisit the prefix used. Since this is using a messaging pattern, shouldn't the prefix correspond to the source of the message/notification as opposed to the target? That is, something liketech.relaycorp.gateway.endpointsinstead oftech.relaycorp.endpoint. -
Nitpick: I think the arrange/act/assert comments are superfluous, as that's what we do in the codebase anyway (including the JS code), except in rare cases where we can't follow that order. It's also inconsistently used (they're missing from
NotifyEndpointsToRenewCertificate, for example)... Which is quite understandable because IMO they don't add anything we can't already see.
PS: You might want to consider registering gradle spotlessApply as a pre-commit hook to avoid ktlint errors, since kitlint is complaining again with the last commit.
app/src/main/java/tech/relaycorp/gateway/domain/endpoint/EndpointNotifyAction.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/tech/relaycorp/gateway/domain/endpoint/EndpointNotifyAction.kt
Outdated
Show resolved
Hide resolved
| package tech.relaycorp.gateway.domain.endpoint | ||
|
|
||
| enum class EndpointNotifyAction(val action: String) { | ||
| ParcelToReceive("tech.relaycorp.endpoint.NEW_PARCEL"), |
There was a problem hiding this comment.
Is this replacing tech.relaycorp.endpoint.NOTIFY from the deleted file AwalaAndroid.kt? If so, that'd be a breaking change, which is OK at this stage of the project, but I'd prefer to have a PR ready for the Ping app as well so that we can merge it immediately after merging this PR. This is to minimise the period of time during which the production apps will be incompatible.
There was a problem hiding this comment.
I remember having the intel that for now breaking changes were ok and that's why I did. But yes this is from the deleted file AwalaAndroid.kt. I will start the PR for the Ping app ASAP
There was a problem hiding this comment.
need permissions for: relaycorp/relaynet-echo-android
There was a problem hiding this comment.
I remember having the intel that for now breaking changes were ok and that's why I did
Breaking changes are fine indeed, but if the breaking change also introduces an interoperability issue that breaks production, we should try to do a simultaneous release of the affected components to minimise the disruption.
need permissions for: relaycorp/relaynet-echo-android
We're not using that anymore so I should probably delete it. Or do you mean relaynet-ping-android? I think you do have access to that one.
app/src/main/java/tech/relaycorp/gateway/domain/endpoint/EndpointNotifyAction.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/tech/relaycorp/gateway/domain/endpoint/GetEndpointReceiver.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/tech/relaycorp/gateway/domain/endpoint/EndpointNotifyAction.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/tech/relaycorp/gateway/domain/endpoint/NotifyEndpointsOfParcels.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/tech/relaycorp/gateway/domain/endpoint/NotifyEndpointsToRenewCertificate.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/tech/relaycorp/gateway/domain/endpoint/NotifyEndpointsToRenewCertificate.kt
Outdated
Show resolved
Hide resolved
gnarea
changed the title
There was a problem hiding this comment.
Thanks for the changes!
Let's hold off merging this one until relaycorp/awala-endpoint-android#199 is good to go.
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Closes #539