Simple Resource Class Selection one pager

[negz]

Description of your changes

This one pager cleans up the proposal that we have been discussing in #870. In short, it proposes removing portable resource classes and matching resource claims directly to (non-portable) resource classes using label selectors. This maintains portability of resource classes, while removing a layer of complexity that has been a sticking point for potential Crossplane users (#922, #703 (comment)). As a nice side effect, it enables rudimentary scheduling of resource claims to resource classes.

Fixes #870 (and possibly #922, #91, #113, #703).

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]

[upbound-bot]

[hasheddan]

Really well designed proposal (per usual), thanks @negz ! I left a few comments that I think may be useful to address, but am also willing to approve and circle back around on this if needed given that this effort may be time-sensitive.

A few housekeeping things to mention:

1. This will pretty drastically increase the scope of required updates to the current docs for v0.4, but the changes should be pretty straightforward.
2. We will need to update examples/ as well.

I am more than happy to assist in any of the aforementioned efforts. Please feel free to let me know if I can be of service. Thanks again for this awesome proposed improvement!

[upbound-bot]

[upbound-bot]

[soorena776]

Thanks for a great proposal, enjoyed reading!
Added several nitpicks, and I have a general concern:

One of the most appealing features of Crossplane in my view, is that it abstracts workloads from the underlying provider. My favorite analogy that I often use, is the html language and different browsers. An html document can be considered the workload in Crossplane world, and the browsers are the cloud providers. As an html document without any modifications can produce identical results on different browsers, I would also expect a workload to run and operate the same on different cloud providers, without any modifications.

I understand that you touched this as a challenge in the #rudimentary_scheduling section, but I am a bit concerned that this will be a beginning to workload modification depending where it is being deployed, and infrastructure details leaks into the workload metadata. Could we at least don't retire the existing concept of namespace level non-portable resource classes which enable defining completely portable claims?

[negz]

@soorena776 (Quotes edited a little for brevity)

I would also expect a workload to run and operate the same on different cloud providers, without any modifications. I am a bit concerned that this will be a beginning to workload modification depending where it is being deployed, and infrastructure details leaks into the workload metadata.

Can you elaborate on how using label selectors might leak more details into workload metadata than we do today? Keep in mind:

1. Contemporary resource classes already allow the author to specify fields like engineVersion, bucketACL, etc.
2. We're not opinionated about how the label schema must be used. i.e. You don't have to match on labels like us-east-1. You could match on {workload: gitlab, default: "true"} for similar behaviour to today's "just use the default class for this workload" behaviour.

Could we at least don't retire the existing concept of namespace level non-portable resource classes which enable defining completely portable claims?

A big part of this design is to simplify things. I think if this design results in us:

1. Introducing new concepts.
2. Removing no concepts.

Then it has failed at that goal, and in fact done the opposite. For that reason I don't think we should keep portable resource classes around if we move forward with label selector derived claim->class relationships.

[hasheddan]

I believe there is likely significant review from others that will need to take place here, but I personally am pleased with the status of this design and am comfortable moving forward on it.

[soorena776]

@negz The scenario that I am having in mind is when you have the WordPress workload, and you want to apply it in different cloud providers. I would like to be able to use this file without any modification (think of it as part of a packaged solution) to different environments that I have. The current labeling proposal would require me to modify this workload and add labels to the MySql Claim class selector.

As for achieving the simplification goal, one way to think about it would be to say we now simplify how one could tie a claim to a resource class, but also have more advanced ways of doing it, if you really wanted to keep your workloads truly portable across providers.

[negz]

The current labeling proposal would require me to modify this workload and add labels to the MySql Claim class selector.

It sounds like by workload we're talking about the things an application stack might create. e.g. A controller installed by a Wordpress app creates a KubernetesApplication and a MySQLInstance for every Wordpress a user creates. Currently that MySQLInstance can either:

a. Specify a portable class, by name:

apiVersion: database.crossplane.io/v1alpha1
kind: MySQLInstance
metadata:
  namespace: acme-team
  name: wordpress-database-239df
spec:
  classRef:
    wordpress-database-default  # A portable class in the acme-team namespace

b. Rely on a default resource class to exist:

