{"action":"created","issue":{"url":"https://api.github.com/repos/crossplaneio/crossplane/issues/870","repository_url":"https://api.github.com/repos/crossplaneio/crossplane","labels_url":"https://api.github.com/repos/crossplaneio/crossplane/issues/870/labels{/name}","comments_url":"https://api.github.com/repos/crossplaneio/crossplane/issues/870/comments","events_url":"https://api.github.com/repos/crossplaneio/crossplane/issues/870/events","html_url":"https://github.com/crossplaneio/crossplane/issues/870","id":501345284,"node_id":"MDU6SXNzdWU1MDEzNDUyODQ=","number":870,"title":"Match claims to classes using label selectors","user":{"login":"negz","id":1049349,"node_id":"MDQ6VXNlcjEwNDkzNDk=","avatar_url":"https://avatars2.githubusercontent.com/u/1049349?v=4","gravatar_id":"","url":"https://api.github.com/users/negz","html_url":"https://github.com/negz","followers_url":"https://api.github.com/users/negz/followers","following_url":"https://api.github.com/users/negz/following{/other_user}","gists_url":"https://api.github.com/users/negz/gists{/gist_id}","starred_url":"https://api.github.com/users/negz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/negz/subscriptions","organizations_url":"https://api.github.com/users/negz/orgs","repos_url":"https://api.github.com/users/negz/repos","events_url":"https://api.github.com/users/negz/events{/privacy}","received_events_url":"https://api.github.com/users/negz/received_events","type":"User","site_admin":false},"labels":[{"id":1049593770,"node_id":"MDU6TGFiZWwxMDQ5NTkzNzcw","url":"https://api.github.com/repos/crossplaneio/crossplane/labels/enhancement","name":"enhancement","color":"a2eeef","default":true},{"id":1393350821,"node_id":"MDU6TGFiZWwxMzkzMzUwODIx","url":"https://api.github.com/repos/crossplaneio/crossplane/labels/proposal","name":"proposal","color":"f7ef5b","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":10,"created_at":"2019-10-02T08:20:26Z","updated_at":"2019-10-09T16:26:31Z","closed_at":null,"author_association":"MEMBER","body":"<!--\r\nThank you for helping to improve Crossplane!\r\n\r\nPlease be sure to search for open issues before raising a new one. We use issues\r\nfor bug reports and feature requests. Please find us at https://slack.crossplane.io\r\nfor questions, support, and discussion.\r\n-->\r\n\r\n## What problem are you facing?\r\n<!--\r\nPlease tell us a little about your use case - it's okay if it's hypothetical!\r\nLeading with this context helps frame the feature request so we can ensure we\r\nimplement it sensibly.\r\n--->\r\nIn Crossplane 0.3 we introduced support for \"provider specific\" (aka \"non-portable\", aka \"[strongly typed](https://github.com/crossplaneio/crossplane/blob/69e69c/design/one-pager-strongly-typed-class.md)\") resource classes. During the development cycle [we identified](https://github.com/crossplaneio/crossplane/issues/723) that if a claim must explicitly reference a provider specific resource class in order to enable dynamic provisioning, then the claim itself is now provider specific and no longer portable across providers. We solved this issue by introducing [portable resource classes](https://github.com/crossplaneio/crossplane/blob/69e69c/design/one-pager-default-resource-class.md).\r\n\r\nA portable resource class is effectively an indirection to a non-portable resource class. Portable resource classes can be set as the default for a particular namespace, and will be used by any resource claims of their corresponding kind that do not specify a resource class for dynamic provisioning, or a managed resource for static provisioning. This can be thought of as \"publishing\" non-portable resource classes (which may exist in a distinct namespace used to logically group infrastructure) to other namespaces, where applications may use them to satisfy claims for their infrastructure requirements.\r\n\r\nThis pattern is flexible and powerful, but Crossplane maintainers and [community members](https://github.com/crossplaneio/crossplane/issues/703#issuecomment-536958545) have observed that it's verbose; three distinct Kubernetes resources (a claim, portable class, and non-portable class) must exist for dynamic provisioning to occur.\r\n\r\n## How could Crossplane help solve your problem?\r\n<!--\r\nLet us know how you think Crossplane could help with your use case. \r\n-->\r\nCrossplane could remove the need for portable resource classes by using [label selectors](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors) at the resource claim level, rather than the concept of a default class.\r\n\r\n### User experience\r\n\r\nResource claim authors who have a specific, non-portable resource class in mind just set the claim's `classRef` to that resource class (including its type metadata) just like they did before https://github.com/crossplaneio/crossplane/issues/723. Resource claim authors who have a specific, managed resource in mind just set the claim's `resourceRef` as they do today for static provisioning. Resource claim authors who don't know exactly what resource class they want omit the `classRef`, omit the `resourceRef`, and supply zero or more `classSelector` labels to match.\r\n\r\nFor example:\r\n\r\n```yaml\r\n---\r\n# Imagine this resource claim is created by a hypothetical GitLab stack\r\n# controller to satisfy a request for a new GitLab installation. It does not\r\n# specifically name a resource class, but declares it wants an experimental\r\n# grade PostgreSQLInstance that is compatible with the Gitlab stack, in\r\n# region us-east-1.\r\napiVersion: database.crossplane.io/v1alpha1\r\nkind: PostgreSQLInstance\r\nmetadata:\r\n  name: gitlab-database\r\n  namespace: gitlab-experiments\r\nspec:\r\n  classSelector:\r\n    matchLabels:\r\n      stack: gitlab\r\n      grade: experimental\r\n      region: us-east-1\r\n  writeConnectionSecretToRef:\r\n    name: app-postgresql-connection\r\n```\r\n\r\nNote that under this proposal the concept of a \"portable class\" is removed, so any reference to \"class\" or \"resource class\" means \"non-portable resource class\", for example `CloudSQLInstanceClass`.\r\n\r\nFrom the claim author's perspective Crossplane would handle dynamic provisioning of the above `PostgreSQLInstance` claim by:\r\n\r\n1. \"Finding\" all classes that match its `.spec.classSelector.matchLabels`.\r\n1. \"Picking one at random\".\r\n1. Seting the `PostgreSQLInstance` `.spec.classRef` to the randomly selected resource class.\r\n1. Dynamically provisioning and binding as we do today.\r\n\r\nThis proposal has the following properties:\r\n\r\n* It supports portable, strongly typed, dynamic provisioning using only two concepts; resource claims and resource classes.\r\n* It works the same for a hypothetical non-portable `CloudSQLInstanceClaim` as it does for a portable `PostgreSQLInstanceClaim`.\r\n* It approximates support for \"default resource classes\". Label a resource class `{default: \"true\"}` to make it the global default for compatible resource claims (that select that label). Label a resource class `{default: \"true\", namespace: app-project1-dev}`, to make it the \"default for a namespace\" (actually, the default for any claim that selects those labels).\r\n* It makes resource class selection non-deterministic. If multiple resource classes match the labels, one is chosen at random.\r\n* It removes the concept of \"publishing\" a class to a namespace. This means a cluster operator can set a class as the \"default\" (kind of - see above) for app namespaces that don't exist yet, but also makes it harder for app operators to determine what resource classes they could choose from, presuming the resource classes lived in a distinct infrastructure namespace.\r\n* It enables a rudimentary form of \"matching\" (dare I say scheduling?) of resource claims by decoupling the language used to match claims to classes from the (strongly typed) language used to describe classes.\r\n\r\nElaborating on that last bullet, it's hard to match the requirements of a resource claim to the capabilities of a resource class on `spec` alone due to the \"lowest common denominator\" problem of multi cloud. The `RedisCluster` claim, for example, exposes a field named `engineVersion`, which specifies its desired version of Redis (e.g. `\"3.2\"`). Ideally we'd use this snippet of information to intelligently match a `RedisCluster` claim to a resource class for dynamic provisioning, but that is hard(TM). The following resource classes could satisfy a `RedisCluster` claim today:\r\n\r\n* GCP `CloudMemorystoreInstanceClass` has an equivalent field to `engineVersion` - it's named `redisVersion` and expects the version to be written as `\"REDIS_3_2\"`, not `\"3.2\"`.\r\n* AWS `ReplicationGroupClass` has an `engineVersion` field that expects the patch version to be included, i.e. `\"3.2.0\"`.\r\n* Azure `RedisClass` has no field for engine version. It's just always version 3.2.\r\n\r\nWe could try to solve this by having resource claims expose the intersection (or the union?) of every possible resource class's set of fields in some standardised fashion, then teach each resource claim controller how to match the standardised resource claim fields to the provider specific resource class fields, or we could just use labels.\r\n\r\nFor example this resource claim specifies that it needs Redis 3.2:\r\n\r\n```yaml\r\n---\r\napiVersion: cache.crossplane.io/v1alpha1\r\nkind: RedisCluster\r\nmetadata:\r\n  name: gitlab-cache\r\n  namespace: gitlab-experiments\r\nspec:\r\n  classSelector:\r\n    matchLabels:\r\n      stack: gitlab\r\n      engineVersion: \"3.2\"\r\n  writeConnectionSecretToRef:\r\n    name: gitlab-cache-connection\r\n```\r\n\r\nWhile these resource classes specify that they can satisfy claims that need Redis 3.2, while also using documented, high fidelity, validated, schemas when dynamically provisioning managed resources:\r\n\r\n```yaml\r\n---\r\napiVersion: cache.gcp.crossplane.io/v1alpha2\r\nkind: CloudMemorystoreInstanceClass\r\nmetadata:\r\n  name: gitlab-development-default\r\n  namespace: gcp-infra-dev\r\n  labels:\r\n    engineVersion: \"3.2\"\r\nspecTemplate:\r\n  tier: STANDARD_HA\r\n  region: us-west2\r\n  memorySizeGb: 1\r\n  redisVersion: REDIS_3_2\r\n  providerRef:\r\n    name: example\r\n    namespace: gcp-infra-dev\r\n  reclaimPolicy: Delete\r\n---\r\napiVersion: cache.azure.crossplane.io/v1alpha2\r\nkind: RedisClass\r\nmetadata:\r\n  name: azure-redis-standard\r\n  namespace: azure-infra-dev\r\n  labels:\r\n    engineVersion: \"3.2\"\r\nspecTemplate:\r\n  resourceGroupName: group-westus-1\r\n  location: West US\r\n  sku:\r\n    name: Basic\r\n    family: C\r\n    capacity: 0\r\n  enableNonSslPort: true\r\n  providerRef:\r\n    name: example\r\n    namespace: azure-infra-dev\r\n  reclaimPolicy: Delete\r\n```\r\n\r\n### Implementation Details\r\n\r\nThere are a lot of quotation marks in the above steps because what would actually be happening under the hood would be more like a race to satisfy the claim. In Crossplane there is not one controller for each resource claim kind, but rather one controller for each possible (resource claim, managed resource) tuple. This allows an infrastructure stack that adds support for a managed resource that could satisfy a resource claim to implement the dynamic provisioning and claim binding logic for said claim without having to touch Crossplane core. Put otherwise, `crossplaneio/stack-gcp` can add support for `CloudSQLInstance` resources to satisfy `MySQLInstance` claims in `crossplaneio/stack-gcp`, instead of teaching `crossplaneio/crossplane` how to dynamically provision a `CloudSQLInstance`. Hence the race. In reality the following would happen for the example above:\r\n\r\n* All resource claim controllers are updated to only reconcile resources with a `resourceRef` or a `classRef`.\r\n* A new \"class selector\" controller is introduced for each (resource claim, resource class) tuple. Its job is to set the `classRef` for resource classes that omit it, by using their `classSelector`.\r\n\r\nUpon the creation of a `PostgreSQLInstance` without a `classRef` or `resourceRef`:\r\n\r\n1. Every class selector controller watching for `PostgreSQLInstance` claims has a reconcile queued. I'll use the `(PostgreSQLInstance, CloudSQLInstanceClass)` controller for this example, but `(PostgreSQLInstance, RDSInstanceClass)` and `(PostgreSQLInstance, MysqlServerClass)` controllers would run through the process in parallel.\r\n1. The controller would list all `CloudSQLInstanceClass` resources (in any namespace) that matched the `PostgresSQLInstance` claim' label selectors.\r\n1. If no `CloudSQLInstanceClass` matched the labels, the reconcile is done. Otherwise, one of the matching `CloudSQLInstanceClass` resources is selected at random.\r\n1. The controller sleeps for a small, jittered amount of time then sets the `classRef` of the `PostgreSQLInstance` to the selected `CloudSQLInstanceClass`. The jitter reduces the chance of two controllers trying to set the `classRef` at the same time. If two controllers _do_ try to set the `classRef` at the same time one will fail to commit the change due to the `PostgreSQLInstance` claim's resource version having changed since it was read.\r\n1. The reconcile is done. With the `classRef` set the `PostgresSQLInstance` now passes the watch predicates of the `(PostgreSQLInstance, CloudSQLInstance)` resource claim reconciler, which dynamically provisions and binds it."},"comment":{"url":"https://api.github.com/repos/crossplaneio/crossplane/issues/comments/540078939","html_url":"https://github.com/crossplaneio/crossplane/issues/870#issuecomment-540078939","issue_url":"https://api.github.com/repos/crossplaneio/crossplane/issues/870","id":540078939,"node_id":"MDEyOklzc3VlQ29tbWVudDU0MDA3ODkzOQ==","user":{"login":"prasek","id":284840,"node_id":"MDQ6VXNlcjI4NDg0MA==","avatar_url":"https://avatars1.githubusercontent.com/u/284840?v=4","gravatar_id":"","url":"https://api.github.com/users/prasek","html_url":"https://github.com/prasek","followers_url":"https://api.github.com/users/prasek/followers","following_url":"https://api.github.com/users/prasek/following{/other_user}","gists_url":"https://api.github.com/users/prasek/gists{/gist_id}","starred_url":"https://api.github.com/users/prasek/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/prasek/subscriptions","organizations_url":"https://api.github.com/users/prasek/orgs","repos_url":"https://api.github.com/users/prasek/repos","events_url":"https://api.github.com/users/prasek/events{/privacy}","received_events_url":"https://api.github.com/users/prasek/received_events","type":"User","site_admin":false},"created_at":"2019-10-09T16:26:31Z","updated_at":"2019-10-09T16:26:31Z","author_association":"MEMBER","body":"@negz Thanks for writing this up and @hasheddan for the discussion!\r\n\r\nThe `select-class-namespace` annotation addresses many of the concerns for globally matching across any namespace, and allows the namespace owner to incrementally expand the scope of label selector matches beyond local (same namespace matches). If the `select-class-namespace` was omitted by the namespace owner, defaulting to local matches (same namespace as the claim) would reduce the complexity of labels required at scale to get a unique match and be more predictable for new users who didn’t add the annotation to their namespace in a larger shared cluster. \r\n\r\nLabels forgo validated unique names that portable resource class names provide, but if we move back to pre #723 behavior that can still be achieved with a `claim.spec.classRef` to a non-portable class. However that wouldn’t be a portable claim, so we really are losing that aspect with labels.\r\n\r\nSo with this proposal, portable claims would need to use label selectors.\r\n\r\nThe `select-class-namespace` annotation would be a variant of #922 option (a) and a great starting point — possibly for the portable classes we have today although that would collapse namespaces and the unique class names rendering them potentially ambiguous.\r\n\r\nSo perhaps #922 option (b) adding `claim.spec.classRef.namespace` back in for referencing portable classes in a non-local infra namespace is the simplest thing we could do to solve #922 and minimize the number of portable classes needed at scale.\r\n\r\nWe could also add support for non-portable `claim.spec.classRef` (pre #723) back in as well for people that don’t care about portability, or want to solve portability by adding conditional logic to a Helm chart or Stack that drops non-portable claims and classes into an app project namespace, so everything is local.\r\n\r\nReally like the label/selectors approach but need to think about some of the issues @hasheddan raised, although the select-class-namespace` annotation does address a lot of them IMO, esp. if it defaults to local matching when omitted."}}
{"actor":{"display_login":"prasek"}}