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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RAC] Add mapping update logic to RuleDataClient #102586

Merged
merged 21 commits into from
Jul 8, 2021
Merged

[RAC] Add mapping update logic to RuleDataClient #102586

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
4b78e40
Add component template versioning to RuleDataClient
marshallmain Jun 17, 2021
33461c1
Add versioning for index templates
marshallmain Jun 17, 2021
19b9eaf
Address PR comments, add error handling inside rolloverAliasIfNeeded
marshallmain Jun 21, 2021
3a1fe7c
Merge branch 'master' into rule-data-client-index-versioning
kibanamachine Jun 21, 2021
bbd8335
Fix security alerts index name, rollover func param, more robust roll…
marshallmain Jun 23, 2021
4ccf63a
Add empty mapping check to createOrUpdateIndexTemplate
marshallmain Jun 23, 2021
c9c7a87
Fix error path in createWriteTargetIfNeeded to suppress resource_alre…
marshallmain Jun 23, 2021
cd184c3
Merge branch 'rule-data-client-index-versioning' of github.com:marsha…
marshallmain Jun 23, 2021
a45a70a
Add code comments around rollover logic
marshallmain Jun 23, 2021
be0f777
Replace numeric versions with semver
marshallmain Jun 24, 2021
c8e2642
Use optional chaining operator to fetch current write index mapping
marshallmain Jun 24, 2021
7a4bc11
Fix template version number
marshallmain Jun 24, 2021
efcf928
Merge branch 'master' into rule-data-client-index-versioning
kibanamachine Jun 24, 2021
852a8b0
Move mapping updates to plugin startup, remove dependency on componen…
marshallmain Jul 1, 2021
bae03df
Undo changes to lifecycle and persistance rule type factories
marshallmain Jul 1, 2021
5e99679
Merge branch 'master' into rule-data-client-index-versioning
marshallmain Jul 1, 2021
0be60e1
Remove test code
marshallmain Jul 1, 2021
a87dd50
Simplify race mitigation logic
marshallmain Jul 1, 2021
88a4d9e
Remove outdated comment
marshallmain Jul 1, 2021
c1aaa5e
Merge branch 'master' into rule-data-client-index-versioning
kibanamachine Jul 6, 2021
d3f099a
Add unit tests, log unexpected errors instead of rethrowing
marshallmain Jul 6, 2021
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
64 changes: 36 additions & 28 deletions x-pack/plugins/apm/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,41 +136,49 @@ export class APMPlugin
}

await ruleDataService.createOrUpdateComponentTemplate({
name: componentTemplateName,
body: {
template: {
settings: {
number_of_shards: 1,
},
mappings: mappingFromFieldMap({
[SERVICE_NAME]: {
type: 'keyword',
},
[SERVICE_ENVIRONMENT]: {
type: 'keyword',
},
[TRANSACTION_TYPE]: {
type: 'keyword',
template: {
name: componentTemplateName,
body: {
template: {
settings: {
number_of_shards: 1,
},
[PROCESSOR_EVENT]: {
type: 'keyword',
},
}),
mappings: mappingFromFieldMap({
[SERVICE_NAME]: {
type: 'keyword',
},
[SERVICE_ENVIRONMENT]: {
type: 'keyword',
},
[TRANSACTION_TYPE]: {
type: 'keyword',
},
[PROCESSOR_EVENT]: {
type: 'keyword',
},
}),
},
},
},
templateVersion: 1,
});

await ruleDataService.createOrUpdateIndexTemplate({
name: ruleDataService.getFullAssetName('apm-index-template'),
body: {
index_patterns: [
ruleDataService.getFullAssetName('observability-apm*'),
],
composed_of: [
ruleDataService.getFullAssetName(TECHNICAL_COMPONENT_TEMPLATE_NAME),
componentTemplateName,
],
template: {
name: ruleDataService.getFullAssetName('apm-index-template'),
body: {
index_patterns: [
ruleDataService.getFullAssetName('observability-apm*'),
],
composed_of: [
ruleDataService.getFullAssetName(
TECHNICAL_COMPONENT_TEMPLATE_NAME
),
componentTemplateName,
],
},
},
templateVersion: 1,
});
});

Expand Down
91 changes: 70 additions & 21 deletions x-pack/plugins/rule_registry/server/rule_data_client/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
* 2.0.
*/

import { isEmpty } from 'lodash';
import type { estypes } from '@elastic/elasticsearch';
import { IndicesRolloverResponse } from '@elastic/elasticsearch/api/types';
import { ResponseError } from '@elastic/elasticsearch/lib/errors';
import { get } from 'lodash';
import { IndexPatternsFetcher } from '../../../../../src/plugins/data/server';
import {
IRuleDataClient,
Expand Down Expand Up @@ -70,9 +70,10 @@ export class RuleDataClient implements IRuleDataClient {
};
}

getWriter(options: { namespace?: string } = {}): RuleDataWriter {
async getWriter(options: { namespace?: string } = {}): Promise<RuleDataWriter> {
const { namespace } = options;
const alias = getNamespacedAlias({ alias: this.options.alias, namespace });
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: why we use parameter object + destructuring in private functions when there's only 2 parameters? Wouldn't it be cleaner to getNamespacedAlias(this.options.alias, namespace)?

I understand when some public API accepts options via an object (e.g. the async getWriter(options: { namespace?: string } = {}): Promise<RuleDataWriter> { above). You probably want extensibility etc. But for internal or private functions this is unnecessary if the number of parameters is <= 3 imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong opinion either way. Sometimes it's nice to see the parameter names at the function call site as a reminder of what each one is. I just left it the way it is to follow the existing pattern.

await this.rolloverAliasIfNeeded({ namespace });
Copy link
Contributor

Choose a reason for hiding this comment

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

rolloverAliasIfNeeded accepts namespace and calls const alias = getNamespacedAlias({ alias: this.options.alias, namespace }); inside. Can we just pass the alias already created above?

const alias = getNamespacedAlias({ alias: this.options.alias, namespace });
await this.rolloverAliasIfNeeded(alias);

Copy link
Contributor

Choose a reason for hiding this comment

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

Why rolloverAliasIfNeeded is called at the point when we get a writer, but index creation (createWriteTargetIfNeeded) is deferred to the point when we actually index something, and get an exception saying that the index does not exist?

Can/should we combine the two into a single rolloverAliasOrCreateInitialIndex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we can just pass in the alias directly, good point.

The call to createWriteTargetIfNeeded is deferred at @dgieselaar 's request to avoid populating the cluster with more empty indices than necessary. We can defer it since attempting to write to an index that doesn't exist returns an error that we can catch, then create the index and write again. We can't defer the call to rolloverAliasIfNeeded because there most likely won't be an error to catch if we try to write to an index whose mapping hasn't been updated - for most of the mapping updates we make (e.g. adding a new field to the mapping), it will still be able to write the document but the fields newly added to the template simply won't be mapped in the new documents. By rolling over before returning the writer, we can ensure that when we do write documents the write index has the updated mappings

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't defer the call to rolloverAliasIfNeeded because there most likely won't be an error to catch if we try to write to an index whose mapping hasn't been updated - for most of the mapping updates we make (e.g. adding a new field to the mapping), it will still be able to write the document but the fields newly added to the template simply won't be mapped in the new documents.

👍 yeah, understood that part, agree

As for deferring createWriteTargetIfNeeded until the very last moment, I'm personally not sure if this is super necessary because I suspect the time between the moment of getting a writer and attempting to write something will be very short. Exception would be if we get a writer but don't write anything - in this case we'd create the initial index, but it would stay empty for a while.

Both options work for me, I think this is not super important, just thought that maybe combining them into a rolloverAliasOrCreateInitialIndex would make the code slightly simpler.

Copy link
Contributor

@xcrzx xcrzx Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Having await this.rolloverAliasIfNeeded({ namespace }); inside getWriter could carry significant performance penalty.

Let's consider solution-side code that performs many writes during a request or rule execution. A single log write operation could look like that:

await ruleDataClientInstance.getWriter({ namespace }).bulk(logsToWrite);

And I can imagine a rule that performs tens of writes like that during the execution. From the solution developer's perspective, nothing bad happens; they write logs N times. But under the hood, every write request is accompanied by at least three additional requests (total requests = N*4):

// Expected request
await clusterClient.bulk()
// Additional "hidden" requests
await clusterClient.indices.rollover({ dry_run: true })
await clusterClient.indices.get()
await clusterClient.indices.simulateIndexTemplate()

To avoid making these extra requests on every write, the developer should instantiate the writer and reuse it. But I think it's (a) not always convenient to do, and (b) the developer should know implementation details of getWriter and be aware of the possible pitfall.

We can envisage use cases as above and protect API users from harming performance by having a map of instantiated writers (Map<Namespace, RuleDataWriter>()) and returning a writer for a given namespace before even calling rolloverAliasIfNeeded if it is already in the map.

What do 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.

To avoid making these extra requests on every write, the developer should instantiate the writer and reuse it.

That's I'd say how it should be done. If you're in an executor function, you will get a namespace from the rule parameters and get the writer. Then you'll pass it down to your classes and functions as a dependency to write events.

Same if you need to log stuff from route handlers.

having a map of instantiated writers (Map<Namespace, RuleDataWriter>())

Hehe, that's similar to what I had in my draft :) I think this could be handy in general if someone would always use the API like that: await ruleDataClientInstance.getWriter({ namespace }).bulk(logsToWrite);, but maybe YAGNI.

I'd say let's consider that in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is an area that is a little bit tricky. I had in mind adding some dev docs to explain the expected usage pattern (create a writer inside the alert executor and reuse it for the duration of the executor).

Typescript does do a little bit more to guide devs towards that pattern naturally - since getWriter is async, you have to do something like

const writer = await ruleDataClientInstance.getWriter({ namespace });
await writer.bulk(logsToWrite);

which might help clue devs in that writers are meant to be reused.

On the flip side, the potential issue with caching the writers and therefore only running the rollover logic once at instantiation time is that if the mappings change while Kibana is running then we won't detect it unless we add a way to manually re-check the mapping versions for an index. Our built-in mappings won't change this way, but we've had multiple cases of advanced users modifying the mappings to add their own custom fields. If we do add the user-defined templates (#101016 (comment)) then those would be changing while Kibana is running - so we'd want to check the template versions frequently to make sure changes to those templates get propagated to the indices.

So in my mind the RuleDataWriter is designed to live for a few seconds, e.g. for the duration of a rule execution, after which it should be discarded and a new one instantiated. My hope was that with docs we can provide simple guidance about creating and using a single writer within the rule executor and devs that way won't have to worry too much about the internals - they just know that you make 1 writer in the executor.

Maybe we could also add an optional parameter to getWriter to disable the rollover check, for use in route handlers that don't want to pay the overhead of the rollover check?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's I'd say how it should be done. If you're in an executor function, you will get a namespace from the rule parameters and get the writer. Then you'll pass it down to your classes and functions as a dependency to write events.

Yeah, it should be done that way. But my point is that the current API design doesn't enforce that. And in fact, it is much easier to use the API in the wrong way. Currently, all classes/functions depend on ruleDataClient instead of writer, like here, and here. And I'm also doing something similar in other places in my PR, just because it's much easier. I already have ruleDataClient as a dependency; why bothering extracting a writer and passing it down as another dependency? And some developers (including me 🙂) prefer to just copy-paste working code from other places instead of reading docs and exploring how things work under the hood. That's why I think adding docs wouldn't make much difference. That said, it's still a good practice to have docs.

I think a well-designed system should make it easy to do the right things and hard to do the wrong things. That's why I'm highlighting the current API as it potentially could be misused. But, as we have plans to improve RuleDataClient in a follow PR, let's discuss the API design separately. It is not a blocker for current changes to be merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will try to address this concern in the next PR within #101016 (will be a parallel PR to this one). The idea is to bind RuleDataClient to a namespace, so you could get an instance of it only if you know the namespace you're gonna work with:

// somewhere in the setup phase of security_solution
await ruleDataService.bootstrap(detectionAlertsLogDefinition);

// somewhere in an executor or a route handler
const detectionAlertsClient = await ruleDataService.getClient(detectionAlertsLogDefinition, namespace);

// or we could even make it sync
const detectionAlertsClient = ruleDataService.getClient(detectionAlertsLogDefinition, namespace);

const writer = detectionAlertsClient.getWriter();
const reader = detectionAlertsClient.getReader();

@marshallmain @xcrzx

return {
bulk: async (request) => {
const clusterClient = await this.getClusterClient();
Expand All @@ -89,7 +90,7 @@ export class RuleDataClient implements IRuleDataClient {
response.body.items.length > 0 &&
response.body.items?.[0]?.index?.error?.type === 'index_not_found_exception'
) {
return this.createOrUpdateWriteTarget({ namespace }).then(() => {
return this.createWriteTargetIfNeeded({ namespace }).then(() => {
return clusterClient.bulk(requestWithDefaultParameters);
});
}
Expand All @@ -102,15 +103,78 @@ export class RuleDataClient implements IRuleDataClient {
};
}

async createOrUpdateWriteTarget({ namespace }: { namespace?: string }) {
async rolloverAliasIfNeeded({ namespace }: { namespace?: string }) {
const clusterClient = await this.getClusterClient();
const alias = getNamespacedAlias({ alias: this.options.alias, namespace });
let simulatedRollover: IndicesRolloverResponse;
try {
({ body: simulatedRollover } = await clusterClient.indices.rollover({
alias,
dry_run: true,
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

A thought on race conditions between different parts of index bootstrapping in this implementation.

If I noticed that correctly, in this implementation there are 3 places where certain parts of index bootstrapping logic are implemented:

  1. RuleDataPluginService.init(). This guy is called externally from the rule_registry plugin setup phase.

    const ruleDataService = new RuleDataPluginService({
  logger,
  isWriteEnabled: config.write.enabled,
  index: config.index,
  getClusterClient: async () => {
    const deps = await startDependencies;
    return deps.core.elasticsearch.client.asInternalUser;
  },
});

ruleDataService.init().catch((originalError) => {
  const error = new Error('Failed installing assets');
  // @ts-ignore
  error.stack = originalError.stack;
  logger.error(error);
});

  2. Bootstrapping specific to a security solution log (e.g. log of detection alerts). Is called from the security_solution setup phase.

      const ready = once(async () => {
    const componentTemplateName = ruleDataService.getFullAssetName(
      'security-solution-mappings'
    );

    if (!ruleDataService.isWriteEnabled()) {
      return;
    }

    await ruleDataService.createOrUpdateComponentTemplate({
      template: {
        name: componentTemplateName,
        body: {
          template: {
            settings: {
              number_of_shards: 1,
            },
            mappings: {}, // TODO: Add mappings here via `mappingFromFieldMap()`
          },
        },
      },
      templateVersion: 1,
    });

    await ruleDataService.createOrUpdateIndexTemplate({
      template: {
        name: ruleDataService.getFullAssetName('security-solution-index-template'),
        body: {
          index_patterns: [ruleDataService.getFullAssetName('security-solution*')],
          composed_of: [
            ruleDataService.getFullAssetName(TECHNICAL_COMPONENT_TEMPLATE_NAME),
            ruleDataService.getFullAssetName(ECS_COMPONENT_TEMPLATE_NAME),
            componentTemplateName,
          ],
        },
      },
      templateVersion: 1,
    });
  });

  ready().catch((err) => {
    this.logger!.error(err);
  });

  ruleDataClient = new RuleDataClient({
    alias: plugins.ruleRegistry.ruleDataService.getFullAssetName('security-solution'),
    getClusterClient: async () => {
      const coreStart = await start();
      return coreStart.elasticsearch.client.asInternalUser;
    },
    ready,
  });

  3. Bootstrapping specific to a concrete log within a namespace. This is done here in RuleDataClient: rolloverAliasIfNeeded and createWriteTargetIfNeeded.

We need to guarantee that 1 needs to be executed once and before 2, which in turn needs to be executed once and before 3.

While I think this seems to be implemented correctly, it took a while for me to trace all those parts of bootstrapping, dependencies between them, how/where different ready functions are being passed and called, etc. So imho we have 3 places where bootstrapping happens, but the order is a bit implicit, and it's hard to construct the full picture in the head.

Wondering if we could make it at least slightly more explicit.

Off the top of my head:

  • We could rename getClusterClient() to waitUntilBootstrappingFinished() or smth like that. It would still return cluster client.
  • We could pass cluster client as a parameter to rolloverAliasIfNeeded() and other functions that depend on it.
  • We could explicitly call waitUntilBootstrappingFinished() in public methods when appropriate and only once.

Ideally would be nice to encapsulate the whole bootstrapping (specific to a log) in a single class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely agree that there's room for improvement on the handling of ready signals here. To me, the biggest source of confusion/possible future bugs is that the solution plugins must define a ready signal as part of step 2 (Bootstrapping specific to a security solution log) and that ready signal is then passed in to the RuleDataClient. So steps 1 and 3 are handled by the rule registry infrastructure, but solution devs still have to have fairly detailed knowledge of how the bootstrap process works to understand the requirements around "ready signal" they must implement as part of step 2.

To solve this, I envision adding a function to the RuleDataPluginService that would look something like async bootstrapSolutionTemplates(logName, componentTemplates[], indexTemplate): Promise<RuleDataClient>. This function would be responsible for executing the tasks in step 2 and then constructing a RuleDataClient to read and write from indices based on those templates. Then in the solution plugin setup we would call const ruleDataClientPromise = ruleDataService.bootstrapSolutionTemplates(...), purposely not awaiting the call to not block during setup. However, we can pass the ruleDataClientPromise to rule types that need it and they can await the promise inside the executors ensuring that bootstrapSolutionTemplates finishes before they use the RuleDataClient.

I think with this approach, we'd still need the ready signal internal to the RuleDataPluginService to ensure that calls to bootstrapSolutionTemplates don't execute until after RuleDataPluginService.init() completes. But, since the RuleDataClient would come from bootstrapSolutionTemplates, we wouldn't need a ready signal inside the RuleDataClient at all - as soon as the RuleDataClient is created by bootstrapSolutionTemplates it's ready to be used.

Thoughts? Do you think this would sufficiently encapsulate and simplify the bootstrap process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I'm hoping we can defer the actual implementation of this encapsulation to a follow up PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts? Do you think this would sufficiently encapsulate and simplify the bootstrap process?

Yeah, this could be an option (if you really don't like the declarative way of resolving log definitions :P ). Injecting a hook. Btw, bootstrapSolutionTemplates() could be sync and return RuleDataClient right away. It would call the bootstrapping internally in parallel and make sure the ready signal is implemented properly.

One thing I'd keep in mind when implementing this is #102586 (comment) - how do we make sure that inside the hook a developer doesn't have to reference common templates manually.

Also I'm hoping we can defer the actual implementation of this encapsulation to a follow up PR

Yeah, of course, let's merge this PR sooner! 🚢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of bootstrapSolutionTemplates as a way to specify an index mapping in a declarative way rather than as a hook. Can we pair on it soon and discuss? I might be misunderstanding what declarative is in this context.

} catch (err) {
if (err?.meta?.body?.error?.type !== 'index_not_found_exception') {
throw err;
}
return;
}

const [writeIndexMapping, simulatedIndexMapping] = await Promise.all([
clusterClient.indices.get({
index: simulatedRollover.old_index,
}),
clusterClient.indices.simulateIndexTemplate({
name: simulatedRollover.new_index,
}),
]);
const currentVersions = get(writeIndexMapping, [
Copy link
Member

Choose a reason for hiding this comment

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

why use get instead of the optional changing operator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see much difference since currentVersions is any either way. I chose this way to be consistent with how we're getting targetVersions below - simulatedIndex is typed as an empty object so the easiest way to get a field from it was just to use get.

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 we can add (minimal) types for writeIndexMapping and simulatedIndex then. Once the client itself adds them we can remove it again. That way, if the types change, or are incorrect, we are notified via a TypeScript error.

'body',
simulatedRollover.old_index,
'mappings',
'_meta',
'versions',
]);
const targetVersions = get(simulatedIndexMapping, [
'body',
'template',
'mappings',
'_meta',
'versions',
]);
const componentTemplateRemoved =
Object.keys(currentVersions).length > Object.keys(targetVersions).length;
Copy link
Contributor

Choose a reason for hiding this comment

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

This line fails with TypeError: Cannot convert undefined or null to object when currentVersions is undefined. Let's add an additional guard here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, how's that possible. Both current and simulated indices should have this versions object in _meta (I guess?). Maybe this is an indication of a bug here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it happened when switching from another branch to this one while using the same ES cluster? I'm extracting this logic into a function and making it a little more robust with guards around the cases where currentVersions or targetVersions are undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my case, there was no _meta field in the response. Not sure how it happened, tbh.

const componentTemplateUpdated = Object.entries(targetVersions).reduce(
(acc, [templateName, targetTemplateVersion]) => {
const currentTemplateVersion = get(currentVersions, templateName);
const templateUpdated =
currentTemplateVersion == null || currentTemplateVersion < Number(targetTemplateVersion);
return acc || templateUpdated;
},
false
);
marshallmain marked this conversation as resolved.
Show resolved Hide resolved
const needRollover = componentTemplateRemoved || componentTemplateUpdated;
if (needRollover) {
try {
await clusterClient.indices.rollover({
alias,
new_index: simulatedRollover.new_index,
});
} catch (err) {
if (err?.meta?.body?.error?.type !== 'resource_already_exists_exception') {
throw err;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is there any doc describing exceptions from different methods of ElasticsearchClient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not aware of any...it would be amazing if we had one though.

}
}
}

async createWriteTargetIfNeeded({ namespace }: { namespace?: string }) {
const alias = getNamespacedAlias({ alias: this.options.alias, namespace });

const clusterClient = await this.getClusterClient();

const { body: aliasExists } = await clusterClient.indices.existsAlias({
name: alias,
});

const concreteIndexName = `${alias}-000001`;

if (!aliasExists) {
Expand All @@ -132,20 +196,5 @@ export class RuleDataClient implements IRuleDataClient {
}
}
}

const { body: simulateResponse } = await clusterClient.transport.request({
method: 'POST',
path: `/_index_template/_simulate_index/${concreteIndexName}`,
});

const mappings: estypes.MappingTypeMapping = simulateResponse.template.mappings;

if (isEmpty(mappings)) {
throw new Error(
'No mappings would be generated for this index, possibly due to failed/misconfigured bootstrapping'
);
}
Comment on lines -155 to -158
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this should be removed. I don't see the same check in rolloverAliasIfNeeded , and I think we should be explicit about this, because teams new to RAC will run into it - I did, too, and it is a little annoying to figure out and repair.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sort of check seems like it belongs with the template bootstrapping logic IMO. At this point we've already created the index, so simulating it seems redundant. Can we move this check so it happens sometime before the index is created, maybe at index template creation time?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with moving it, but I do would like to keep it, somewhere. But let's figure out first where this all needs to happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be always some common mappings specified (currently they are specified in templates installed in init() - TECHNICAL_COMPONENT_TEMPLATE_NAME and ECS_COMPONENT_TEMPLATE_NAME). We just have to make sure that final index templates always reference (composed_of) these common component templates.

I'd say this is a matter of having an API that doesn't provide 100% flexibility in index bootstrapping and makes sure that index template is always created and it always references the common templates - doesn't matter how the client of the library uses it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this check to index template creation time in 4ccf63a


await clusterClient.indices.putMapping({ index: `${alias}*`, body: mappings });
}
}
4 changes: 2 additions & 2 deletions x-pack/plugins/rule_registry/server/rule_data_client/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ export interface RuleDataWriter {

export interface IRuleDataClient {
getReader(options?: { namespace?: string }): RuleDataReader;
getWriter(options?: { namespace?: string }): RuleDataWriter;
createOrUpdateWriteTarget(options: { namespace?: string }): Promise<void>;
getWriter(options?: { namespace?: string }): Promise<RuleDataWriter>;
createWriteTargetIfNeeded(options: { namespace?: string }): Promise<void>;
}

export interface RuleDataClientConstructorOptions {
Expand Down
75 changes: 59 additions & 16 deletions x-pack/plugins/rule_registry/server/rule_data_plugin_service/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import { ClusterPutComponentTemplate } from '@elastic/elasticsearch/api/requestParams';
import { estypes } from '@elastic/elasticsearch';
import { ElasticsearchClient, Logger } from 'kibana/server';
import { merge } from 'lodash';
import { technicalComponentTemplate } from '../../common/assets/component_templates/technical_component_template';
import {
DEFAULT_ILM_POLICY_ID,
Expand Down Expand Up @@ -78,36 +79,68 @@ export class RuleDataPluginService {
});

await this._createOrUpdateComponentTemplate({
name: this.getFullAssetName(TECHNICAL_COMPONENT_TEMPLATE_NAME),
body: technicalComponentTemplate,
template: {
name: this.getFullAssetName(TECHNICAL_COMPONENT_TEMPLATE_NAME),
body: technicalComponentTemplate,
},
templateVersion: 1,
});

await this._createOrUpdateComponentTemplate({
name: this.getFullAssetName(ECS_COMPONENT_TEMPLATE_NAME),
body: ecsComponentTemplate,
template: {
name: this.getFullAssetName(ECS_COMPONENT_TEMPLATE_NAME),
body: ecsComponentTemplate,
},
templateVersion: 1,
});

this.options.logger.info(`Installed all assets`);

this.signal.complete();
}

private async _createOrUpdateComponentTemplate(
template: ClusterPutComponentTemplate<ClusterPutComponentTemplateBody>
) {
private async _createOrUpdateComponentTemplate({
template,
templateVersion,
}: {
template: ClusterPutComponentTemplate<ClusterPutComponentTemplateBody>;
templateVersion: number;
}) {
this.assertWriteEnabled();

const clusterClient = await this.getClusterClient();
this.options.logger.debug(`Installing component template ${template.name}`);
return clusterClient.cluster.putComponentTemplate(template);
const mergedTemplate = merge(
{
body: {
template: { mappings: { _meta: { versions: { [template.name]: templateVersion } } } },
},
},
template
);
return clusterClient.cluster.putComponentTemplate(mergedTemplate);
}

private async _createOrUpdateIndexTemplate(template: PutIndexTemplateRequest) {
private async _createOrUpdateIndexTemplate({
template,
templateVersion,
}: {
template: PutIndexTemplateRequest;
templateVersion: number;
}) {
this.assertWriteEnabled();

const clusterClient = await this.getClusterClient();
this.options.logger.debug(`Installing index template ${template.name}`);
return clusterClient.indices.putIndexTemplate(template);
const mergedTemplate = merge(
{
body: {
template: { mappings: { _meta: { versions: { [template.name]: templateVersion } } } },
},
},
template
);
return clusterClient.indices.putIndexTemplate(mergedTemplate);
}

private async _createOrUpdateLifecyclePolicy(policy: estypes.IlmPutLifecycleRequest) {
Expand All @@ -118,16 +151,26 @@ export class RuleDataPluginService {
return clusterClient.ilm.putLifecycle(policy);
}

async createOrUpdateComponentTemplate(
template: ClusterPutComponentTemplate<ClusterPutComponentTemplateBody>
) {
async createOrUpdateComponentTemplate({
template,
templateVersion,
}: {
template: ClusterPutComponentTemplate<ClusterPutComponentTemplateBody>;
templateVersion: number;
}) {
await this.wait();
return this._createOrUpdateComponentTemplate(template);
return this._createOrUpdateComponentTemplate({ template, templateVersion });
}

async createOrUpdateIndexTemplate(template: PutIndexTemplateRequest) {
async createOrUpdateIndexTemplate({
template,
templateVersion,
}: {
template: PutIndexTemplateRequest;
templateVersion: number;
}) {
await this.wait();
return this._createOrUpdateIndexTemplate(template);
return this._createOrUpdateIndexTemplate({ template, templateVersion });
}

async createOrUpdateLifecyclePolicy(policy: estypes.IlmPutLifecycleRequest) {
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/rule_registry/server/utils/create_lifecycle_rule_type_factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,9 @@ export const createLifecycleRuleTypeFactory: CreateLifecycleRuleTypeFactory = ({
});
}

await ruleDataClient.getWriter().bulk({
const ruleDataClientWriter = await ruleDataClient.getWriter();

await ruleDataClientWriter.bulk({
body: eventsToIndex
.flatMap((event) => [{ index: {} }, event])
.concat(
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/rule_registry/server/utils/create_persistence_rule_type_factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ export const createPersistenceRuleTypeFactory: CreatePersistenceRuleTypeFactory
logger.debug(`Found ${numAlerts} alerts.`);

if (ruleDataClient && numAlerts) {
await ruleDataClient.getWriter().bulk({
const ruleDataClientWriter = await ruleDataClient.getWriter();
await ruleDataClientWriter.bulk({
body: currentAlerts.flatMap((event) => [{ index: {} }, event]),
});
}
Expand Down
Loading