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

Expose provisioned service binding #615

Merged
merged 3 commits into from
Feb 23, 2021

Conversation

ChunyiLyu
Copy link
Contributor

@ChunyiLyu ChunyiLyu commented Feb 22, 2021

This closes #605

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Summary Of Changes

This PR is to conform to the provisioned service binding, defined in Service Binding Specifications for K8s. The main purpose is to consume is in the messaging topology operator, and it is not meant to be the final solution for what RabbitmqCluster would expose in its service binding.

  • exposes rabbitmqcluster.Status.Binding, which sets to the default user secret object. It will be used in the messaging topology operator
  • default user secret object already expose username and password
  • add field provider and type which sets to rabbitmq to the default user secret. provider and type are recommended by the provisioned service spec. Adding a new key to the default user secret object does impact the conf.d file we mount onto rabbitmq container because the operator copies only the default user username and password into the conf.d file (see initContainer logic here)
  • I am aware of other properties that you can expose, for example host, ip.... However most of them need more discussions, and I think we can start with username, password, type, and provider which are information that's already there and unlikely to change its format.
  • I am not handing the ServiceReference and SecretReference that's already exposed in our rabbitmqcluster.Status. I think some more digging is going to be needed for us to determine whether we can safely delete them (to not be redundant) I don't think deleting them is a blocker to merge this PR. I've created a followup issue to look into it.
  • you can set secret type to service.binding/{type}. However secret type is immutable, so I am leaving it out.
  • Commits will be squashed when merging.

Additional Context

Local Testing

All tests passed.

- following provisioned service service binding
duck type
See: https://k8s-service-bindings.github.io/spec/#provisioned-service
- follow recommendation on provisioned service
binding spec
@ChunyiLyu ChunyiLyu force-pushed the expose-provisioned-service-binding branch from 32c46ee to 59b8ffc Compare February 22, 2021 13:44
@scothis
Copy link

scothis commented Feb 22, 2021

I think we can start with username, password, and type which are information that's already there and unlikely to change its format.

Looks like the Secret includes username, password and provider instead of type. type is the sole required field for the binding as it is how an application can distinguish one binding from another at runtime.

I understand if you want to think more deeply about the value of type along with other discovery fields like host, post, uri. The Cluster resource won't actually confirm to the spec until the type fields is added.

@nebhale
Copy link

nebhale commented Feb 22, 2021

@ChunyiLyu I wanted to provide a couple points of spec guidance that you might not be aware of.

  • The only required entry in the secret is type. The spec is written as SHOULD (The Secret data SHOULD contain a type entry...) because there's a mechanism for adding type later, but we consider that a way to help non-compliant secrets become complaint, rather than how a secret, built with knowledge of the spec, should be configured.
  • Not having some kind of endpoint information will be limiting in practice. For example for Boot application to use RabbitMQ, three properties must be set: spring.activemq.broker-url, spring.activemq.user, spring.activemq.password. The secret, automatically bound by the spec, will only contain two of those. Getting the third piece of information, in an automated way, will require a fair bit of extra work and be placed into a different location than the credentials. Now this isn't insurmountable (people use this mechanism in Kubernetes all the time today), but it doesn't have the seamless experience that the spec is aiming to provide.

@ChunyiLyu
Copy link
Contributor Author

ChunyiLyu commented Feb 22, 2021

@scothis @nebhale To clarify, this PR is not a final solution of what RabbitmqCluster service binding should look like. My intent was to expose some information through the binding as soon as possible so it can be used in a different operator which is still under development. As for broker url to connect to RabbitMQ, there has been some discussion within the team that it might make sense to expose it at a different level.

I just realized the type of secret is immutable. This is a problem for the cluster operator because default user secret is in use already. If the operator sets service type to be something else, clusters created by new versions of the operator would have a different secret type for its default user secret, which is not ideal. Perhaps having a dedicated service binding secret would be a cleaner solution. I will close this PR for now because more consideration is needed.

@ChunyiLyu ChunyiLyu closed this Feb 22, 2021
@scothis
Copy link

scothis commented Feb 22, 2021

My intent was to expose some information through the binding as soon as possible

👍

I just realized the type of secret is immutable.

There are two type fields. .type, and .data.type. Since we need the type to end up mounted in the application, it's .data.type that is required by the spec.

- following provisioned service binding duck type
requirement
@ChunyiLyu ChunyiLyu reopened this Feb 23, 2021
Copy link
Contributor

@coro coro left a comment

Choose a reason for hiding this comment

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

LGTM. Do we also want to comply with:

To facilitate discoverability, it is RECOMMENDED that a CustomResourceDefinition exposing a Provisioned Service add service.binding/provisioned-service: "true" as a label.

@ChunyiLyu ChunyiLyu merged commit ac9a212 into main Feb 23, 2021
@ChunyiLyu ChunyiLyu deleted the expose-provisioned-service-binding branch February 23, 2021 14:04
@scothis
Copy link

scothis commented Feb 23, 2021

@coro fyi, the key service.binding/provisioned-service is very likely to change before the spec reaches 1.0. Adding a label is relatively simple, but I don't want you to be caught off guard by the change. In general, service.binding is a placeholder api group that will change spec wide.

@MirahImage
Copy link
Member

@scothis thanks for the heads up

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.

Conform to the Provisioned-Service ducktype
5 participants