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

Collapse two unenroll functions into one #94848

Merged
merged 6 commits into from
Mar 18, 2021
Merged

Collapse two unenroll functions into one #94848

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5564f25
Collapse two unenroll functions into one
Mar 17, 2021
1db2dd9
Restore different agent filtering for force unenroll
Mar 17, 2021
3d7b0f5
Merge branch 'master' into fleet-reduce-unenroll-duplication
kibanamachine Mar 17, 2021
669525f
Merge branch 'master' into fleet-reduce-unenroll-duplication
kibanamachine Mar 17, 2021
9a6ee18
Merge branch 'master' into fleet-reduce-unenroll-duplication
kibanamachine Mar 17, 2021
c71ed57
Undo changes to unenroll API tests
Mar 18, 2021
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
17 changes: 9 additions & 8 deletions x-pack/plugins/fleet/server/routes/agent/unenroll_handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,19 +52,20 @@ export const postBulkAgentsUnenrollHandler: RequestHandler<
body: { message: 'Requires Gold license' },
});
}

const soClient = context.core.savedObjects.client;
const esClient = context.core.elasticsearch.client.asInternalUser;
const unenrollAgents =
request.body?.force === true ? AgentService.forceUnenrollAgents : AgentService.unenrollAgents;
const agentOptions = Array.isArray(request.body.agents)
? { agentIds: request.body.agents }
: { kuery: request.body.agents };

try {
if (Array.isArray(request.body.agents)) {
await unenrollAgents(soClient, esClient, { agentIds: request.body.agents });
} else {
await unenrollAgents(soClient, esClient, { kuery: request.body.agents });
}

await AgentService.unenrollAgents(soClient, esClient, {
...agentOptions,
force: request.body?.force,
});
const body: PostBulkAgentUnenrollResponse = {};

return response.ok({ body });
} catch (error) {
return defaultIngestErrorHandler({ error, response });
Expand Down
114 changes: 53 additions & 61 deletions x-pack/plugins/fleet/server/services/agents/unenroll.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,12 @@ export async function unenrollAgent(
export async function unenrollAgents(
soClient: SavedObjectsClientContract,
esClient: ElasticsearchClient,
options: GetAgentsOptions
options: GetAgentsOptions & { force?: boolean }
) {
// start with all agents specified
const agents = await getAgents(esClient, options);

// Filter to agents that are not already unenrolled, or unenrolling
// Filter to those not already unenrolled, or unenrolling
const agentsEnrolled = agents.filter(
(agent) => !agent.unenrollment_started_at && !agent.unenrolled_at
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was one difference between the filtering logic of the two functions. force* only tested unenrolled_at vs this function which also checks unenrollment_started_at

- (agent) => !agent.unenrolled_at;
+ (agent) => !agent.unenrollment_started_at && !agent.unenrolled_at

I believe this is the correct logic, but let me know if it should be different if force: true

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 the difference was correct, as you could have to force unenroll an agent that started the enrollment before but never finished.

);
Expand All @@ -71,30 +72,59 @@ export async function unenrollAgents(
)
);
const agentsToUpdate = agentsEnrolled.filter((_, index) => settled[index].status === 'fulfilled');

const now = new Date().toISOString();

// Create unenroll action for each agent
await bulkCreateAgentActions(
soClient,
esClient,
agentsToUpdate.map((agent) => ({
agent_id: agent.id,
created_at: now,
type: 'UNENROLL',
}))
);
if (options.force) {
// Get all API keys that need to be invalidated
const apiKeys = agentsToUpdate.reduce<string[]>((keys, agent) => {
if (agent.access_api_key_id) {
keys.push(agent.access_api_key_id);
}
if (agent.default_api_key_id) {
keys.push(agent.default_api_key_id);
}

return keys;
}, []);

// Invalidate all API keys
if (apiKeys.length) {
await APIKeyService.invalidateAPIKeys(soClient, apiKeys);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an await here to prevent unhandled rejection and match usage elsewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

nice, I think the removal of await was unintentional

}
// Update the necessary agents
return bulkUpdateAgents(
esClient,
agentsToUpdate.map((agent) => ({
agentId: agent.id,
data: {
active: false,
unenrolled_at: now,
},
}))
);
} else {
// Create unenroll action for each agent
await bulkCreateAgentActions(
soClient,
esClient,
agentsToUpdate.map((agent) => ({
agent_id: agent.id,
created_at: now,
type: 'UNENROLL',
}))
);

// Update the necessary agents
return bulkUpdateAgents(
esClient,
agentsToUpdate.map((agent) => ({
agentId: agent.id,
data: {
unenrollment_started_at: now,
},
}))
);
// Update the necessary agents
return bulkUpdateAgents(
esClient,
agentsToUpdate.map((agent) => ({
agentId: agent.id,
data: {
unenrollment_started_at: now,
},
}))
);
}
}

export async function forceUnenrollAgent(
Expand All @@ -118,41 +148,3 @@ export async function forceUnenrollAgent(
unenrolled_at: new Date().toISOString(),
});
}

export async function forceUnenrollAgents(
soClient: SavedObjectsClientContract,
esClient: ElasticsearchClient,
options: GetAgentsOptions
) {
// Filter to agents that are not already unenrolled
const agents = await getAgents(esClient, options);
const agentsToUpdate = agents.filter((agent) => !agent.unenrolled_at);
const now = new Date().toISOString();
const apiKeys: string[] = [];

// Get all API keys that need to be invalidated
agentsToUpdate.forEach((agent) => {
if (agent.access_api_key_id) {
apiKeys.push(agent.access_api_key_id);
}
if (agent.default_api_key_id) {
apiKeys.push(agent.default_api_key_id);
}
});

// Invalidate all API keys
if (apiKeys.length) {
APIKeyService.invalidateAPIKeys(soClient, apiKeys);
}
// Update the necessary agents
return bulkUpdateAgents(
esClient,
agentsToUpdate.map((agent) => ({
agentId: agent.id,
data: {
active: false,
unenrolled_at: now,
},
}))
);
}
7 changes: 3 additions & 4 deletions x-pack/test/fleet_api_integration/apis/agents/unenroll.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,10 @@ export default function (providerContext: FtrProviderContext) {
skipIfNoDockerRegistry(providerContext);
let accessAPIKeyId: string;
let outputAPIKeyId: string;
before(async () => {
await esArchiver.load('fleet/agents');
});
setupFleetAndAgents(providerContext);
beforeEach(async () => {
await esArchiver.load('fleet/agents');

const { body: accessAPIKeyBody } = await esClient.security.createApiKey({
body: {
name: `test access api key: ${uuid.v4()}`,
Expand Down Expand Up @@ -61,7 +60,7 @@ export default function (providerContext: FtrProviderContext) {
},
});
});
after(async () => {
afterEach(async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the cleanup to each it, instead of only describe. Many of the tests are destructive and can interfere with later tests which makes them flaky. I believe each is meant to be self-contained.

Copy link
Member

Choose a reason for hiding this comment

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

You may still have to have a before and after that load empty fleet server fixtures as the setupFleetAndAgents will probably mess the .fleet* indices otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add them both back, but do you know of any way for us to test/confirm? If they pass CI should we leave them out or would you still prefer to restore them?

Copy link
Member

Choose a reason for hiding this comment

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

if CI pass leave them out :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While developing, this test was failing it wouldn't remove 2 agents which were enrolled in a managed policy earlier. That's why I added the reset between tests. However, CI kept failing in the before each for that test with a ResponseError: invalid_alias_name_exception.

I reverted the changes and CI passed 🤷🏻 🎉

await esArchiver.unload('fleet/agents');
});

Expand Down