From e7f6fa3057fd871ada09f90e49bf9ecadb6f0c44 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 8 Sep 2022 20:03:48 +0200 Subject: [PATCH] fix: provide more context when group/plural are missing (#4397) * fix: provide more context when group/plural are missing Fixes #4396 * fix: format * fix: plural should never be null * fix: remove NAME constants --- CHANGELOG.md | 2 +- .../kubernetes/model/annotation/Group.java | 2 +- .../kubernetes/model/annotation/Version.java | 12 +- .../kubernetes/api/model/HasMetadata.java | 112 +++++++++++------- 4 files changed, 74 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e8822820c..e2aad570fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ #### Improvements * Fix #4348: Introduce specific annotations for the generators * Fix #4365: The Watch retry logic will handle more cases, as well as perform an exceptional close for events that are not properly handled. Informers can directly provide those exceptional outcomes via the SharedIndexInformer.stopped CompletableFuture. +* Fix #4396: Provide more error context when @Group/@Version annotations are missing #### Dependency Upgrade * Fix #4243: Update Tekton pipeline model to v0.39.0 @@ -37,7 +38,6 @@ fix #4373: NO_PROXY should allow URIs with hyphens ("circleci-internal-outer-bui * Fix #4320: corrected leader transitions field on leader election leases * Fix #4360: JUnit dependencies aren't leaked in child modules - #### Improvements * Fix #887: added KubernetesClient.visitResources to search and perform other operations across all resources. * Fix #3960: adding a KubernetesMockServer.expectCustomResource helper method and additional mock crd support diff --git a/kubernetes-model-generator/kubernetes-model-common/src/main/java/io/fabric8/kubernetes/model/annotation/Group.java b/kubernetes-model-generator/kubernetes-model-common/src/main/java/io/fabric8/kubernetes/model/annotation/Group.java index a2b35bab85..c080563e36 100644 --- a/kubernetes-model-generator/kubernetes-model-common/src/main/java/io/fabric8/kubernetes/model/annotation/Group.java +++ b/kubernetes-model-generator/kubernetes-model-common/src/main/java/io/fabric8/kubernetes/model/annotation/Group.java @@ -27,7 +27,7 @@ * determine the `apiVersion` field associated with the annotated resource. * See https://kubernetes.io/docs/reference/using-api/#api-groups for more details. */ -@Target({TYPE}) +@Target({ TYPE }) @Retention(RUNTIME) public @interface Group { diff --git a/kubernetes-model-generator/kubernetes-model-common/src/main/java/io/fabric8/kubernetes/model/annotation/Version.java b/kubernetes-model-generator/kubernetes-model-common/src/main/java/io/fabric8/kubernetes/model/annotation/Version.java index a69586377a..6eef7eb79c 100644 --- a/kubernetes-model-generator/kubernetes-model-common/src/main/java/io/fabric8/kubernetes/model/annotation/Version.java +++ b/kubernetes-model-generator/kubernetes-model-common/src/main/java/io/fabric8/kubernetes/model/annotation/Version.java @@ -15,28 +15,28 @@ */ package io.fabric8.kubernetes.model.annotation; -import static java.lang.annotation.ElementType.TYPE; - import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; +import static java.lang.annotation.ElementType.TYPE; + /** * Allows to specify which version of the API the annotated class is defined under. Together with {@link Group}, this allows to * determine the `apiVersion` field associated with the annotated resource. * See https://kubernetes.io/docs/reference/using-api/#api-versioning for more details. */ -@Target({TYPE}) +@Target({ TYPE }) @Retention(RetentionPolicy.RUNTIME) public @interface Version { - + /** * The name of this version. * * @return the name of this version */ String value(); - + /** * Whether or not this version corresponds to the persisted version for the associated CRD. Note that only one version can set * {@code storage} to {@code true} for a given CRD. @@ -44,7 +44,7 @@ * @return {@code true} if this version corresponds to the persisted version for the associated CRD, {@code false} otherwise */ boolean storage() default true; - + /** * Whether this version is served (i.e. enabled for consumption from the REST API) or not. * diff --git a/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/HasMetadata.java b/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/HasMetadata.java index f7d82abdf2..44e4864794 100644 --- a/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/HasMetadata.java +++ b/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/HasMetadata.java @@ -22,6 +22,7 @@ import io.fabric8.kubernetes.model.annotation.Plural; import io.fabric8.kubernetes.model.annotation.Singular; import io.fabric8.kubernetes.model.annotation.Version; + import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -45,8 +46,8 @@ public interface HasMetadata extends KubernetesResource { * Pattern that checks the format of finalizer identifiers, which should be in {@code /} format. */ Pattern FINALIZER_NAME_MATCHER = Pattern.compile( - "^((" + DNS_LABEL_REGEXP + "\\.)+" + DNS_LABEL_START + 2 + DNS_LABEL_END + ")/" - + DNS_LABEL_REGEXP); + "^((" + DNS_LABEL_REGEXP + "\\.)+" + DNS_LABEL_START + 2 + DNS_LABEL_END + ")/" + + DNS_LABEL_REGEXP); ObjectMeta getMetadata(); @@ -84,8 +85,8 @@ static String getApiVersion(Class clazz) { } if (group != null || version != null) { throw new IllegalArgumentException( - "You need to specify both @" + Group.class.getSimpleName() + " and @" - + Version.class.getSimpleName() + " annotations if you specify either"); + "You need to specify both @" + Group.class.getSimpleName() + " and @" + Version.class.getSimpleName() + + " annotations if you specify either"); } return null; } @@ -129,7 +130,7 @@ default String getApiVersion() { static String getPlural(Class clazz) { final Plural fromAnnotation = clazz.getAnnotation(Plural.class); return (fromAnnotation != null ? fromAnnotation.value().toLowerCase(Locale.ROOT) - : Pluralize.toPlural(getSingular(clazz))); + : Pluralize.toPlural(getSingular(clazz))); } @JsonIgnore @@ -144,12 +145,12 @@ default String getPlural() { * * @param clazz the class whose singular form we want to retrieve * @return the singular form defined by the {@link Singular} annotation or a computed default - * value + * value */ static String getSingular(Class clazz) { final Singular fromAnnotation = clazz.getAnnotation(Singular.class); return (fromAnnotation != null ? fromAnnotation.value() : HasMetadata.getKind(clazz)) - .toLowerCase(Locale.ROOT); + .toLowerCase(Locale.ROOT); } @JsonIgnore @@ -158,7 +159,14 @@ default String getSingular() { } static String getFullResourceName(Class clazz) { - return getFullResourceName(getPlural(clazz), getGroup(clazz)); + final String plural = getPlural(clazz); + final String group = getGroup(clazz); + if (group == null) { + throw new IllegalArgumentException( + "Should provide non-null group. Is " + clazz.getName() + " properly annotated with @" + + Group.class.getSimpleName() + " and/or @" + Version.class.getSimpleName() + "?"); + } + return getFullResourceName(plural, group); } static String getFullResourceName(String plural, String group) { @@ -168,6 +176,7 @@ static String getFullResourceName(String plural, String group) { } @JsonIgnore + @SuppressWarnings("unused") default String getFullResourceName() { return getFullResourceName(getClass()); } @@ -180,7 +189,7 @@ default String getFullResourceName() { @JsonIgnore default boolean isMarkedForDeletion() { final String deletionTimestamp = optionalMetadata().map(ObjectMeta::getDeletionTimestamp) - .orElse(null); + .orElse(null); return deletionTimestamp != null && !deletionTimestamp.isEmpty(); } @@ -202,8 +211,10 @@ default List getFinalizers() { /** * 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 /} format. - * @return {@code true} if the finalizer was successfully added, {@code false} otherwise (in particular, if the object is marked for deletion) + * @param finalizer the identifier of the finalizer to add to this {@code HasMetadata} in + * {@code /} 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 finalizer identifier is null or is invalid */ default boolean addFinalizer(String finalizer) { @@ -222,13 +233,14 @@ default boolean addFinalizer(String finalizer) { return metadata.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."); + + "'. Must consist of a domain name, a forward slash and the valid kubernetes name."); } } /** * Determines whether the specified finalizer is valid according to the - * finalizer + * finalizer * specification. * * @param finalizer the identifier of the finalizer which validity we want to check @@ -246,6 +258,7 @@ static boolean validateFinalizer(String finalizer) { return false; } } + /** * @see HasMetadata#validateFinalizer(String) * @@ -253,7 +266,7 @@ static boolean validateFinalizer(String finalizer) { * @return {@code true} if the identifier is valid, {@code false} otherwise */ default boolean isFinalizerValid(String finalizer) { - return HasMetadata.validateFinalizer(finalizer); + return HasMetadata.validateFinalizer(finalizer); } /** @@ -291,7 +304,8 @@ default boolean hasOwnerReferenceFor(String ownerUid) { * Retrieves the {@link OwnerReference} associated with the specified owner if it's part of this {@code HasMetadata}'s owners. * * @param owner the potential owner of which we want to retrieve the associated {@link OwnerReference} - * @return an {@link Optional} containing the {@link OwnerReference} associated with the specified owner if it exists or {@link Optional#empty()} otherwise. + * @return an {@link Optional} containing the {@link OwnerReference} associated with the specified owner if it exists or + * {@link Optional#empty()} otherwise. */ default Optional getOwnerReferenceFor(HasMetadata owner) { if (owner == null) { @@ -303,12 +317,13 @@ default Optional getOwnerReferenceFor(HasMetadata owner) { } /** - * Retrieves the {@link OwnerReference} associated with the owner identified by the specified UID if it's part of this{@code HasMetadata}'s owners. + * Retrieves the {@link OwnerReference} associated with the owner identified by the specified UID if it's part of + * this{@code HasMetadata}'s owners. * * @param ownerUid the UID of the potential owner of which we want to retrieve the associated {@link - * OwnerReference} + * OwnerReference} * @return an {@link Optional} containing the {@link OwnerReference} associated with the - * owner identified by the specified UID if it exists or {@link Optional#empty()} otherwise. + * owner identified by the specified UID if it exists or {@link Optional#empty()} otherwise. */ default Optional getOwnerReferenceFor(String ownerUid) { if (ownerUid == null || ownerUid.isEmpty()) { @@ -316,11 +331,11 @@ default Optional getOwnerReferenceFor(String ownerUid) { } return optionalMetadata() - .map(m -> Optional.ofNullable(m.getOwnerReferences()).orElse(Collections.emptyList())) - .orElse(Collections.emptyList()) - .stream() - .filter(o -> ownerUid.equals(o.getUid())) - .findFirst(); + .map(m -> Optional.ofNullable(m.getOwnerReferences()).orElse(Collections.emptyList())) + .orElse(Collections.emptyList()) + .stream() + .filter(o -> ownerUid.equals(o.getUid())) + .findFirst(); } /** @@ -332,36 +347,38 @@ default Optional getOwnerReferenceFor(String ownerUid) { default OwnerReference addOwnerReference(HasMetadata owner) { if (owner == null) { throw new IllegalArgumentException("Cannot add a reference to a null owner to " - + optionalMetadata().map(m -> "'" + m.getName() + "' ").orElse("unnamed ") - + getKind()); + + optionalMetadata().map(m -> "'" + m.getName() + "' ").orElse("unnamed ") + + getKind()); } final ObjectMeta metadata = owner.getMetadata(); if (metadata == null) { throw new IllegalArgumentException("Cannot add a reference to an owner without metadata to " - + optionalMetadata().map(m -> "'" + m.getName() + "' ").orElse("unnamed ") - + getKind()); + + optionalMetadata().map(m -> "'" + m.getName() + "' ").orElse("unnamed ") + + getKind()); } final OwnerReference ownerReference = new OwnerReferenceBuilder() - .withUid(metadata.getUid()) - .withApiVersion(owner.getApiVersion()) - .withName(metadata.getName()) - .withKind(owner.getKind()) - .build(); + .withUid(metadata.getUid()) + .withApiVersion(owner.getApiVersion()) + .withName(metadata.getName()) + .withKind(owner.getKind()) + .build(); return addOwnerReference(sanitizeAndValidate(ownerReference)); } /** - * Adds the specified, supposed valid (for example because validated by calling {@link #sanitizeAndValidate(OwnerReference)} beforehand), {@link OwnerReference} to this {@code HasMetadata} + * Adds the specified, supposed valid (for example because validated by calling {@link #sanitizeAndValidate(OwnerReference)} + * beforehand), {@link OwnerReference} to this {@code HasMetadata} + * * @param ownerReference the {@link OwnerReference} to add to this {@code HasMetadata}'s owner references * @return the newly added {@link OwnerReference} */ default OwnerReference addOwnerReference(OwnerReference ownerReference) { if (ownerReference == null) { throw new IllegalArgumentException("Cannot add a null reference to " - + optionalMetadata().map(m -> "'" + m.getName() + "' ").orElse("unnamed ") - + getKind()); + + optionalMetadata().map(m -> "'" + m.getName() + "' ").orElse("unnamed ") + + getKind()); } final Optional existing = getOwnerReferenceFor(ownerReference.getUid()); @@ -385,6 +402,7 @@ default OwnerReference addOwnerReference(OwnerReference ownerReference) { /** * Sanitizes and validates the specified {@link OwnerReference}, presumably to add it + * * @param ownerReference the {@link OwnerReference} to sanitize and validate * @return the sanitized and validated {@link OwnerReference} which should be used instead of the original one */ @@ -410,34 +428,36 @@ static OwnerReference sanitizeAndValidate(OwnerReference ownerReference) { } }; final Supplier exceptionSupplier = () -> new IllegalArgumentException( - error.toString()); + error.toString()); final Optional uid = trimmedFieldIfValid.apply("uid", ownerReference.getUid()); final Optional apiVersion = trimmedFieldIfValid.apply("apiVersion", - ownerReference.getApiVersion()); + ownerReference.getApiVersion()); final Optional name = trimmedFieldIfValid.apply("name", ownerReference.getName()); final Optional kind = trimmedFieldIfValid.apply("kind", ownerReference.getKind()); // check that required values are present ownerReference = new OwnerReferenceBuilder(ownerReference) - .withUid(uid.orElseThrow(exceptionSupplier)) - .withApiVersion(apiVersion.orElseThrow(exceptionSupplier)) - .withName(name.orElseThrow(exceptionSupplier)) - .withKind(kind.orElseThrow(exceptionSupplier)) - .build(); + .withUid(uid.orElseThrow(exceptionSupplier)) + .withApiVersion(apiVersion.orElseThrow(exceptionSupplier)) + .withName(name.orElseThrow(exceptionSupplier)) + .withKind(kind.orElseThrow(exceptionSupplier)) + .build(); return ownerReference; } /** - * Removes the {@link OwnerReference} identified by the specified UID if it's part of this {@code HasMetadata}'s owner references + * Removes the {@link OwnerReference} identified by the specified UID if it's part of this {@code HasMetadata}'s owner + * references + * * @param ownerUid the UID of the {@link OwnerReference} to remove */ default void removeOwnerReference(String ownerUid) { if (ownerUid != null && !ownerUid.isEmpty()) { optionalMetadata() - .map(m -> Optional.ofNullable(m.getOwnerReferences()).orElse(Collections.emptyList())) - .orElse(Collections.emptyList()) - .removeIf(o -> ownerUid.equals(o.getUid())); + .map(m -> Optional.ofNullable(m.getOwnerReferences()).orElse(Collections.emptyList())) + .orElse(Collections.emptyList()) + .removeIf(o -> ownerUid.equals(o.getUid())); } }