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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Fleet] Add retry logic to serverless API check #176808

Merged
merged 13 commits into from
Feb 21, 2024
Merged

[Fleet] Add retry logic to serverless API check #176808

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
07e238f
[Fleet] Add retry logic to serverless API check
jillguyonnet Feb 13, 2024
ecd5949
Add retry logic to default output
jillguyonnet Feb 13, 2024
6c47b47
Troubleshooting
jillguyonnet Feb 13, 2024
e7e3d34
Try adding delay between retries
jillguyonnet Feb 13, 2024
2854b8b
Even more troubleshooting
jillguyonnet Feb 14, 2024
5d0e687
Skip test
jillguyonnet Feb 14, 2024
c9e24b7
More and more troubleshooting
jillguyonnet Feb 14, 2024
6e658f9
WIP
jillguyonnet Feb 15, 2024
1565cd3
Add retrying in tests too
jillguyonnet Feb 15, 2024
cfe7518
Remove manual retrying
jillguyonnet Feb 19, 2024
4a6aeca
Revert output logic change
jillguyonnet Feb 20, 2024
0171160
Extract Fleet setup
jillguyonnet Feb 20, 2024
bfecf00
Fix unit test
jillguyonnet Feb 20, 2024
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
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/server/routes/fleet_server_hosts/handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ async function checkFleetServerHostsWriteAPIsAllowed(
return;
}

// Fleet Server hosts must have the default host URL in serverless.
const serverlessDefaultFleetServerHost = await getFleetServerHost(
soClient,
SERVERLESS_DEFAULT_FLEET_SERVER_HOST_ID
Expand Down
24 changes: 17 additions & 7 deletions x-pack/plugins/fleet/server/routes/output/handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,21 +166,31 @@ async function validateOutputServerless(
if (!cloudSetup?.isServerlessEnabled) {
return;
}

if (output.type === outputType.RemoteElasticsearch) {
throw Boom.badRequest('Output type remote_elasticsearch not supported in serverless');
}

// Elasticsearch outputs must have the default host URL in serverless.
// No need to validate on update if hosts are not passed.
if (outputId && !output.hosts) {
// No need to validate for other types.
if (output.type !== outputType.Elasticsearch) {
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 type here could be undefined as our PUT are in fact PATCH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks. I've modified the unit tests slightly to test that update works as expected when the output type is not passed.

return;
}
const defaultOutput = await outputService.get(soClient, SERVERLESS_DEFAULT_OUTPUT_ID);
let originalOutput;

if (outputId) {
originalOutput = await outputService.get(soClient, outputId);
// No need to validate on update if hosts are not passed.
if (!output.hosts) {
return;
}
const originalOutput = await outputService.get(soClient, outputId);
// No need to validate on update if updating to another type.
if (originalOutput.type && originalOutput.type !== outputType.Elasticsearch) {
return;
}
}
const type = output.type || originalOutput?.type;
if (type === outputType.Elasticsearch && !isEqual(output.hosts, defaultOutput.hosts)) {

const defaultOutput = await outputService.get(soClient, SERVERLESS_DEFAULT_OUTPUT_ID);
if (!isEqual(output.hosts, defaultOutput.hosts)) {
throw Boom.badRequest(
`Elasticsearch output host must have default URL in serverless: ${defaultOutput.hosts}`
);
Expand Down
54 changes: 47 additions & 7 deletions x-pack/test_serverless/api_integration/test_suites/observability/fleet/fleet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,45 @@ import { FtrProviderContext } from '../../../ftr_provider_context';
export default function ({ getService }: FtrProviderContext) {
const svlCommonApi = getService('svlCommonApi');
const supertest = getService('supertest');
const retry = getService('retry');

// FLAKY: https://github.com/elastic/kibana/issues/176858
describe.skip('fleet', function () {
const defaultFleetServerHostId = 'default-fleet-server';
const defaultFleetServerHostUrl = 'https://localhost:8220';
const defaultElasticsearchOutputId = 'es-default-output';
const defaultElasticsearchOutputHostUrl = 'https://localhost:9200';

async function expectDefaultFleetServer() {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to move expectDefaultFleetServer and expectDefaultElasticsearchOutput to a helper to avoid duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not super easy to read IMHO because of the different details (e.g. body.item.host_urls vs. body.item.hosts) 馃槄 maybe something like:

async function expectDefault({ name, endpoint, defaultId, hostField, defaultHostUrl }) {
    await retry.waitForWithTimeout(`get default ${name}`, 30_000, async () => {
      const { body, status } = await supertest.get(`/api/fleet/${endpoint}/${defaultId}`);
      if (status === 200 && body.item[hostField].includes(defaultHostUrl)) {
        return true;
      } else {
        throw new Error(`Expected default ${name} id ${defaultId} to exist`);
      }
    });
  }

and

const fleetServer = {
    name: 'Fleet Server',
    endpoint: 'fleet_server_hosts',
    defaultId: 'default-fleet-server',
    hostField: 'host_urls',
    defaultHostUrl: 'https://localhost:8220',
  };

Any thoughts or recommendation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't mean joining the 2 functions, but the duplication in observability/fleet and security/fleet.
The suggested refactor could make sense too, though not really necessary, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, that makes a lot more sense 馃檲 I wasn't sure as these two tests are identical anyway, but I tried to extract this and part of the config as they are linked. Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great 馃憤

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for suggesting 馃憤 I've re-run the flaky test runner for good measure, all green: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5237

await retry.waitForWithTimeout('get default fleet server', 30_000, async () => {
const { body, status } = await supertest.get(
`/api/fleet/fleet_server_hosts/${defaultFleetServerHostId}`
);
if (status === 200 && body.item.host_urls.includes(defaultFleetServerHostUrl)) {
return true;
} else {
throw new Error(`Expected default Fleet Server id ${defaultFleetServerHostId} to exist`);
}
});
}

async function expectDefaultElasticsearchOutput() {
await retry.waitForWithTimeout('get default Elasticsearch output', 30_000, async () => {
const { body, status } = await supertest.get(
`/api/fleet/outputs/${defaultElasticsearchOutputId}`
);
if (status === 200 && body.item.hosts.includes(defaultElasticsearchOutputHostUrl)) {
return true;
} else {
throw new Error(
`Expected default Elasticsearch output id ${defaultElasticsearchOutputId} to exist`
);
}
});
}

describe('fleet', function () {
it('rejects request to create a new fleet server hosts if host url is different from default', async () => {
await expectDefaultFleetServer();

const { body, status } = await supertest
.post('/api/fleet/fleet_server_hosts')
.set(svlCommonApi.getInternalRequestHeader())
Expand All @@ -27,12 +62,14 @@ export default function ({ getService }: FtrProviderContext) {
expect(body).toEqual({
statusCode: 403,
error: 'Forbidden',
message: 'Fleet server host must have default URL in serverless: https://localhost:8220',
message: `Fleet server host must have default URL in serverless: ${defaultFleetServerHostUrl}`,
});
expect(status).toBe(403);
});

it('accepts request to create a new fleet server hosts if host url is same as default', async () => {
await expectDefaultFleetServer();

const { body, status } = await supertest
.post('/api/fleet/fleet_server_hosts')
.set(svlCommonApi.getInternalRequestHeader())
Expand All @@ -44,13 +81,15 @@ export default function ({ getService }: FtrProviderContext) {
expect(body).toEqual({
item: expect.objectContaining({
name: 'Test Fleet server host',
host_urls: ['https://localhost:8220'],
host_urls: [defaultFleetServerHostUrl],
}),
});
expect(status).toBe(200);
});

it('rejects request to create a new elasticsearch output if host is different from default', async () => {
await expectDefaultElasticsearchOutput();

const { body, status } = await supertest
.post('/api/fleet/outputs')
.set(svlCommonApi.getInternalRequestHeader())
Expand All @@ -64,13 +103,14 @@ export default function ({ getService }: FtrProviderContext) {
expect(body).toEqual({
statusCode: 400,
error: 'Bad Request',
message:
'Elasticsearch output host must have default URL in serverless: https://localhost:9200',
message: `Elasticsearch output host must have default URL in serverless: ${defaultElasticsearchOutputHostUrl}`,
});
expect(status).toBe(400);
});

it('accepts request to create a new elasticsearch output if host url is same as default', async () => {
await expectDefaultElasticsearchOutput();

const { body, status } = await supertest
.post('/api/fleet/outputs')
.set(svlCommonApi.getInternalRequestHeader())
Expand All @@ -83,7 +123,7 @@ export default function ({ getService }: FtrProviderContext) {
expect(body).toEqual({
item: expect.objectContaining({
name: 'Test output',
hosts: ['https://localhost:9200'],
hosts: [defaultElasticsearchOutputHostUrl],
}),
});
expect(status).toBe(200);
Expand Down
51 changes: 46 additions & 5 deletions x-pack/test_serverless/api_integration/test_suites/security/fleet/fleet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,46 @@ import { FtrProviderContext } from '../../../ftr_provider_context';
export default function ({ getService }: FtrProviderContext) {
const svlCommonApi = getService('svlCommonApi');
const supertest = getService('supertest');
const retry = getService('retry');

const defaultFleetServerHostId = 'default-fleet-server';
const defaultFleetServerHostUrl = 'https://localhost:8220';
const defaultElasticsearchOutputId = 'es-default-output';
const defaultElasticsearchOutputHostUrl = 'https://localhost:9200';

async function expectDefaultFleetServer() {
await retry.waitForWithTimeout('get default fleet server', 30_000, async () => {
const { body, status } = await supertest.get(
`/api/fleet/fleet_server_hosts/${defaultFleetServerHostId}`
);
if (status === 200 && body.item.host_urls.includes(defaultFleetServerHostUrl)) {
return true;
} else {
throw new Error(`Expected default Fleet Server id ${defaultFleetServerHostId} to exist`);
}
});
}

async function expectDefaultElasticsearchOutput() {
await retry.waitForWithTimeout('get default Elasticsearch output', 30_000, async () => {
const { body, status } = await supertest.get(
`/api/fleet/outputs/${defaultElasticsearchOutputId}`
);
if (status === 200 && body.item.hosts.includes(defaultElasticsearchOutputHostUrl)) {
return true;
} else {
throw new Error(
`Expected default Elasticsearch output id ${defaultElasticsearchOutputId} to exist`
);
}
});
}

// FLAKY: https://github.com/elastic/kibana/issues/176754
describe.skip('fleet', function () {
it('rejects request to create a new fleet server hosts if host url is different from default', async () => {
await expectDefaultFleetServer();

const { body, status } = await supertest
.post('/api/fleet/fleet_server_hosts')
.set(svlCommonApi.getInternalRequestHeader())
Expand All @@ -27,12 +63,14 @@ export default function ({ getService }: FtrProviderContext) {
expect(body).toEqual({
statusCode: 403,
error: 'Forbidden',
message: 'Fleet server host must have default URL in serverless: https://localhost:8220',
message: `Fleet server host must have default URL in serverless: ${defaultFleetServerHostUrl}`,
});
expect(status).toBe(403);
});

it('accepts request to create a new fleet server hosts if host url is same as default', async () => {
await expectDefaultFleetServer();

const { body, status } = await supertest
.post('/api/fleet/fleet_server_hosts')
.set(svlCommonApi.getInternalRequestHeader())
Expand All @@ -44,13 +82,15 @@ export default function ({ getService }: FtrProviderContext) {
expect(body).toEqual({
item: expect.objectContaining({
name: 'Test Fleet server host',
host_urls: ['https://localhost:8220'],
host_urls: [defaultFleetServerHostUrl],
}),
});
expect(status).toBe(200);
});

it('rejects request to create a new elasticsearch output if host is different from default', async () => {
await expectDefaultElasticsearchOutput();

const { body, status } = await supertest
.post('/api/fleet/outputs')
.set(svlCommonApi.getInternalRequestHeader())
Expand All @@ -64,13 +104,14 @@ export default function ({ getService }: FtrProviderContext) {
expect(body).toEqual({
statusCode: 400,
error: 'Bad Request',
message:
'Elasticsearch output host must have default URL in serverless: https://localhost:9200',
message: `Elasticsearch output host must have default URL in serverless: ${defaultElasticsearchOutputHostUrl}`,
});
expect(status).toBe(400);
});

it('accepts request to create a new elasticsearch output if host url is same as default', async () => {
await expectDefaultElasticsearchOutput();

const { body, status } = await supertest
.post('/api/fleet/outputs')
.set(svlCommonApi.getInternalRequestHeader())
Expand All @@ -83,7 +124,7 @@ export default function ({ getService }: FtrProviderContext) {
expect(body).toEqual({
item: expect.objectContaining({
name: 'Test output',
hosts: ['https://localhost:9200'],
hosts: [defaultElasticsearchOutputHostUrl],
}),
});
expect(status).toBe(200);
Expand Down