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.

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

[7.9] [Ingest Manager] Don't retain POST /setup results. fixes #74587 (#75372) #75587

Merged
merged 2 commits into from
Aug 22, 2020

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Aug 20, 2020

Backports the following commits to 7.9:

elastic#75372)

* add retries for registry requests.

works, afaict. no tests. one TS issue.

* Fix TS issue. Add link to node-fetch error docs

* Restore some accidentally deleted code.

* Add more comments. Remove logging.

* Add tests for plugin setup service & handlers

* Add tests for Registry retry logic

* Extract setup retry logic to separate function/file

* Add tests for setup retry logic

```
  firstSuccessOrTryAgain
    ✓ reject/throws is called again & its value returned (18ms)
    ✓ the first success value is cached (2ms)
```

* More straightforward(?) tests for setup caching

* Revert cached setup. Still limit 1 call at a time

Terrible tests. Committing & pushing to see if it fixes failures like https://github.com/elastic/kibana/pull/74507/checks?check_run_id=980178887

https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/67892/execution/node/663/log/

```
07:36:56               └-> "before all" hook
07:36:56               └-> should not allow to enroll an agent with a invalid enrollment
07:36:56                 └-> "before each" hook: global before each
07:36:56                 └-> "before each" hook: beforeSetupWithDockerRegistry
07:36:56                 │ proc [kibana]  error  [11:36:56.369]  Error: Internal Server Error
07:36:56                 │ proc [kibana]     at HapiResponseAdapter.toError (/dev/shm/workspace/parallel/5/kibana/build/kibana-build-xpack/src/core/server/http/router/response_adapter.js:132:19)
07:36:56                 │ proc [kibana]     at HapiResponseAdapter.toHapiResponse (/dev/shm/workspace/parallel/5/kibana/build/kibana-build-xpack/src/core/server/http/router/response_adapter.js:86:19)
07:36:56                 │ proc [kibana]     at HapiResponseAdapter.handle (/dev/shm/workspace/parallel/5/kibana/build/kibana-build-xpack/src/core/server/http/router/response_adapter.js:81:17)
07:36:56                 │ proc [kibana]     at Router.handle (/dev/shm/workspace/parallel/5/kibana/build/kibana-build-xpack/src/core/server/http/router/router.js:164:34)
07:36:56                 │ proc [kibana]     at process._tickCallback (internal/process/next_tick.js:68:7)
07:36:56                 │ proc [kibana]   log   [11:36:56.581] [info][authentication][plugins][security] Authentication attempt failed: [security_exception] missing authentication credentials for REST request [/_security/_authenticate], with { header={ WWW-Authenticate={ 0="ApiKey" & 1="Basic realm=\"security\" charset=\"UTF-8\"" } } }
07:36:56                 └- ✓ pass  (60ms) "Ingest Manager Endpoints Fleet Endpoints fleet_agents_enroll should not allow to enroll an agent with a invalid enrollment"
07:36:56               └-> should not allow to enroll an agent with a shared id if it already exists
07:36:56                 └-> "before each" hook: global before each
07:36:56                 └-> "before each" hook: beforeSetupWithDockerRegistry
07:36:56                 └- ✓ pass  (111ms) "Ingest Manager Endpoints Fleet Endpoints fleet_agents_enroll should not allow to enroll an agent with a shared id if it already exists "
07:36:56               └-> should not allow to enroll an agent with a version > kibana
07:36:56                 └-> "before each" hook: global before each
07:36:56                 └-> "before each" hook: beforeSetupWithDockerRegistry
07:36:56                 └- ✓ pass  (58ms) "Ingest Manager Endpoints Fleet Endpoints fleet_agents_enroll should not allow to enroll an agent with a version > kibana"
07:36:56               └-> should allow to enroll an agent with a valid enrollment token
07:36:56                 └-> "before each" hook: global before each
07:36:56                 └-> "before each" hook: beforeSetupWithDockerRegistry
07:36:56                 └- ✖ fail: Ingest Manager Endpoints Fleet Endpoints fleet_agents_enroll should allow to enroll an agent with a valid enrollment token
07:36:56                 │      Error: expected 200 "OK", got 500 "Internal Server Error"
07:36:56                 │       at Test._assertStatus (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:268:12)
07:36:56                 │       at Test._assertFunction (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:283:11)
07:36:56                 │       at Test.assert (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:173:18)
07:36:56                 │       at assert (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:131:12)
07:36:56                 │       at /dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:128:5
07:36:56                 │       at Test.Request.callback (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/index.js:718:3)
07:36:56                 │       at parser (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/index.js:906:18)
07:36:56                 │       at IncomingMessage.res.on (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/parsers/json.js:19:7)
07:36:56                 │       at endReadableNT (_stream_readable.js:1145:12)
07:36:56                 │       at process._tickCallback (internal/process/next_tick.js:63:19)
07:36:56                 │
07:36:56                 │
```

