-
Notifications
You must be signed in to change notification settings - Fork 197
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
fix: Clear the inventory data hash on re-authentication #1719
base: master
Are you sure you want to change the base?
Conversation
@jo-lund, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline". my commands and optionsYou can trigger a pipeline on multiple prs with:
You can start a fast pipeline, disabling full integration tests with:
You can trigger GitHub->GitLab branch sync with:
You can cherry pick to a given branch or branches with:
|
@mender-test-bot start pipeline |
Hello 😺 I created a pipeline for you here: Pipeline-1593862884 Build Configuration Matrix
|
@mender-test-bot start pipeline |
Hello 😺 I created a pipeline for you here: Pipeline-1595533100 Build Configuration Matrix
|
1c44ba8
to
2ff5efa
Compare
@mender-test-bot start pipeline |
Hello 😺 I created a pipeline for you here: Pipeline-1595781641 Build Configuration Matrix
|
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.
To me this is not a good way to achieve the desired behavior. It is a working solution, but from a high-level/abstract perspective pulling inventory into the Authenticator is wrong. IMHO, the Authenticator should have zero knowledge (dependency) of (on) things like inventory, deployments checking,..., whatever else.
So let me suggest that we add a new callback to the Authenticator to call on re-authentication. It's a cleaner design and also more future proof because next time we need to do something else in case of re-authentication we won't have to modify the Authenticator itself.
src/api/auth.hpp
Outdated
namespace inventory { | ||
class InventoryAPI; | ||
} | ||
} // namespace 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.
This should not be needed. mender-update/inventory.hpp
should be #include
d instead.
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.
I used a forward declaration here exactly to avoid using include, which can reduce compile time etc.
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.
Yeah, I got it. A nice habit for sure. But we use #ifndef-#define
guards that should prevent those issues and we should be consistent.
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.
Include guards and forward declarations are not the same and serve different purposes. When you include a file the whole file will be inserted into the current one together with all the includes in that file as well (as long as they have not been included already because they have defined an include guard). Include guards prevent the same header file from being included multiple times and therefore prevent redefinition of the same symbol.
With forward declarations you get just the symbol with incomplete type. This can reduce compile time substantially, e.g. because you don't need to recompile several files when a header changes.
I agree, but the problem here is that HandleReceivedToken can be called from a dbus signal handler which is decoupled from the state machine itself. |
Yes, I know. That's why I told Lluis immediately when discovered this issue that the solution won't be easy and straightforward. But if we allow passing the re-authentication callback as an argument (to the constructor, for example), it can be a lambda with a capture of the |
Yes, that would be a better design, and it would be nice to have a generic API for this. I was actually looking for a publish/subscribe mechanism in the code when implementing this change. |
👍 That would be nice. But we only have callbacks and lambdas, hoping for the best regarding memory management and object lifetime. 😀 |
This fixes the case where e.g. a third party tool re-authenticates by using the D-Bus API while mender-update is waiting for the next poll interval to expire. The inventory will now be resubmitted when the poll triggers. Ticket: MEN-7873 Changelog: Title Signed-off-by: John Olav Lund <[email protected]>
2ff5efa
to
9d1d579
Compare
Merging these commits will result in the following changelog entries: Changelogsmender (failed-inventory-update)New changes in mender since master: Bug Fixes
|
This fixes the case where e.g. a third party tool re-authenticates by using the D-Bus API while mender-update is waiting for the next poll interval to expire. The inventory will now be resubmitted when the poll triggers.
Ticket: MEN-7873
Changelog: Title