-
Notifications
You must be signed in to change notification settings - Fork 82
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
invalidate remote api keys on unenroll #3154
Conversation
} | ||
apiKeys := agent.APIKeyIDs() | ||
zlog.Info().Any("fleet.policy.apiKeyIDsToRetire", apiKeys).Msg("handleUnenroll invalidate API keys") | ||
ack.invalidateAPIKeys(ctx, zlog, apiKeys, "") |
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.
building []ToRetireAPIKeyIdsItems
to reuse the logic in invalidateAPIKeys
to invalidate against remote es
Do we require changelog if this pr is a fix for a feature in the same minor (8.12)? |
Fleet Server e2e tests failing, any ideas how to fix this?
Tried to use es version 8.13-snapshot in e2e tests, but it looks like there is no such version yet: https://artifacts-api.elastic.co/v1/versions/ |
/test |
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.
lgtm, let's get the tests passing and merge
/test |
@@ -740,3 +694,49 @@ func makeUpdatePolicyBody(policyID string, newRev, coordIdx int64) []byte { | |||
|
|||
return buf.Bytes() | |||
} | |||
|
|||
func invalidateAPIKeys(ctx context.Context, zlog zerolog.Logger, bulk bulk.Bulk, toRetireAPIKeyIDs []model.ToRetireAPIKeyIdsItems, skip string) { |
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.
moved this function outside of AckT
so it is accessible from handleCheckin
too
internal/pkg/api/handleCheckin.go
Outdated
@@ -119,6 +119,11 @@ func (ct *CheckinT) handleCheckin(zlog zerolog.Logger, w http.ResponseWriter, r | |||
|
|||
agent, err := authAgent(r, &id, ct.bulker, ct.cache) | |||
if err != nil { | |||
// invalidate remote API keys of force unenrolled agents | |||
if err == ErrAgentInactive && agent != nil { |
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.
Added testing steps of the force unenroll scenario to the pr description.
Decided to invalidate remote API keys on checkin here, when finding that the agent is inactive, because the agent becomes inactive after force unenroll. It's not guaranteed that the agent is going to check-in after force unenroll, but we can't do the invalidation in kibana, as kibana doesn't have access to the remote es service token as it is a secret.
Fleet-server also doesn't handle force unenroll actions, and even if it would, fleet server only reads actions on agent checkin. So I didn't find a better way to invalidate api keys after force unenroll.
@michel-laterman thanks for the review! added a fix for force unenroll too in this commit: 35f3539 |
Quality Gate passedThe SonarQube Quality Gate passed, but some issues were introduced. 1 New issue |
* invalidate remote api keys on unenroll * fix test * using latest es snapshot in tests * try with latest 8.12 snapshot * use new 8.13 snapshot * try without build id * invalidate remote api key after force unenroll * fix lint * added integration test to force unenroll * added integration test on unenroll * fix lint
* invalidate remote api keys on unenroll (#3154) * invalidate remote api keys on unenroll * fix test * using latest es snapshot in tests * try with latest 8.12 snapshot * use new 8.13 snapshot * try without build id * invalidate remote api key after force unenroll * fix lint * added integration test to force unenroll * added integration test on unenroll * fix lint * use latest snapshot * try with 8.12-snapshot * change back version in version.go
@@ -1,4 +1,4 @@ | |||
ELASTICSEARCH_VERSION=8.12.0-9d443b17-SNAPSHOT | |||
ELASTICSEARCH_VERSION=8.13.0-SNAPSHOT |
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 change broke the automation with updatecli, see https://github.com/elastic/fleet-server/actions/runs/7197121542
is that the expected behaviour?
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.
sorry, I changed it to fix e2e tests, it was not working with a specific 8.13 build
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.
how can we fix this?
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.
Using 8.13.0-jqmw70gd
in main
and 8.12.0-8db20ea5
in 8.12
I took those values from https://github.com/elastic/fleet-server/actions/runs/7197121542/job/19603532844#step:3:213
https://storage.googleapis.com/artifacts-api/snapshots/8.12.json
https://storage.googleapis.com/artifacts-api/snapshots/main.json
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.
do you mind raising prs to fix this or do you want me to?
* invalidate remote api keys on unenroll * fix test * using latest es snapshot in tests * try with latest 8.12 snapshot * use new 8.13 snapshot * try without build id * invalidate remote api key after force unenroll * fix lint * added integration test to force unenroll * added integration test on unenroll * fix lint (cherry picked from commit d4413c5) # Conflicts: # dev-tools/integration/.env
What is the problem this PR solves?
If an agent using remote es output is unenrolled, the remote API key was not invalidated.
How does this PR solve the problem?
Enhanced the logic that invalidates api keys on unenroll to take into account the output, so that remote api keys are invalidated with the remote es client (bulker).
How to test this PR locally
Invalidate remote API key on agent unenroll
these logs show up in Fleet Server:
Invalidate remote API key on agent force unenroll
logs in Fleet Server:
Design Checklist
Checklist
./changelog/fragments
using the changelog toolRelated issues
Closes https://github.com/elastic/ingest-dev/issues/2739