Skip to content

Redefine Provisioned Service for flexibility #223

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

Closed
wants to merge 1 commit into from

Conversation

baijum
Copy link
Contributor

@baijum baijum commented Dec 10, 2022

There are real world Services where the resource schema is auto-generated and no controll over what fields may get added to the status. See this discussion in ACK project.

aws-controllers-k8s/community#1539 (comment)

Signed-off-by: Baiju Muthukadan [email protected]

There are real world Services where the resource schema is
auto-generated and no controll over what fields may get added to the
status.  See this discussion in ACK project.

aws-controllers-k8s/community#1539 (comment)

Signed-off-by: Baiju Muthukadan <[email protected]>
1. a `.status.binding` field which is a `LocalObjectReference`-able (containing a single field `name`) to a `Secret`.
2. `.metadata.name` pointing to the Secret

The `Secret` **MUST** exist in the same namespace as the resource. The `Secret` data **SHOULD** contain a `type` entry with a value that identifies the abstract classification of the binding. The `Secret` type (`.type` verses `.data.type` fields) **SHOULD** reflect this value as `servicebinding.io/{type}`, substituting `{type}` with the `Secret` data type. It is **RECOMMENDED** that the `Secret` data also contain a `provider` entry with a value that identifies the provider of the binding. The `Secret` data **MAY** contain any other entry. To facilitate discoverability, it is **RECOMMENDED** that a `CustomResourceDefinition` exposing a Provisioned Service add `servicebinding.io/provisioned-service: "true"` as a label.

Choose a reason for hiding this comment

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

@baijum one other possibility I can think of is checking first for a servicebinding.io/binding-fieldpath annotation on a CR and if found, use that information to find the field within the Spec or Status that houses the Binding. That way, things like ACK could store the binding in, say, Status.ACKResourceMetadata.ServiceBinding and set an annotation on CR's of servicebinding.io/binding-fieldpath=status.ackResourceMetadata.serviceBinding. This would be extensible and allow for use cases where the CRD's schema is generated or cannot otherwise accomodate a Status.Binding field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaypipes Using annotation to specify the binding field path is a good idea. If the annotation is missing in a Provisioned Service, the default location would be the current field path (i.e., .status.binding). So, this change would be backward compatible.

@scothis
Copy link
Member

scothis commented Dec 12, 2022

As a general concern, any changes we make to how binding secrets are discovered will have the effect of breaking part of the ecosystem. Implementations of the 1.0 version of the spec will not understand these new semantics and will not find the binding secret. It also increases the complexity for implementors as there are more moving parts they need to be aware of and implement. This isn't to say we shouldn't try to solve this problem, but it does mean we need to be deliberate about the changes we make.

As a specific concern, creating a lookup hierarchy to discover the binding secret is problematic. If we implemented the behavior as proposed then all Provisioned Services would have at least a brief moment in their lifecycle where the resource exists, but has not exposed a status as no resource can be created with status. At that moment, the secret would resolve to the name of the resource. If the secret doesn't exist, then we're projecting a volume that can never be resolved and the workload will fail. Worse, if the secret does exist but wasn't intended to be used as binding secret then we're injecting ambiguous semantics into the workload. Binding the wrong service to a workload in production can be disastrous, even if for a moment.

A solution to the problem needs to satisfy to these requirements:

  • runtime compatible: we cannot introduce new requirements unless the new behavior is opt-in for a user
  • unambiguous: is must always be clear where a binding secret is discovered
  • limit impact to existing implementations: forcing existing implementations to adopt new behavior or break will not foster an ecosystem

Potential solutions that follow these requirements:

  1. spec extension: since extensions are inherently opt-in, there's no disruption to the existing ecosystem
  2. mapping resource: define a mapping resource that adapts the existing ACK resources to the existing Provisioned Service duck type

@arthurdm
Copy link
Member

Thanks for working with other communities and increasing the adoption of the spec @baijum! 👍

I agree with @scothis's second suggestion:

mapping resource: define a mapping resource that adapts the existing ACK resources to the existing Provisioned Service duck type

I believe we'll encounter many CRs in k8s that each have their specific requirements and restrictions for their .status, so if we can have synthetic resources that maps these into the Provisioned Service, that's a clean way to go.

@baijum
Copy link
Contributor Author

baijum commented Feb 17, 2023

Based on working group discussion yesterday, we have decided to close this PR. The alternatives suggested in the comments can be used by users.

@baijum baijum closed this Feb 17, 2023
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.

4 participants