apiVersion: database.crossplane.io/v1alpha1
kind: MySQLInstance
metadata:
  namespace: acme-team
  name: wordpress-database-239df
spec:
  # No classRef or resourceRef. A default portable class is selected if exactly one exists

Under this proposal, it could instead write the below:

apiVersion: database.crossplane.io/v1alpha1
kind: MySQLInstance
metadata:
  namespace: acme-team
  name: wordpress-database-239df
spec:
  classSelector:
    wordpress-database-default: "true"

It's not obvious to me how this is less portable than the status quo (or at least option a). Perhaps it's not clear that I'm not proposing any specific labelling schema be required? Any labels I've included in the design doc are just examples of patterns an organisation or stack author could use.

[soorena776]

@negz I think I have a better understanding of your vision now. I was under the impression that the labels are first given to the resource classes, and then related claims' classSelector are populated. However this could be also done as not touching the claims, but labeling the resources classes (non-portable layer) accordingly. Thanks for the explanation!

[soorena776]

Barring the nitpick comments /LGTM

[upbound-bot]

[upbound-bot]

[upbound-bot]

[prasek]

Looks great and definitely simplifies things. Dropped a few minor comments. Nice work!

[muvaf]

Great work @negz ! Late to the party, but I just wanted to drop a few thoughts:

• If I'm not mistaken, this proposal doesn't prevent me from using any resource class I want. The current structure does that by exposing only the classes that you can use in your namespace but here we're going with requiring the user to get another k8s cluster with Crossplane running in it. So, the cost(both maintenance and physical cost) of running Crossplane might have increased a little bit.
• This is a little bit radical at this point and mostly speculative. Why do we implement a controller for an action that will happen only once throughout the lifetime of a CR instance? No drift to correct or an observation to report back etc. Though it's technically more challenging, a mutating webhook could be considered.

[muvaf]

How about we allow managed resource CRDs to have label that they can tell what resource claim they can be used for? CloudSQLInstance CRD can have a label crossplane.io/resource-claim: MySqlInstance. One generic MySqlInstance controller could get all CRDs with that label, lists all instance of these CRDs and pick one randomly. Pretty much like kinds would register themselves to a portable resource claim kind as candidate.

[negz]

This is an intriguing idea. I like that it would allow us to assess all possible classes that could satisfy a claim in one place (instead of assessing each kind of class separately). If we did want to have classes effectively register support for a kind of claim, I think the CRD is a good place to do so.

Two potential issues I could see, though:

• If we did want to expand to making claim scheduling decisions by comparing the spec of the claim to the spec of the class we'd probably need whatever was doing that comparison to have access to the class's Go struct. It would probably be possible to do this using *unstructured.Unstructured, but we'd be working with raw JSON (informed by the CRD schema) to make scheduling decisions.
• If/when we introduced non-portable claims like a DynamoDb claim, the scheduling/defaulting controllers would like in the stack where that claim was defined, while portable claims like MySQLInstance would have their scheduling/defaulting controllers defined in Crossplane runtime. It's probably a stretch to call this an "issue" - more of an asymmetry.

[muvaf]

If we did want to expand to making claim scheduling decisions by comparing the spec of the claim to the spec of the class

I don't understand why we'd need to do that comparison. In my imagination, this controller's job would be just to choose a class and put its reference to classRef of MySqlInstance claim that it reconciles. The type of the class would be unknown but only thing needed for referencing is metadata anyway.

It's probably a stretch to call this an "issue" - more of an asymmetry.

I agree. On the positive side, stack developers can implement their own unique scheduling mechanism for non-portable claims if they really wanted to.

[upbound-bot]

[negz]

If I'm not mistaken, this proposal doesn't prevent me from using any resource class I want.

That's true. It's worth noting it was also true in Crossplane 0.2, but is a regression in access control compared to 0.3 similar to the regression @prasek pointed out regarding RBAC access. I think we're trading a little flexibility for a little simplicity here.

Though it's technically more challenging, a mutating webhook could be considered.

Good point - I think a controller will work fine to begin with and is the path of least resistance, but I agree a webhook would probably make more sense. I'll add this to future considerations.

[upbound-bot]

[negz]

Going to merge this but leave it in draft until we've worked through the technical implementation a little more.