Adds Service Bundle contract document #808
Conversation
openshift-ci-robot
added
the
size/L
docs/service-bundle.md
Outdated
|
|
||
| The container will be run in a pod by itself with ``restartPolicy: Never``. The | ||
| pod will run in a sandbox namespace created by the broker for that specific | ||
| operation only. The namespace will be deleted at some point after the pod |
There was a problem hiding this comment.
If the pod fails OR certain parameters are set the namespace and pod could not be deleted.
I don't know if it matters for this document.
There was a problem hiding this comment.
I was trying to make the point that even if the namespace sticks around for a while, it is not intended to be in any way permanent. An administrator can make the broker keep it for one reason or another so they can inspect it, but the normal expectation should be that it is transient.
Maybe I'll add another sentence explaining ^.
There was a problem hiding this comment.
It's not always the case that the namespace or pod is deleted, it should at least be called out that it's possible to keep them regardless of the outcome, as a lot of developers working with this are going to be interested in that fact.
docs/service-bundle.md
Outdated
| operation only. The namespace will be deleted at some point after the pod | ||
| finishes. | ||
|
|
||
| APB is run with a service account granted access to the ``POD_NAMESPACE`` and |
There was a problem hiding this comment.
Should we also discuss the how the permissions are given to the APB?
Eventually, APB authors are going to need to know the permissions they are running at right?
There was a problem hiding this comment.
Yeah I agree with Shawn. Worth calling out in this document.
There was a problem hiding this comment.
Good call. Would you mind supplying that info? Or pointing me to the right place where I can find it?
There was a problem hiding this comment.
Permission for the service account comes from the openshift.sandbox_role from the config. The default for the template is edit but I think that most things deploy with this set as admin IIRC. here is a link to the roles https://kubernetes.io/docs/admin/authorization/rbac/#user-facing-roles
There was a problem hiding this comment.
I'm reading up on all of this, including this page: https://github.com/openshift/ansible-service-broker/blob/master/docs/administration.md
But I think I have too many questions in my mind about how things fit together to write this right now. Would one of you mind writing a couple of sentences about this?
docs/service-bundle.md
Outdated
|
|
||
| Keys contained in the Secret: | ||
|
|
||
| * ca.crt: a base64-encoded CA certificate that was used to sign the certificate |
There was a problem hiding this comment.
My understanding is that the `service-ca.crt is used to sign the certificate used by the broker to server its API as well as etcds API
There was a problem hiding this comment.
You appear to be right based on this: https://github.com/openshift/ansible-service-broker/blob/master/scripts/run_latest_build.sh#L89
I'm not sure what to do with this extra ca.crt that we don't know the purpose of.
There was a problem hiding this comment.
This is valuable to document, so not questioning its place in here, but do for my own edification, do we have a use-case for APB authors to interact with these files directly? The majority of the time, they just serve to authenticate the SA to do the APB work, no?
There was a problem hiding this comment.
There really isn't reason for a Service Bundle developer (APBs included as an instance of a Service Bundle) to interact with these directly. We may just want to link to the k8s documentation on what happens to service accounts in pods.
There was a problem hiding this comment.
Does the broker mount this information to this location? Or is it something kubernetes is doing automatically for some reason?
There was a problem hiding this comment.
Few questions. I feel like I'm also seeing a lot of content and code get written that are introducing generic automation broker and service bundle ideas and language, but we really don't have a migration plan into that world for our work. I think it would be wise to hold a meeting to talk about that migration plan, open questions, and execute on it fairly soon. Having everything in a halfway state is not a situation I would like to be in.
EDIT: To be clear, I think using the new language is correct and what we should be using moving forwards, I would just like a plan for migrating our existing work so that we remain consistent. That meeting also shouldn't block acceptance of this PR, it's going to be an ongoing process.
|
|
||
| ## Label | ||
|
|
||
| Every service bundle container image has a label ‘com.redhat.apb.spec’ that |
There was a problem hiding this comment.
Has anyone asked the question: Is an APB considered one of many types of service bundles that simply adheres to the contract, and chooses to implement its behavior in Ansible? I think this label may need to be more generic.
There was a problem hiding this comment.
Yeah perhaps worth removing redhat too eh? Label could just become bundle.spec.
There was a problem hiding this comment.
I know that @mhrivnak and I had discussed this and the answer was yes. An APB is an instance of a Service Bundle adhering to the contract.
It wasn't clear to me, @rthallisey if you are suggesting the label be com.redhat.bundle.spec or just bundle.spec. Only ask since we already have com.redhat.apb.runtime (which would probably remain since it is APB specific).
There was a problem hiding this comment.
Hey guys, I think that this is a follow-up issue:
Please review how accurately this represents the current relationship between a Service Bundle and the Automation Broker as implemented today. Please do not (yet) review the technical merits of this design.
A discussion that we need to have I just don't think that this is the right place just yet IMO
|
|
||
| * Action: a single word such as “provision”. The full list is below. | ||
| * --extra-vars: This is ansible-specific and always has exactly the value | ||
| “--extra-vars”. An Ansible Playbook Bundle uses this and the third argument |
There was a problem hiding this comment.
This happens to be ansible specific due to our heritage, but do we want to express arguments in a more generic way and have an apb-base perform a transform before delivering them to the playbook?
There was a problem hiding this comment.
I agree with making args more generic and having Service Bundle base images do the transform.
| * --extra-vars: This is ansible-specific and always has exactly the value | ||
| “--extra-vars”. An Ansible Playbook Bundle uses this and the third argument | ||
| as-is when calling ansible by appending them to the ansible command. | ||
| * JSON: a json document of information that is useful to the bundle. Currently |
There was a problem hiding this comment.
I honestly didn't know about this. If it's possible to pass in additional JSON, is that just a 3rd raw string? An example could be very helpful here for all the permutations.
There was a problem hiding this comment.
I think the JSON is just the value for the extra vars parameter (aka. all the parameters being passed to the bundle)
There was a problem hiding this comment.
Gotcha, sorry was confused. That makes sense.
docs/service-bundle.md
Outdated
|
|
||
| * cluster: the type of cluster on which the service bundle is being run | ||
| (currently either ‘openshift’ or ‘kubernetes’). | ||
| * _apb_plan_id: the name of a “plan”, as defined through the OSB API, that is |
There was a problem hiding this comment.
_apb prefixed params need to be reconsidered in a world of generic bundles. The prefix should probably be changed to be agnostic.
| The secret should have a single key called “fields” whose value is a | ||
| base64-encoded JSON document. | ||
|
|
||
| Ansible Playbook Bundles use the “asb_encode_binding” module to do this. |
There was a problem hiding this comment.
Again, language here is not generic.
There was a problem hiding this comment.
Here I was intentionally ansible-specific to call out an example of how this is done in practice. If someone wants to see this taking place, they can look for this ansible module being used in an APB.
There was a problem hiding this comment.
ACK, makes sense. I'd say my comments re: the generic nature of this remain concerns of mine, but we're meeting to tackle that problem specifically, so for the purposes of this PR it's safe to ignore.
| JSON document under the key ``_apb_provision_creds``, as a sub-document. As such, | ||
| it can be useful for making root-level credentials created during provision | ||
| available to other operations that need to use them. If a request for non-async | ||
| binding is made and the bundle does not run, any “provision credentials” that |
There was a problem hiding this comment.
I think this is an artifact of our development and not something that belongs as part of a service bundle contract. It should be technically possible to do a sync bind and run an APB, IMO.
There was a problem hiding this comment.
It is possible to request a synchronous bind, and the broker may run a service bundle or not depending on its settings and what the bundle supports.
Is there a change you think should be made to this documentation right now? Or are you suggesting the behavior should change in the future and possibly be removed from the contract?
There was a problem hiding this comment.
@mhrivnak I think I mis-read this saying "bundles do not run for sync requests", which upon rereading again, is not what you meant.
docs/service-bundle.md
Outdated
|
|
||
| The container will be run in a pod by itself with ``restartPolicy: Never``. The | ||
| pod will run in a sandbox namespace created by the broker for that specific | ||
| operation only. The namespace will be deleted at some point after the pod |
There was a problem hiding this comment.
It's not always the case that the namespace or pod is deleted, it should at least be called out that it's possible to keep them regardless of the outcome, as a lot of developers working with this are going to be interested in that fact.
|
|
||
| ### Proxy | ||
|
|
||
| If there are proxy settings for the broker, they will be forwarded to the pod |
There was a problem hiding this comment.
This is correct, but doesn't entirely address the story. We also include lowercased variants, which is actually technically important because there are a lot of ansible modules (more specifically, python libraries) that are utterly inconsistent in their expectations, or downright don't respect the presence of these env vars. The first boto being one of them.
In addition, we take no action to pass on these variables to the app pods that are spawned by your APB. That means you are responsible for writing your APB to check for proxy settings in their environment, and conditionally applying them to your service if it is required. There is interest in some kind of framework support for delivering these settings automatically, but that's not currently available.
There was a problem hiding this comment.
Wow. It is good to know that. That reminds me that it may also be a requirement of a Service Bundle developer to annotate the objects they create in the cluster a particular way, like the service instance they belong to.
There was a problem hiding this comment.
If we are moving to generic definition of service bundle then perhaps its not even worth mentioning in the contract document. The contract should be agnostic of what the bundle is actually doing. In this case we would be assuming a service bundle is deploying on-cluster services.
Perhaps we also create a "Best Practices" document for provision on-cluster services? Unsure.
There was a problem hiding this comment.
The hope is that we might be able to use PodPresets to deliver them automatically via label, and then we could write a module that wraps that to make it easy for developers.
docs/service-bundle.md
Outdated
|
|
||
| Keys contained in the Secret: | ||
|
|
||
| * ca.crt: a base64-encoded CA certificate that was used to sign the certificate |
There was a problem hiding this comment.
This is valuable to document, so not questioning its place in here, but do for my own edification, do we have a use-case for APB authors to interact with these files directly? The majority of the time, they just serve to authenticate the SA to do the APB work, no?
|
|
||
| ## Label | ||
|
|
||
| Every service bundle container image has a label ‘com.redhat.apb.spec’ that |
| * 1 - error | ||
| * 8 - action not implemented/supported | ||
|
|
||
| ### Encoded Credentials |
There was a problem hiding this comment.
I think we should also have a section for last_operation since it's something we pass back to the broker.
There was a problem hiding this comment.
Just for clarity, last_operation is an annotation on the running "Service Bundle" pod that can be read from the broker to get progress information right?
There was a problem hiding this comment.
Yes. The annotation on the pod is called apb_last_operation.
There was a problem hiding this comment.
@rthallisey I don't think this document should read 'bundle.spec' because that would need to go through a proposal process. @mhrivnak is simply trying to document what it does today but use the terms "service bundle" in place of APB for the vernacular. Which leads to some confusing items as pointed out by @eriknelson, but it is where we are today. We will have to update this doc as we migrate.
| metadata](https://github.com/ansibleplaybookbundle/ansible-playbook-bundle/blob/master/docs/developers.md#apb-spec-file) | ||
| related to the service bundle. Example: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Does the apb.yml become bundle.yml?
There was a problem hiding this comment.
That would be part of a proposal doc. So as far as I can tell, it should remain apb.yml until we decide.
|
As a reminder, let's please keep review within scope of making this reflect the current reality, and not let the discussion run away to a myriad of desired changes to how things work. I am also eager to have those discussions, but we'll benefit from first articulating a well-defined frame of reference. I appreciate the feedback so far and will make a round of edits this morning. Thanks! |
@eriknelson I agree they should be documented even though the APB author might not interact with them directly, but it could help them understand why some things might not be working for them. |
docs/service-bundle.md
Outdated
| # Service Bundle Specification | ||
|
|
||
| A Service Bundle is a container image that the broker can use to manage | ||
| deployment of a service in a cluster. It adheres to the properties and |
There was a problem hiding this comment.
the deployment of a service
docs/service-bundle.md
Outdated
|
|
||
| * cluster: the type of cluster on which the service bundle is being run | ||
| (currently either ‘openshift’ or ‘kubernetes’). | ||
| * _apb_plan_id: the name of a “plan”, as defined through the OSB API, that is |
docs/service-bundle.md
Outdated
| * namespace: the target namespace in which resources should be created, | ||
| deleted, or acted upon. | ||
|
|
||
| Other keys represent parameter names from the Plan, the values for which have |
docs/service-bundle.md
Outdated
| ### Service Account | ||
|
|
||
| Details of a kubernetes service account are mounted at | ||
| ``/var/run/secrets/kubernetes.io/serviceaccount`` |
There was a problem hiding this comment.
https://kubernetes.io/docs/admin/service-accounts-admin/ I think we may want to avoid getting into the specifics here.
There was a problem hiding this comment.
The apb-base has this generic kube-config that it allows it to simply use the service account that was given.
|
|
||
| * 0 - success | ||
| * 1 - error | ||
| * 8 - action not implemented/supported |
There was a problem hiding this comment.
I've seen us return 2 as well, I'll find out when that was happening.
mhrivnak
force-pushed
the
service-bundle-doc
branch
from
ea86369
to
5dfef0a
Compare
This document captures a detailed description of the existing contract between a Service Bundle and the Automation Broker.
Please review how accurately this represents the current relationship between a Service Bundle and the Automation Broker as implemented today. Please do not (yet) review the technical merits of this design. Once this document is reviewed and accepted as an accurate representation of reality, a next step will be to review the design on technical grounds and look for opportunities to improve it.