One-pager for Managed Resources API Patterns

[muvaf]

Description of your changes

This one-pager serves as ground for the discussions that will hopefully get us closer to having a consistent managed resource API.

I think that we should aim to make implementation of managed resources as mechanical as possible. This guide will serve as starting point on how we want things to look like even if we do/don't have code-generation sometime in the future.

Fixes #831, #728

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, examples, or release notes.
• Updated the RBAC permissions in clusterrole.yaml to include any new types.

[upbound-bot]

[upbound-bot]

[upbound-bot]

[negz]

Thanks for getting started on this @muvaf! I added a few initial thoughts.

https://docs.google.com/document/d/10MrgDAw9r22swcX7yCptRWmxU7dcndgql8_6nZfSL5U/edit#

I also updated the above document in which I've tried to compile a list of all the potential changes that have been bouncing around over the last few weeks. We definitely do not need to tackle all of them, but I think naming external resources and immutable spec fields could be in scope for this document.

[upbound-bot]

[upbound-bot]

[muvaf]

@negz Thanks for pointing out that document! I've made additions but I didn't want to touch all of the topics as I want this document to be more about the steps where the developer tries to develop the parts of the managed resource that is related to the relation between provider's API and our managed reconciler. For example, naming or high fidelity depend on the resource/API you call but reclaim policy is generic for all managed resources.

The parts that appear in the Google doc but are not included in this document:

• Importing existing resources. The naming section touches on how to make this easier but, in general, I don't go into details of that.
• Dependency references. As we talked, this will have its own one-pager.
• Unclaimable managed resources. This is more about general managed resource logic that we have instead of provider<>managed resource/its controller relation.
• Reclaim policy. This is also a generic one that doesn't depend on the specific resource that the dev is trying to implement.
• Claim & Class references in Spec and claim<>class<>nonportable class<>managed resource relations. This one is also generic.

[upbound-bot]

[displague]

What if:

• the provider auto-generates unique names (foo12345, foo-adjective-animal)
• the provider lets you supply a name
• the character max is less than 253

I think this may be a common case worth included in this ruleset. With a CSI PVC driver, I truncated the PVC name for use as the provider label, but in that particular case there was no auto-generated name from the provider.

[muvaf]

How about this; as long as resourceType+UID is less than what's allowed by the provider, use it. If it's longer that allowed, do not cut the tail, implement your own random generator for UID part whose length is suitable.

[negz]

Thanks for patiently waiting for feedback on this @muvaf! It's been a long week of planning meetings and reviews. 😓

I want this document to be more about the steps where the developer tries to develop the parts of the managed resource that is related to the relation between provider's API and our managed reconciler.

That makes sense. I do think we'll need to somehow tie the topics touched on in this document together with those covered in others (e.g. dependencies, reclaim policy) to make sure everything makes sense as a consistent whole. I believe @bassam suggested it would be a good idea to write up example YAMLs representing our ideal of how various Crossplane resources would be shaped. That's probably something we should do separately to this document.

[negz]

My understanding is that we may need to make a recommendation around this Azure-ish case for spec inputs as well as status outputs. Two examples of this are:

• Creating or updating an Azure virtual network. The Azure API allows you to specify a set of subnets and peerings that also exist as distinct Azure API objects and thus could be modeled as distinct managed resources.
• Creating or updating a GKE cluster. The API allows you to provide a set of node pools, that also exist as distinct GKE API objects and thus could be modeled as distinct managed resources.

Perhaps Crossplane should take a stance something like "if this external resource could be CRUD as either a subfield of a parent resource or as a distinct resource, Crossplane supports only the latter". So basically we'd omit the nodePools of a GKE cluster managed resource and instead require you to create distinct node pool managed resources instead.

[muvaf]

if this external resource could be CRUD as either a subfield of a parent resource or as a distinct resource, Crossplane supports only the latter

I thought about this a lot and I think we might have to do this way even though we want to support referring resources that are not managed by the given Crossplane instance or Crossplane has no support for that resource type yet.

Let's say we created a struct that has identification information of []*SubnetReference and included this as a field in Azure VirtualNetwork to support adding non-Crossplane-managed Subnet resource.

type VirtualNetwork struct {
  subnets []*SubnetReference
}

I created a VirtualNetwork object with this array having a subnet named A. Azure will create the VirtualNetwork and the Subnet A. Then I went ahead and created an actual Subnet CR with subnet name B that refers to my VirtualNetwork. In that case my desired state about how VirtualNetwork's subnets should be constructed is divided; virtual network controller will delete B since it sees the desired state of VirtualNetwork having only A subnet, on the other hand, subnet controller will say B who refers to my VirtualNetwork should exist. So, possibly, one will delete and the other one will create.

In the current implementation, VirtualNetwork object doesn't mention any Subnet at all. So, the desired state of Subnet part of the VirtualNetwork is managed only by our Subnet resource and that is consistent. So, I guess we'll either expand VirtualNetwork resource to make it the only way of creating Subnets or we will eliminate other resource references in it.

We can expand VirtualNetwork and remove Subnet, but we don't know where this might end. It can be a really long tail of resources provisioned in only one CR. So, we might have to take that stance where we say you have to create a CR for that sub-resource if we have support for it. I think having external resource policy read-only mode(crossplane/crossplane-runtime#22 (comment)) will help users to add resource only for reference purposes even though they don't want to manage them with Crossplane or with that Crossplane instance.

I think this analogy applies to GKE node pools as well.

[hasheddan]

