feat: improve RBAC generation for typeless dependents

annotations/src/main/java/io/quarkiverse/operatorsdk/annotations/RBACVerbs.java

@@ -12,6 +12,7 @@ public class RBACVerbs {
     public static final String WATCH = "watch";
     public static final String DELETE = "delete";
     public static final String[] UPDATE_VERBS = new String[] { PATCH, UPDATE };
+    public static final String[] CREATE_VERBS = new String[] { CREATE, PATCH };
     public static final String[] READ_VERBS = new String[] { GET, LIST, WATCH };
     public static final String[] ALL_COMMON_VERBS;
 

common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ConfigurationUtils.java

@@ -80,7 +80,12 @@ public static <T> ClassInfo getClassInfoForInstantiation(AnnotationValue toInsta
             Class<T> interfaceClass,
             IndexView index) {
         final var expectedTypeDN = toInstantiate.asClass().name();
-        return index.getClassByName(expectedTypeDN);
+        final var clazz = index.getClassByName(expectedTypeDN);
+        if (clazz == null) {
+            throw new IllegalStateException(expectedTypeDN
+                    + " class was not found in Jandex index. If you see this in a test, don't forget to add the class to the application root when setting up the test.");
+        }
+        return clazz;
     }
 
     @SuppressWarnings("unchecked")

core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/AddClusterRolesDecorator.java

@@ -1,17 +1,25 @@
 package io.quarkiverse.operatorsdk.deployment;
 
-import java.util.*;
-import java.util.function.Function;
+import java.util.Collection;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
 
-import org.jetbrains.annotations.NotNull;
+import org.jboss.logging.Logger;
 
 import io.dekorate.kubernetes.decorator.ResourceProvidingDecorator;
+import io.fabric8.kubernetes.api.Pluralize;
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.api.model.KubernetesListBuilder;
 import io.fabric8.kubernetes.api.model.rbac.ClusterRole;
 import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBuilder;
 import io.fabric8.kubernetes.api.model.rbac.PolicyRule;
 import io.fabric8.kubernetes.api.model.rbac.PolicyRuleBuilder;
+import io.javaoperatorsdk.operator.api.config.Utils;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
 import io.javaoperatorsdk.operator.processing.dependent.Creator;
 import io.javaoperatorsdk.operator.processing.dependent.Updater;
@@ -32,6 +40,7 @@ public class AddClusterRolesDecorator extends ResourceProvidingDecorator<Kuberne
                     .build());
     private static final String CR_API_VERSION = HasMetadata.getApiVersion(ClusterRole.class);
     private static final String CR_KIND = HasMetadata.getKind(ClusterRole.class);
+    private static final Logger log = Logger.getLogger(AddClusterRolesDecorator.class);
     private final Collection<QuarkusControllerConfiguration<?>> configs;
 
     private final boolean validateCRDs;
@@ -57,21 +66,40 @@ public void visit(KubernetesListBuilder list) {
     }
 
     public static ClusterRole createClusterRole(QuarkusControllerConfiguration<?> cri) {
+        final var rules = new LinkedHashMap<String, PolicyRule>();
+        final var clusterRolePolicyRuleFromPrimaryResource = getClusterRolePolicyRuleFromPrimaryResource(cri);
+        final var primaryRuleKey = getKeyFor(clusterRolePolicyRuleFromPrimaryResource);
+        rules.put(primaryRuleKey, clusterRolePolicyRuleFromPrimaryResource);
 
-        Set<PolicyRule> collectedRules = new LinkedHashSet<>();
-        collectedRules.add(getClusterRolePolicyRuleFromPrimaryResource(cri));
-        collectedRules.addAll(getClusterRolePolicyRulesFromDependentResources(cri));
-        collectedRules.addAll(cri.getAdditionalRBACRules());
+        collectAndMergeIfNeededRulesFrom(getClusterRolePolicyRulesFromDependentResources(cri), rules);
+        collectAndMergeIfNeededRulesFrom(cri.getAdditionalRBACRules(), rules);
 
         return new ClusterRoleBuilder()
                 .withNewMetadata()
                 .withName(getClusterRoleName(cri.getName()))
                 .endMetadata()
-                .addAllToRules(mergePolicyRulesOfSameGroupsAndKinds(collectedRules))
+                .addAllToRules(rules.values())
                 .build();
     }
 
