fix: add isFinalizerValid to encapsulate validation logic

Fixes fabric8io#2628
metacosm · Nov 24, 2020 · 2d16e01 · 2d16e01
1 parent e9dd426
commit 2d16e01
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,11 @@
 
 #### New Features
 
+### 5.0.0-alpha-3 (2020-11-24)
+
+#### Improvements
+* Fix #2628: Add `isFinalizerValid` method on `HasMetadata` to encapsulate validation logic
+
 ### 5.0.0-alpha-2 (2020-11-24)
 
 #### Improvements

diff --git a/...ator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/HasMetadata.java b/...ator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/HasMetadata.java
@@ -24,7 +24,7 @@ public interface HasMetadata extends KubernetesResource {
   String DNS_LABEL_START = "(?!-)[A-Za-z0-9-]{";
   String DNS_LABEL_END = ",63}(?<!-)";
   String DNS_LABEL_REGEXP = DNS_LABEL_START + 1 + DNS_LABEL_END;
-  Matcher FINALIZER_NAME_MATCHER = Pattern.compile("^(" + DNS_LABEL_REGEXP + "\\.)+" + DNS_LABEL_START + 2 + DNS_LABEL_END + "/" + DNS_LABEL_REGEXP).matcher("");
+  Pattern FINALIZER_NAME_MATCHER = Pattern.compile("^((" + DNS_LABEL_REGEXP + "\\.)+" + DNS_LABEL_START + 2 + DNS_LABEL_END + ")/" + DNS_LABEL_REGEXP);
 
   ObjectMeta getMetadata();
 
@@ -58,28 +58,46 @@ default boolean hasFinalizer(String finalizer) {
   }
 
   /**
-   * Adds the specified finalizer to this {@code HasMetadata}.
-   * <p>
-   * See https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#finalizers for more details.
+   * Adds the specified finalizer to this {@code HasMetadata} if it's valid. See {@link #isFinalizerValid(String)}.
    *
    * @param finalizer the identifier of the finalizer to add to this {@code HasMetadata} in {@code <domain name>/<finalizer name>} format.
    * @return {@code true} if the finalizer was successfully added, {@code false} otherwise (in particular, if the object is marked for deletion)
-   * @throws IllegalArgumentException if the specified is null or is invalid
+   * @throws IllegalArgumentException if the specified finalizer identifier is null or is invalid
    */
   default boolean addFinalizer(String finalizer) {
-    if (finalizer == null) {
-      throw new IllegalArgumentException("Must pass a non-null finalizer.");
+    if (finalizer == null || finalizer.trim().isEmpty()) {
+      throw new IllegalArgumentException("Must pass a non-null, non-blank finalizer.");
     }
     if (isMarkedForDeletion() || hasFinalizer(finalizer)) {
       return false;
     }
-    if (FINALIZER_NAME_MATCHER.reset(finalizer).matches()) {
+    if (isFinalizerValid(finalizer)) {
       return getMetadata().getFinalizers().add(finalizer);
     } else {
       throw new IllegalArgumentException("Invalid finalizer name: '" + finalizer + "'. Must consist of a domain name, a forward slash and the valid kubernetes name.");
     }
   }
 
+  /**
+   * Determines whether the specified finalizer is valid according to the
+   * <a href='https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#finalizer'>finalizer specification</a>.
+   * 
+   * @param finalizer the identifier of the finalizer which validity we want to check
+   * @return {@code true} if the identifier is valid, {@code false} otherwise
+   */
+  default boolean isFinalizerValid(String finalizer) {
+    if (finalizer == null) {
+      return false;
+    }
+    final Matcher matcher = FINALIZER_NAME_MATCHER.matcher(finalizer);
+    if (matcher.matches()) {
+      final String group = matcher.group(1);
+      return group.length() < 256;
+    } else {
+      return false;
+    }
+  }
+
   /**
    * Removes the specified finalizer if it's held by this {@code HasMetadata}.
    *

diff --git a/.../kubernetes-model-core/src/test/java/io/fabric8/kubernetes/api/model/HasMetadataTest.java b/.../kubernetes-model-core/src/test/java/io/fabric8/kubernetes/api/model/HasMetadataTest.java
@@ -51,12 +51,17 @@ public boolean isMarkedForDeletion() {
   void invalidFinalizersShouldFail() {
     HasMetadata hasMetadata = new Default();
     assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer(null));
+    assertFalse(hasMetadata.isFinalizerValid(null));
     assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer(""));
+    assertFalse(hasMetadata.isFinalizerValid(""));
     assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer("/"));
+    assertFalse(hasMetadata.isFinalizerValid("/"));
+    assertTrue(hasMetadata.isFinalizerValid("fabric8.io/finalizer"));
     assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer("-fabric8.io/finalizer"));
     assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer("fabric8.i/finalizer"));
     assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer("fabric8./finalizer"));
-    assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer("fabric8.label12345678901234567890123456789012345678901234567890qwertyuiopasdfghjkl/finalizer"));
+    assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer("this-label-is-too-long-12345678901234567890123456789012345678901234567890qwertyuiopasdfghjkl.io/finalizer"));
+    assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer("this.dns.name.is.way.way.too.long.more.than.255.characters.12345678901234567890.qwertyuiop.asdfghjkl.zxcvbnm.qwertyuiop.adfghjkl.zxcvbnm.mnbvcxz.lkjhgfdsa.poiuytrewq12345678901234567890.qwertyuiop.asdfghjkl.zxcvbnm.qwertyuiop.adfghjkl.zxcvbnm.mnbvcxz.lkjhgfdsa.poiuytrewq.io/finalizer"));
     assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer(".io/finalizer"));
     assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer("fabric8.io/-finalizer"));
     assertThrows(IllegalArgumentException.class, () -> hasMetadata.addFinalizer("fabric8.io/finalizer-"));