Skip to content

Commit

Permalink
[UA] Remove delete from .tasks permission check (#203379)
Browse files Browse the repository at this point in the history
## Summary

* Removes `delete` permission check on `.tasks`
* Task doc deletion best effort
* Creates a `warning` level log if deletion from `.tasks` fails like:
```
[2024-12-09T10:50:28.398+01:00][WARN ][plugins.upgradeAssistant.reindex_worker] ResponseError: security_exception
log.ts:66
	Root causes:
log.ts:66
		security_exception: action [indices:data/write/bulk[s]] is unauthorized for API key id [___] of user [elastic] on restricted indices [.tasks], this action is granted by the index privileges [create_doc,create,delete,index,write,all]
```

## How to test

1. Follow [these
steps](elastic/kibana-team#1249 (comment)),
but instead of creating a data stream create an index in 7.x
2. Start ES on v8.x
3. Checkout Kibana `8.x` locally and apply [the
diff](https://patch-diff.githubusercontent.com/raw/elastic/kibana/pull/203379.diff)
from this branch
4. Start Kibana
5. Log in as `elastic` or some other admin/superuser
6. Go to UA and reindex the index you created

## Resources

### Outdated 7.x guidance


https://www.elastic.co/guide/en/elasticsearch/reference/7.17/docs-update-by-query.html#docs-update-by-query-task-api

> When you are done with a task, you should delete the task document so
Elasticsearch can reclaim the space.
  • Loading branch information
jloleysens authored Dec 9, 2024
1 parent 7f1d436 commit 1f09537
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { esIndicesStateCheck } from '../es_indices_state_check';
import { versionService } from '../version';

import { ReindexService, reindexServiceFactory } from './reindex_service';
import { error } from './error';

const asApiResponse = <T>(body: T): TransportResult<T> =>
({
Expand Down Expand Up @@ -111,7 +112,7 @@ describe('reindexService', () => {
},
{
names: ['.tasks'],
privileges: ['read', 'delete'],
privileges: ['read'],
},
],
},
Expand Down Expand Up @@ -141,7 +142,7 @@ describe('reindexService', () => {
},
{
names: ['.tasks'],
privileges: ['read', 'delete'],
privileges: ['read'],
},
],
},
Expand Down Expand Up @@ -611,11 +612,11 @@ describe('reindexService', () => {
});
});

it('fails if docs created is less than count in source index', async () => {
it('does not throw if task doc deletion returns a bad result', async () => {
clusterClient.asCurrentUser.tasks.get.mockResponseOnce({
completed: true,
// @ts-expect-error not full interface
task: { status: { created: 95, total: 95 } },
task: { status: { created: 100, total: 100 } },
});

clusterClient.asCurrentUser.count.mockResponseOnce(
Expand All @@ -625,11 +626,51 @@ describe('reindexService', () => {
}
);

clusterClient.asCurrentUser.delete.mockResponseOnce({
// @ts-expect-error not known result
result: '!?',
});

const updatedOp = await service.processNextStep(reindexOp);
expect(updatedOp.attributes.lastCompletedStep).toEqual(ReindexStep.reindexStarted);
expect(updatedOp.attributes.status).toEqual(ReindexStatus.failed);
expect(updatedOp.attributes.errorMessage).not.toBeNull();
expect(log.error).toHaveBeenCalledWith(expect.any(String));
expect(updatedOp.attributes.lastCompletedStep).toEqual(ReindexStep.reindexCompleted);
expect(updatedOp.attributes.reindexTaskPercComplete).toEqual(1);
expect(clusterClient.asCurrentUser.delete).toHaveBeenCalledWith({
index: '.tasks',
id: 'xyz',
});
expect(log.warn).toHaveBeenCalledTimes(1);
expect(log.warn).toHaveBeenCalledWith(
error.reindexTaskCannotBeDeleted(
`Could not delete reindexing task xyz, got response "!?"`
)
);
});

it('does not throw if task doc deletion throws', async () => {
clusterClient.asCurrentUser.tasks.get.mockResponseOnce({
completed: true,
// @ts-expect-error not full interface
task: { status: { created: 100, total: 100 } },
});

clusterClient.asCurrentUser.count.mockResponseOnce(
// @ts-expect-error not full interface
{
count: 100,
}
);

clusterClient.asCurrentUser.delete.mockRejectedValue(new Error('FAILED!'));

const updatedOp = await service.processNextStep(reindexOp);
expect(updatedOp.attributes.lastCompletedStep).toEqual(ReindexStep.reindexCompleted);
expect(updatedOp.attributes.reindexTaskPercComplete).toEqual(1);
expect(clusterClient.asCurrentUser.delete).toHaveBeenCalledWith({
index: '.tasks',
id: 'xyz',
});
expect(log.warn).toHaveBeenCalledTimes(1);
expect(log.warn).toHaveBeenCalledWith(new Error('FAILED!'));
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,14 +308,21 @@ export const reindexServiceFactory = (
});
}

// Delete the task from ES .tasks index
const deleteTaskResp = await esClient.delete({
index: '.tasks',
id: taskId,
});

if (deleteTaskResp.result !== 'deleted') {
throw error.reindexTaskCannotBeDeleted(`Could not delete reindexing task ${taskId}`);
try {
// Delete the task from ES .tasks index
const deleteTaskResp = await esClient.delete({
index: '.tasks',
id: taskId,
});
if (deleteTaskResp.result !== 'deleted') {
log.warn(
error.reindexTaskCannotBeDeleted(
`Could not delete reindexing task ${taskId}, got response "${deleteTaskResp.result}"`
)
);
}
} catch (e) {
log.warn(e);
}

return reindexOp;
Expand Down Expand Up @@ -396,24 +403,21 @@ export const reindexServiceFactory = (
names.push(sourceName);
}

// Otherwise, query for required privileges for this index.
const body = {
cluster: ['manage'],
index: [
{
names,
allow_restricted_indices: true,
privileges: ['all'],
},
{
names: ['.tasks'],
privileges: ['read', 'delete'],
},
],
} as any;

const resp = await esClient.security.hasPrivileges({
body,
body: {
cluster: ['manage'],
index: [
{
names,
allow_restricted_indices: true,
privileges: ['all'],
},
{
names: ['.tasks'],
privileges: ['read'],
},
],
},
});

return resp.has_all_requested;
Expand Down

0 comments on commit 1f09537

Please sign in to comment.