One thing that came up when syncing with @muvaf on this topic was that, as mentioned, we are currently ignoring the embedded Subnet slice on the Azure VirtualNetwork, so adding a subnet via a Subnet custom resource does not cause the VirtualNetwork controller to update the VirtualNetwork. However, I am concerned that if some other part of the VirtualNetwork changes that causes the controller to update, that it may delete any subnets that were created. The subnets would then be recreated by the Subnet controller, but it could have some pretty severe negative effects on other resources. It is possible that the Azure API will not delete subnets if update is called on a virtual network, but I am not certain. I am going to test this out today.

[hasheddan]

@muvaf @negz I confirmed this behavior and have opened an issue to track crossplane-contrib/provider-azure#23

[negz]

Are you still thinking this would be a superset of the spec? i.e. That we'd include the current observed state of all spec fields in case they diverged from the spec? I think that's implied here, but I think it may be worth making it explicit given that it's an uncommon pattern.

(FWIW I'm still super on the fence about whether we should do this.)

[negz]

Ah - I see this is covered below in immutable properties but I still think it's worth pointing out out here.

[muvaf]

As long as we can come up with something else that covers the following cases, I'd be more than happy to change that.

• It should be sufficient to allow access to Status sub-resource(via RBAC) for all-purpose information extraction by other entities. Meaning, I would like to be able to extract the current settings, identification information, metadata etc. only by looking at Status.
• We need to represent the most up-to-date state in happy cases as well as error cases. If user changed the desired state and if we are in the middle of applying it/couldn't apply it/never plan to apply it, the user should be able to know get the current state even if no error shows up. If we have an update-able config only in Spec, we're not able to do that.

However, we can also discuss the need for two points mentioned here. For instance, if we decide the first one is not worth covering, we can remove the creation-time and metadata fields from Status and keep only update-able fields and identification fields(for references) there.

[negz]

Give there's a solid technical reason to do this, but that it's an uncommon pattern for Kubernetes, I suggest we avoid doing it for the time being but make sure there's an issue tracking the idea. We can then implement this if the main problem we foresee (hidden drift between the desired and actual state) becomes a real issue based on user feedback.

[muvaf]

Opened #925

[upbound-bot]

[muvaf]

I do think we'll need to somehow tie the topics touched on in this document together with those covered in others (e.g. dependencies, reclaim policy) to make sure everything makes sense as a consistent whole.

Makes sense. What we could do is maybe have a design doc that has summary of every feature managed resources have in one paragraph and then link to the related one-pager/design doc/issue for details about that specific feature.

I believe @bassam suggested it would be a good idea to write up example YAMLs representing our ideal of how various Crossplane resources would be shaped. That's probably something we should do separately to this document.

I believe if we refactor one or two managed resources to adopt this pattern, then we can easily point to them and also use their YAMLs in the documents. After merging this, I'll open issues for that refactoring effort. I tried to add example snippets to this document anyway.

@negz Thanks a lot for your thorough review! I have made the necessary changes and answered your comments.

[upbound-bot]

[muvaf]

@negz That's how it'd look like in an example resource crossplane-contrib/provider-gcp#41
Though that doesn't cover all the cases in the one-pager, it's simple enough to cover the basic scenario and applicability of the things mentioned

Here is creation YAML:

apiVersion: compute.gcp.crossplane.io/v1alpha2
kind: Network
metadata:
  name: example-network
  namespace: gcp
spec:
  providerRef:
    name: gcp-provider
    namespace: gcp
  reclaimPolicy: Delete

After the reconciliation:

apiVersion: compute.gcp.crossplane.io/v1alpha2
kind: Network
metadata:
  annotations:
    crossplane.io/external-name: example-network
  creationTimestamp: "2019-10-02T16:39:32Z"
  finalizers:
  - finalizer.managedresource.crossplane.io
  generation: 8
  name: example-network
  namespace: gcp
  resourceVersion: "6485"
  selfLink: /apis/compute.gcp.crossplane.io/v1alpha2/namespaces/gcp/networks/example-network
  uid: 05e0ec72-cc7d-4bcc-99fc-e11d0cb2bf6e
spec:
  forProvider:
    IPv4Range: 10.240.0.0/16
    autoCreateSubnetworks: false
    description: ""
    routingConfig:
      routingMode: GLOBAL
  providerRef:
    name: gcp-provider
    namespace: gcp
  reclaimPolicy: Delete
  writeConnectionSecretToRef: {}
status:
  atProvider:
    IPv4Range: 10.240.0.0/16
    autoCreateSubnetworks: false
    creationTimestamp: "2019-10-02T09:48:57.869-07:00"
    description: ""
    gatewayIPv4: 10.240.0.1
    id: 4711805157157464598
    routingConfig:
      routingMode: GLOBAL
    selfLink: https://www.googleapis.com/compute/v1/projects/crossplane-playground/global/networks/example-network
  conditions:
  - lastTransitionTime: "2019-10-02T16:50:51Z"
    reason: Managed resource is available for use
    status: "True"
    type: Ready
  - lastTransitionTime: "2019-10-02T17:19:36Z"
    reason: Successfully reconciled managed resource
    status: "True"
    type: Synced

Note how Spec has been populated with the provider's defaults instead of Golang defaults that may not be what provider thinks should be default when a setting is not given.

[muvaf]

@negz updated with the alignment that we landed on with you and bassam.

[upbound-bot]

[negz]

There are a few open comments from @soorena776 and myself that I think are worth addressing, but this is pretty much good to go. Let me know once you feel everything is addressed and I'll merge. Thanks for driving this complicated effort!

[muvaf]

@negz I think this is ready to merge!

[upbound-bot]