Adopted resource CRD Group common across services

[surajkota]

Describe the bug
Adopted resource API Group is set to services.k8s.aws which is an issue for users who want to use multiple services. Multiple controllers will listen to the events on the same adopted resource CR (Kind, group, version)
and each controller will try to adopt the target AWS resource and update the same source adoption resource CR.
Although only one will succeed because the reconciler will not be able to instantiate a resourceManager object if the target is from a different service it's still a problem because the status will be incorrect and always say ACK.Adopted False from the controller which fails to adopt(backoff and error).

Another problem of having the same CRD group is when user updates one of the controllers and the spec/version etc have changed

Workaround
The problem can be mitigated by changing installation to namespace scoped but it is not the right solution and is not supported in ACK as of today

Steps to reproduce
Run 2 service controllers with runtime 0.1.0 and adopt a resource

Expected outcome
Set CRD group to <service-name>.services.k8s.aws instead of services.k8s.aws like any other resource in the service. It is only capable of adopting resources of the same service anyway

[RedbackThomson]

This was an intentional design decision. Each controller should listen to any new adopted resource, and if it recognises the target type then it would run through the adoption reconciliation loop. Otherwise, it would silently early exit.

The status collision sounds like a genuine issue - that the service conrollers which do not have the registered resourceManager are not exiting early enough.

[surajkota]

how would the reconciler differentiate between failure cause though?
resourceManager might not be registered because the resource is from a different service vs
RM does not exist for the resource in the same service

---

Otherwise, it would silently early exit.

IMO it should be an error and not be silent

Also installing the same CRD in multiple controllers is not correct because it will create issues when users upgrade one of the controllers

[RedbackThomson]

resourceManager might not be registered because the resource is from a different service vs
RM does not exist for the resource in the same service

There is no reason to differentiate between the two. As far as its concerned, the adoption reconciler is only able to reconcile the resources it has been compiled to understand. If it was not registered with a given resource, then there was an issue in compilation?

IMO it should be an error and not be silent

This would create a LOT of noise in the logs. Every controller would attempt to reconcile every adopted resource and throw the same error.

Also installing the same CRD in multiple controllers is not correct because it will create issues when users upgrade one of the controllers

This could be fixed by updating the API version if we make changes to any of the spec fields? I do agree that there should be a better way to install shared CRDs without overwriting existing installations, though. However, adopted resources were the first shared CRD between any controllers.

[jaypipes]

The adopted resource CRD is in APIGroup services.k8s.aws, which is intentional. It is a CR Kind that is intentionally supposed to be watched by many service controllers and only acted upon if the AdoptedResource.Spec.Kubernetes.Kind is one of the registered Kinds that that controller is responsible for (i.e. that that controller has a resourceManager for).

Installing a CRD into Namespace scope is not the right solution, because we don't install different controllers into different namespaces and doing so wouldn't solve any of the problems highlighted in this issue regarding multiple controllers using different versions of the AdoptedResource CRD.

@surajkota when @RedbackThomson said that the controller silently early exits, he did not imply that this was an error. Rather, that the controller recognized it didn't have a resource manager matching the requested AdoptedResource.Spec.Kubernetes.Kind and therefore was doing the equivalent of calling continue in a for loop.

Regarding the problem of attempting to install a single AdoptedResource CRD twice, that is not a problem with anything in ACK but rather a problem with whomever/whatever is attempting to install the same AdoptedResource CRD into a K8s cluster that has already had it installed. In that case, that script, Helm Chart or human will need to do the equivalent of this:

# Only install CRD if not already there...
if ! `kubectl api-resources -o name | grep AdoptedResource`; then
  kubectl apply -f /path/to/adopted_resource.crd.yaml
fi

[RedbackThomson]

Spoke with @surajkota about how service controllers can give a verbose output when the AdoptedResource CR target API group matches their respective resource managers. The adoption reconciler will ignore the AdoptedResource if the target API group does not match, if it does match but no resource managers match the target Kind then it will set ACK.Adopted to False and add an additional condition with a common failure message.

Regarding installing the CRDs at the cluster scope, we will be following the de facto standard of documenting the installation process as initially installing CRDs (and other cluster-scope resources) and then installing the namespace-scope deployments/roles. Deleting CRDs as part of uninstalling the controller is not well supported by Helm and should be maintained separately.