Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recommend CRD label to advertise provisioned services #117

Merged

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Oct 2, 2020

Refs #81

@scothis
Copy link
Contributor Author

scothis commented Oct 2, 2020

Is there a better value for the label than "true"?

@baijum
Copy link
Member

baijum commented Oct 2, 2020

Is there a better value for the label than "true"?

"true" looks fine to me.

/LGTM

README.md Outdated
@@ -104,7 +104,7 @@ An implementation is not compliant if it fails to satisfy one or more of the MUS

# Provisioned Service

A Provisioned Service resource **MUST** define a `.status.binding.name` which is a `LocalObjectReference`-able to a `Secret`. The `Secret` **MUST** be in the same namespace as the resource. The `Secret` **SHOULD** contain a `type` entry with a value that identifies the abstract classification of the binding. It is **RECOMMENDED** that the `Secret` also contain a `provider` entry with a value that identifies the provider of the binding. The `Secret` **MAY** contain any other entry.
A Provisioned Service resource **MUST** define a `.status.binding.name` which is a `LocalObjectReference`-able to a `Secret`. The `Secret` **MUST** be in the same namespace as the resource. The `Secret` **SHOULD** contain a `type` entry with a value that identifies the abstract classification of the binding. It is **RECOMMENDED** that the `Secret` also contain a `provider` entry with a value that identifies the provider of the binding. The `Secret` **MAY** contain any other entry. To facilitate discoverability, it is **RECOMMENDED** that a `CustomResourceDefinition` exposing a Provisioned Service add `service.binding/provisioned-service=true` as a label.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A Provisioned Service resource **MUST** define a `.status.binding.name` which is a `LocalObjectReference`-able to a `Secret`. The `Secret` **MUST** be in the same namespace as the resource. The `Secret` **SHOULD** contain a `type` entry with a value that identifies the abstract classification of the binding. It is **RECOMMENDED** that the `Secret` also contain a `provider` entry with a value that identifies the provider of the binding. The `Secret` **MAY** contain any other entry. To facilitate discoverability, it is **RECOMMENDED** that a `CustomResourceDefinition` exposing a Provisioned Service add `service.binding/provisioned-service=true` as a label.
A Provisioned Service resource **MUST** define a `.status.binding.name` which is a `LocalObjectReference`-able to a `Secret`. The `Secret` **MUST** be in the same namespace as the resource. The `Secret` **SHOULD** contain a `type` entry with a value that identifies the abstract classification of the binding. It is **RECOMMENDED** that the `Secret` also contain a `provider` entry with a value that identifies the provider of the binding. The `Secret` **MAY** contain any other entry. To facilitate discoverability, it is **RECOMMENDED** that a `CustomResourceDefinition` exposing a Provisioned Service add `service.binding/provisioned-service: "true"` as a label.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scothis Is this a string in a typical scenario? It doesn’t use the bool type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the type for labels is map[string]string

@arthurdm
Copy link
Member

thanks a lot for this @scothis

I am wondering if this label should be in the CRs instead of the CRDs, for two purposes:

  1. Now that we support Secrets as direct services, we would like to annotate which Secrets are provisioned services
  2. It is more useful to a user to know which provisioned service CRs they can bind to as opposed to which CRDs they can bind to - since it's the CR information they'll need to put in their ServiceBinding.

@scothis
Copy link
Contributor Author

scothis commented Oct 13, 2020

I am wondering if this label should be in the CRs instead of the CRDs, for two purposes

See #81 (comment)

  1. Now that we support Secrets as direct services, we would like to annotate which Secrets are provisioned services

Restricting which specific resource are bindable is a separate concern that should probably be managed with something like OPA/Gatekeeper.

  1. It is more useful to a user to know which provisioned service CRs they can bind to as opposed to which CRDs they can bind to - since it's the CR information they'll need to put in their ServiceBinding.

Before you can discover what CRs are bindable, you have to discover which kinds are bindable. If a CRD is labeled as bindable, then all CRs of that kind are bindable (absent the resource being in an intermediate or error state).

@sbose78
Copy link
Contributor

sbose78 commented Oct 13, 2020

Would there be use cases to let a user query which services are bindable?

If yes, supporting it on the CR too helps users query those without necessarily having the permissions to query CRDs.

@arthurdm
Copy link
Member

Would there be use cases to let a user query which services are bindable?

hey @sbose78 - ya, I definitely had this use case in mind. I agree that supporting both CRD and CR gives enough flexibility to cover it.

@arthurdm
Copy link
Member

arthurdm commented Oct 26, 2020

@scothis - thoughts on augmenting the PR to support the label in either the CRD (for types that are bindable) or the CR (for service instances that are bindable)?

@scothis
Copy link
Contributor Author

scothis commented Oct 27, 2020

thoughts on augmenting the PR to support the label in either the CRD (for types that are bindable) or the CR (for service instances that are bindable)?

There's no mechanism in Kubernetes for querying with a label selector across resources. So any CR that is discoverable, first needs it's CRD to be discoverable. If the CRD is discovered, then you don't need to make individual resource discoverable (they already are).

In the case of Secrets via direct binding, #130 was merged which should address that concern.

supporting it on the CR too helps users query those without necessarily having the permissions to query CRDs.

There are better ways to solve this aspect of the problem, for example the Knative Discovery API. I don't think it's the spec's place to bless any particular approach, instead the spec should define the minimal amount of metadata required for these other approaches to flourish.

Copy link
Member

@arthurdm arthurdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I think #130 solves my biggest concern with Secrets.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
@scothis scothis force-pushed the provisioned-service-discovery branch from 4d2a732 to 8bb7bc7 Compare November 2, 2020 16:19
@arthurdm arthurdm merged commit 2bb663d into servicebinding:master Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants