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

Use updated onPreAuth from Platform #71552

Merged
merged 7 commits into from
Jul 14, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions x-pack/plugins/ingest_manager/common/constants/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ export const PACKAGE_CONFIG_API_ROOT = `${API_ROOT}/package_configs`;
export const AGENT_CONFIG_API_ROOT = `${API_ROOT}/agent_configs`;
export const FLEET_API_ROOT = `${API_ROOT}/fleet`;

export const LIMITED_CONCURRENCY_ROUTE_TAG = 'ingest:limited-concurrency';

// EPM API routes
const EPM_PACKAGES_MANY = `${EPM_API_ROOT}/packages`;
const EPM_PACKAGES_ONE = `${EPM_PACKAGES_MANY}/{pkgkey}`;
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/ingest_manager/server/constants/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export {
AGENT_UPDATE_ACTIONS_INTERVAL_MS,
INDEX_PATTERN_PLACEHOLDER_SUFFIX,
// Routes
LIMITED_CONCURRENCY_ROUTE_TAG,
PLUGIN_ID,
EPM_API_ROUTES,
DATA_STREAM_API_ROUTES,
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/ingest_manager/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
} from './constants';
import { registerSavedObjects, registerEncryptedSavedObjects } from './saved_objects';
import {
preAuthHandler,
registerEPMRoutes,
registerPackageConfigRoutes,
registerDataStreamRoutes,
Expand Down Expand Up @@ -231,6 +232,9 @@ export class IngestManagerPlugin
);
}
} else {
// we currently only use this global interceptor if fleet is enabled
// since it would run this func on *every* req (other plugins, CSS, etc)
this.httpSetup.registerOnPreAuth(preAuthHandler);
jfsiii marked this conversation as resolved.
Show resolved Hide resolved
registerAgentRoutes(router);
registerEnrollmentApiKeyRoutes(router);
registerInstallScriptRoutes({
Expand Down
8 changes: 4 additions & 4 deletions x-pack/plugins/ingest_manager/server/routes/agent/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*/

import { IRouter } from 'src/core/server';
import { PLUGIN_ID, AGENT_API_ROUTES } from '../../constants';
import { PLUGIN_ID, AGENT_API_ROUTES, LIMITED_CONCURRENCY_ROUTE_TAG } from '../../constants';
import {
GetAgentsRequestSchema,
GetOneAgentRequestSchema,
Expand Down Expand Up @@ -85,7 +85,7 @@ export const registerRoutes = (router: IRouter) => {
{
path: AGENT_API_ROUTES.CHECKIN_PATTERN,
validate: PostAgentCheckinRequestSchema,
options: { tags: [] },
options: { tags: [LIMITED_CONCURRENCY_ROUTE_TAG] },
},
postAgentCheckinHandler
);
Expand All @@ -95,7 +95,7 @@ export const registerRoutes = (router: IRouter) => {
{
path: AGENT_API_ROUTES.ENROLL_PATTERN,
validate: PostAgentEnrollRequestSchema,
options: { tags: [] },
options: { tags: [LIMITED_CONCURRENCY_ROUTE_TAG] },
},
postAgentEnrollHandler
);
Expand All @@ -105,7 +105,7 @@ export const registerRoutes = (router: IRouter) => {
{
path: AGENT_API_ROUTES.ACKS_PATTERN,
validate: PostAgentAcksRequestSchema,
options: { tags: [] },
options: { tags: [LIMITED_CONCURRENCY_ROUTE_TAG] },
},
postAgentAcksHandlerBuilder({
acknowledgeAgentActions: AgentService.acknowledgeAgentActions,
Expand Down
63 changes: 63 additions & 0 deletions x-pack/plugins/ingest_manager/server/routes/global_interceptors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { KibanaRequest, LifecycleResponseFactory, OnPreAuthToolkit } from 'kibana/server';
import { LIMITED_CONCURRENCY_ROUTE_TAG } from '../../common';

class MaxCounter {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can handle this any number of ways. I just chose something that allowed the logic to be defined outside the route handler

constructor(private readonly max: number = 1) {}
private counter = 0;
valueOf() {
return this.counter;
}
increase() {
if (this.counter < this.max) {
this.counter += 1;
}
}
decrease() {
this.counter += 1;
}
lessThanMax() {
return this.counter < this.max;
}
}

function shouldHandleRequest(request: KibanaRequest) {
const tags = request.route.options.tags;
return tags.includes(LIMITED_CONCURRENCY_ROUTE_TAG);
}

const LIMITED_CONCURRENCY_MAX_REQUESTS = 250;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should this be configurable? Do we expose as a config flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

++ to having it as a config option, it fits nicely under xpack.ingestManager.fleet

the default value of 250 seems low to me, but I'm not up to date on our load testing results

Copy link
Contributor

Choose a reason for hiding this comment

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

This indeed seems low, how did you get to that number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolving this since we've switched to 0 (off) by default

const counter = new MaxCounter(LIMITED_CONCURRENCY_MAX_REQUESTS);

export function preAuthHandler(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I test this handler? If so, can someone point me in the right direction :) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

for testing, maybe looking at routes/package_config/handlers.test.ts would be helpful? it has usage of httpServerMock.createKibanaRequest for creating a request object, that method has options for creating it with tags, etc.

then I think you could mock LIMITED_CONCURRENCY_MAX_REQUESTS to 1 and mock toolkit.next() to resolve after waiting for a second. then kicking off multiple calls to the handler inside a Promise.all should return an array of of responses whose status codes are [200, 503, 503, ...]

Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to name this something Fleet or agent-related to emphasize that this handler is for those routes

request: KibanaRequest,
response: LifecycleResponseFactory,
toolkit: OnPreAuthToolkit
) {
if (!shouldHandleRequest(request)) {
return toolkit.next();
}

if (!counter.lessThanMax()) {
return response.customError({
body: 'Too Many Agents',
statusCode: 503,
headers: {
'Retry-After': '30',
},
});
}

counter.increase();

// requests.events.aborted$ has a bug where it's fired even when the request completes...
Copy link
Contributor

Choose a reason for hiding this comment

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

how can we decrease the counter if/when the bug is fixed? 🤔

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 some discussion about this starting in #70495 (comment)

It's a "bug" but there is a test to ensure it works. There's also a possibility Platform will add a request.events.completed$ which we can use instead

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a "bug" but there is a test to ensure it works. There's also a possibility Platform will add a request.events.completed$ which we can use instead

This is entirely pedantic, it's not really a bug as much as an "implementation detail" per #70495 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// we can take advantage of this bug just for load testing...
request.events.aborted$.toPromise().then(() => counter.decrease());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the comment says this decrements the counter when a request completes. However, /checkin is using long-polling so I don't think the request "completes" in the same way.

As @roncohen mentions in (3) in #71552 (comment), I think the counter will increment with each /checkin but not decrement.

It seems like we could revert to using core.http.registerOnPreResponse to decrement during a checkin response , but the reason we moved away from that was it missed aborted/failed connections #70495 (comment) and as the first line says requests.events.aborted$ has a bug where it's fired even when the request completes so that will result in double-decrementing.

Perhaps we could do some checking in both the response and aborted handler to see if theres a 429/503 like the initial POC https://github.com/elastic/kibana/pull/70495/files#diff-f3ee51f84520a4eb03ca041ff4c3d9a2R182

cc @kobelb @nchaulet

Copy link
Contributor

@roncohen roncohen Jul 14, 2020

Choose a reason for hiding this comment

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

@jfsiii I think we need to do the decrementing inside the check-in route when the actual work of checking in the agent has completed and the request transitions to long polling mode. Could the interceptor augment the context passed to the route such that the route can decrement when it sees fit? And then we need to ensure that a given request can only be decremented once, so it doesn't get decremented again when the response finally gets send back.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @roncohen here, I think the behavior is to "protect from multiple connection" until enough agents have transitioned to long polling where their usage become low.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could the interceptor augment the context passed to the route such that the route can decrement when it sees fit?

I don't believe that it can at the moment... I think we have two options here

  1. Don't use the pre-auth handler to reply with a 429 for these long-polling routes. If we're doing all of the rate-limiting within the route handler itself, this becomes possible without any changes from the Kibana platform.
  2. Make changes to the Kibana platform to allow us to correlate the request seen in the pre-auth handler to the request within the route handler and the other lifecycle methods. This would allow us to only decrement the counter once by using a WeakSet. @joshdover, is there some other primitive or concept within the Kibana platform that we could use to fulfill this behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

2. is there some other primitive or concept within the Kibana platform that we could use to fulfill this behavior?

Not that is exposed currently. I am working on adding support for an id property on KibanaRequest that would be stable across lifecycle methods (#71019), but that will not make it for 7.9. I'm also not sure it would be safe to use for this use-case because the id could come from a proxy that is sending an X-Opaque-Id header which we can't guarantee will be unique? Maybe we need an internal ID that is unique that we can rely on?

We do keep a reference to the original Hapi request on the KibanaRequest object but it's been very purposefully hidden so that plugins can't abuse it.


return toolkit.next();
}
1 change: 1 addition & 0 deletions x-pack/plugins/ingest_manager/server/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ export { registerRoutes as registerInstallScriptRoutes } from './install_script'
export { registerRoutes as registerOutputRoutes } from './output';
export { registerRoutes as registerSettingsRoutes } from './settings';
export { registerRoutes as registerAppRoutes } from './app';
export { preAuthHandler } from './global_interceptors';