Proposal for saving extracted credentials #768
Conversation
shawn-hurley
added
proposal
needs-review
3.10 | release-1.2
shawn-hurley
requested review from
jmrodri,
mhrivnak,
eriknelson and
rthallisey
openshift-ci-robot
added
the
size/S
| namespace: <Namespace for the broker> | ||
| ``` | ||
|
|
||
| [Gob encoding](https://godoc.org/encoding/gob) will allow us to save arbitrary data in a key, and our secrets keys will look rational to a user who looks are our secret. |
| The functions for saving and retrieving will be in the `clients` package. This means the callers will be required to use the underlying extracted credentials type `map[string]interface{}` because we do not want a circular dependency between `apb` package and `clients` package. | ||
|
|
||
| ### Work Items | ||
| - [ ] Add kubernetes client methods to save and retrieve extracted credentials to the [namespace](https://github.com/openshift/ansible-service-broker/blob/master/docs/config.md#openshift-configuration). |
There was a problem hiding this comment.
it looks like you are saying that the credentials will be saved in the broker's namespace. Could you be explicit?
* Deal with where extracted credentials will be updated. * Add more explict statements around the secrets.
openshift-ci-robot
added
size/M
| .... | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: |
There was a problem hiding this comment.
It may be a good to store the ClusterServiceClass externalName (i.e the human readable value) and the action in a label on the secret. The purpose of this would be to assist debugging after the fact. It is likely that over a long period of time or during development, there may be many secrets created. If I am trying to find the secrets created by a particular ServiceClass it would be nice to be able to use the label selector
Something like:
metadata:
labels:
clusterServiceClassName/ apbName : "dh-service-apb"
action:"provision/bind"
Should allow me to do
oc get secrets -l apbName=dh-myservice-apb -l action=bind
There was a problem hiding this comment.
This sounds good.
I will add the ability to pass labels to the runtime package functions and to the clients package functions.
| - [ ] Add kubernetes client methods to interact with extracted credentials to the [namespace](https://github.com/openshift/ansible-service-broker/blob/master/docs/config.md#openshift-configuration). | ||
| - [ ] Add runtime methods for interacting with extracted credentials. These methods should be overridable. | ||
| - [ ] Remove all dao implementation and interface methods regarding extracted credentials. | ||
| - [ ] Remove all instances of interacting with dao extracted credentials in the `broker` package. Add back call to APB package to get extracted credentials when needed. |
There was a problem hiding this comment.
Are there also places where the credentials are deleted. Should we call out in the proposal that the credentials should be cleaned up if a service instance is deprovisioned or a binding deleted?
There was a problem hiding this comment.
Those actions are already taken currently and are covered by removing all the interacting work items and the update APB package to take the correct actions. I don't feel that the delete is different then update them on an update or create them on provision or binding. Just my opinion if it helps people understand what the PR is doing I will add.
|
Hey guys, I think that I am going to change the implementation details of the runtime overridable stuff. I will be editing this proposal but wanted to give a quick shout out. |
| <labels provided> | ||
| ``` | ||
|
|
||
| [Gob encoding](https://godoc.org/encoding/gob) will allow us to save arbitrary data in the secret for a key. The secrets keys will look rational to a user who looks at the created secret. This user would need permissions to see the secret, but if someone is looking at the secret making it obvious what data is in there will be helpful. |
There was a problem hiding this comment.
I'm not familiar with this pkg so excuse me if these should be obvious:
- Correct me if I'm wrong, but this is not required to accomplish what we're doing? What is the benefit, an independent storage format?
- We've talked about encrypting credentials, is that something that can be done easily here? (Or is that unnecessary thanks to our usage of secrets?)
There was a problem hiding this comment.
Additionally, is base64 encryption enough? Is that what the rest of openshift deems secure enough?
There was a problem hiding this comment.
We have a map[string]interface{}. The secrets data needs to be a[]byte transforming arbitrary data, interface{} to a byte array is tricky. This is one of the options that I found how to do this. I thought it would be helpful because the how to decode the value is included in the encoding so that even outside people could just use the built-in gob decoder for golang and decode the value.
With that being said, we could just marshal the entire map[string]interface to json and only have 1 key in the secret called credentials. I didn't think this was as good because then when a user who is debugging stuff has to know how we store it and the above way they will just have encoded values but the keys for the extracted credentials will be at the top level of the secret.
This is not encrypting the secret, but secrets are well understood and as far as I know, the correct place to put this data without encrypting it. So I was not planning on adding encryption here.
There was a problem hiding this comment.
+1 on both accounts. Thanks @shawn-hurley.
|
|
||
| The functions for saving and retrieving will be in the `clients` package. This means the callers will be required to use the underlying extracted credentials type `map[string]interface{}` because we do not want a circular dependency between `apb` package and `clients` package. | ||
|
|
||
| We will interact with the secrets from the namespace defined in the configuration by the `openshift.namespace` value. |
There was a problem hiding this comment.
It occurs to me reading this that we really should remove references to openshift from our configuration if we're looking to be cluster agnostic. Not at all in the scope of this proposal, just thought I'd call it out. We probably need an issue to track it.
| <labels provided> | ||
| ``` | ||
|
|
||
| [Gob encoding](https://godoc.org/encoding/gob) will allow us to save arbitrary data in the secret for a key. The secrets keys will look rational to a user who looks at the created secret. This user would need permissions to see the secret, but if someone is looking at the secret making it obvious what data is in there will be helpful. |
There was a problem hiding this comment.
Is this allowed in secrets? Will this make the secrets unusable outside of the broker?
There was a problem hiding this comment.
Most likely that is the case. If that becomes a problem I assume that we could revisit this then. Do you have a use case in mind?
|
|
||
| The functions for saving and retrieving will be in the `clients` package. This means the callers will be required to use the underlying extracted credentials type `map[string]interface{}` because we do not want a circular dependency between `apb` package and `clients` package. | ||
|
|
||
| We will interact with the secrets from the namespace defined in the configuration by the `openshift.namespace` value. |
There was a problem hiding this comment.
Is it possible to change this after starting the broker? Wondering about recovering.
There was a problem hiding this comment.
are you asking about changing on restart?
If you change this value on a restart and start to run the broker with a new namespace for resources yeah that would be a problem. This will also be a bigger problem once CRDs are a thing too. CRDs are going to be namespaced so they only live in that namespace. If you change this value then IMO you're saying to run a brand new broker.
There was a problem hiding this comment.
If someone is changing openshift.namespace while the broker is down, that is opening a can of worms that is outside the scope of this PR. Correct me if I'm wrong, but recovery is designed to allow the broker to restart and pick up outstanding operations after having being bounced by k8s. The config shouldn't have changed in that timeframe.
No description provided.