-
Notifications
You must be signed in to change notification settings - Fork 213
[RFC] Service Credential Binding Rotation for Apps #1198
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Some thoughts on binding names:
|
- CC just allows multiple bindings per app/service - require that binding names don't change on cred rotation - removed --recreate from cf bind-service - added option to keep last x bindings on cleanup
Reworked the RFC to address most of the discussion points:
Please review again. |
- add reasoning for the conditions of multiple bindings - allow multiple bindings to UPSI (even though there is no benefit but less constaints on API) - add reasoning for limiting the number of multiple bindings - cf cleanup-outdated-service-bindings: use flags for optional params
A full service binding rotation flow with the CF CLI could look like: | ||
``` | ||
cf bind-service myApp myService -c {<parameters>} | ||
cf bind-service myApp myService2 -c {<parameters>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be the same binding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample shows how you could rotate the bindings of an app which is bound to two different services. Does this answer the question?
- multiple bindings to the same service instance are intended for credential rotation | ||
- VCAP_SERVICES structure doesn't allow multiple bindings to the same service instance so different binding names don't make sense | ||
|
||
The number of multiple service credential bindings for the same app and service instance should be limited. The limit prevents a DoS threat and eventually reminds users to clean up old, likely outdated bindings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand a bit more on the DoS thread and how a limit would mitigate these risks? Are we expecting the creation of a binding to be really expansive? Also do we allow to create multiple bindings at the same time? If not there already will a natural delay. If you are thinking about database pollution that might be a valid concern, but this would be the first time we implement an arbitrary limit on a particular resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, the bindings are implicitly limited by app and service quotas (assuming they are not unlimited which they should not be in productive public foundations). With multiple service bindings per app/service instance, we give up on this implicit limit. That's why an additional limit. Adding a real (org and space) quota for bindings is probably an overkill -> so the idea is to add a foundation wide limit (configurable via bosh).
Service bindings are expensive resources. Creation is not just an API call and a DB record but also a background job that talks to the broker (and this communication can even be asynchronous).
There are limits (in addition to the CF quotas) already today:
- cc.droplets.max_staged_droplets_stored
- cc.max_annotations_per_resource
- cc.max_labels_per_resource
- cc.maximum_app_disk_in_mb
- cc.packages.max_package_size
We should not have any resources that can be created infinitely via the CF API as they open the door for misuse. Creation rate is limited by CF API rate limits but they are rather high and it won't be too hard to attack with multiple users...
|
||
### CC adoption of OSBAPI 2.17 | ||
|
||
The CC could use the service binding rotation functionality introduced with the OSBAPI 2.17 and introduce a dedicated API endpoint for rotation, e.g. `POST /v3/service_credential_bindings/:guid/actions/duplicate` (good naming to be found). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this naming be defined as part of this RFC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the need to define the naming for this in this RFC because it is future work which could happen or not.
During the TOC meeting on July 15th we decided to start the FCP for this RFC with the goal to accept it. Please have a final review on it. |
Link for easy viewing