Skip to content

Commit

Permalink
refactor: simplify SSA check in RBAC generation
Browse files Browse the repository at this point in the history 
Signed-off-by: Chris Laprun <[email protected]>
  • Loading branch information
@metacosm
metacosm committed Sep 25, 2024
1 parent 149ae31 commit 08cf81f
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 50 deletions.
26 changes: 13 additions & 13 deletions .github/workflows/build-for-quarkus-version.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,13 +183,13 @@ jobs:
- name: Check-out Fabric8 client PR if requested
uses: actions/checkout@v4
if: "${{ inputs.fkc-pr != '' }}"
if: "${{ inputs.fkc-pr != '' && inputs.fkc-version == '' }}"
with:
repository: fabric8io/kubernetes-client
path: fkc

- name: Build Fabric8 client PR if requested
if: "${{ inputs.fkc-pr != '' }}"
if: "${{ inputs.fkc-pr != '' && inputs.fkc-version == '' }}"
id: build-fkc-pr
run: |
cd fkc
Expand Down Expand Up @@ -232,11 +232,6 @@ jobs:
echo "Computed Maven profiles: ${maven_profiles}"
echo "maven_profiles=${maven_profiles}" >> $GITHUB_OUTPUT
- name: Change Quarkus version
run: |
echo "Using Quarkus ${{ steps.quarkus-version.outputs.quarkus_version }}"
./mvnw versions:set-property -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -Dproperty=quarkus.version -DnewVersion=${{ steps.quarkus-version.outputs.quarkus_version }}
- name: Change Fabric8 client version
id: fkc-version
if: "${{ inputs.fkc-pr != '' || inputs.fkc-version != '' }}"
Expand All @@ -251,22 +246,27 @@ jobs:
./mvnw versions:set-property -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -Dproperty=fabric8-client.version -DnewVersion=${fkc_version}
echo "fkc_version=${fkc_version}" >> $GITHUB_OUTPUT
- name: Change Quarkus version
run: |
echo "Using Quarkus ${{ steps.quarkus-version.outputs.quarkus_version }}"
./mvnw versions:set-property -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -Dproperty=quarkus.version -DnewVersion=${{ steps.quarkus-version.outputs.quarkus_version }} ${{inputs.mvnArgs}}
- name: Output versions being used
run: |
echo "QOSDK version: $(./mvnw help:evaluate -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -Dexpression=project.version -q -DforceStdout)"
echo "JOSDK version: $(./mvnw help:evaluate -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -Dexpression=java-operator-sdk.version -q -DforceStdout)"
echo "QOSDK version: $(./mvnw help:evaluate -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -Dexpression=project.version -q -DforceStdout ${{inputs.mvnArgs}})"
echo "JOSDK version: $(./mvnw help:evaluate -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -Dexpression=java-operator-sdk.version -q -DforceStdout ${{inputs.mvnArgs}})"
echo "JOSDK Fabric8 version: ${{ steps.build-josdk-pr.outputs.josdk_f8_version }}"
echo "JOSDK overridden Fabric8 version ${{ steps.fkc-version.outputs.fkc_version }}"
echo "Quarkus version: $(./mvnw help:evaluate -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -Dexpression=quarkus.version -q -DforceStdout)"
echo "Quarkus version: $(./mvnw help:evaluate -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -Dexpression=quarkus.version -q -DforceStdout ${{inputs.mvnArgs}})"
echo "Quarkus Fabric8 version: ${{ steps.build-quarkus-pr.outputs.quarkus_f8_version }}"
echo "Effective Fabric8 version: $(./mvnw dependency:tree -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -Dincludes=io.fabric8:kubernetes-client-api -pl core/deployment | grep io.fabric8:kubernetes-client-api -m1 | cut -d ':' -f 4)"
echo "Effective Fabric8 version: $(./mvnw dependency:tree -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' ${{inputs.mvnArgs}} -Dincludes=io.fabric8:kubernetes-client-api -pl core/deployment | grep io.fabric8:kubernetes-client-api -m1 | cut -d ':' -f 4)"
- name: Build with Maven (JVM)
run: ./mvnw -B formatter:validate install -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' ${{inputs.mvnArgs}} --file pom.xml

- name: Dependency tree on failure
if: failure()
run: ./mvnw -B dependency:tree -Dverbose
run: ./mvnw -B dependency:tree -Dverbose ${{inputs.mvnArgs}}

- name: Kubernetes KinD Cluster
uses: container-tools/kind-action@v2
Expand Down Expand Up @@ -332,7 +332,7 @@ jobs:
path: samples/joke/app.log

- name: Build with Maven (Native)
run: ./mvnw -B install -Dnative --file pom.xml -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -pl '${{inputs.native-modules}}' -amd
run: ./mvnw -B install -Dnative --file pom.xml -P'${{steps.set-mvn-profiles.outputs.maven_profiles}}' -pl '${{inputs.native-modules}}' -amd ${{inputs.mvnArgs}}

- name: Run Joke sample in native mode
if: ${{ contains(inputs.native-modules, 'samples') }}
Expand Down
50 changes: 13 additions & 37 deletions ...loyment/src/main/java/io/quarkiverse/operatorsdk/deployment/AddClusterRolesDecorator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,9 @@ private static Set<PolicyRule> getClusterRolePolicyRulesFromDependentResources(Q

// only process Kubernetes dependents
if (HasMetadata.class.isAssignableFrom(associatedResourceClass)) {
var resourceGroup = HasMetadata.getGroup(associatedResourceClass);
var resourcePlural = HasMetadata.getPlural(associatedResourceClass);
final var asHasMetadataClass = (Class<? extends HasMetadata>) associatedResourceClass;
var resourceGroup = HasMetadata.getGroup(asHasMetadataClass);
var resourcePlural = HasMetadata.getPlural(asHasMetadataClass);

final var verbs = new TreeSet<>(List.of(RBACVerbs.READ_VERBS));
if (Updater.class.isAssignableFrom(dependentResourceClass)) {
Expand All @@ -124,34 +125,24 @@ private static Set<PolicyRule> getClusterRolePolicyRulesFromDependentResources(Q
verbs.add(RBACVerbs.DELETE);
}
final var isCreator = Creator.class.isAssignableFrom(dependentResourceClass);
boolean shouldDoubleCheckPatch = false;
if (isCreator) {
verbs.add(RBACVerbs.CREATE);

// PATCH verb is also needed when using SSA to be able to add the finalizer when creating the resource
// Here, we optimistically add PATCH method if the resource configuration states that SSA should be
// used, despite this not being a correct/complete determination of whether the resource actually
// uses SSA. This can only be determined by instantiating the dependent, which is why, if we can
// instantiate it, we double-check the SSA status later on and remove the PATCH method if we can
// actually determine that it's not needed
final Object dependentResourceConfig = spec.getDependentResourceConfig();
if (dependentResourceConfig instanceof KubernetesDependentResourceConfig<?> kubernetesDependentResourceConfig) {
if (kubernetesDependentResourceConfig.useSSA().orElse(false)) {
verbs.add(RBACVerbs.PATCH);
shouldDoubleCheckPatch = true;
}
}
}

// Check if we're dealing with typeless Kubernetes resource or if we need to deal with SSA
boolean ignore = false;
KubernetesDependentResource<?, ?> kubeResource = null;
if (KubernetesDependentResource.class.isAssignableFrom(dependentResourceClass)) {
final var asKubeDRClass = (Class<? extends KubernetesDependentResource<?, ?>>) dependentResourceClass;

// PATCH is also required when creating resources to add finalizers when using SSA
if (isCreator && cri.getConfigurationService().shouldUseSSA(asKubeDRClass, asHasMetadataClass,
(KubernetesDependentResourceConfig<? extends HasMetadata>) spec.getDependentResourceConfig())) {
verbs.add(RBACVerbs.PATCH);
}

try {
//noinspection rawtypes
kubeResource = Utils.instantiate(
(Class<? extends KubernetesDependentResource>) dependentResourceClass,
KubernetesDependentResource.class, ADD_CLUSTER_ROLES_DECORATOR);
final var kubeResource = Utils.instantiate(asKubeDRClass, KubernetesDependentResource.class,
ADD_CLUSTER_ROLES_DECORATOR);

if (kubeResource instanceof GenericKubernetesDependentResource<? extends HasMetadata> genericKubeRes) {
final var gvk = genericKubeRes.getGroupVersionKind();
Expand All @@ -166,21 +157,6 @@ private static Set<PolicyRule> getClusterRolePolicyRulesFromDependentResources(Q
}

if (!ignore) {
// if we need to double check if we really should use SSA
if (shouldDoubleCheckPatch) {
// we can only check if we managed to instantiate the dependent, though
if (kubeResource != null) {
if (!cri.getConfigurationService().shouldUseSSA(kubeResource)) {
verbs.remove(RBACVerbs.PATCH);
}
} else {
// if we couldn't double check, warn the user
log.warn("Couldn't verify that dependent " + dependentResourceClass.getName()
+ " really needs PATCH permission for SSA because it couldn't be instantiated. This means that a PATCH verb might have been added to the rule (group: "
+ resourceGroup + " / plural: " + resourcePlural + ") when not needed.");
}
}

final var dependentRule = new PolicyRuleBuilder()
.addToApiGroups(resourceGroup)
.addToResources(resourcePlural);
Expand Down

0 comments on commit 08cf81f

Please sign in to comment.