[Fleet] Update .fleet-agent on policy change ack even without ES output #2119
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
nchaulet
force-pushed
the
fix-2105-update-agent-on-policy-change-ack
branch
from
7e4ab94 to
6fa4b9e
Compare
internal/pkg/api/handleAck.go
Outdated
| err := ack.updateAgentDoc(ctx, zlog, | ||
| agent.Id, | ||
| currRev, currCoord, | ||
| agent.PolicyID) |
There was a problem hiding this comment.
[Blocker]
I'm wondering if there could be a problem doing it here, before updating the API keys.
Did you test the case when something fails after this call, but before the method returns?
I think it'd be safer to move it to the end of the method to keep the implementation closer to what it was.
There was a problem hiding this comment.
I'm not saying you need to change, but I'm not sure if doing it here could be a problem.
There was a problem hiding this comment.
another thing, do you think you could add a test to endure:
- the document is updated
updateAgentDocis called in the right moment
There was a problem hiding this comment.
I think it's better to update the agent doc to know which policy it's running even if we fail to invalidate retired API keys, what do you think?
There was a problem hiding this comment.
I think we might run into trouble. The agent document would state policy x+1, but the API keys would have permission for policy x. I cannot access the impact of it, but I don't think it's a good thing regardless of the impact.
There was a problem hiding this comment.
Let me ask if someone in my team can foresee any issue with either option
There was a problem hiding this comment.
from what I understand the code here after is invalidating old API keys, I think the worse case scenario will be: The agent document would state policy x+1, with old API keys still valid.
There was a problem hiding this comment.
I didn't get any extra input from the team. However I'm still believe it isn't the best idea. It might cause the policy to do not be properly updated later. As the policy will be in the right revision, fleet-server will consider there is nothing to be done, whereas there is.
It'll keep the data more consistent if we have it back where it was
Co-authored-by: Anderson Queiroz <[email protected]>
internal/pkg/api/handleAck.go
Outdated
| err := ack.updateAgentDoc(ctx, zlog, | ||
| agent.Id, | ||
| currRev, currCoord, | ||
| agent.PolicyID) |
There was a problem hiding this comment.
I didn't get any extra input from the team. However I'm still believe it isn't the best idea. It might cause the policy to do not be properly updated later. As the policy will be in the right revision, fleet-server will consider there is nothing to be done, whereas there is.
It'll keep the data more consistent if we have it back where it was
|
Kudos, SonarCloud Quality Gate passed!
|
AndersonQ
added
backport-v8.6.0
…ut (#2119) (#2267) (cherry picked from commit e7fe0a7) Co-authored-by: Nicolas Chaulet <[email protected]>
|
For anyone interested in this - this didn't make it into 8.6.0 - it will be available in 8.6.1 |
Description
Resolve #2105
Fleet server update the
.fleet-agentdocument when an agent ack a policy change, there was a bug introduced where the document was only updated if the agent had one Elasticsearch output, so for agent with only logstash the revision was never updated, and the agent stayed inout of datein the Fleet UI.That PR fix that.