-
Notifications
You must be signed in to change notification settings - Fork 205
Fix/deduplication privilege level change #12068
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
base: main
Are you sure you want to change the base?
Fix/deduplication privilege level change #12068
Conversation
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
internal/pkg/agent/application/actions/handlers/handler_action_privilege_level_change.go
Outdated
Show resolved
Hide resolved
internal/pkg/agent/application/actions/handlers/handler_action_privilege_level_change.go
Outdated
Show resolved
Hide resolved
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 addition to the unit test added for the new function, targetingSameUser, would it be possible to write a unit test covering the new logic in the handle method? The test could be skipped if utils.HasRoot() returns true so that you only need to test the various cases in the if !isRoot block, i.e. the new logic added in this PR. Essentially, we'd want to check if ackCommitFn, featuring a mocked acker, is called if we're targeting the same user, indicating that deduplication is happening OR if we're not targeting the same user, the expected error is thrown.
…:michalpristas/elastic-agent into fix/deduplication-privilege-level-change
💛 Build succeeded, but was flaky
Failed CI StepsHistory
|
|
An integration test that sends the privilege level change action twice in a row would probably be the best thing to test this, considering how coupled it is to how agent is installed I am skeptical we can cover the action implementation purely with unit tests. There are integration tests for the command line version of this already but I don't see one for the action.
|
This PR adds some kind of deduplication layer for privilege level change, in case we're already running unelevated it checks desired user and group and if they match current user action is considered duplicate and acked successfully only with warning in logs.
In case of mismatch it fails with error
Fixes: #11993