-    @NotNull
+    private static void collectAndMergeIfNeededRulesFrom(Collection<PolicyRule> newRules,
+            Map<String, PolicyRule> existingRules) {
+        newRules.forEach(newPolicyRule -> {
+            final var key = getKeyFor(newPolicyRule);
+            existingRules.merge(key, newPolicyRule, (existing, npr) -> {
+                Set<String> verbs1 = new TreeSet<>(existing.getVerbs());
+                verbs1.addAll(npr.getVerbs());
+                existing.setVerbs(verbs1.stream().toList());
+                return existing;
+            });
+        });
+    }
+
+    private static String getKeyFor(PolicyRule rule) {
+        return rule.getApiGroups().stream().sorted().collect(Collectors.joining("-")) + "/"
+                + rule.getResources().stream().sorted().collect(Collectors.joining("-"));
+    }
+
     private static Set<PolicyRule> getClusterRolePolicyRulesFromDependentResources(QuarkusControllerConfiguration<?> cri) {
         Set<PolicyRule> rules = new LinkedHashSet<>();
         final Map<String, DependentResourceSpecMetadata<?, ?, ?>> dependentsMetadata = cri.getDependentsMetadata();
@@ -81,42 +109,37 @@ private static Set<PolicyRule> getClusterRolePolicyRulesFromDependentResources(Q
 
             // only process Kubernetes dependents
             if (HasMetadata.class.isAssignableFrom(associatedResourceClass)) {
-                String resourceGroup = HasMetadata.getGroup(associatedResourceClass);
-                String resourcePlural = HasMetadata.getPlural(associatedResourceClass);
-
-                // https://github.com/operator-framework/java-operator-sdk/pull/2515
-                // Workaround for typeless resource, no necessary when this pull merged
+                var resourceGroup = HasMetadata.getGroup(associatedResourceClass);
+                var resourcePlural = HasMetadata.getPlural(associatedResourceClass);
                 if (GenericKubernetesDependentResource.class.isAssignableFrom(dependentResourceClass)) {
                     try {
-                        // Only applied class with non-parameter constructor
-                        if (Arrays.stream(dependentResourceClass.getConstructors()).anyMatch(i -> i.getParameterCount() == 0)) {
-                            @SuppressWarnings("rawtypes")
-                            GenericKubernetesDependentResource genericKubernetesResource = (GenericKubernetesDependentResource) dependentResourceClass
-                                    .getConstructor().newInstance();
-                            resourceGroup = genericKubernetesResource.getGroupVersionKind().getGroup();
-                            resourcePlural = "*";
-                        }
+                        @SuppressWarnings({ "unchecked", "rawtypes" })
+                        final var genericKubernetesResource = Utils.instantiate(
+                                (Class<? extends GenericKubernetesDependentResource>) dependentResourceClass,
+                                GenericKubernetesDependentResource.class, "AddClusterRolesDecorator");
+                        final var gvk = genericKubernetesResource.getGroupVersionKind();
+                        resourceGroup = gvk.getGroup();
+                        // todo: use plural form on GVK if available, see https://github.com/operator-framework/java-operator-sdk/pull/2515
+                        resourcePlural = Pluralize.toPlural(gvk.getKind());
                     } catch (Exception e) {
-                        throw new RuntimeException(e);
+                        log.warn("Ignoring " + dependentResourceClass.getName()
+                                + " for generic resource role processing as it cannot be instantiated", e);
                     }
                 }
-
+                final var verbs = new TreeSet<>(List.of(RBACVerbs.READ_VERBS));
                 final var dependentRule = new PolicyRuleBuilder()
                         .addToApiGroups(resourceGroup)
-                        .addToResources(resourcePlural)
-                        .addToVerbs(RBACVerbs.READ_VERBS);
+                        .addToResources(resourcePlural);
                 if (Updater.class.isAssignableFrom(dependentResourceClass)) {
-                    dependentRule.addToVerbs(RBACVerbs.UPDATE_VERBS);
+                    verbs.addAll(List.of(RBACVerbs.UPDATE_VERBS));
                 }
                 if (Deleter.class.isAssignableFrom(dependentResourceClass)) {
-                    dependentRule.addToVerbs(RBACVerbs.DELETE);
+                    verbs.add(RBACVerbs.DELETE);
                 }
                 if (Creator.class.isAssignableFrom(dependentResourceClass)) {
-                    dependentRule.addToVerbs(RBACVerbs.CREATE);
-                    if (!dependentRule.getVerbs().contains(RBACVerbs.PATCH)) {
-                        dependentRule.addToVerbs(RBACVerbs.PATCH);
-                    }
+                    verbs.addAll(List.of(RBACVerbs.CREATE_VERBS));
                 }
+                dependentRule.addToVerbs(verbs.toArray(String[]::new));
                 rules.add(dependentRule.build());
             }
         });
@@ -144,57 +167,6 @@ private static PolicyRule getClusterRolePolicyRuleFromPrimaryResource(QuarkusCon
         return rule.build();
     }
 
