Iilun commented on code in PR #708: URL: https://github.com/apache/pekko-management/pull/708#discussion_r2965682031
########## lease-kubernetes/src/test/scala/org/apache/pekko/coordination/lease/kubernetes/MakeDNS1039CompatibleSpec.scala: ########## @@ -0,0 +1,195 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF 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. + */ + +package org.apache.pekko.coordination.lease.kubernetes + +import org.scalatest.matchers.should.Matchers +import org.scalatest.wordspec.AnyWordSpec + +class MakeDNS1039CompatibleSpec extends AnyWordSpec with Matchers { + + "makeDNS1039Compatible" should { + + "leave a simple lowercase name unchanged" in { + AbstractKubernetesLease.makeDNS1039Compatible("my-lease") shouldEqual "my-lease" + } + + "convert underscores and dots to hyphens" in { + AbstractKubernetesLease.makeDNS1039Compatible("my.lease_name") shouldEqual "my-lease-name" + } + + "strip leading and trailing hyphens after normalization" in { + AbstractKubernetesLease.makeDNS1039Compatible("-my-lease-") shouldEqual "my-lease" + } + + "remove characters that are not allowed in DNS 1039 labels" in { + AbstractKubernetesLease.makeDNS1039Compatible("my@lease!name") shouldEqual "myleasename" + } + + "convert uppercase to lowercase" in { + AbstractKubernetesLease.makeDNS1039Compatible("MyLease") shouldEqual "mylease" + } + + "truncate to 63 characters by default (no hash when hashLength is 0)" in { + val longName = "a" * 100 + AbstractKubernetesLease.makeDNS1039Compatible(longName, 63, 0).length shouldEqual 63 + } + + "truncate to a custom maxLength (no hash when hashLength is 0)" in { + val longName = "a" * 100 + AbstractKubernetesLease.makeDNS1039Compatible(longName, 40, 0).length shouldEqual 40 + } + + "trim trailing hyphens after truncation (no hash)" in { + val name = "a" * 30 + "-" + "b" * 30 + val result = AbstractKubernetesLease.makeDNS1039Compatible(name, 31, 0) + (result should not).endWith("-") + } + + "not truncate when name fits within maxLength" in { + val name63 = "a" * 63 + AbstractKubernetesLease.makeDNS1039Compatible(name63, 63, 8) shouldEqual name63 + (AbstractKubernetesLease.makeDNS1039Compatible(name63 + "extra", 63, 8) should not).equal(name63 + "extra") + } + + "append hash suffix when truncation is needed and hashLength > 0" in { + val longName = "a" * 100 + val result = AbstractKubernetesLease.makeDNS1039Compatible(longName, 63, 8) + result.length shouldEqual 63 + (result.takeRight(8) should fullyMatch).regex("[a-z2-7]{8}") + result should include("-") + } + + "produce a deterministic hash suffix for the same input" in { + val name = "my-very-long-lease-name-that-exceeds-the-maximum-allowed-kubernetes-length" + val r1 = AbstractKubernetesLease.makeDNS1039Compatible(name, 63, 8) + val r2 = AbstractKubernetesLease.makeDNS1039Compatible(name, 63, 8) + r1 shouldEqual r2 + } + + "produce different hash suffixes for different original names that truncate to the same prefix" in { + // Both names normalize to 'a' * N, but originate from different strings + val name1 = "a" * 100 + val name2 = "A" * 100 // normalizes to same 'a'*100 but is a different original + val r1 = AbstractKubernetesLease.makeDNS1039Compatible(name1, 63, 8) + val r2 = AbstractKubernetesLease.makeDNS1039Compatible(name2, 63, 8) + // The prefix is identical but hash suffixes differ because originals differ + (r1 should not).equal(r2) + } + + "produce different results for long names that differ only in the last character" in { + // Both names are 70 chars and share the first 69 chars; they truncate to the same prefix + // without a hash but must produce different DNS1039 results with one + val base = "a" * 69 + val name1 = base + "b" + val name2 = base + "c" + val r1 = AbstractKubernetesLease.makeDNS1039Compatible(name1, 63, 8) + val r2 = AbstractKubernetesLease.makeDNS1039Compatible(name2, 63, 8) + (r1 should not).equal(r2) + // Both must be valid length + r1.length shouldEqual 63 + r2.length shouldEqual 63 + } + + "produce different results for long names that differ only in the last two characters" in { + val base = "a" * 68 + val name1 = base + "bc" + val name2 = base + "de" + val r1 = AbstractKubernetesLease.makeDNS1039Compatible(name1, 63, 8) + val r2 = AbstractKubernetesLease.makeDNS1039Compatible(name2, 63, 8) + (r1 should not).equal(r2) + r1.length shouldEqual 63 + r2.length shouldEqual 63 + } + + "produce only valid DNS 1039 characters when hash suffix is added" in { + val longName = "My-Very-Long-Lease.Name_With_Special-Characters-That-Exceeds-63-Chars-Limit" + val result = AbstractKubernetesLease.makeDNS1039Compatible(longName, 63, 8) + (result should fullyMatch).regex("[a-z][a-z0-9-]*[a-z0-9]") + result.length should be <= 63 + } + + "respect a custom hashLength" in { + val longName = "a" * 100 + val result = AbstractKubernetesLease.makeDNS1039Compatible(longName, 63, 12) + result.length shouldEqual 63 + // last 12 chars are the hash suffix + (result.takeRight(12) should fullyMatch).regex("[a-z2-7]{12}") + } + + "not add hash when hashLength is 0 even if truncation occurs" in { + val longName = "a" * 100 + val result = AbstractKubernetesLease.makeDNS1039Compatible(longName, 63, 0) + result shouldEqual "a" * 63 + } + + "return only hash chars when hashLength equals maxLength" in { + val longName = "a" * 100 + // SHA-256 → 32 bytes → 52 base32 chars; take(maxLength=63) returns the full 52-char hash + // because the base32 digest (52 chars) is shorter than maxLength + val result = AbstractKubernetesLease.makeDNS1039Compatible(longName, 63, 63) + result.length should be <= 63 + (result should fullyMatch).regex("[a-z2-7]+") Review Comment: I do think this is `too few` in your example. Here the test wants to check that the input was hashed. The regex match does not guarantees this, as the input `aaaaaaaa...` will match this regex if it was only truncated, and not hashed. Checking that the input matches the regex (valid hash) and it is not the same as the truncated input (a * 63) would work here in my opinion. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