* New name & tests for one-at-a-time /setup behavior

`firstPromiseBlocksAndFufills` for "the first promise created blocks others from being created, then fufills all with that first result"

* More (better?) renaming

* Fix name in test description

* Fix spelling typo.

* Remove registry retry code & tests

* Use async fn's .catch to avoid unhandled rejection

Add explicit `isPending` value instead of overloading role of `status`. Could probably do without it, but it makes the intent more clear.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/ingest_manager/server/services/setup.ts
@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 20, 2020

@elasticmachine merge upstream

@jfsiii jfsiii requested a review from ruflin August 20, 2020 22:55
@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 20, 2020

/cc @jen-huang I think I did this correctly. I use the awaitIfPending & createSetupSideEffects from master/7.x but kept the 7.9 variables/types

import { agentConfigService } from './agent_config';
import { outputService } from './output';
import { ensureInstalledDefaultPackages } from './epm/packages/install';
import { ensureDefaultIndices } from './epm/kibana/index_pattern/install';
import {
packageToPackageConfig,
PackageConfig,
AgentConfig,
Installation,
Output,
DEFAULT_AGENT_CONFIGS_PACKAGES,
decodeCloudId,
} from '../../common';
import { getPackageInfo } from './epm/packages';
import { packageConfigService } from './package_config';

async function createSetupSideEffects(
soClient: SavedObjectsClientContract,
callCluster: CallESAsCurrentUser
): Promise<SetupStatus> {
const [installedPackages, defaultOutput, config] = await Promise.all([
// packages installed by default
ensureInstalledDefaultPackages(soClient, callCluster),
outputService.ensureDefaultOutput(soClient),
agentConfigService.ensureDefaultAgentConfig(soClient),
ensureDefaultIndices(callCluster),
settingsService.getSettings(soClient).catch((e: any) => {
if (e.isBoom && e.output.statusCode === 404) {
const http = appContextService.getHttpSetup();
const serverInfo = http.getServerInfo();
const basePath = http.basePath;
const cloud = appContextService.getCloud();
const cloudId = cloud?.isCloudEnabled && cloud.cloudId;
const cloudUrl = cloudId && decodeCloudId(cloudId)?.kibanaUrl;
const flagsUrl = appContextService.getConfig()?.fleet?.kibana?.host;
const defaultUrl = url.format({
protocol: serverInfo.protocol,
hostname: serverInfo.hostname,
port: serverInfo.port,
pathname: basePath.serverBasePath,
});
return settingsService.saveSettings(soClient, {
agent_auto_upgrade: true,
package_auto_upgrade: true,
kibana_url: cloudUrl || flagsUrl || defaultUrl,
});
}
return Promise.reject(e);
}),
]);
// ensure default packages are added to the default conifg
const configWithPackageConfigs = await agentConfigService.get(soClient, config.id, true);
if (!configWithPackageConfigs) {
throw new Error('Config not found');
}
if (
configWithPackageConfigs.package_configs.length &&
typeof configWithPackageConfigs.package_configs[0] === 'string'
) {
throw new Error('Config not found');
}
for (const installedPackage of installedPackages) {
const packageShouldBeInstalled = DEFAULT_AGENT_CONFIGS_PACKAGES.some(
(packageName) => installedPackage.name === packageName
);
if (!packageShouldBeInstalled) {
continue;
}
const isInstalled = configWithPackageConfigs.package_configs.some(
(d: PackageConfig | string) => {
return typeof d !== 'string' && d.package?.name === installedPackage.name;
}
);
if (!isInstalled) {
await addPackageToConfig(
soClient,
callCluster,
installedPackage,
configWithPackageConfigs,
defaultOutput
);
}
}
return { isIntialized: true };
}

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

I tested this locally using repro steps from #74587 and things look good 👍🏻

Code looks like it was backported correctly too. The renaming changes into master/7.x made 7.9 backports more difficult as manual updating is needed, so thanks for catching everything.

@jfsiii jfsiii merged commit e663d79 into elastic:7.9 Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants