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.
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
|
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. |
…plication-privilege-level-change
…plication-privilege-level-change
|
This pull request is now in conflicts. Could you fix it? 🙏 |
…plication-privilege-level-change
…:michalpristas/elastic-agent into fix/deduplication-privilege-level-change
|
LGTM, @ycombinator can you re-review? |
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
…_privilege_level_change.go Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>
…_privilege_level_change.go Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>
…plication-privilege-level-change
blakerouse
left a comment
There was a problem hiding this comment.
Overall this looks good. I don't see any issues in the code. Just one comment inline about the log level used.
Looks like still waiting for a green CI, once its green I will give me +1.
internal/pkg/agent/application/actions/handlers/handler_action_privilege_level_change.go
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
History
|
blakerouse
left a comment
There was a problem hiding this comment.
Thanks for changing this to info level. Looks good!
(cherry picked from commit 7c07fb5)
|
If we're fixing a bug here, I feel like we should include a changelog entry? |
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