From 3ad74ac9a9a2487b3272d460428e946f6194b2c2 Mon Sep 17 00:00:00 2001 From: charlesc Date: Mon, 26 Aug 2024 17:27:51 +0800 Subject: [PATCH] feat: support typeless kubernetes resources when generating manifests (#931) * Support typeless kubernetes resources when generating manifests Typeless kubernetes resource has no Group and Plural value, so generating wrong policy rule. Commit summary: 1) Use ApiGroup value from GVK and '*' for Plural value if dependent resource is assignable from GenericKubernetesDependentResource 2) Merge verbs in rule for optimization if with same ApiGroups and Resources * cleanup code-checking problems * fix missing assertion * Refactor AddClusterRolesDecorator for more understandable --------- Co-authored-by: cc --- .../deployment/AddClusterRolesDecorator.java | 138 ++++++++++++++---- .../operatorsdk/test/OperatorSDKTest.java | 25 ++-- .../test/sources/TestReconciler.java | 4 +- .../sources/TypelessAnotherKubeResource.java | 18 +++ .../test/sources/TypelessKubeResource.java | 17 +++ 5 files changed, 162 insertions(+), 40 deletions(-) create mode 100644 core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TypelessAnotherKubeResource.java create mode 100644 core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TypelessKubeResource.java diff --git a/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/AddClusterRolesDecorator.java b/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/AddClusterRolesDecorator.java index 4e98add7..80901c6a 100644 --- a/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/AddClusterRolesDecorator.java +++ b/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/AddClusterRolesDecorator.java @@ -1,17 +1,21 @@ package io.quarkiverse.operatorsdk.deployment; -import java.util.Collection; -import java.util.Map; +import java.util.*; +import java.util.function.Function; + +import org.jetbrains.annotations.NotNull; import io.dekorate.kubernetes.decorator.ResourceProvidingDecorator; 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.reconciler.dependent.Deleter; import io.javaoperatorsdk.operator.processing.dependent.Creator; import io.javaoperatorsdk.operator.processing.dependent.Updater; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesDependentResource; import io.quarkiverse.operatorsdk.annotations.RBACVerbs; import io.quarkiverse.operatorsdk.runtime.DependentResourceSpecMetadata; import io.quarkiverse.operatorsdk.runtime.QuarkusControllerConfiguration; @@ -53,30 +57,23 @@ public void visit(KubernetesListBuilder list) { } public static ClusterRole createClusterRole(QuarkusControllerConfiguration cri) { - final var rule = new PolicyRuleBuilder(); - final var resourceClass = cri.getResourceClass(); - final var plural = HasMetadata.getPlural(resourceClass); - rule.addToResources(plural); - // if the resource has a non-Void status, also add the status resource - if (cri.isStatusPresentAndNotVoid()) { - rule.addToResources(plural + "/status"); - } - - // add finalizers sub-resource because it's used in several contexts, even in the absence of finalizers - // see: https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#ownerreferencespermissionenforcement - rule.addToResources(plural + "/finalizers"); + Set collectedRules = new LinkedHashSet<>(); + collectedRules.add(getClusterRolePolicyRuleFromPrimaryResource(cri)); + collectedRules.addAll(getClusterRolePolicyRulesFromDependentResources(cri)); + collectedRules.addAll(cri.getAdditionalRBACRules()); - rule.addToApiGroups(HasMetadata.getGroup(resourceClass)) - .addToVerbs(RBACVerbs.ALL_COMMON_VERBS) - .build(); - - final var clusterRoleBuilder = new ClusterRoleBuilder() + return new ClusterRoleBuilder() .withNewMetadata() .withName(getClusterRoleName(cri.getName())) .endMetadata() - .addToRules(rule.build()); + .addAllToRules(mergePolicyRulesOfSameGroupsAndKinds(collectedRules)) + .build(); + } + @NotNull + private static Set getClusterRolePolicyRulesFromDependentResources(QuarkusControllerConfiguration cri) { + Set rules = new LinkedHashSet<>(); final Map> dependentsMetadata = cri.getDependentsMetadata(); dependentsMetadata.forEach((name, spec) -> { final var dependentResourceClass = spec.getDependentResourceClass(); @@ -84,9 +81,29 @@ public static ClusterRole createClusterRole(QuarkusControllerConfiguration cr // 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 + 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 = "*"; + } + } catch (Exception e) { + throw new RuntimeException(e); + } + } + final var dependentRule = new PolicyRuleBuilder() - .addToApiGroups(HasMetadata.getGroup(associatedResourceClass)) - .addToResources(HasMetadata.getPlural(associatedResourceClass)) + .addToApiGroups(resourceGroup) + .addToResources(resourcePlural) .addToVerbs(RBACVerbs.READ_VERBS); if (Updater.class.isAssignableFrom(dependentResourceClass)) { dependentRule.addToVerbs(RBACVerbs.UPDATE_VERBS); @@ -100,15 +117,82 @@ public static ClusterRole createClusterRole(QuarkusControllerConfiguration cr dependentRule.addToVerbs(RBACVerbs.PATCH); } } - clusterRoleBuilder.addToRules(dependentRule.build()); + rules.add(dependentRule.build()); } - }); + return rules; + } + + private static PolicyRule getClusterRolePolicyRuleFromPrimaryResource(QuarkusControllerConfiguration cri) { + final var rule = new PolicyRuleBuilder(); + final var resourceClass = cri.getResourceClass(); + final var plural = HasMetadata.getPlural(resourceClass); + rule.addToResources(plural); - // add additional RBAC rules - clusterRoleBuilder.addAllToRules(cri.getAdditionalRBACRules()); + // if the resource has a non-Void status, also add the status resource + if (cri.isStatusPresentAndNotVoid()) { + rule.addToResources(plural + "/status"); + } + + // add finalizers sub-resource because it's used in several contexts, even in the absence of finalizers + // see: https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#ownerreferencespermissionenforcement + rule.addToResources(plural + "/finalizers"); - return clusterRoleBuilder.build(); + rule.addToApiGroups(HasMetadata.getGroup(resourceClass)) + .addToVerbs(RBACVerbs.ALL_COMMON_VERBS) + .build(); + 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 mergePolicyRulesOfSameGroupsAndKinds(Set collectedRules) { + Set 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 verbs1 = new LinkedHashSet<>(r.getVerbs()); + Set verbs2 = new LinkedHashSet<>(i.getVerbs()); + verbs1.addAll(verbs2); + r.setVerbs(verbs1.stream().toList()); + }); + } + }); + return mergedRules; + } + + @NotNull + private static Function 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) { diff --git a/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/OperatorSDKTest.java b/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/OperatorSDKTest.java index d27c8710..846dc0d9 100644 --- a/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/OperatorSDKTest.java +++ b/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/OperatorSDKTest.java @@ -28,17 +28,7 @@ import io.quarkiverse.operatorsdk.annotations.RBACVerbs; import io.quarkiverse.operatorsdk.deployment.AddClusterRolesDecorator; import io.quarkiverse.operatorsdk.deployment.AddRoleBindingsDecorator; -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.*; import io.quarkus.test.ProdBuildResults; import io.quarkus.test.ProdModeTestResults; import io.quarkus.test.QuarkusProdModeTest; @@ -53,6 +43,7 @@ public class OperatorSDKTest { .withApplicationRoot((jar) -> jar .addClasses(TestReconciler.class, TestCR.class, CRUDConfigMap.class, ReadOnlySecret.class, CreateOnlyService.class, NonKubeResource.class, Foo.class, + TypelessKubeResource.class, TypelessAnotherKubeResource.class, SimpleReconciler.class, SimpleCR.class, SimpleSpec.class, SimpleStatus.class)); @ProdBuildResults @@ -79,7 +70,7 @@ public void shouldCreateRolesAndRoleBindings() throws IOException { .map(ClusterRole.class::cast) .forEach(cr -> { final var rules = cr.getRules(); - assertEquals(5, rules.size()); + assertEquals(6, rules.size()); assertTrue(rules.stream() .filter(rule -> rule.getApiGroups().equals(List.of(HasMetadata.getGroup(TestCR.class)))) .anyMatch(rule -> { @@ -108,6 +99,16 @@ public void shouldCreateRolesAndRoleBindings() throws IOException { .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 + 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); }); // check that we have a role binding for TestReconciler and that it uses the operator-level specified namespace diff --git a/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TestReconciler.java b/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TestReconciler.java index 0217ed52..3b788188 100644 --- a/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TestReconciler.java +++ b/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TestReconciler.java @@ -12,7 +12,9 @@ @Dependent(type = CRUDConfigMap.class), @Dependent(type = ReadOnlySecret.class), @Dependent(type = CreateOnlyService.class), - @Dependent(type = NonKubeResource.class) + @Dependent(type = NonKubeResource.class), + @Dependent(type = TypelessKubeResource.class), + @Dependent(type = TypelessAnotherKubeResource.class) }) @RBACRule(verbs = RBACVerbs.UPDATE, apiGroups = RBACRule.ALL, resources = RBACRule.ALL) public class TestReconciler implements Reconciler { diff --git a/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TypelessAnotherKubeResource.java b/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TypelessAnotherKubeResource.java new file mode 100644 index 00000000..14b718ef --- /dev/null +++ b/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TypelessAnotherKubeResource.java @@ -0,0 +1,18 @@ +package io.quarkiverse.operatorsdk.test.sources; + +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 implements Deleter { + + 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() { + super(GVK); + } + +} diff --git a/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TypelessKubeResource.java b/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TypelessKubeResource.java new file mode 100644 index 00000000..e6da7caf --- /dev/null +++ b/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/sources/TypelessKubeResource.java @@ -0,0 +1,17 @@ +package io.quarkiverse.operatorsdk.test.sources; + +import io.javaoperatorsdk.operator.processing.GroupVersionKind; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesDependentResource; + +public class TypelessKubeResource extends GenericKubernetesDependentResource { + + public static final String GROUP = "crd.josdk.quarkiverse.io"; + public static final String KIND = "typeless"; + public static final String VERSION = "v1"; + private static final GroupVersionKind GVK = new GroupVersionKind(GROUP, VERSION, KIND); + + public TypelessKubeResource() { + super(GVK); + } + +}