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

Native controllers #17496

Closed
wants to merge 113 commits into from
Closed

Native controllers #17496

wants to merge 113 commits into from

Conversation

kobelb
Copy link
Contributor

@kobelb kobelb commented Apr 2, 2018

Native Controllers

Implements system call filtering for Linux and Windows, we don't currently support this level of protection with macOS, which is fine because we don't support macOS as an official operating system. This can currently be disabled via server.systemCallFilters.enable: false in the kibana.yml. I left this as an undocumented setting for the time being, as the only current use is with in the unit tests.

Native controllers are currently the only supported way of running code outside of the context of the system call filters, and it's currently only implemented by Reporting.

When installing a plugin that has a native controller, the user will be warned with the following prompt before the install configures:

@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@        WARNING: plugin forks a native controller        @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
This plugin launches a native controller that is not subject
to the system call filters.

Continue with installation? [y/N]

When the Kibana server is starting up, we scan for plugins that contain a native controller and fork an instance of the src/server/native_controllers/native_controller_start.js that will require the actual native controller when it later receives the start IPC message. We then activate the system call filtering so no additional processes will be able to be spawned. The rest of the plugin discovery process then occurs, and once we are able to determine whether the plugins are enabled/disabled we either kill the native_controller_start.js process or send it the start message which loads the actual native controller code.

Reporting Native Controller

With the implementation of the system call filters, we're no longer able to spawn headless browsers from the Kibana server directly. Instead we have to implement a native controller that is responsible for spawning the browsers, and all communication must then go through the native controller.

Reporting's native_controller/index.js is loaded in a separate process when the Kibana server is initializing, and then we're handed the native controller process in the plugin's init method. The native controller itself responds to two types of messages: kill and spawn.

The native_controller_client is used from within the Kibana server process to communicate with the native controller and the headless browsers that it spawns. The SyntheticProcess provides the necessary interfaces that emulates a browser process, while multiplexing all communication with the actual browser process through the native controller over IPC.

We also pulled the @elastic/node-phantom-simple code into the source code as we needed to make some modifications so we could pass it an instance of the SyntheticProcess instead of having it spawn itself.

kobelb and others added 30 commits March 13, 2018 08:42
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kobelb
Copy link
Contributor Author

kobelb commented May 11, 2018

Jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kobelb
Copy link
Contributor Author

kobelb commented May 11, 2018

Jenkins, test this

2 passes never hurt anyone 🤞

// we have to spawn a process to actually run the test, or else we end up locking
// down the test runner process and not letting us spawn anything in sunsequent tests
test('sandboxes', async (done) => {
const cp = fork(path.resolve(__dirname, 'test_process.js'));
Copy link
Contributor

Choose a reason for hiding this comment

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

We remove certain test-related files globally from builds, like *.test.js and things under __tests__ directories. Since this test_process.js file doesn't follow any particular convention that we track, it's going to erroneously make its way into builds.

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, I'll rename it so it's caught by these.

// this can happen before we have the logging configured, so we log to both
// console.error and server.log to ensure something is output somewhere.
const message = `${nativeController.pluginId}'s native controller exited with code ${code}`;
console.error(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Under what conditions can this fire before logging is configured?

If we can determine that logging is configured, we should conditionally log to console.error based on whether server.log is available rather than always logging to console.error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the native controller logic immediately errors out, logging generally won't be configured, or if the path of the native controller is incorrect.

The issue is that kbnServer.server.log will be there immediately after https://github.com/elastic/kibana/blob/master/src/server/kbn_server.js#L39 but won't be configured properly until after
https://github.com/elastic/kibana/blob/master/src/server/kbn_server.js#L42 and HapiJS doesn't expose a way to see if it's been "configured"

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

triggerStarted();
});

test(`doesn't throw error when enabled plugin doesn't have a nativeController`, async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be broken down into more tests.

The current test is technically verifying a couple things, but the test name gives the impression that it only cares about one enabled plugin without a native controller, so a future refactor could end up leaving us in a place where the actual behaviors are not completely tested.

Ideally, we'll be testing three scenarios:

  1. No plugins are enabled
  2. One plugin is enabled but it doesn't have a native controller
  3. Multiple plugins are enabled and none of them have native controllers

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kobelb
Copy link
Contributor Author

kobelb commented May 22, 2018

This PR is on hold because of the impending merging of the "new platform". The plan forward is to implement native controllers as a component of the new platform, as having legacy and new platform plugins using the native controllers complicates the start-up process.

@epixa epixa added the blocked label May 24, 2018
@azasypkin
Copy link
Member

This PR is on hold because of the impending merging of the "new platform". The plan forward is to implement native controllers as a component of the new platform, as having legacy and new platform plugins using the native controllers complicates the start-up process.

Hey, #22190 landed a while ago, do you still need anything from the platform to move this forward?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@epixa
Copy link
Contributor

epixa commented Sep 25, 2018

We need new platform plugins as well so we can move the reporting browser stuff to new platform native controllers.

@azasypkin
Copy link
Member

Okay, got it, thanks

@spalger
Copy link
Contributor

spalger commented Sep 19, 2019

Please resubmit if you'd like

@spalger spalger closed this Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants