Skip to content
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.

Sign up for GitHub

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

[Fleet] Update .fleet-agent on policy change ack even without ES output #2119

Merged
merged 5 commits into from
Dec 7, 2022
Merged

[Fleet] Update .fleet-agent on policy change ack even without ES output #2119

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6fa4b9e
[Fleet] Update .fleet-agent on policy change ack even without ES output
nchaulet Nov 24, 2022
30e486e
Merge branch 'main' into fix-2105-update-agent-on-policy-change-ack
nchaulet Nov 29, 2022
291553c
Update internal/pkg/api/handleAck.go
nchaulet Nov 30, 2022
52896ca
Update after review
nchaulet Dec 5, 2022
396a87c
Merge branch 'main' into fix-2105-update-agent-on-policy-change-ack
nchaulet Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions internal/pkg/api/handleAck.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,15 @@ func (ack *AckT) handlePolicyChange(ctx context.Context, zlog zerolog.Logger, ag
return nil
}

err := ack.updateAgentDoc(ctx, zlog,
agent.Id,
currRev, currCoord,
agent.PolicyID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying you need to change, but I'm not sure if doing it here could be a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another thing, do you think you could add a test to endure:

  • the document is updated
  • updateAgentDoc is called in the right moment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me ask if someone in my team can foresee any issue with either option

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just updated


if err != nil {
nchaulet marked this conversation as resolved.
Show resolved Hide resolved
return err
}

for _, output := range agent.Outputs {
if output.Type != policy.OutputTypeElasticsearch {
continue
Expand All @@ -346,8 +355,6 @@ func (ack *AckT) handlePolicyChange(ctx context.Context, zlog zerolog.Logger, ag
err := ack.updateAPIKey(ctx,
zlog,
agent.Id,
currRev, currCoord,
agent.PolicyID,
output.APIKeyID, output.PermissionsHash, output.ToRetireAPIKeyIds)
if err != nil {
return err
Expand All @@ -361,8 +368,7 @@ func (ack *AckT) handlePolicyChange(ctx context.Context, zlog zerolog.Logger, ag
func (ack *AckT) updateAPIKey(ctx context.Context,
zlog zerolog.Logger,
agentID string,
currRev, currCoord int64,
policyID, apiKeyID, permissionHash string,
apiKeyID, permissionHash string,
toRetireAPIKeyIDs []model.ToRetireAPIKeyIdsItems) error {

if apiKeyID != "" {
Expand Down Expand Up @@ -407,6 +413,15 @@ func (ack *AckT) updateAPIKey(ctx context.Context,
ack.invalidateAPIKeys(ctx, toRetireAPIKeyIDs, apiKeyID)
}

return nil
}

func (ack *AckT) updateAgentDoc(ctx context.Context,
zlog zerolog.Logger,
agentID string,
currRev, currCoord int64,
policyID string,
) error {
body := makeUpdatePolicyBody(
policyID,
currRev,
Expand Down