stacks: multiple namespace support

[displague]

Description of your changes

Fixes #1051

When the Stack controller reconciles a Stack it will look for the expected CRDs. If the CRDs are not present, the reconcile will be requeued.

Any of the matching CRDs which contain the app.kubernetes.io/managed-by: stack-manager label (which StackInstall controller's unpack provides) will receive additional labels:

• namespace.crossplane.io/{ns}: "true" an existing label being deferred from unpack to stack reconciliation
• parent.stacks.crossplane.io/{ns}-{name}: "true" a new label to support multiple parents of a CRD

When a Stack is installed into a namespace the existing CRDs will gain additional copies of these labels, for each namespace or stack within that namespace.

When a Stack is deleted from a namespace the CRDs will have parent.stacks.crossplane.io/{ns}-{name} labels removed by the Stack controller. The namespace.crossplane.io/{ns} label will also be removed if there are no remaining parent.stacks.crossplane.io/{ns}* labels.

When a Stack is deleted and a CRD with the app.kubernetes.io/managed-by: stack-manager label contains none of the these two types of labels, the CRD will be deleted by the StackInstall controller.

Honoring the existing namespace.crossplane.io/{ns}: "true" discoverability labels prevents this PR from deleting existing Stack CRDs.

We may also want to revisit the need for single parent labels which are currently used for detecting and deleting owned resources that cross the namespace/cluster scope boundary.

(Updated comment) Single parent labeling is modified in this PR. CRDs no longer receive these labels on unpack. When a StackInstall is removed, any CRD parent labels referring to that StackInstall are also removed from any matching CRDs.

If two StackInstalls sharing a common CRD are installed and deleted within a reconciliation window a race could have the newly reused CRD deleted before the new Stack can label it.

How has this code been tested?

Upgrading Stack-Manager

1. Install the Wordpress and GCP Stack before this version of Stack-Manager is applied.
2. Update Stack Manager to this version.
   • observe existing CRDs are not deleted

Multiple Stacks

1. Install this version of the Stack Manager

2. Install Stack GCP into crossplane-system

3. Install Stack Wordpress into two namespaces

   • observe CRDs are given the labels mentioned in the description above

   $ kubectl get crd -o jsonpath='{.metadata.labels}' wordpressinstances.wordpress.samples.stacks.crossplane.io ; echo 
   map[app.kubernetes.io/managed-by:stack-manager crossplane.io/scope:namespace namespace.crossplane.io/default:true namespace.crossplane.io/precipice:true parent.stacks.crossplane.io/default-stack-wordpress:true parent.stacks.crossplane.io/precipice-stack-wordpress:true]
   

4. Delete one of these StackInstalls

   • observe CRDs lose the appropriate labels

   $ kubectl delete stackinstall -n default stack-wordpress
   $ kubectl get crd -o jsonpath='{.metadata.labels}' wordpressinstances.wordpress.samples.stacks.crossplane.io ; echo 
   map[app.kubernetes.io/managed-by:stack-manager crossplane.io/scope:namespace namespace.crossplane.io/precipice:true parent.stacks.crossplane.io/precipice-stack-wordpress:true]
   

5. Delete the other StackInstall

   • observe that the CRD has been deleted

Checklist

I have:

• Run make reviewable to ensure this PR is ready for review.
• Ensured this PR contains a neat, self documenting set of commits.
• Updated any relevant documentation and examples.
• Reported all new error conditions into the log or as an event, as
  appropriate.

For more about what we believe makes a pull request complete, see our
definition of done.

[displague]

I'm opening this up for review. I'll be adding some more tests and docs but the code is in the expected final state (pending review feedback).

[displague]

Griping about "GoLangCI: Processing Timeout." here: golangci/golangci-worker#56 (comment)

Rebasing on latest master for a force push to trigger the rebuild.

[jbw976]

looks pretty good overall, just some minor comments to discuss!

[jbw976]

should this check specifically for a "not found error", instead of "any error" that it appears to be checking for?

[displague]

I took a stab at this, but within this function I can't replicate the parameters needed to match kerrors.NewNotFound() (specifically, I can't get the schema.GroupResource{} Resource -- "customresourcedefinitions" in this case).

After looking into how to accomplish this with Gomega, I started wondering why we are introducing Gomega at all.

This assertNoKubernetesObject mirrors assertKubernetesObject. Both are already present between stackinstall_test and stack_test (I don't recall which received assertKubernetesObject first).

In a future PR I think we can replace this with a function that doesn't use Gomega. The semantics and detailed reporting don't provide much advantage over what we already get with cmp.Diff. Without the Gomega semantics I would just check if the error had a 404 Code (which I can still do with Gomega, I'd rather avoid building this function up).

I can revisit this within this PR when the other problems are addressed.

[jbw976]

I am always in support of removing Gomega usage.

[jbw976]

this func looks like a lot of new logic from the diff, but I think that's probably not the case and it's mostly moved/refactored logic. This section of permissions management is the one i'd be the most concerned about for regression risk, so hopefully we have done the testing validation to ensure this still works as expected.

[displague]

I agree that the refactor here is hard to follow by diff.

Before

create calls processRBAC
processRBAC calls createPersonaClusterRoles
createPersonaClusterRoles fetches all CRDs using crdsFromStack before doing role specific work
crdsFromStack calls crdListsDiffer which is a naive len(fetched) == len(stack.spec.crds) check (caused #1290)

After

create calls processRBAC (no change)
create calls processCRDs (new)
processRBAC no longer calls createPersonaClusterRoles
processCRDs fetches all CRDs using crdsFromStack (improved for multi-parented and unmanaged CRDs (fixes #1290))
crdsFromStack now verifies that the expected version of the CRD exists in the fetched CRD
crdsFromStack is now usable for deletion where some CRDs could already be missing

processCRDs walks a list of handlers which include:

• createPersonaClusterRoles (inner crd-loop is unmodified, outer crd fetch + loop + len-check is not needed)
• multi-parent labeller (new)
• namespace discovery labeller (moved here away from unpack, because the managed crd may already exist)

processCRDs uses crdListFulfilled (replacing crdListsDiffer) to verify that all expected CRDs are present (managed or not) (hmm, this could be just another "handler" in the list)