[NU-2048] (K8s DM) Fix for: k8s object name sanitizing strategy somet…

…imes generated invalid object names, in other cases, (#7616) it generated names with unnecessary characters appended
TouK · Mar 6, 2025 · 419ad72 · 419ad72
1 parent 4f40272
commit 419ad72
Show file tree

Hide file tree

Showing 16 changed files with 255 additions and 86 deletions.
diff --git a/docs/Changelog.md b/docs/Changelog.md
@@ -112,7 +112,9 @@
 * [#7614](https://github.com/TouK/nussknacker/pull/7614) SpelTemplate as a main text editor
   * Added a new parameter editor type: SpelParameterEditor which works the same as RawParameterEditor.
   * Added a list of editors to the UIParameter.
-
+* [#7616](https://github.com/TouK/nussknacker/pull/7616) (K8s DM) Fix for: k8s object name sanitizing strategy sometimes generated invalid object names, in other cases, 
+  it generated names with unnecessary characters appended
+
 ## 1.18
 
 #### Highlights

diff --git a/...ploymentManager/src/main/scala/pl/touk/nussknacker/k8s/manager/K8sDeploymentManager.scala b/...ploymentManager/src/main/scala/pl/touk/nussknacker/k8s/manager/K8sDeploymentManager.scala
@@ -13,7 +13,7 @@ import pl.touk.nussknacker.engine.canonicalgraph.CanonicalProcess
 import pl.touk.nussknacker.engine.deployment.ExternalDeploymentId
 import pl.touk.nussknacker.engine.util.Implicits.RichScalaMap
 import pl.touk.nussknacker.k8s.manager.K8sDeploymentManager._
-import pl.touk.nussknacker.k8s.manager.K8sUtils.{sanitizeLabel, sanitizeObjectName, shortHash}
+import pl.touk.nussknacker.k8s.manager.K8sUtils.{sanitizeLabel, shortHash}
 import pl.touk.nussknacker.k8s.manager.deployment._
 import pl.touk.nussknacker.k8s.manager.deployment.K8sScalingConfig.DividingParallelismConfig
 import pl.touk.nussknacker.k8s.manager.ingress.IngressPreparer
@@ -332,7 +332,7 @@ class K8sDeploymentManager(
   private def configMapForData(
       processVersion: ProcessVersion,
       canonicalProcess: CanonicalProcess,
-      nussknackerInstanceName: Option[String]
+      nussknackerInstanceName: OptionalNussknackerInstanceName
   )(
       data: Map[String, String],
       additionalLabels: Map[String, String] = Map.empty,
@@ -361,7 +361,7 @@ class K8sDeploymentManager(
   private def secretForData(
       processVersion: ProcessVersion,
       canonicalProcess: CanonicalProcess,
-      nussknackerInstanceName: Option[String]
+      nussknackerInstanceName: OptionalNussknackerInstanceName
   )(data: Map[String, String], additionalLabels: Map[String, String] = Map.empty): Secret = {
     val scenario = canonicalProcess.asJson.spaces2
     val objectName =
@@ -441,11 +441,14 @@ object K8sDeploymentManager {
   /*
     Labels contain scenario name, scenario id and version.
    */
-  private[manager] def labelsForScenario(processVersion: ProcessVersion, nussknackerInstanceName: Option[String]) = Map(
+  private[manager] def labelsForScenario(
+      processVersion: ProcessVersion,
+      nussknackerInstanceName: OptionalNussknackerInstanceName
+  ) = Map(
     scenarioNameLabel    -> scenarioNameLabelValue(processVersion.processName),
     scenarioIdLabel      -> processVersion.processId.value.toString,
     scenarioVersionLabel -> processVersion.versionId.value.toString
-  ) ++ nussknackerInstanceName.map(nussknackerInstanceNameLabel -> _)
+  ) ++ nussknackerInstanceName.valueOpt.map(nussknackerInstanceNameLabel -> _)
 
   private[manager] def versionAnnotationForScenario(processVersion: ProcessVersion) =
     Map(scenarioVersionAnnotation -> processVersion.asJson.spaces2)
@@ -458,32 +461,17 @@ object K8sDeploymentManager {
    */
   private[manager] def objectNameForScenario(
       processVersion: ProcessVersion,
-      nussknackerInstanceName: Option[String],
+      nussknackerInstanceName: OptionalNussknackerInstanceName,
       hashInput: Option[String]
   ): String = {
     // we simulate (more or less) --append-hash kubectl behaviour...
     val hashToAppend      = hashInput.map(input => "-" + shortHash(input)).getOrElse("")
     val plainScenarioName = s"scenario-${processVersion.processId.value}-${processVersion.processName}"
-    val scenarioName      = objectNamePrefixedWithNussknackerInstanceName(nussknackerInstanceName, plainScenarioName)
-    sanitizeObjectName(scenarioName, hashToAppend)
-  }
-
-  private[manager] def objectNamePrefixedWithNussknackerInstanceName(
-      nussknackerInstanceName: Option[String],
-      objectName: String
-  ) =
-    sanitizeObjectName(
-      objectNamePrefixedWithNussknackerInstanceNameWithoutSanitization(nussknackerInstanceName, objectName)
+    K8sUtils.sanitizeObjectName(
+      nussknackerInstanceName.objectNameWithoutSanitization(plainScenarioName),
+      hashToAppend
     )
-
-  private[manager] def objectNamePrefixedWithNussknackerInstanceNameWithoutSanitization(
-      nussknackerInstanceName: Option[String],
-      objectName: String
-  ) =
-    nussknackerInstanceNamePrefix(nussknackerInstanceName) + objectName
-
-  private[manager] def nussknackerInstanceNamePrefix(nussknackerInstanceName: Option[String]) =
-    nussknackerInstanceName.map(_ + "-").getOrElse("")
+  }
 
   private[manager] def parseVersionAnnotation(deployment: ObjectResource): Option[ProcessVersion] = {
     deployment.metadata.annotations

diff --git a/...ntManager/src/main/scala/pl/touk/nussknacker/k8s/manager/K8sDeploymentManagerConfig.scala b/...ntManager/src/main/scala/pl/touk/nussknacker/k8s/manager/K8sDeploymentManagerConfig.scala
@@ -1,8 +1,7 @@
 package pl.touk.nussknacker.k8s.manager
 
 import com.typesafe.config.{Config, ConfigFactory}
-import net.ceedubs.ficus.Ficus._
-import net.ceedubs.ficus.readers.ArbitraryTypeReader._
+import net.ceedubs.ficus.readers.ValueReader
 import pl.touk.nussknacker.engine.util.config.ConfigEnrichments.RichConfig
 import pl.touk.nussknacker.engine.util.config.ScalaMajorVersionConfig
 import pl.touk.nussknacker.engine.version.BuildInfo
@@ -11,10 +10,18 @@ import pl.touk.nussknacker.k8s.manager.ingress.IngressConfig
 
 import scala.concurrent.duration._
 
-import K8sScalingConfig.valueReader
-
 object K8sDeploymentManagerConfig {
+
+  import net.ceedubs.ficus.Ficus._
+  import net.ceedubs.ficus.readers.ArbitraryTypeReader._
+
+  private implicit val scalingConfigValueReader: ValueReader[K8sScalingConfig] = K8sScalingConfig.valueReader
+
+  private implicit val optionalNussknackerInstanceNameValueReader: ValueReader[OptionalNussknackerInstanceName] =
+    OptionalNussknackerInstanceName.valueReader
+
   def parse(config: Config): K8sDeploymentManagerConfig = config.rootAs[K8sDeploymentManagerConfig]
+
 }
 
 case class K8sDeploymentManagerConfig(
@@ -23,7 +30,7 @@ case class K8sDeploymentManagerConfig(
     scalingConfig: Option[K8sScalingConfig] = None,
     configExecutionOverrides: Config = ConfigFactory.empty(),
     k8sDeploymentConfig: Config = ConfigFactory.empty(),
-    nussknackerInstanceName: Option[String] = None,
+    nussknackerInstanceName: OptionalNussknackerInstanceName = OptionalNussknackerInstanceName.empty,
     logbackConfigPath: Option[String] = None,
     commonConfigMapForLogback: Option[String] = None,
     ingress: Option[IngressConfig] = None,

diff --git a/...e/lite/k8sDeploymentManager/src/main/scala/pl/touk/nussknacker/k8s/manager/K8sUtils.scala b/...e/lite/k8sDeploymentManager/src/main/scala/pl/touk/nussknacker/k8s/manager/K8sUtils.scala
@@ -38,26 +38,68 @@ object K8sUtils {
 
   // Object names cannot have underscores in name...
   def sanitizeObjectName(original: String, append: String = ""): String = {
-    sanitizeName(original, canHaveUnderscore = false, append = append)
+    sanitizeName(
+      original,
+      canHaveUnderscore = false,
+      firstCharacterShouldBeAlphaNumer = true,
+      lastCharacterShouldBeAlphaNumer = append.isEmpty,
+      maxObjectNameLength - append.length
+    ) + append
+  }
+
+  def sanitizeObjectName(
+      name: String,
+      firstCharacterShouldBeAlphaNumer: Boolean,
+      lastCharacterShouldBeAlphaNumer: Boolean,
+      maxLength: Int
+  ): String = {
+    sanitizeName(
+      name,
+      canHaveUnderscore = false,
+      firstCharacterShouldBeAlphaNumer = firstCharacterShouldBeAlphaNumer,
+      lastCharacterShouldBeAlphaNumer = lastCharacterShouldBeAlphaNumer,
+      maxLength
+    )
   }
 
   // Value label: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set
   def sanitizeLabel(original: String, append: String = ""): String = {
-    sanitizeName(original, canHaveUnderscore = true, append = append)
+    sanitizeName(
+      original,
+      canHaveUnderscore = true,
+      firstCharacterShouldBeAlphaNumer = true,
+      lastCharacterShouldBeAlphaNumer = append.isEmpty,
+      maxObjectNameLength - append.length
+    ) + append
   }
 
-  // TODO: generate better correct name for 'strange' scenario names?
-  private[manager] def sanitizeName(base: String, canHaveUnderscore: Boolean, append: String = ""): String = {
-    val underscores = if (canHaveUnderscore) "_" else ""
-    base.toLowerCase
-      .replaceAll(s"[^a-zA-Z0-9${underscores}\\-.]+", "-")
-      // need to have alphanumeric at beginning and end...
-      .replaceAll("^([^a-zA-Z0-9])", "x$1")
-      .replaceAll("([^a-zA-Z0-9])$", "$1x")
-      .take(maxObjectNameLength - append.length) + append
+  private def sanitizeName(
+      name: String,
+      canHaveUnderscore: Boolean,
+      firstCharacterShouldBeAlphaNumer: Boolean,
+      lastCharacterShouldBeAlphaNumer: Boolean,
+      maxLength: Int
+  ): String = {
+    val withCorrectCharsInTheMiddle = name.toLowerCase.replaceAll(s"[^a-zA-Z0-9_\\-.]+", "-")
+    val withCorrectFirstChar =
+      if (firstCharacterShouldBeAlphaNumer)
+        withCorrectCharsInTheMiddle.replaceAll("^([^a-zA-Z0-9])", "x$1")
+      else
+        withCorrectCharsInTheMiddle
+    val truncated = withCorrectFirstChar.take(maxLength)
+    if (lastCharacterShouldBeAlphaNumer) {
+      val canAddAnotherCharacter = truncated.length < maxLength
+      if (canAddAnotherCharacter) {
+        truncated.replaceAll("([^a-zA-Z0-9])$", "$1x")
+      } else {
+        truncated.replaceAll("([^a-zA-Z0-9])$", "x")
+      }
+    } else {
+      truncated
+    }
   }
 
   // https://github.com/kubernetes/kubectl/blob/master/pkg/util/hash/hash.go#L105 - we don't care about bad words...
-  private[manager] def shortHash(data: String): String = DigestUtils.sha256Hex(data).take(10)
+  def shortHash(data: String): String = DigestUtils.sha256Hex(data).take(10)
 
 }
diff --git a/...ager/src/main/scala/pl/touk/nussknacker/k8s/manager/OptionalNussknackerInstanceName.scala b/...ager/src/main/scala/pl/touk/nussknacker/k8s/manager/OptionalNussknackerInstanceName.scala
@@ -0,0 +1,24 @@
+package pl.touk.nussknacker.k8s.manager
+
+import net.ceedubs.ficus.Ficus
+import net.ceedubs.ficus.Ficus._
+import net.ceedubs.ficus.readers.ValueReader
+
+class OptionalNussknackerInstanceName private (val valueOpt: Option[String]) {
+  def isEmpty: Boolean = valueOpt.isEmpty
+
+  def objectNameWithoutSanitization(mainObjectNamePart: String): String =
+    instanceNamePrefix + mainObjectNamePart
+
+  def instanceNamePrefix: String = valueOpt.map(_ + "-").getOrElse("")
+}
+
+object OptionalNussknackerInstanceName {
+  val empty: OptionalNussknackerInstanceName = new OptionalNussknackerInstanceName(None)
+
+  // This method is for unit tests purpose only, for production usage, this name is parsed from configuration
+  def forInstanceName(value: String): OptionalNussknackerInstanceName = new OptionalNussknackerInstanceName(Some(value))
+
+  implicit val valueReader: ValueReader[OptionalNussknackerInstanceName] =
+    Ficus.optionValueReader[String].map(new OptionalNussknackerInstanceName(_))
+}
diff --git a/...ger/src/main/scala/pl/touk/nussknacker/k8s/manager/RequestResponseScenarioValidator.scala b/...ger/src/main/scala/pl/touk/nussknacker/k8s/manager/RequestResponseScenarioValidator.scala
@@ -11,7 +11,8 @@ import pl.touk.nussknacker.engine.api.process.ProcessName
 import pl.touk.nussknacker.engine.canonicalgraph.CanonicalProcess
 import pl.touk.nussknacker.k8s.manager.service.ServicePreparer
 
-class RequestResponseScenarioValidator(nussknackerInstanceName: Option[String]) extends CustomProcessValidator {
+class RequestResponseScenarioValidator(nussknackerInstanceName: OptionalNussknackerInstanceName)
+    extends CustomProcessValidator {
 
   def validate(scenario: CanonicalProcess): ValidatedNel[ProcessCompilationError, Unit] = {
     scenario.metaData.typeSpecificData match {
@@ -33,15 +34,15 @@ class RequestResponseScenarioValidator(nussknackerInstanceName: Option[String])
     // We don't sanitize / validate against url because k8s object names are more restrictively validated than urls, see https://datatracker.ietf.org/doc/html/rfc3986
     val withoutSanitization = ServicePreparer.serviceNameWithoutSanitization(nussknackerInstanceName, slug)
     val withSanitization    = ServicePreparer.serviceName(nussknackerInstanceName, slug)
-    val prefix              = K8sDeploymentManager.nussknackerInstanceNamePrefix(nussknackerInstanceName)
+    val prefix              = nussknackerInstanceName.instanceNamePrefix
     Validated.cond(
       withSanitization == withoutSanitization,
       (),
       NonEmptyList.of(
         SpecificDataValidationError(
           ParameterName(RequestResponseMetaData.slugName),
           "Allowed characters include lowercase letters, digits, hyphen, " +
-            s"name must start and end alphanumeric character, total length ${if (prefix.isEmpty) s"(including prefix '$prefix') "
+            s"name must start and end alphanumeric character, total length ${if (nussknackerInstanceName.isEmpty) s"(including prefix '$prefix') "
               else ""}cannot be more than ${K8sUtils.maxObjectNameLength}"
         )
       )

diff --git a/...mentManager/src/main/scala/pl/touk/nussknacker/k8s/manager/RequestResponseSlugUtils.scala b/...mentManager/src/main/scala/pl/touk/nussknacker/k8s/manager/RequestResponseSlugUtils.scala
@@ -2,24 +2,32 @@ package pl.touk.nussknacker.k8s.manager
 
 import pl.touk.nussknacker.engine.api.RequestResponseMetaData
 import pl.touk.nussknacker.engine.api.process.ProcessName
-import pl.touk.nussknacker.k8s.manager.service.ServicePreparer.serviceName
 
 object RequestResponseSlugUtils {
 
   private[manager] def determineSlug(
       scenarioName: ProcessName,
       rrMetaData: RequestResponseMetaData,
-      nussknackerInstanceName: Option[String]
+      nussknackerInstanceName: OptionalNussknackerInstanceName
   ) = {
     rrMetaData.slug.filterNot(_.isEmpty).getOrElse(defaultSlug(scenarioName, nussknackerInstanceName))
   }
 
   // We don't encode url because k8s object names are more restrictively validated than urls, see https://datatracker.ietf.org/doc/html/rfc3986
   // and all invalid characters will be clean
-  private[manager] def defaultSlug(scenarioName: ProcessName, nussknackerInstanceName: Option[String]): String = {
+  private[manager] def defaultSlug(
+      scenarioName: ProcessName,
+      nussknackerInstanceName: OptionalNussknackerInstanceName
+  ): String = {
     val maxSlugLength =
-      K8sUtils.maxObjectNameLength - K8sDeploymentManager.nussknackerInstanceNamePrefix(nussknackerInstanceName).length
-    serviceName(None, scenarioName.value).take(maxSlugLength)
+      K8sUtils.maxObjectNameLength - nussknackerInstanceName.instanceNamePrefix.length
+    K8sUtils
+      .sanitizeObjectName(
+        scenarioName.value,
+        firstCharacterShouldBeAlphaNumer = false, // because we will add instance name in final service name
+        lastCharacterShouldBeAlphaNumer = true,
+        maxSlugLength
+      )
   }
 
 }
diff --git a/...anager/src/main/scala/pl/touk/nussknacker/k8s/manager/deployment/DeploymentPreparer.scala b/...anager/src/main/scala/pl/touk/nussknacker/k8s/manager/deployment/DeploymentPreparer.scala
@@ -6,14 +6,14 @@ import monocle.Iso
 import monocle.macros.GenLens
 import monocle.std.option._
 import pl.touk.nussknacker.engine.api.{LiteStreamMetaData, ProcessVersion, RequestResponseMetaData, TypeSpecificData}
+import pl.touk.nussknacker.k8s.manager.{K8sDeploymentManagerConfig, OptionalNussknackerInstanceName}
 import pl.touk.nussknacker.k8s.manager.K8sDeploymentManager.{
   labelsForScenario,
   objectNameForScenario,
   scenarioIdLabel,
   scenarioVersionAnnotation,
   versionAnnotationForScenario
 }
-import pl.touk.nussknacker.k8s.manager.K8sDeploymentManagerConfig
 import skuber.{Container, EnvVar, HTTPGetAction, LabelSelector, Pod, Probe, Volume}
 import skuber.EnvVar.FieldRef
 import skuber.LabelSelector.IsEqualRequirement
@@ -58,7 +58,7 @@ class DeploymentPreparer(config: K8sDeploymentManagerConfig) extends LazyLogging
       typeSpecificData: TypeSpecificData,
       resourcesToMount: MountableResources,
       determinedReplicasCount: Int,
-      nussknackerInstanceName: Option[String]
+      nussknackerInstanceName: OptionalNussknackerInstanceName
   ) = {
     val objectName  = objectNameForScenario(processVersion, config.nussknackerInstanceName, None)
     val annotations = versionAnnotationForScenario(processVersion)

diff --git a/...ymentManager/src/main/scala/pl/touk/nussknacker/k8s/manager/ingress/IngressPreparer.scala b/...ymentManager/src/main/scala/pl/touk/nussknacker/k8s/manager/ingress/IngressPreparer.scala
@@ -4,7 +4,7 @@ import com.typesafe.config.{Config, ConfigFactory, ConfigRenderOptions}
 import monocle.macros.GenLens
 import monocle.std.option.some
 import pl.touk.nussknacker.engine.api.{LiteStreamMetaData, ProcessVersion, RequestResponseMetaData, TypeSpecificData}
-import pl.touk.nussknacker.k8s.manager.{K8sDeploymentManager, RequestResponseSlugUtils}
+import pl.touk.nussknacker.k8s.manager.{K8sDeploymentManager, OptionalNussknackerInstanceName, RequestResponseSlugUtils}
 import pl.touk.nussknacker.k8s.manager.K8sDeploymentManager.labelsForScenario
 import pl.touk.nussknacker.k8s.manager.ingress.IngressPreparer.rewriteAnnotation
 import play.api.libs.json.Json
@@ -17,7 +17,7 @@ case class IngressConfig(
     config: Config = ConfigFactory.empty()
 )
 
-class IngressPreparer(config: IngressConfig, nuInstanceName: Option[String]) {
+class IngressPreparer(config: IngressConfig, nuInstanceName: OptionalNussknackerInstanceName) {
 
   def prepare(
       processVersion: ProcessVersion,

diff --git a/...ymentManager/src/main/scala/pl/touk/nussknacker/k8s/manager/service/ServicePreparer.scala b/...ymentManager/src/main/scala/pl/touk/nussknacker/k8s/manager/service/ServicePreparer.scala
@@ -1,11 +1,11 @@
 package pl.touk.nussknacker.k8s.manager.service
 
 import pl.touk.nussknacker.engine.api.{LiteStreamMetaData, MetaData, ProcessVersion, RequestResponseMetaData}
+import pl.touk.nussknacker.k8s.manager.{K8sDeploymentManagerConfig, K8sUtils, OptionalNussknackerInstanceName}
 import pl.touk.nussknacker.k8s.manager.K8sDeploymentManager._
-import pl.touk.nussknacker.k8s.manager.K8sDeploymentManagerConfig
 import pl.touk.nussknacker.k8s.manager.K8sUtils.sanitizeObjectName
 import pl.touk.nussknacker.k8s.manager.RequestResponseSlugUtils.determineSlug
-import pl.touk.nussknacker.k8s.manager.service.ServicePreparer.{runtimePodTargetPort, serviceName}
+import pl.touk.nussknacker.k8s.manager.service.ServicePreparer.runtimePodTargetPort
 import skuber.{ObjectMeta, Service}
 import skuber.Service.Port
 
@@ -26,7 +26,7 @@ class ServicePreparer(config: K8sDeploymentManagerConfig) {
       processVersion: ProcessVersion,
       rrMetaData: RequestResponseMetaData
   ): Service = {
-    val objectName = serviceName(
+    val objectName = ServicePreparer.serviceName(
       config.nussknackerInstanceName,
       determineSlug(processVersion.processName, rrMetaData, config.nussknackerInstanceName)
     )
@@ -35,7 +35,7 @@ class ServicePreparer(config: K8sDeploymentManagerConfig) {
     val selectors = Map(
       // here we use id to avoid sanitization problems
       scenarioIdLabel -> processVersion.processId.value.toString
-    ) ++ config.nussknackerInstanceName.map(nussknackerInstanceNameLabel -> _)
+    ) ++ config.nussknackerInstanceName.valueOpt.map(nussknackerInstanceNameLabel -> _)
 
     Service(
       metadata = ObjectMeta(name = objectName, labels = labels, annotations = annotations),
@@ -54,10 +54,13 @@ object ServicePreparer {
   // see http.port in runtimes image's application.conf
   val runtimePodTargetPort = 8080
 
-  private[manager] def serviceName(nussknackerInstanceName: Option[String], slug: String): String =
-    sanitizeObjectName(serviceNameWithoutSanitization(nussknackerInstanceName, slug))
+  private[manager] def serviceName(nussknackerInstanceName: OptionalNussknackerInstanceName, slug: String): String =
+    K8sUtils.sanitizeObjectName(serviceNameWithoutSanitization(nussknackerInstanceName, slug))
 
-  private[manager] def serviceNameWithoutSanitization(nussknackerInstanceName: Option[String], slug: String): String =
-    objectNamePrefixedWithNussknackerInstanceNameWithoutSanitization(nussknackerInstanceName, slug)
+  private[manager] def serviceNameWithoutSanitization(
+      nussknackerInstanceName: OptionalNussknackerInstanceName,
+      slug: String
+  ): String =
+    nussknackerInstanceName.objectNameWithoutSanitization(slug)
 
 }