-    /**
-     * Remove duplicated rules with same groups and resources, from which merge all verbs
-     *
-     * @param collectedRules may contain duplicated rules with same groups and resources, but different verbs
-     * @return no duplicated rules
-     */
-    @NotNull
-    private static Set<PolicyRule> mergePolicyRulesOfSameGroupsAndKinds(Set<PolicyRule> collectedRules) {
-        Set<PolicyRule> mergedRules = new LinkedHashSet<>();
-        collectedRules.stream()
-                .map(wrapEqualOfGroupsAndKinds()).forEach(i -> {
-                    if (!mergedRules.add(i)) {
-                        mergedRules.stream().filter(j -> Objects.equals(j, i)).findAny().ifPresent(r -> {
-                            Set<String> verbs1 = new LinkedHashSet<>(r.getVerbs());
-                            Set<String> verbs2 = new LinkedHashSet<>(i.getVerbs());
-                            verbs1.addAll(verbs2);
-                            r.setVerbs(verbs1.stream().toList());
-                        });
-                    }
-                });
-        return mergedRules;
-    }
-
-    @NotNull
-    private static Function<PolicyRule, PolicyRule> wrapEqualOfGroupsAndKinds() {
-        return i -> new PolicyRule(i.getApiGroups(), i.getNonResourceURLs(), i.getResourceNames(), i.getResources(),
-                i.getVerbs()) {
-            @Override
-            public boolean equals(Object o) {
-                if (o == null)
-                    return false;
-                if (o instanceof PolicyRule) {
-                    if (Objects.equals(
-                            this.getApiGroups().stream().sorted().toList(),
-                            ((PolicyRule) o).getApiGroups().stream().sorted().toList())) {
-                        return Objects.equals(
-                                getResources().stream().sorted().toList(),
-                                ((PolicyRule) o).getResources().stream().sorted().toList());
-                    }
-                }
-                return false;
-            }
-
-            @Override
-            public int hashCode() {
-                // equals method called only with same hashCode
-                return 0;
-            }
-        };
-    }
-
     public static String getClusterRoleName(String controller) {
         return controller + "-cluster-role";
     }

core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/OperatorSDKTest.java

@@ -14,27 +14,42 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
+import io.fabric8.kubernetes.api.Pluralize;
 import io.fabric8.kubernetes.api.model.ConfigMap;
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.api.model.Secret;
 import io.fabric8.kubernetes.api.model.Service;
 import io.fabric8.kubernetes.api.model.ServiceAccount;
 import io.fabric8.kubernetes.api.model.rbac.ClusterRole;
 import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBinding;
+import io.fabric8.kubernetes.api.model.rbac.PolicyRule;
 import io.fabric8.kubernetes.api.model.rbac.RoleBinding;
 import io.fabric8.kubernetes.client.CustomResource;
 import io.fabric8.kubernetes.client.utils.Serialization;
 import io.quarkiverse.operatorsdk.annotations.RBACRule;
 import io.quarkiverse.operatorsdk.annotations.RBACVerbs;
 import io.quarkiverse.operatorsdk.deployment.AddClusterRolesDecorator;
 import io.quarkiverse.operatorsdk.deployment.AddRoleBindingsDecorator;
-import io.quarkiverse.operatorsdk.test.sources.*;
+import io.quarkiverse.operatorsdk.test.sources.CRUDConfigMap;
+import io.quarkiverse.operatorsdk.test.sources.CreateOnlyService;
+import io.quarkiverse.operatorsdk.test.sources.Foo;
+import io.quarkiverse.operatorsdk.test.sources.NonKubeResource;
+import io.quarkiverse.operatorsdk.test.sources.ReadOnlySecret;
+import io.quarkiverse.operatorsdk.test.sources.SimpleCR;
+import io.quarkiverse.operatorsdk.test.sources.SimpleReconciler;
+import io.quarkiverse.operatorsdk.test.sources.SimpleSpec;
+import io.quarkiverse.operatorsdk.test.sources.SimpleStatus;
+import io.quarkiverse.operatorsdk.test.sources.TestCR;
+import io.quarkiverse.operatorsdk.test.sources.TestReconciler;
+import io.quarkiverse.operatorsdk.test.sources.TypelessAnotherKubeResource;
+import io.quarkiverse.operatorsdk.test.sources.TypelessKubeResource;
 import io.quarkus.test.ProdBuildResults;
 import io.quarkus.test.ProdModeTestResults;
 import io.quarkus.test.QuarkusProdModeTest;
 
 public class OperatorSDKTest {
 
+    public static final List<String> READ_VERBS_LIST = Arrays.asList(READ_VERBS);
     private static final String APPLICATION_NAME = "test";
     // Start unit test with your extension loaded
     @RegisterExtension
@@ -49,6 +64,22 @@ public class OperatorSDKTest {
     @ProdBuildResults
     private ProdModeTestResults prodModeTestResults;
 
+    private static boolean hasReadAndAdditionalVerbsOnly(PolicyRule rule, String... additionalVerbs) {
+        final var verbs = rule.getVerbs();
+        return (verbs.size() == READ_VERBS_LIST.size() + additionalVerbs.length) && verbs.containsAll(READ_VERBS_LIST)
+                && verbs.containsAll(List.of(additionalVerbs));
+    }
+
+    private static boolean isReadOnly(PolicyRule rule) {
+        return rule.getVerbs().equals(READ_VERBS_LIST);
+    }
+
+    private static boolean hasOnlyCommonVerbs(PolicyRule rule) {
+        final var verbs = rule.getVerbs();
+        return verbs.size() == ALL_COMMON_VERBS.length
+                && verbs.containsAll(Arrays.asList(ALL_COMMON_VERBS));
+    }
+
     @Test
     public void shouldCreateRolesAndRoleBindings() throws IOException {
         final var kubernetesDir = prodModeTestResults.getBuildDir().resolve("kubernetes");
@@ -82,33 +113,27 @@ public void shouldCreateRolesAndRoleBindings() throws IOException {
                             }));
                     assertTrue(rules.stream()
                             .filter(rule -> rule.getResources().equals(List.of(HasMetadata.getPlural(Secret.class))))
-                            .anyMatch(rule -> rule.getVerbs().equals(Arrays.asList(READ_VERBS))));
+                            .anyMatch(OperatorSDKTest::isReadOnly));
                     assertTrue(rules.stream()
                             .filter(rule -> rule.getResources().equals(List.of(HasMetadata.getPlural(
                                     Service.class))))
-                            .anyMatch(rule -> rule.getVerbs().containsAll(Arrays.asList(READ_VERBS))
-                                    && rule.getVerbs().contains(CREATE)));
+                            .anyMatch(rule -> hasReadAndAdditionalVerbsOnly(rule, CREATE_VERBS)));
                     assertTrue(rules.stream()
                             .filter(rule -> rule.getResources().equals(List.of(HasMetadata.getPlural(ConfigMap.class))))
-                            .anyMatch(rule -> {
-                                final var verbs = rule.getVerbs();
-                                return verbs.size() == ALL_COMMON_VERBS.length
-                                        && verbs.containsAll(Arrays.asList(ALL_COMMON_VERBS));
-                            }));
+                            .anyMatch(OperatorSDKTest::hasOnlyCommonVerbs));
                     assertTrue(rules.stream()
                             .filter(rule -> rule.getResources().equals(List.of(RBACRule.ALL)))
                             .anyMatch(rule -> rule.getVerbs().equals(List.of(UPDATE))
                                     && rule.getApiGroups().equals(List.of(RBACRule.ALL))));
-
-                    // TODO: need update, https://github.com/operator-framework/java-operator-sdk/pull/2515
-                    // expected generic kubernetes resource: apiGroups is Group from GVK and resources should be '*'
-                    // verbs should contain merged 'delete'
-                    // count should be 1, as TypelessKubeResource and TypelessAnotherKubeResource have same GROUP
+                    // Both typeless dependents are using the same GVK so the verbs associated with their policy rules should be merged into a single one
+                    assertEquals(1, rules.stream()
+                            .filter(rule -> rule.getApiGroups().equals(List.of(TypelessKubeResource.GROUP))
+                                    && rule.getResources().equals(List.of(Pluralize.toPlural(TypelessKubeResource.KIND))))
+                            .count());
                     assertTrue(rules.stream()
-                            .filter(rule -> rule.getApiGroups().equals(List.of(TypelessKubeResource.GROUP)))
-                            .filter(rule -> rule.getResources().equals(List.of("*")))
-                            .filter(rule -> rule.getVerbs().contains("delete"))
-                            .count() == 1);
+                            .filter(rule -> rule.getApiGroups().equals(List.of(TypelessKubeResource.GROUP))
+                                    && rule.getResources().equals(List.of(Pluralize.toPlural(TypelessKubeResource.KIND))))
+                            .allMatch(rule -> hasReadAndAdditionalVerbsOnly(rule, DELETE)));
                 });
 
         // check that we have a role binding for TestReconciler and that it uses the operator-level specified namespace

core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TypelessAnotherKubeResource.java

@@ -1,14 +1,13 @@
 package io.quarkiverse.operatorsdk.test.sources;
 
+import static io.quarkiverse.operatorsdk.test.sources.TypelessKubeResource.*;
+
 import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
 import io.javaoperatorsdk.operator.processing.GroupVersionKind;
 import io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesDependentResource;
 
 public class TypelessAnotherKubeResource extends GenericKubernetesDependentResource<TestCR> implements Deleter<TestCR> {
 
-    public static final String GROUP = "crd.josdk.quarkiverse.io";
-    public static final String KIND = "typelessAnother";
-    public static final String VERSION = "v1";
     private static final GroupVersionKind GVK = new GroupVersionKind(GROUP, VERSION, KIND);
 
     public TypelessAnotherKubeResource() {