Service Binding Specification for Kubernetes #1539

baijum · 2022-11-09T09:09:45Z

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

Issue #1289 , if available:

Description of changes:

Proposal to add support for Service Binding Specification for Kubernetes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

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

ack-bot · 2022-11-09T09:09:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: baijum
To complete the pull request process, please assign redbackthomson after the PR has been reviewed.
You can assign the PR to them by writing /assign @redbackthomson in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jaypipes

Hi @baijum, cool idea! I like this proposal. Couple inline questions/comments for you to think about.

docs/design/proposals/provisioned-service/provisioned-service.md

jaypipes · 2022-11-09T22:36:35Z

docs/design/proposals/provisioned-service/provisioned-service.md

+	// Binding exposes the Secret for Service Binding to conform the Provisioned Service
+	// as per the Service Binding Specification for Kubernetes.
+	// Ref. https://github.com/servicebinding/spec#provisioned-service
+	Binding *ServiceBindingSecretReference `json:"binding,omitempty"`


We would want to place this field within the Status.ACKResourceMetadata subfield. That way, it won't conflict with any resources that might have a Binding field already in their status...

The Service Binding Specification expects the Provisioned Service (e.g., DBInstance) to point to the Secret resource from .status.binding.name attribute. I think we can confirm if that field is not conflicting with any existing fields.

@baijum the problem is that the Status and Spec fields for all ACK resources are automatically generated by looking at the AWS service API's model definition. We don't know if there are APIs that have a Binding field in any Create operation's Output shape. If there were, that would conflict with this.

Can we check if the.status.binding.name attribute exists for any API? Also is it possible to reserve this field for service binding, so that it will not conflict in the future?

We could do this, sure, but the problem is we cannot predict the future. Since ACK CRDs are generated from the upstream API models, what happens if an upstream service API adds a Binding field in the future? This is why we place these kinds of fields into Status.ACKResourceMetadata to ensure that we don't conflict.

I have created a proposal to the spec here: servicebinding/spec#223
Let me know if creating a Secret resource with the same name as that of DBInstance resource (or any other service) would be acceptable.

@baijum I left a comment in the other PR. I would be OK with a separate reconciler (ala the FieldExportReconciler we already have in ACK) that would create/manage Secret resources like servicebinding.io expects.

+1 with Jay's comment here. This functionality seems similar enough to FieldExport, in that it accesses CRD fields and outputs into a Secret, that I believe it could simply be handled by that reconciler. In fact, we could add this feature by simply adding an optional field within the FieldExport CRD that provides an alternative to from.path, instead being from.bindingSpecification: true/false (name TBD). This would sidestep the need to reserve the .status.binding field.

jaypipes · 2022-11-09T22:39:20Z

docs/design/proposals/provisioned-service/provisioned-service.md

+
+Since a Secret resource needs to be created, updated, and deleted, the
+ClusterRole requires "*create",* "*update", and "delete"* permissions.
+Currently, this permission is not set in the existing controllers.


The current ClusterRole allows getting, patching, listing and watching Secret resources. Why would we need to add Create and Delete permissions?

The Service Binding Specification expects the Provisioned Service (e.g., DBInstance) to point to a Secret resource with all values required for connectivity from the application. In the case of DBInstance, the corresponding controller needs to ensure the presence of the Secret resource. To create a Secret, Create permission is required. Similarly when the DBInstance is getting deleted, cleaning up the corresponding Secret resource would be good. Hence the Delete permission.

@baijum how does the controller know what the Secret's name will be?

The DBInstance controller generates the Secret. For example, the name could be the same as the DBInstance name.

The DBInstance controller generates the Secret. For example, the name could be the same as the DBInstance name.

@baijum ACK's approach is generally to give that power to the Kubernetes user, which is why the separate reconciler in ACK processes FieldExport CRs that allow the Kubernetes user to specify the name of the Secret or ConfigMap to export fields into.

jaypipes · 2022-11-09T22:40:30Z

docs/design/proposals/provisioned-service/provisioned-service.md

+|**Field**  | **Value**                |  **Remarks**
+|-----------|--------------------------|-------------------------------
+|type       |  postgresql/mysql        |   Based on Spec.Engine
+|provider   |  aws                     |   
+|host       |  Status.Endpoint.Address |   
+|port       |  Status.Endpoint.Port    |   
+|username   |  Spec.MasterUsername     |   
+|password   |                          |   Based on Spec.MasterUserPassword
+|database   |  Spec.Engine?            |   


Are the above fields intended to be populated as keys within a Secret?

Yes, the keys will change depending on the service. The above keys are examples that can be used with a relational database.

Co-authored-by: Jay Pipes <[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]>

RedbackThomson · 2023-01-20T17:41:52Z

docs/design/proposals/provisioned-service/provisioned-service.md

+	// Binding exposes the Secret for Service Binding to conform the Provisioned Service
+	// as per the Service Binding Specification for Kubernetes.
+	// Ref. https://github.com/servicebinding/spec#provisioned-service
+	Binding *ServiceBindingSecretReference `json:"binding,omitempty"`


+1 with Jay's comment here. This functionality seems similar enough to FieldExport, in that it accesses CRD fields and outputs into a Secret, that I believe it could simply be handled by that reconciler. In fact, we could add this feature by simply adding an optional field within the FieldExport CRD that provides an alternative to from.path, instead being from.bindingSpecification: true/false (name TBD). This would sidestep the need to reserve the .status.binding field.

RedbackThomson · 2023-01-20T17:42:56Z

docs/design/proposals/provisioned-service/provisioned-service.md

+### Implementation
+
+Since the API changes required for various custom resources are
+standard, and they can be generated for all controllers. However, the
+changes for each of these controllers can be implemented gradually.


This implementation doesn't really touch on how we know which fields are required for the binding specification? I imagine that each of the bindings will need to be manually crafted. It would be good to have a section that describes how controller maintainers can add new bindings and how they know what the spec should be.

baijum · 2023-02-17T01:39:35Z

I am closing this proposal as FieldExport can be used as a solution for the problem.

RedbackThomson · 2023-02-21T17:56:26Z

@baijum I see you have closed this PR. Do you have an example for the ACK community with how FieldExport could implement the service binding spec?

baijum · 2023-02-22T03:56:41Z

@baijum I see you have closed this PR. Do you have an example for the ACK community with how FieldExport could implement the service binding spec?

As I explained here: https://gist.github.com/baijum/96158408eaf544f692acfad42f2b49df
The Secret generated through FieldExport can be used for ServiceBiding. The Secret must have type field and optionally it can also add provider, something like this:

apiVersion: v1
kind: Secret
metadata:
  name: rds-postgres-field-exports
  namespace: ack-system
stringData:
  type: postgresql
  provider: was

ack-prow · 2023-10-12T16:26:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: baijum
Once this PR has been reviewed and has the lgtm label, please assign jljaco for approval by writing /assign @jljaco in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ack-bot · 2024-04-09T18:56:31Z

Issues go stale after 180d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 60d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

ack-bot · 2024-06-08T19:44:20Z

Stale issues rot after 60d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 60d of inactivity.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle rotten

ack-bot · 2024-08-07T20:27:03Z

Rotten issues close after 60d of inactivity.
Reopen the issue with /reopen.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/close

ack-prow · 2024-08-07T20:27:08Z

@ack-bot: Closed this PR.

In response to this:

Rotten issues close after 60d of inactivity.
Reopen the issue with /reopen.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Service Binding Specification for Kubernetes

707f1bf

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

ack-bot requested review from a-hilaly and RedbackThomson November 9, 2022 09:09

baijum mentioned this pull request Nov 9, 2022

Creating new status fields for RDS DBInstance CRD #1289

Closed

jaypipes reviewed Nov 9, 2022

View reviewed changes

Fix typo

5b92b59

Co-authored-by: Jay Pipes <[email protected]>

baijum mentioned this pull request Dec 10, 2022

Redefine Provisioned Service for flexibility servicebinding/spec#223

Closed

RedbackThomson reviewed Jan 20, 2023

View reviewed changes

baijum closed this Feb 17, 2023

a-hilaly reopened this Oct 12, 2023

ack-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 9, 2024

ack-prow bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 8, 2024

ack-prow bot closed this Aug 7, 2024

Service Binding Specification for Kubernetes #1539

Service Binding Specification for Kubernetes #1539

Uh oh!

Conversation

baijum commented Nov 9, 2022

Uh oh!

ack-bot commented Nov 9, 2022

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baijum commented Feb 17, 2023

Uh oh!

RedbackThomson commented Feb 21, 2023

Uh oh!

baijum commented Feb 22, 2023

Uh oh!

ack-prow bot commented Oct 12, 2023

Uh oh!

ack-bot commented Apr 9, 2024

Uh oh!

ack-bot commented Jun 8, 2024

Uh oh!

ack-bot commented Aug 7, 2024

Uh oh!

ack-prow bot commented Aug 7, 2024

Uh oh!

Uh oh!