Make migration mapping change detection more robust (#28252)

Modify the way migrations detect mapping changes. The previous approach simply
diffed the index mappings against the mappings defined in Kibana. The problem was
that sometimes the index mappings will *always* differ. For example, if an index template
is affecting the .kibana* indices. Or if Elasticsearch adds a new magical mapping that
appears in the index, even though not specified (this happened in the 7.0 release). So,
instead of diffing, we now store hashes of our mappings in _meta, and compare against
those.

src/server/saved_objects/migrations/README.md

@@ -6,10 +6,8 @@ Migrations are the mechanism by which saved object indices are kept up to date w
 
 When Kibana boots, prior to serving any requests, it performs a check to see if the kibana index needs to be migrated.
 
-* It searches the index for documents that are out of date, and it diffs the persisted index mappings with the mappings defined by the current system.
-* If the Kibana index does not exist, it is created.
-* If there are out of date docs, or breaking mapping changes, or the current index is not aliased, the index is migrated.
-* If there are minor mapping changes, such as adding a new property, the new mappings are applied to the current index.
+- If there are out of date docs, or mapping changes, or the current index is not aliased, the index is migrated.
+- If the Kibana index does not exist, it is created.
 
 All of this happens prior to Kibana serving any http requests.
 

src/server/saved_objects/migrations/core/__snapshots__/build_active_mappings.test.ts.snap

@@ -2,6 +2,18 @@
 
 exports[`buildActiveMappings combines all mappings and includes core mappings 1`] = `
 Object {
+  "_meta": Object {
+    "migrationMappingPropertyHashes": Object {
+      "aaa": "625b32086eb1d1203564cf85062dd22e",
+      "bbb": "18c78c995965207ed3f6e7fc5c6e55fe",
+      "config": "87aca8fdb053154f11383fce3dbf3edf",
+      "migrationVersion": "4a1746014a75ade3a714e1db5763276f",
+      "namespace": "2f4316de49999235636386fe51dc06c1",
+      "references": "7997cf5a56cc02bdc9c93361bde732b0",
+      "type": "2f4316de49999235636386fe51dc06c1",
+      "updated_at": "00da57df13e94e9d98437d13ace4bfe0",
+    },
+  },
   "dynamic": "strict",
   "properties": Object {
     "aaa": Object {

src/server/saved_objects/migrations/core/build_active_mappings.test.ts

@@ -17,7 +17,8 @@
  * under the License.
  */
 
-import { buildActiveMappings } from './build_active_mappings';
+import { buildActiveMappings, diffMappings } from './build_active_mappings';
+import { IndexMapping } from './call_cluster';
 
 describe('buildActiveMappings', () => {
   test('combines all mappings and includes core mappings', () => {
@@ -44,4 +45,153 @@ describe('buildActiveMappings', () => {
       /Invalid mapping \"_hm\"\. Mappings cannot start with _/
     );
   });
+
+  test('generated hashes are stable', () => {
+    const properties = {
+      aaa: { a: '...', b: '...', c: new Date('2019-01-02'), d: [{ hello: 'world' }] },
+      bbb: { c: new Date('2019-01-02'), d: [{ hello: 'world' }], a: '...', b: '...' },
+      ccc: { c: new Date('2020-01-02'), d: [{ hello: 'world' }], a: '...', b: '...' },
+    };
+
+    const mappings = buildActiveMappings({ properties });
+    const hashes = mappings._meta!.migrationMappingPropertyHashes!;
+
+    expect(hashes.aaa).toBeDefined();
+    expect(hashes.aaa).toEqual(hashes.bbb);
+    expect(hashes.aaa).not.toEqual(hashes.ccc);
+  });
+});
+
+describe('diffMappings', () => {
+  test('is different if expected contains extra hashes', () => {
+    const actual: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'bar' },
+      },
+      dynamic: 'strict',
+      properties: {},
+    };
+    const expected: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'bar', baz: 'qux' },
+      },
+      dynamic: 'strict',
+      properties: {},
+    };
+
+    expect(diffMappings(actual, expected)!.changedProp).toEqual('properties.baz');
+  });
+
+  test('does nothing if actual contains extra hashes', () => {
+    const actual: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'bar', baz: 'qux' },
+      },
+      dynamic: 'strict',
+      properties: {},
+    };
+    const expected: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'bar' },
+      },
+      dynamic: 'strict',
+      properties: {},
+    };
+
+    expect(diffMappings(actual, expected)).toBeUndefined();
+  });
+
+  test('does nothing if actual hashes are identical to expected, but properties differ', () => {
+    const actual: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'bar' },
+      },
+      dynamic: 'strict',
+      properties: {
+        foo: 'bar',
+      },
+    };
+    const expected: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'bar' },
+      },
+      dynamic: 'strict',
+      properties: {
+        foo: 'baz',
+      },
+    };
+
+    expect(diffMappings(actual, expected)).toBeUndefined();
+  });
+
+  test('is different if meta hashes change', () => {
+    const actual: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'bar' },
+      },
+      dynamic: 'strict',
+      properties: {},
+    };
+    const expected: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'baz' },
+      },
+      dynamic: 'strict',
+      properties: {},
+    };
+
+    expect(diffMappings(actual, expected)!.changedProp).toEqual('properties.foo');
+  });
+
+  test('is different if dynamic is different', () => {
+    const actual: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'bar' },
+      },
+      dynamic: 'strict',
+      properties: {},
+    };
+    const expected: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'bar' },
+      },
+      dynamic: 'abcde',
+      properties: {},
+    };
+
+    expect(diffMappings(actual, expected)!.changedProp).toEqual('dynamic');
+  });
+
+  test('is different if migrationMappingPropertyHashes is missing from actual', () => {
+    const actual: IndexMapping = {
+      _meta: {},
+      dynamic: 'strict',
+      properties: {},
+    };
+    const expected: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'bar' },
+      },
+      dynamic: 'strict',
+      properties: {},
+    };
+
+    expect(diffMappings(actual, expected)!.changedProp).toEqual('_meta');
+  });
+
+  test('is different if _meta is missing from actual', () => {
+    const actual: IndexMapping = {
+      dynamic: 'strict',
+      properties: {},
+    };
+    const expected: IndexMapping = {
+      _meta: {
+        migrationMappingPropertyHashes: { foo: 'bar' },
+      },
+      dynamic: 'strict',
+      properties: {},
+    };
+
+    expect(diffMappings(actual, expected)!.changedProp).toEqual('_meta');
+  });
 });

src/server/saved_objects/migrations/core/build_active_mappings.ts

@@ -18,9 +18,10 @@
  */
 
 /*
- * This file contains logic to build the index mappings for a migration.
+ * This file contains logic to build and diff the index mappings for a migration.
  */
 
+import crypto from 'crypto';
 import _ from 'lodash';
 import { IndexMapping, MappingProperties } from './call_cluster';
 
@@ -38,12 +39,90 @@ export function buildActiveMappings({
   properties: MappingProperties;
 }): IndexMapping {
   const mapping = defaultMapping();
+
+  properties = validateAndMerge(mapping.properties, properties);
+
   return _.cloneDeep({
     ...mapping,
-    properties: validateAndMerge(mapping.properties, properties),
+    properties,
+    _meta: {
+      migrationMappingPropertyHashes: md5Values(properties),
+    },
   });
 }
 
+/**
+ * Diffs the actual vs expected mappings. The properties are compared using md5 hashes stored in _meta, because
+ * actual and expected mappings *can* differ, but if the md5 hashes stored in actual._meta.migrationMappingPropertyHashes
+ * match our expectations, we don't require a migration. This allows ES to tack on additional mappings that Kibana
+ * doesn't know about or expect, without triggering continual migrations.
+ */
+export function diffMappings(actual: IndexMapping, expected: IndexMapping) {
+  if (actual.dynamic !== expected.dynamic) {
+    return { changedProp: 'dynamic' };
+  }
+
+  if (!actual._meta || !actual._meta.migrationMappingPropertyHashes) {
+    return { changedProp: '_meta' };
+  }
+
+  const changedProp = findChangedProp(
+    actual._meta.migrationMappingPropertyHashes,
+    expected._meta!.migrationMappingPropertyHashes
+  );
+
+  return changedProp ? { changedProp: `properties.${changedProp}` } : undefined;
+}
+
+// Convert an object to an md5 hash string, using a stable serialization (canonicalStringify)
+function md5Object(obj: any) {
+  return crypto
+    .createHash('md5')
+    .update(canonicalStringify(obj))
+    .digest('hex');
+}
+
+// JSON.stringify is non-canonical, meaning the same object may produce slightly
+// different JSON, depending on compiler optimizations (e.g. object keys
+// are not guaranteed to be sorted). This function consistently produces the same
+// string, if passed an object of the same shape. If the outpuf of this function
+// changes from one release to another, migrations will run, so it's important
+// that this function remains stable across releases.
+function canonicalStringify(obj: any): string {
+  if (Array.isArray(obj)) {
+    return `[${obj.map(canonicalStringify)}]`;
+  }
+
+  if (!obj || typeof obj !== 'object') {
+    return JSON.stringify(obj);
+  }
+
+  const keys = Object.keys(obj);
+
+  // This is important for properly handling Date
+  if (!keys.length) {
+    return JSON.stringify(obj);
+  }
+
+  const sortedObj = keys
+    .sort((a, b) => a.localeCompare(b))
+    .map(k => `${k}: ${canonicalStringify(obj[k])}`);
+
+  return `{${sortedObj}}`;
+}
+
+// Convert an object's values to md5 hash strings
+function md5Values(obj: any) {
+  return _.mapValues(obj, md5Object);
+}
+
+// If something exists in actual, but is missing in expected, we don't
+// care, as it could be a disabled plugin, etc, and keeping stale stuff
+// around is better than migrating unecessesarily.
+function findChangedProp(actual: any, expected: any) {
+  return Object.keys(expected).find(k => actual[k] !== expected[k]);
+}
+
 /**
  * These mappings are required for any saved object index.
  *

src/server/saved_objects/migrations/core/call_cluster.ts

@@ -34,9 +34,10 @@ export interface CallCluster {
   (path: 'indices.getAlias', opts: { name: string } & Ignorable): Promise<AliasResult | NotFound>;
   (path: 'indices.getMapping', opts: IndexOpts): Promise<MappingResult>;
   (path: 'indices.getSettings', opts: IndexOpts): Promise<IndexSettingsResult>;
-  (path: 'indices.putMapping', opts: PutMappingOpts): Promise<any>;
   (path: 'indices.refresh', opts: IndexOpts): Promise<any>;
   (path: 'indices.updateAliases', opts: UpdateAliasesOpts): Promise<any>;
+  (path: 'indices.deleteTemplate', opts: { name: string }): Promise<any>;
+  (path: 'cat.templates', opts: { format: 'json'; name: string }): Promise<Array<{ name: string }>>;
   (path: 'reindex', opts: ReindexOpts): Promise<any>;
   (path: 'scroll', opts: ScrollOpts): Promise<SearchResults>;
   (path: 'search', opts: SearchOpts): Promise<SearchResults>;
@@ -189,7 +190,15 @@ export interface MappingProperties {
   [type: string]: any;
 }
 
+export interface MappingMeta {
+  // A dictionary of key -> md5 hash (e.g. 'dashboard': '24234qdfa3aefa3wa')
+  // with each key being a root-level mapping property, and each value being
+  // the md5 hash of that mapping's value when the index was created.
+  migrationMappingPropertyHashes?: { [k: string]: string };
+}
+
 export interface IndexMapping {
   dynamic: string;
   properties: MappingProperties;
+  _meta?: MappingMeta;
 }

src/server/saved_objects/migrations/core/determine_migration_action.test.ts

@@ -1,221 +0,0 @@
-/*
- * Licensed to Elasticsearch B.V. under one or more contributor
- * license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright
- * ownership. Elasticsearch B.V. licenses this file to you under
- * the Apache License, Version 2.0 (the "License"); you may
- * not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *    http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied.  See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-import { determineMigrationAction, MigrationAction } from './determine_migration_action';
-
-describe('determineMigrationAction', () => {
-  test('requires no action if mappings are identical', () => {
-    const actual = {
-      dynamic: 'strict',
-      properties: {
-        hello: {
-          properties: {
-            name: { type: 'text' },
-          },
-        },
-      },
-    };
-    const expected = {
-      dynamic: 'strict',
-      properties: {
-        hello: {
-          properties: {
-            name: { type: 'text' },
-          },
-        },
-      },
-    };
-
-    expect(determineMigrationAction(actual, expected)).toEqual(MigrationAction.None);
-  });
-
-  test('requires no action if mappings differ only by dynamic properties', () => {
-    const actual = {
-      dynamic: 'strict',
-      properties: {
-        hello: { dynamic: true, foo: 'bar' },
-        world: { baz: 'bing' },
-      },
-    };
-    const expected = {
-      dynamic: 'strict',
-      properties: {
-        hello: { dynamic: 'true', goober: 'pea' },
-        world: { baz: 'bing' },
-      },
-    };
-
-    expect(determineMigrationAction(actual, expected)).toEqual(MigrationAction.None);
-  });
-
-  test('requires no action if mappings differ only by equivalent coerced properties', () => {
-    const actual = {
-      dynamic: 'strict',
-      properties: {
-        hello: { dynamic: 'false', baz: '2', foo: 'bar' },
-        world: { baz: 'bing' },
-      },
-    };
-    const expected = {
-      dynamic: 'strict',
-      properties: {
-        hello: { dynamic: false, baz: 2, foo: 'bar' },
-        world: { baz: 'bing' },
-      },
-    };
-
-    expect(determineMigrationAction(actual, expected)).toEqual(MigrationAction.None);
-  });
-
-  test('requires no action if a root property has been disabled', () => {
-    const actual = {
-      dynamic: 'strict',
-      properties: {
-        hello: { dynamic: true, foo: 'bar' },
-        world: { baz: 'bing' },
-      },
-    };
-    const expected = {
-      dynamic: 'strict',
-      properties: {
-        world: { baz: 'bing' },
-      },
-    };
-
-    expect(determineMigrationAction(actual, expected)).toEqual(MigrationAction.None);
-  });
-
-  test('requires migration if a sub-property differs', () => {
-    const actual = {
-      dynamic: 'strict',
-      properties: {
-        world: { type: 'text' },
-      },
-    };
-    const expected = {
-      dynamic: 'strict',
-      properties: {
-        world: { type: 'keword' },
-      },
-    };
-
-    expect(determineMigrationAction(actual, expected)).toEqual(MigrationAction.Migrate);
-  });
-
-  test('requires migration if a type changes', () => {
-    const actual = {
-      dynamic: 'strict',
-      properties: {
-        meaning: { type: 'text' },
-      },
-    };
-    const expected = {
-      dynamic: 'strict',
-      properties: {
-        meaning: 42,
-      },
-    };
-
-    expect(determineMigrationAction(actual, expected)).toEqual(MigrationAction.Migrate);
-  });
-
-  test('requires migration if doc dynamic value differs', () => {
-    const actual = {
-      dynamic: 'strict',
-      properties: {
-        world: { type: 'text' },
-      },
-    };
-    const expected = {
-      dynamic: 'true',
-      properties: {
-        world: { type: 'text' },
-      },
-    };
-
-    expect(determineMigrationAction(actual, expected)).toEqual(MigrationAction.Migrate);
-  });
-
-  test('requires patching if we added a root property', () => {
-    const actual = {
-      dynamic: 'strict',
-      properties: {},
-    };
-    const expected = {
-      dynamic: 'strict',
-      properties: {
-        world: { type: 'keword' },
-      },
-    };
-
-    expect(determineMigrationAction(actual, expected)).toEqual(MigrationAction.Patch);
-  });
-
-  test('requires patching if we added a sub-property', () => {
-    const actual = {
-      dynamic: 'strict',
-      properties: {
-        world: {
-          properties: {
-            a: 'a',
-          },
-        },
-      },
-    };
-    const expected = {
-      dynamic: 'strict',
-      properties: {
-        world: {
-          properties: {
-            a: 'a',
-            b: 'b',
-          },
-        },
-      },
-    };
-
-    expect(determineMigrationAction(actual, expected)).toEqual(MigrationAction.Patch);
-  });
-
-  test('requires migration if a sub property has been removed', () => {
-    const actual = {
-      dynamic: 'strict',
-      properties: {
-        world: {
-          properties: {
-            a: 'a',
-            b: 'b',
-          },
-        },
-      },
-    };
-    const expected = {
-      dynamic: 'strict',
-      properties: {
-        world: {
-          properties: {
-            b: 'b',
-          },
-        },
-      },
-    };
-
-    expect(determineMigrationAction(actual, expected)).toEqual(MigrationAction.Migrate);
-  });
-});

src/server/saved_objects/migrations/core/determine_migration_action.ts

@@ -1,95 +0,0 @@
-/*
- * Licensed to Elasticsearch B.V. under one or more contributor
- * license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright
- * ownership. Elasticsearch B.V. licenses this file to you under
- * the Apache License, Version 2.0 (the "License"); you may
- * not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *    http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied.  See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-import _ from 'lodash';
-import { IndexMapping } from './call_cluster';
-
-export enum MigrationAction {
-  None = 0,
-  Patch = 1,
-  Migrate = 2,
-}
-
-/**
- * Provides logic that diffs the actual index mappings with the expected
- * mappings. It ignores differences in dynamic mappings.
- *
- * If mappings differ in a patchable way, the result is 'patch', if mappings
- * differ in a way that requires migration, the result is 'migrate', and if
- * the mappings are equivalent, the result is 'none'.
- */
-export function determineMigrationAction(
-  actual: IndexMapping,
-  expected: IndexMapping
-): MigrationAction {
-  if (actual.dynamic !== expected.dynamic) {
-    return MigrationAction.Migrate;
-  }
-
-  const actualProps = actual.properties;
-  const expectedProps = expected.properties;
-
-  // There's a special case for root-level properties: if a root property is in actual,
-  // but not in expected, it is treated like a disabled plugin and requires no action.
-  return Object.keys(expectedProps).reduce((acc: number, key: string) => {
-    return Math.max(acc, diffSubProperty(actualProps[key], expectedProps[key]));
-  }, MigrationAction.None);
-}
-
-function diffSubProperty(actual: any, expected: any): MigrationAction {
-  // We've added a sub-property
-  if (actual === undefined && expected !== undefined) {
-    return MigrationAction.Patch;
-  }
-
-  // We've removed a sub property
-  if (actual !== undefined && expected === undefined) {
-    return MigrationAction.Migrate;
-  }
-
-  // If a property has changed to/from dynamic, we need to migrate,
-  // otherwise, we ignore dynamic properties, as they can differ
-  if (isDynamic(actual) || isDynamic(expected)) {
-    return isDynamic(actual) !== isDynamic(expected)
-      ? MigrationAction.Migrate
-      : MigrationAction.None;
-  }
-
-  // We have a leaf property, so we do a comparison. A change (e.g. 'text' -> 'keyword')
-  // should result in a migration.
-  if (typeof actual !== 'object') {
-    // We perform a string comparison here, because Elasticsearch coerces some primitives
-    // to string (such as dynamic: true and dynamic: 'true'), so we report a mapping
-    // equivalency if the string comparison checks out. This does mean that {} === '[object Object]'
-    // by this logic, but that is an edge case which should not occur in mapping definitions.
-    return `${actual}` === `${expected}` ? MigrationAction.None : MigrationAction.Migrate;
-  }
-
-  // Recursively compare the sub properties
-  const keys = _.uniq(Object.keys(actual).concat(Object.keys(expected)));
-  return keys.reduce((acc: number, key: string) => {
-    return acc === MigrationAction.Migrate
-      ? acc
-      : Math.max(acc, diffSubProperty(actual[key], expected[key]));
-  }, MigrationAction.None);
-}
-
-function isDynamic(prop: any) {
-  return prop && `${prop.dynamic}` === 'true';
-}

src/server/saved_objects/migrations/core/elastic_index.test.ts

@@ -121,30 +121,6 @@ describe('ElasticIndex', () => {
     });
   });
 
-  describe('putMappings', () => {
-    test('it calls indices.putMapping', async () => {
-      const callCluster = sinon.spy(async (path: string, { body, type, index }: any) => {
-        expect(path).toEqual('indices.putMapping');
-        expect(index).toEqual('.shazm');
-        expect(body).toEqual({
-          dynamic: 'strict',
-          properties: {
-            foo: 'bar',
-          },
-        });
-      });
-
-      await Index.putMappings(callCluster, '.shazm', {
-        dynamic: 'strict',
-        properties: {
-          foo: 'bar',
-        },
-      });
-
-      sinon.assert.called(callCluster);
-    });
-  });
-
   describe('claimAlias', () => {
     function assertCalled(callCluster: sinon.SinonSpy) {
       expect(callCluster.args.map(([path]) => path)).toEqual([

src/server/saved_objects/migrations/core/elastic_index.ts

@@ -209,26 +209,6 @@ export async function migrationsUpToDate(
   }
 }
 
-/**
- * Applies the specified mappings to the index.
- *
- * @param {CallCluster} callCluster
- * @param {string} index
- * @param {IndexMapping} mappings
- */
-export function putMappings(callCluster: CallCluster, index: string, mappings: IndexMapping) {
-  return callCluster('indices.putMapping', {
-    index,
-
-    // HACK: This is a temporary workaround for a disconnect between
-    // elasticsearchjs and Elasticsearch 7.0. The JS library requires
-    // type, but Elasticsearch 7.0 has deprecated type...
-    include_type_name: true,
-    type: '_doc',
-    body: mappings,
-  } as any);
-}
-
 export async function createIndex(
   callCluster: CallCluster,
   index: string,

src/server/saved_objects/migrations/core/index_migrator.test.ts

@@ -25,47 +25,6 @@ import { CallCluster } from './call_cluster';
 import { IndexMigrator } from './index_migrator';
 
 describe('IndexMigrator', () => {
-  test('patches the index mappings if the index is already migrated', async () => {
-    const opts = defaultOpts();
-    const callCluster = clusterStub(opts);
-
-    opts.mappingProperties = { foo: { type: 'text' } };
-
-    withIndex(callCluster);
-
-    const result = await new IndexMigrator(opts).migrate();
-
-    expect(ranMigration(opts)).toBeFalsy();
-    expect(result.status).toEqual('patched');
-    sinon.assert.calledWith(callCluster, 'indices.putMapping', {
-      body: {
-        dynamic: 'strict',
-        properties: {
-          config: {
-            dynamic: 'true',
-            properties: { buildNum: { type: 'keyword' } },
-          },
-          foo: { type: 'text' },
-          migrationVersion: { dynamic: 'true', type: 'object' },
-          namespace: { type: 'keyword' },
-          type: { type: 'keyword' },
-          updated_at: { type: 'date' },
-          references: {
-            type: 'nested',
-            properties: {
-              name: { type: 'keyword' },
-              type: { type: 'keyword' },
-              id: { type: 'keyword' },
-            },
-          },
-        },
-      },
-      include_type_name: true,
-      type: '_doc',
-      index: '.kibana_1',
-    });
-  });
-
   test('creates the index if it does not exist', async () => {
     const opts = defaultOpts();
     const callCluster = clusterStub(opts);
@@ -81,6 +40,17 @@ describe('IndexMigrator', () => {
       body: {
         mappings: {
           dynamic: 'strict',
+          _meta: {
+            migrationMappingPropertyHashes: {
+              config: '87aca8fdb053154f11383fce3dbf3edf',
+              foo: '18c78c995965207ed3f6e7fc5c6e55fe',
+              migrationVersion: '4a1746014a75ade3a714e1db5763276f',
+              namespace: '2f4316de49999235636386fe51dc06c1',
+              references: '7997cf5a56cc02bdc9c93361bde732b0',
+              type: '2f4316de49999235636386fe51dc06c1',
+              updated_at: '00da57df13e94e9d98437d13ace4bfe0',
+            },
+          },
           properties: {
             config: {
               dynamic: 'true',
@@ -196,6 +166,17 @@ describe('IndexMigrator', () => {
       body: {
         mappings: {
           dynamic: 'strict',
+          _meta: {
+            migrationMappingPropertyHashes: {
+              config: '87aca8fdb053154f11383fce3dbf3edf',
+              foo: '625b32086eb1d1203564cf85062dd22e',
+              migrationVersion: '4a1746014a75ade3a714e1db5763276f',
+              namespace: '2f4316de49999235636386fe51dc06c1',
+              references: '7997cf5a56cc02bdc9c93361bde732b0',
+              type: '2f4316de49999235636386fe51dc06c1',
+              updated_at: '00da57df13e94e9d98437d13ace4bfe0',
+            },
+          },
           properties: {
             author: { type: 'text' },
             config: {

src/server/saved_objects/migrations/core/index_migrator.ts

@@ -17,7 +17,7 @@
  * under the License.
  */
 
-import { determineMigrationAction, MigrationAction } from './determine_migration_action';
+import { diffMappings } from './build_active_mappings';
 import * as Index from './elastic_index';
 import { migrateRawDocs } from './migrate_raw_docs';
 import { Context, migrationContext, MigrationOpts } from './migration_context';
@@ -53,22 +53,15 @@ export class IndexMigrator {
       pollInterval: context.pollInterval,
 
       async isMigrated() {
-        const action = await requiredAction(context);
-        return action === MigrationAction.None;
+        return requiresMigration(context);
       },
 
       async runMigration() {
-        const action = await requiredAction(context);
-
-        if (action === MigrationAction.None) {
-          return { status: 'skipped' };
+        if (await requiresMigration(context)) {
+          return migrateIndex(context);
         }
 
-        if (action === MigrationAction.Patch) {
-          return patchSourceMappings(context);
-        }
-
-        return migrateIndex(context);
+        return { status: 'skipped' };
       },
     });
   }
@@ -77,9 +70,10 @@ export class IndexMigrator {
 /**
  * Determines what action the migration system needs to take (none, patch, migrate).
  */
-async function requiredAction(context: Context): Promise<MigrationAction> {
-  const { callCluster, alias, documentMigrator, dest } = context;
+async function requiresMigration(context: Context): Promise<boolean> {
+  const { callCluster, alias, documentMigrator, dest, log } = context;
 
+  // Have all of our known migrations been run against the index?
   const hasMigrations = await Index.migrationsUpToDate(
     callCluster,
     alias,
@@ -87,29 +81,26 @@ async function requiredAction(context: Context): Promise<MigrationAction> {
   );
 
   if (!hasMigrations) {
-    return MigrationAction.Migrate;
+    return true;
   }
 
+  // Is our index aliased?
   const refreshedSource = await Index.fetchInfo(callCluster, alias);
 
   if (!refreshedSource.aliases[alias]) {
-    return MigrationAction.Migrate;
+    return true;
   }
 
-  return determineMigrationAction(refreshedSource.mappings, dest.mappings);
-}
+  // Do the actual index mappings match our expectations?
+  const diffResult = diffMappings(refreshedSource.mappings, dest.mappings);
 
-/**
- * Applies the latest mappings to the index.
- */
-async function patchSourceMappings(context: Context): Promise<MigrationResult> {
-  const { callCluster, log, source, dest } = context;
+  if (diffResult) {
+    log.info(`Detected mapping change in "${diffResult.changedProp}"`);
 
-  log.info(`Patching ${source.indexName} mappings`);
+    return true;
+  }
 
-  await Index.putMappings(callCluster, source.indexName, dest.mappings);
-
-  return { status: 'patched' };
+  return false;
 }
 
 /**
@@ -120,6 +111,8 @@ async function migrateIndex(context: Context): Promise<MigrationResult> {
   const startTime = Date.now();
   const { callCluster, alias, source, dest, log } = context;
 
+  await deleteIndexTemplates(context);
+
   log.info(`Creating index ${dest.indexName}.`);
 
   await Index.createIndex(callCluster, dest.indexName, dest.mappings);
@@ -142,6 +135,31 @@ async function migrateIndex(context: Context): Promise<MigrationResult> {
   return result;
 }
 
+/**
+ * If the obsoleteIndexTemplatePattern option is specified, this will delete any index templates
+ * that match it.
+ */
+async function deleteIndexTemplates({ callCluster, log, obsoleteIndexTemplatePattern }: Context) {
+  if (!obsoleteIndexTemplatePattern) {
+    return;
+  }
+
+  const templates = await callCluster('cat.templates', {
+    format: 'json',
+    name: obsoleteIndexTemplatePattern,
+  });
+
+  if (!templates.length) {
+    return;
+  }
+
+  const templateNames = templates.map(t => t.name);
+
+  log.info(`Removing index templates: ${templateNames}`);
+
+  return Promise.all(templateNames.map(name => callCluster('indices.deleteTemplate', { name })));
+}
+
 /**
  * Moves all docs from sourceIndex to destIndex, migrating each as necessary.
  * This moves documents from the concrete index, rather than the alias, to prevent

src/server/saved_objects/migrations/core/migration_context.ts

@@ -41,6 +41,12 @@ export interface MigrationOpts {
   mappingProperties: MappingProperties;
   documentMigrator: VersionedTransformer;
   serializer: SavedObjectsSerializer;
+
+  /**
+   * If specified, templates matching the specified pattern will be removed
+   * prior to running migrations. For example: 'kibana_index_template*'
+   */
+  obsoleteIndexTemplatePattern?: string;
 }
 
 export interface Context {
@@ -54,6 +60,7 @@ export interface Context {
   pollInterval: number;
   scrollDuration: string;
   serializer: SavedObjectsSerializer;
+  obsoleteIndexTemplatePattern?: string;
 }
 
 /**
@@ -78,6 +85,7 @@ export async function migrationContext(opts: MigrationOpts): Promise<Context> {
     pollInterval: opts.pollInterval,
     scrollDuration: opts.scrollDuration,
     serializer: opts.serializer,
+    obsoleteIndexTemplatePattern: opts.obsoleteIndexTemplatePattern,
   };
 }
 

src/server/saved_objects/migrations/kibana/__snapshots__/kibana_migrator.test.ts.snap

@@ -2,6 +2,18 @@
 
 exports[`KibanaMigrator getActiveMappings returns full index mappings w/ core properties 1`] = `
 Object {
+  "_meta": Object {
+    "migrationMappingPropertyHashes": Object {
+      "amap": "625b32086eb1d1203564cf85062dd22e",
+      "bmap": "625b32086eb1d1203564cf85062dd22e",
+      "config": "87aca8fdb053154f11383fce3dbf3edf",
+      "migrationVersion": "4a1746014a75ade3a714e1db5763276f",
+      "namespace": "2f4316de49999235636386fe51dc06c1",
+      "references": "7997cf5a56cc02bdc9c93361bde732b0",
+      "type": "2f4316de49999235636386fe51dc06c1",
+      "updated_at": "00da57df13e94e9d98437d13ace4bfe0",
+    },
+  },
   "dynamic": "strict",
   "properties": Object {
     "amap": Object {

src/server/saved_objects/migrations/kibana/kibana_migrator.ts

@@ -100,6 +100,7 @@ export class KibanaMigrator {
       pollInterval: config.get('migrations.pollInterval'),
       scrollDuration: config.get('migrations.scrollDuration'),
       serializer: this.serializer,
+      obsoleteIndexTemplatePattern: 'kibana_index_template*',
     });
 
     return migrator.migrate();

test/api_integration/apis/saved_objects/migrations.js

@@ -66,7 +66,42 @@ export default ({ getService }) => {
       await createIndex({ callCluster, index });
       await createDocs({ callCluster, index, docs: originalDocs });
 
-      const result = await migrateIndex({ callCluster, index, migrations, mappingProperties });
+      // Test that unrelated index templates are unaffected
+      await callCluster('indices.putTemplate', {
+        name: 'migration_test_a_template',
+        body: {
+          index_patterns: 'migration_test_a',
+          mappings: {
+            dynamic: 'strict',
+            properties: { baz: { type: 'text' } },
+          },
+        },
+      });
+
+      // Test that obsolete index templates get removed
+      await callCluster('indices.putTemplate', {
+        name: 'migration_a_template',
+        body: {
+          index_patterns: index,
+          mappings: {
+            dynamic: 'strict',
+            properties: { baz: { type: 'text' } },
+          },
+        },
+      });
+
+      assert.isTrue(await callCluster('indices.existsTemplate', { name: 'migration_a_template' }));
+
+      const result = await migrateIndex({
+        callCluster,
+        index,
+        migrations,
+        mappingProperties,
+        obsoleteIndexTemplatePattern: 'migration_a*',
+      });
+
+      assert.isFalse(await callCluster('indices.existsTemplate', { name: 'migration_a_template' }));
+      assert.isTrue(await callCluster('indices.existsTemplate', { name: 'migration_test_a_template' }));
 
       assert.deepEqual(_.omit(result, 'elapsedMs'), {
         destIndex: '.migration-a_2',
@@ -236,7 +271,7 @@ async function createDocs({ callCluster, index, docs }) {
   await callCluster('indices.refresh', { index });
 }
 
-async function migrateIndex({ callCluster, index, migrations, mappingProperties, validateDoc }) {
+async function migrateIndex({ callCluster, index, migrations, mappingProperties, validateDoc, obsoleteIndexTemplatePattern }) {
   const documentMigrator = new DocumentMigrator({
     kibanaVersion: '99.9.9',
     migrations,
@@ -244,15 +279,16 @@ async function migrateIndex({ callCluster, index, migrations, mappingProperties,
   });
 
   const migrator = new IndexMigrator({
-    batchSize: 10,
     callCluster,
     documentMigrator,
     index,
-    log: _.noop,
+    obsoleteIndexTemplatePattern,
     mappingProperties,
+    batchSize: 10,
+    log: _.noop,
     pollInterval: 50,
     scrollDuration: '5m',
-    serializer: new SavedObjectsSerializer(new SavedObjectsSchema())
+    serializer: new SavedObjectsSerializer(new SavedObjectsSchema()),
   });
 
   return await migrator.migrate();