From dd52b87e96bdb04081c372c3193c13f29623ecd6 Mon Sep 17 00:00:00 2001 From: Beibei Chen Date: Fri, 5 Jan 2024 10:25:53 -0600 Subject: [PATCH 1/4] update regex --- .../google/batch/models/GcpLabel.scala | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpLabel.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpLabel.scala index 303b2ec85f2..5dabccea425 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpLabel.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpLabel.scala @@ -18,13 +18,15 @@ final case class GcpLabel(key: String, value: String) object GcpLabel { val MaxLabelLength = 63 - val GoogleLabelRegexPattern = "[a-z]([-a-z0-9]*[a-z0-9])?" - val GoogleLabelRegex = GoogleLabelRegexPattern.r + val GoogleLabelKeyRegexPattern = "[a-z]([-a-z_0-9]*[a-z0-9])?" // label key must start with letter + val GoogleLabelValueRegexPattern = "[a-z0-9]([-a-z_0-9]*[a-z0-9])?" + val GoogleLabelKeyRegex = GoogleLabelKeyRegexPattern.r + val GoogleLabelValueRegex = GoogleLabelValueRegexPattern.r - // This function is used to coerce a string into one that meets the requirements for a label submission to Google Pipelines API. - // See 'labels' in https://cloud.google.com/genomics/reference/rpc/google.genomics.v1alpha2#google.genomics.v1alpha2.RunPipelineArgs + // This function is used to coerce a string into one that meets the requirements for a label submission to Google Batch API. + // https://cloud.google.com/compute/docs/labeling-resources#requirements def safeGoogleName(mainText: String, emptyAllowed: Boolean = false): String = - validateLabelRegex(mainText) match { + validateLabelRegex(mainText, true) match { case Valid(labelText) => labelText case invalid @ _ if mainText.equals("") && emptyAllowed => mainText case invalid @ _ => @@ -57,15 +59,19 @@ object GcpLabel { } } - def validateLabelRegex(s: String): ErrorOr[String] = - (GoogleLabelRegex.pattern.matcher(s).matches, s.length <= MaxLabelLength) match { + def validateLabelRegex(s: String, isKey: Boolean): ErrorOr[String] = { + val regexPattern = if (isKey) GoogleLabelKeyRegex.pattern else GoogleLabelValueRegex.pattern + val field = if (isKey) "label key" else "label value" + + (regexPattern.matcher(s).matches, s.length <= MaxLabelLength) match { case (true, true) => s.validNel case (false, false) => - s"Invalid label field: `$s` did not match regex '$GoogleLabelRegexPattern' and it is ${s.length} characters. The maximum is $MaxLabelLength.".invalidNel - case (false, _) => s"Invalid label field: `$s` did not match the regex '$GoogleLabelRegexPattern'".invalidNel + s"Invalid $field field: `$s` did not match regex '$regexPattern' and it is ${s.length} characters. The maximum is $MaxLabelLength.".invalidNel + case (false, _) => s"Invalid $field field: `$s` did not match the regex '$regexPattern'".invalidNel case (_, false) => - s"Invalid label field: `$s` is ${s.length} characters. The maximum is $MaxLabelLength.".invalidNel + s"Invalid $field field: `$s` is ${s.length} characters. The maximum is $MaxLabelLength.".invalidNel } + } def safeLabels(values: (String, String)*): Seq[GcpLabel] = { def safeGoogleLabel(kvp: (String, String)): GcpLabel = @@ -74,7 +80,7 @@ object GcpLabel { } def validateLabel(key: String, value: String): ErrorOr[GcpLabel] = - (validateLabelRegex(key), validateLabelRegex(value)).mapN { (validKey, validValue) => + (validateLabelRegex(key, true), validateLabelRegex(value, false)).mapN { (validKey, validValue) => GcpLabel(validKey, validValue) } From 40f1713d3584b0445877035e5bb48d3aa1c1c9b5 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 9 Jan 2024 18:16:01 -0500 Subject: [PATCH 2/4] Fix test --- .../backend/google/batch/models/GcpBatchLabelSpec.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala index 8ae97f4d0ca..53c1091ac5c 100644 --- a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala +++ b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala @@ -25,7 +25,7 @@ class GcpBatchLabelSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matche googleLabelConversions foreach { case (label: String, conversion: String) => it should s"not validate the bad label key '$label'" in { - GcpLabel.validateLabelRegex(label) match { + GcpLabel.validateLabelRegex(label, true) match { case Invalid(_) => // Good! case Valid(_) => fail(s"Label validation succeeded but should have failed.") } From c75b576b1003d1a2ca2155b5b36970cdc3ecd76b Mon Sep 17 00:00:00 2001 From: Beibei Chen Date: Thu, 11 Jan 2024 13:41:44 -0600 Subject: [PATCH 3/4] remove a test case that should be valid now --- .../backend/google/batch/models/GcpBatchLabelSpec.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala index 53c1091ac5c..22e065577e6 100644 --- a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala +++ b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala @@ -18,7 +18,7 @@ class GcpBatchLabelSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matche "cromwell-root-workflow-id-" -> "cromwell-root-workflow-id---x", "0-cromwell-root-workflow-id-" -> "x--0-cromwell-root-workflow-id---x", "Cromwell-root-workflow-id" -> "cromwell-root-workflow-id", - "cromwell_root_workflow_id" -> "cromwell-root-workflow-id", + // "cromwell_root_workflow_id" -> "cromwell-root-workflow-id", "too-long-too-long-too-long-too-long-too-long-too-long-too-long-t" -> "too-long-too-long-too-long-too---g-too-long-too-long-too-long-t", "0-too-long-and-invalid-too-long-and-invalid-too-long-and-invali+" -> "x--0-too-long-and-invalid-too----nvalid-too-long-and-invali---x" ) From 2bd9e6bba00548ea6796f6e6626a28ff1af2d7b6 Mon Sep 17 00:00:00 2001 From: Beibei Chen Date: Thu, 11 Jan 2024 14:17:31 -0600 Subject: [PATCH 4/4] remove test case that is valid after update --- .../cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala index 22e065577e6..a0dcb31a8b6 100644 --- a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala +++ b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchLabelSpec.scala @@ -18,7 +18,6 @@ class GcpBatchLabelSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matche "cromwell-root-workflow-id-" -> "cromwell-root-workflow-id---x", "0-cromwell-root-workflow-id-" -> "x--0-cromwell-root-workflow-id---x", "Cromwell-root-workflow-id" -> "cromwell-root-workflow-id", - // "cromwell_root_workflow_id" -> "cromwell-root-workflow-id", "too-long-too-long-too-long-too-long-too-long-too-long-too-long-t" -> "too-long-too-long-too-long-too---g-too-long-too-long-too-long-t", "0-too-long-and-invalid-too-long-and-invalid-too-long-and-invali+" -> "x--0-too-long-and-invalid-too----nvalid-too-long-and-invali---x" )