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

Pass the parameters sent during a bind back through to unbind #530

Closed
maleck13 opened this issue Nov 2, 2017 · 15 comments · Fixed by #809
Closed

Pass the parameters sent during a bind back through to unbind #530

maleck13 opened this issue Nov 2, 2017 · 15 comments · Fixed by #809
Assignees
Labels
3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 feature

Comments

@maleck13
Copy link
Contributor

maleck13 commented Nov 2, 2017

Feature:

When preforming a bind operation, we pass a set of bind parameters through. These are picked up by the apb pod and can be used during the binding. These parameters may contain information that is subsequently used to create resources in the target service.
For example, when binding to keycloak, I may want to create a bearer client for a specific service. During the bind, I pass the name of the service through as a parameter and construct the name of the client based on the consuming service name.
When unbinding I want to be able to tear down what I created during the bind. In order to do this I need to know what service I am unbinding for. Having access to the params passed during the binding would make this possible.

It looks as though the params are stored with the binding:
https://github.com/openshift/ansible-service-broker/blob/master/pkg/broker/broker.go#L908

So in unbind, if we read back this binding based on the bindingID we can pass the binding params back through to the ubind action.

If this feature makes sense, let me know as I am happy to put together a PR for this

@rthallisey rthallisey added the 3.8 label Nov 2, 2017
@rthallisey
Copy link
Contributor

@shawn-hurley did you work on something similar to this?

@shawn-hurley
Copy link
Contributor

I think this is something that we need to look into. I would like for the APB to decide what it needs during unbinding, and only send the data it needs during the unbind. I think this is an enhancement that we need to think about as we get async unbind & bind.

@cfchase I think had some thoughts on this.

@maleck13
Copy link
Contributor Author

maleck13 commented Nov 2, 2017

What is the rationale behind having to declare the params you are interested in (I assume you mean somewhere like the apb.yaml) seems it may potentially over complicate things? Is there a concern around information leaking?

@rthallisey rthallisey added 3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 and removed 3.8 labels Jan 9, 2018
@maleck13
Copy link
Contributor Author

maleck13 commented Jan 9, 2018

hey guys any more thoughts on this, we would definitely find this useful. I am happy to put together a proposal on this.

@shawn-hurley
Copy link
Contributor

Information leaking is the primary concern that I was thinking of.

I thought that @cfchase had some thoughts on how this could/should work.

@cfchase
Copy link
Contributor

cfchase commented Jan 9, 2018

I agree I'd like to see this get implemented. I think it's really necessary for a properly working async bind and was planning on using it in the unbind, myself: https://github.com/ansibleplaybookbundle/hello-world-db-apb/blob/async-bind/roles/unbind-hello-world-db-apb/tasks/main.yml#L24.

As far as passing the binding parameters to unbind, I didn't have any issues. I couldn't think of a use case I was really worried about the information leakage, so I think someone else might better voice those concerns.

My concerns with service name weren't binding related as I remember and you can probably ignore the below if it's out of scope.

TLDR; We probably need some way to save variables for future use, not just bindings.

My concern with something like service name is that it's not necessarily binding related. For example, to support provisioning multiple times to the same namespace, we could generate service name such as service-{{random}} in the provision action. This wouldn't be in the saved parameters since a user didn't enter it and it wouldn't be saved in *_creds. In this case, we'd still need to pass in the service name from provision to the deprovision in order to properly delete the service. It seemed like we needed a separate module similar to the asb_encode_binding that could save information available in future actions. e.g.

# provision
- set_fact:
    service_name: service-{{ 9999 | random }}

- name: Save some stuff
  asb_save_vars:
    service_name: "{{ service_name }}"
# deprovision
- k8s_v1_service:
    name: {{ saved_vars.service_name }}
    namespace: '{{ namespace }}'
    state: absent

Seems like it would be weird to use asb_encode_binding on something that's not bindable. I think those concerns might be a separate issue, though

@maleck13
Copy link
Contributor Author

I agree with @cfchase, struggling to see an issue with passing the params back through to the unbind playbook as it is the APB that did the provision so has already had access to all the parameters.
The key here for me, is that the broker has the role of managing state for the APB as the APBs are essentially stateless unless they do something like create a known configmap in a users namespace with the data they want to reuse across different actions (but this is really just a hack and not dependable) or put all state into the credentials as this gets passed back to the different playbooks.

@cfchase Funny enough last night I also began thinking about having a asb_set_provision_state and asb_set_bind_state available that would enable the apb to set some state during a provision or bind that would be passed back through to the opposing action. I really like that idea as it gives asb developers a way to handle the lack of state without resorting to putting it a configmap or secret with a user's namespace or setting it as part of the credentials (as these are passed back through currently).

Would definitely be interested in others thoughts on this and would be interested in putting together a proposal.

@maleck13
Copy link
Contributor Author

@david-martin @aidenkeating @wei-lee may be interested in this discussion

@wei-lee
Copy link

wei-lee commented Jan 10, 2018

+1 on having something that are not secrets or configmaps in user namespace to save variables for future use. If we are going to do that, we probably will need some guideline on what kind of data should be saved using this approach, and what kind of data should be saved using asb_encode_binding.

@matzew
Copy link
Member

matzew commented Jan 18, 2018

+1 for supporting this

@philbrookes
Copy link

I think this is a great idea, I'm currently trying to implement a bind playbook which would be much easier if the initial params were available to me, I am currently having to write the initial params to a secret during the provision action so that I can pull them back out in the bind/unbind actions.

It would also be useful if we could save a variable to the APB context from inside any APB action in such a way that it will be sent back as an extra-var to any future actions that APB may run.

e.g. During the provision playbook I generate a random uuid, and save it to my APB context. Then in my deprovision playbook I am passed the uuid variable so that I can easily delete the user with that ID.

@maleck13
Copy link
Contributor Author

@rthallisey @shawn-hurley @eriknelson Planning on putting together a proposal for this in the next week or so

@eriknelson
Copy link
Contributor

Thanks @maleck13!

@jmrodri jmrodri self-assigned this Feb 18, 2018
@jmrodri
Copy link
Contributor

jmrodri commented Feb 18, 2018

Assigned to myself to track, @maleck13 I'll wait for your proposal.

@maleck13
Copy link
Contributor Author

@jmrodri Thanks. Been crazy busy the last week or so. Hoping to get some head space for this in the coming week or two

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants