Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JOSDK doesn't support use of CRUDKubernetesDependentResource(with GC) when owner resource is cluster scoped #2398

Closed
arpitchaudhary opened this issue May 21, 2024 · 6 comments
Milestone

Comments

@arpitchaudhary
Copy link

Bug Report

What did you do?

  1. I created a cluster scoped custom resource and its reconciler
     @ControllerConfiguration(maxReconciliationInterval = @MaxReconciliationInterval(
             interval = 1,
             timeUnit = TimeUnit.HOURS),
             dependents = {
                     @Dependent(name = MyCustomReconciler.NAMESPACE_RESOURCE_NAME,
                             type = NamespaceDependentResource.class),
                     @Dependent(name = MyCustomReconciler.ROLE_BINDING_RESOURCE_NAME,
                             type = RoleBindingDependentResource.class,
                             dependsOn = MyCustomReconciler.NAMESPACE_RESOURCE_NAME)
             }
     )
     public class MyCustomReconciler implements Reconciler<UgabEnv>
  2. Implemented 2 dependent reconcilers to create a namespace and multiple role bindings in the namespace. Bulk Reconciler for RoleBinding
public class RoleBindingDependentResource extends CRUDKubernetesDependentResource<RoleBinding, MyCustom>
        implements BulkDependentResource<RoleBinding, MyCustom>

What did you expect to see?

I expected reconciliation to succeed and any updates in MyCustom resource to lead to updating role bindings as needed based on spec updates in MyCustom resource.

What did you see instead? Under which circumstances?

This lead to error while reconciliation:

RoleBinding <name of role binding> already exists.

This is because the operator tries to recreate the role bindings even when they already exist during second reconciliation or when owner resource (MyCustom) is updated . This happens because:

  1. context.getSecondaryResources(RoleBinding.class); returns empty set even when role bindings exist.
  2. Caused by Mapper.SecondaryToPrimaryMapper(link) only considering owner from same namespace. This is because CRUDKubernetesDependentResource is garbage collected(link)https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java#L89-L90

So, even though ownerRef allows use of cluster scoped resource, JOSDK assumes that the owner is from the same namespace only.

Environment

Kubernetes cluster type:

kind version 0.19.0-alpha+e28a39297dbd55

$ Mention java-operator-sdk version from pom.xml file

Tried with 4.8.0 and 4.9.0

$ java -version

openjdk 17.0.9 2023-10-17 LTS

$ kubectl version

Client Version: version.Info{Major:"1", Minor:"21+", GitVersion:"v1.21.1-rc.0.41+e338d4e93d422d", GitCommit:"e338d4e93d422d796f8e692289ccbcbede94c712", GitTreeState:"clean", BuildDate:"2021-04-30T22:16:08Z", GoVersion:"go1.16.2", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"27", GitVersion:"v1.27.3", GitCommit:"25b4e43193bcda6c7328a6d147b1fb73a33f1598", GitTreeState:"clean", BuildDate:"2023-06-14T09:47:40Z", GoVersion:"go1.20.5", Compiler:"gc", Platform:"linux/arm64"}

Possible Solution

  • Do not assume that Garbage collected (using ownerRef) child resource needs to have owner in same namespace. Owner can also be a cluster scoped resource.

Additional context

Workaround: Use CRUDNoGCKubernetesDependentResource instead of CRUDKubernetesDependentResource

@csviri
Copy link
Collaborator

csviri commented May 21, 2024

Hi @arpitchaudhary , thx for reporting,

The way to handle this (in v4.x.x) is to implement the SecondaryToPrimaryMapper for the Dependent Resoure in your case the RoleBindingDependentResource should implement it.

and just call the prepared mapper for this, see:

public static <T extends HasMetadata> SecondaryToPrimaryMapper<T> fromOwnerReferences(

where the cluster scoped flag should be false.

@csviri
Copy link
Collaborator

csviri commented May 21, 2024

This could be done to some extent automatically, will take a look for v5, since other things related to this are changing.

@csviri csviri added this to the 5.0 milestone May 21, 2024
@arpitchaudhary
Copy link
Author

Thanks for the quick reply. We currently extended CRUDNoGCKubernetesDependentResource in RoleBindingDependentResource. As our MyCustom resource creates the namespace where this role binding is created, we are not too worried about this not getting deleted (as namespace deletion will anyways force deletion of role binding). Is there any downside to this approach?

@csviri
Copy link
Collaborator

csviri commented May 22, 2024

Thanks for the quick reply. We currently extended CRUDNoGCKubernetesDependentResource in RoleBindingDependentResource. As our MyCustom resource creates the namespace where this role binding is created, we are not too worried about this not getting deleted (as namespace deletion will anyways force deletion of role binding). Is there any downside to this approach?

There is only minimal downside, CRUDNoGCKubernetesDependentResource will not use garbage collections, therefore a finalizer is added to your custom resource, which is not a big deal, but when it is handled by GC of k8s it is a bit nicer and a bit more efficient IMHO.

@csviri
Copy link
Collaborator

csviri commented May 22, 2024

see sample how to set SecondaryToPrimaryMapper for a DR here

(for v4.x.x)

@csviri
Copy link
Collaborator

csviri commented May 27, 2024

This is already fixed in v5

@csviri csviri closed this as completed May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants