fix(cats/sql): fix table names with long type names (#4166) (#4207)

* fix(cats/sql): fix table names with long type names * fix(cats/sql): Hash once
spinnaker · Dec 9, 2019 · 024b922 · 024b922
1 parent 327f178
commit 024b922
Show file tree

Hide file tree

Showing 6 changed files with 101 additions and 16 deletions.
diff --git a/cats/cats-sql/src/main/kotlin/com/netflix/spinnaker/cats/sql/cache/SqlCache.kt b/cats/cats-sql/src/main/kotlin/com/netflix/spinnaker/cats/sql/cache/SqlCache.kt
@@ -1,12 +1,14 @@
 package com.netflix.spinnaker.cats.sql.cache
 
 import com.fasterxml.jackson.databind.ObjectMapper
+import com.google.common.hash.Hashing
 import com.netflix.spinnaker.cats.cache.CacheData
 import com.netflix.spinnaker.cats.cache.CacheFilter
 import com.netflix.spinnaker.cats.cache.DefaultCacheData
 import com.netflix.spinnaker.cats.cache.RelationshipCacheFilter
 import com.netflix.spinnaker.cats.cache.WriteableCache
 import com.netflix.spinnaker.clouddriver.core.provider.agent.Namespace.ON_DEMAND
+import com.netflix.spinnaker.config.SqlConstraints
 import com.netflix.spinnaker.config.coroutineThreadPrefix
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
 import com.netflix.spinnaker.kork.sql.config.SqlRetryProperties
@@ -52,12 +54,11 @@ class SqlCache(
   private val sqlRetryProperties: SqlRetryProperties,
   private val tableNamespace: String?,
   private val cacheMetrics: SqlCacheMetrics,
-  private val dynamicConfigService: DynamicConfigService
+  private val dynamicConfigService: DynamicConfigService,
+  private val sqlConstraints: SqlConstraints
 ) : WriteableCache {
 
   companion object {
-    // 352 * 2 + 64 (max rel_type length) == 768; 768 * 4 (utf8mb4) == 3072 == Aurora's max index length
-    private const val MAX_ID_LENGTH = 352
     private const val onDemandType = "onDemand"
 
     private val schemaVersion = SqlSchemaVersion.current()
@@ -518,13 +519,13 @@ class SqlCache(
     val hashes = mutableMapOf<String, String>() // id to sha256(body)
     val apps = mutableMapOf<String, String>()
 
-    items.filter { it.id.length > MAX_ID_LENGTH }
+    items.filter { it.id.length > sqlConstraints.maxIdLength }
       .forEach {
-        log.error("Dropping ${it.id} - character length exceeds MAX_ID_LENGTH ($MAX_ID_LENGTH)")
+        log.error("Dropping ${it.id} - character length exceeds MAX_ID_LENGTH ($sqlConstraints.maxIdLength)")
       }
 
     items
-      .filter { it.id != "_ALL_" && it.id.length <= MAX_ID_LENGTH }
+      .filter { it.id != "_ALL_" && it.id.length <= sqlConstraints.maxIdLength }
       .forEach {
         currentIds.add(it.id)
         val nullKeys = it.attributes
@@ -712,11 +713,11 @@ class SqlCache(
     val newRevRelIds = mutableSetOf<String>()
 
     items
-      .filter { it.id != "_ALL_" && it.id.length <= MAX_ID_LENGTH }
+      .filter { it.id != "_ALL_" && it.id.length <= sqlConstraints.maxIdLength }
       .forEach { cacheData ->
         cacheData.relationships.entries.forEach { rels ->
           val relType = rels.key.substringBefore(delimiter = ":", missingDelimiterValue = "")
-          rels.value.filter { it.length <= MAX_ID_LENGTH }
+          rels.value.filter { it.length <= sqlConstraints.maxIdLength }
             .forEach { r ->
               val fwdKey = "${cacheData.id}|$r"
               val revKey = "$r|${cacheData.id}"
@@ -884,15 +885,46 @@ class SqlCache(
   }
 
   private fun resourceTableName(type: String): String =
-    "cats_v${schemaVersion}_${if (tableNamespace != null) "${tableNamespace}_" else ""}${sanitizeType(type)}"
+    checkTableName("cats_v${schemaVersion}_", sanitizeType(type), "")
 
   private fun relTableName(type: String): String =
-    "cats_v${schemaVersion}_${if (tableNamespace != null) "${tableNamespace}_" else ""}${sanitizeType(type)}_rel"
+    checkTableName("cats_v${schemaVersion}_", sanitizeType(type), "_rel")
 
   private fun sanitizeType(type: String): String {
     return type.replace(typeSanitization, "_")
   }
 
+  /**
+   * Computes the actual name of the table less than MAX_TABLE_NAME_LENGTH characters long.
+   * It always keeps prefix with tablenamespace but can shorten name and suffix in that order.
+   * @return computed table name
+   */
+  private fun checkTableName(prefix: String, name: String, suffix: String): String {
+    var base = prefix
+    if (tableNamespace != null) {
+      base = "${prefix + tableNamespace}_"
+    }
+
+    // Optimistic and most frequent case
+    val tableName = base + name + suffix
+    if (tableName.length < sqlConstraints.maxTableNameLength) {
+      return tableName
+    }
+
+    // Hash the name and keep the suffix
+    val hash = Hashing.murmur3_128().hashBytes((name + suffix).toByteArray()).toString().substring(0..15)
+    val available = sqlConstraints.maxTableNameLength - base.length - suffix.length - hash.length - 1
+    if (available >= 0) {
+      return base + name.substring(0..available) + hash + suffix
+    }
+
+    // Remove suffix
+    if (available + suffix.length >= 0) {
+      return base + name.substring(0..(available + suffix.length)) + hash
+    }
+    throw IllegalArgumentException("property sql.table-namespace $tableNamespace is too long")
+  }
+
   private fun getHash(body: String?): String? {
     if (body.isNullOrBlank()) {
       return null

diff --git a/cats/cats-sql/src/main/kotlin/com/netflix/spinnaker/cats/sql/cache/SqlNamedCacheFactory.kt b/cats/cats-sql/src/main/kotlin/com/netflix/spinnaker/cats/sql/cache/SqlNamedCacheFactory.kt
@@ -3,6 +3,7 @@ package com.netflix.spinnaker.cats.sql.cache
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.cats.cache.NamedCacheFactory
 import com.netflix.spinnaker.cats.cache.WriteableCache
+import com.netflix.spinnaker.config.SqlConstraints
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
 import com.netflix.spinnaker.kork.sql.config.SqlRetryProperties
 import org.jooq.DSLContext
@@ -18,7 +19,9 @@ class SqlNamedCacheFactory(
   private val sqlRetryProperties: SqlRetryProperties,
   private val prefix: String?,
   private val cacheMetrics: SqlCacheMetrics,
-  private val dynamicConfigService: DynamicConfigService
+  private val dynamicConfigService: DynamicConfigService,
+  private val sqlConstraints: SqlConstraints
+
 ) : NamedCacheFactory {
 
   @ExperimentalContracts
@@ -32,7 +35,8 @@ class SqlNamedCacheFactory(
       sqlRetryProperties,
       prefix,
       cacheMetrics,
-      dynamicConfigService
+      dynamicConfigService,
+      sqlConstraints
     )
   }
 }
diff --git a/cats/cats-sql/src/main/kotlin/com/netflix/spinnaker/config/SqlCacheConfiguration.kt b/cats/cats-sql/src/main/kotlin/com/netflix/spinnaker/config/SqlCacheConfiguration.kt
@@ -49,7 +49,7 @@ const val coroutineThreadPrefix = "catsSql"
 @Configuration
 @ConditionalOnProperty("sql.cache.enabled")
 @Import(DefaultSqlConfiguration::class)
-@EnableConfigurationProperties(SqlAgentProperties::class)
+@EnableConfigurationProperties(SqlAgentProperties::class, SqlConstraints::class)
 @ComponentScan("com.netflix.spinnaker.cats.sql.controllers")
 class SqlCacheConfiguration {
 
@@ -95,6 +95,7 @@ class SqlCacheConfiguration {
     sqlProperties: SqlProperties,
     cacheMetrics: SqlCacheMetrics,
     dynamicConfigService: DynamicConfigService,
+    sqlConstraints: SqlConstraints,
     @Value("\${sql.cache.async-pool-size:0}") poolSize: Int,
     @Value("\${sql.table-namespace:#{null}}") tableNamespace: String?
   ): NamedCacheFactory {
@@ -127,7 +128,8 @@ class SqlCacheConfiguration {
       sqlProperties.retries,
       tableNamespace,
       cacheMetrics,
-      dynamicConfigService
+      dynamicConfigService,
+      sqlConstraints
     )
   }
 

diff --git a/cats/cats-sql/src/main/kotlin/com/netflix/spinnaker/config/SqlConstraints.kt b/cats/cats-sql/src/main/kotlin/com/netflix/spinnaker/config/SqlConstraints.kt
@@ -0,0 +1,26 @@
+/*
+ * Copyright 2019 Armory
+ *
+ * Licensed 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.
+ */
+
+package com.netflix.spinnaker.config
+
+import org.springframework.boot.context.properties.ConfigurationProperties
+
+@ConfigurationProperties("sql.constraints")
+class SqlConstraints {
+  var maxTableNameLength: Int = 64
+  // 352 * 2 + 64 (max rel_type length) == 768; 768 * 4 (utf8mb4) == 3072 == Aurora's max index length
+  var maxIdLength: Int = 352
+}
diff --git a/cats/cats-sql/src/test/groovy/com/netflix/spinnaker/cats/sql/SqlCacheSpec.groovy b/cats/cats-sql/src/test/groovy/com/netflix/spinnaker/cats/sql/SqlCacheSpec.groovy
@@ -6,6 +6,7 @@ import com.netflix.spinnaker.cats.cache.RelationshipCacheFilter
 import com.netflix.spinnaker.cats.cache.WriteableCacheSpec
 import com.netflix.spinnaker.cats.sql.cache.SqlCache
 import com.netflix.spinnaker.cats.sql.cache.SqlCacheMetrics
+import com.netflix.spinnaker.config.SqlConstraints
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
 import com.netflix.spinnaker.kork.sql.config.RetryProperties
 import com.netflix.spinnaker.kork.sql.config.SqlRetryProperties
@@ -96,6 +97,23 @@ class SqlCacheSpec extends WriteableCacheSpec {
     null                                                   || null             || "1=1"
   }
 
+  @Unroll
+  def 'max length of table name is checked'() {
+    when:
+    def realName = ((SqlCache) cache).checkTableName("cats_v1_", name, suffix)
+
+    then:
+    realName == expected
+
+    where:
+    name              || expected                                                             || suffix
+    "foo"             || "cats_v1_test_foo"                                                   || ""
+    "abcdefghij" * 10 || "cats_v1_test_abcdefghijabcdefghijabcdefghijabcdeaa7d0fee7e891a66"   || ""
+    "abcdefghij" * 10 || "cats_v1_test_abcdefghijabcdefghijabcdefghija9246690b33571ecc_rel"   || "_rel"
+    "abcdefghij" * 10 || "cats_v1_test_abcdefghijabcdefghijabcdefghijabcdefe546a736182e553"   || "suffix"*10
+
+  }
+
   @Override
   Cache getSubject() {
     def mapper = new ObjectMapper()
@@ -119,7 +137,8 @@ class SqlCacheSpec extends WriteableCacheSpec {
       sqlRetryProperties,
       "test",
       Mock(SqlCacheMetrics),
-      dynamicConfigService
+      dynamicConfigService,
+      new SqlConstraints()
     )
   }
 

diff --git a/cats/cats-sql/src/test/groovy/com/netflix/spinnaker/cats/sql/SqlProviderCacheSpec.groovy b/cats/cats-sql/src/test/groovy/com/netflix/spinnaker/cats/sql/SqlProviderCacheSpec.groovy
@@ -10,6 +10,7 @@ import com.netflix.spinnaker.cats.cache.WriteableCache
 import com.netflix.spinnaker.cats.provider.ProviderCacheSpec
 import com.netflix.spinnaker.cats.sql.cache.SpectatorSqlCacheMetrics
 import com.netflix.spinnaker.cats.sql.cache.SqlCache
+import com.netflix.spinnaker.config.SqlConstraints
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
 import com.netflix.spinnaker.kork.sql.config.RetryProperties
 import com.netflix.spinnaker.kork.sql.config.SqlRetryProperties
@@ -67,7 +68,8 @@ class SqlProviderCacheSpec extends ProviderCacheSpec {
       sqlRetryProperties,
       "test",
       sqlMetrics,
-      dynamicConfigService
+      dynamicConfigService,
+      new SqlConstraints()
     )
 
     return new SqlProviderCache(backingStore)