Service-catalog: Support parameters from secrets

[derekwaynecarr]

The current API stores parameters on an instance as a runtime.RawExtension.

It's very likely parameters may actually hold sensitive information, we should think about how to handle these as secrets as a consequence.

/cc @pmorie @jwforres

[derekwaynecarr]

I would like to see this resolved before promoting APIs from alpha->beta.

[pmorie]

We could follow the pattern for EnvVars in pods. Something like:

type InstanceSpec struct {
    // other fields omitted
    SecretParameters []v1.SecretKeyRef
}

We probably want to do the same thing for bindings as well.

Thoughts?

[arschles]

I would like to see this resolved before promoting APIs from alpha->beta.

I agree. Since 0.0.3 is currently our planned last release before our first beta quality release, I am marking it with that milestone.

[arschles]

We aren't going to get to this by 0.0.3; moving this to 0.0.4

[arschles]

@derekwaynecarr do you know of any progress toward this issue? I am moving to the beta cycle for now

[derekwaynecarr]

It is still on our queue to get to at some point hopefully in next couple
weeks.

On Fri, Apr 21, 2017 at 11:51 PM Aaron Schlesinger [e-mail]
wrote:

@derekwaynecarr https://github.com/derekwaynecarr do you know of any
progress toward this issue? I am moving to the beta cycle for now

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/kubernetes-incubator/service-catalog/issues/621#issuecomment-296240614,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF8dbGFG2WJxvKYf79UgeOH4Y-xG5mSVks5ryNsagaJpZM4Mv0eF
.

[ash2k]

I wanted to raise this/similar question and came across this issue.
I think it should be something like a reference to Secret object per input parameter where needed or maybe a block that defines which variables to pull from which Secrets and then those variables are used to populate concrete parameters of an Instance parameters. This is similar to what is proposed in one of the previous comments. The difference is that an OSB provider expects certain shape of parameters object and Service Catalog should be able to put required values (from multiple Secrets) in correct places. WDYT?

[pmorie]

Refining the sketch I made above, SecretKeyRef isn't sufficient. We need something to say what the name of the parameter should be:

type SecretParameter struct {
  v1.SecretKeyRef

  Name string
}

type Instance struct {
  // other fields omitted
  SecretParameters []SecretParameter
}

The controller will have to merge the Parameters field and the values from derefencing all of the SecretParameters. Internally, the controller deserializes the Parameters field into a map[string]interface{}. In order to merge the secret parameters, we could do the following:

1. We must state the order of precedence between Parameters and SecretParameters
2. SecretParameters must take precedent over Parameters
3. In the event of an unresolvable key, we should error by making the ready condition false and writing an event on the object

Other concerns:

1. We should apply the normal validations for v1.SecretKeyRef
2. We should validate that in the slice of secret parameters, no two parameters have the same name
3. We should add the SecretParameters to the checksum for an instance

[derekwaynecarr]

@pmorie -- per our discussion, I will be working on this this week.

[jwforres]

Generally in the k8s API whenever you are able to reference secrets you are also able to reference config maps. It seems likely someone will add common non-secret configuration to their project, and then want to select those config map keys as values for params.

[derekwaynecarr]

@jwforres --

How about the following so it looks like EnvVar on pod containers? We can then support either pattern.

type ParameterVar struct {
  Name string
  ValueFrom *ParameterVarSource
}

type ParameterVarSource struct {
  ConfigMapKeyRef *ConfigMapKeySelector
  SecretKeyRef *SecretKeySelector
}

type Instance struct {
  // other fields omitted
  ParameterVars []ParameterVar
}

@pmorie -- it would be good to understand if we could collapse the parameters raw extension further as well....

[derekwaynecarr]

@pmorie -- for example, we could just support a Value string on ParameterVar above and be exactly like EnvVar, but I am not sure if that is losing something required in the existing parameters raw struct.

[jwforres]

yeah i'd prefer if we had a single array of params that allowed either
Value or ValueFrom to be consistent with env vars, unless we really need
that raw extensions

@spadgett switching to that format probably means the model object we get
out of http://schemaform.io/ would need to be manipulated to covert from
{key:value} to {name:key, value: value} but we were going to have to
do conversion anyway to support secret params.

On Wed, May 10, 2017 at 2:17 PM, Derek Carr [e-mail]
wrote:

@pmorie https://github.com/pmorie -- for example, we could just support
a Value string on ParameterVar above and be exactly like EnvVar, but I am
not sure if that is losing something required in the existing parameters
raw struct.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes-incubator/service-catalog/issues/621#issuecomment-300569309,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABZk7TQcvZYuNyCDQCGy68c9f0oTGFzCks5r4f8_gaJpZM4Mv0eF
.

[duglin]

+1 with aligning this with how env vars are done.

[pmorie]

@derekwaynecarr

@pmorie -- for example, we could just support a Value string on ParameterVar above and be exactly like EnvVar, but I am not sure if that is losing something required in the existing parameters raw struct.

I think we could do something like that, as long as we retain the ability to pass json/yaml literal values. How about:

type Parameter struct {
  Name string
  Value string // if you want to just specify a literal string
  ValueFrom *ParameterSource
}

type ParameterSource struct {
  ConfigMapKeyRef *ConfigMapKeySelector
  SecretKeyRef *SecretKeySelector
  Raw *runtime.RawExtension
}

type Instance struct {
  // other fields omitted
  Parameters []Parameter
}

I put Value on Parameter directly because it matches our API for env vars and is probably the 80% case.

[derekwaynecarr]

@pmorie - so what is the meaning of Name when using Raw?

[pmorie]

@derekwaynecarr The Name is the key in the blob we'll send, the Raw value is the value of that key.

[derekwaynecarr]

@pmorie -- per our chat, this proposed model prevents the ability to send anonymous literal and lists. in order to do that, we would need a top-level raw parameter bucket. for example, you cant just send 6, or [1, 2, 3] since you must always have a key. does anyone actually do that?

[derekwaynecarr]

@pmorie -- i prefer the EnvVar style as noted above, and eliminating the existing parameter field entirely. @vaikas-google -- can you weigh in? if you agree, i will go and implement what is described here: https://github.com/kubernetes-incubator/service-catalog/issues/621#issuecomment-300601305

[vaikas]

If we need to provide references from other sources, then modeling it after env vars sounds like a solid approach and consistent with previous examples, so :+1: As far as not being able to send data without a key, I think that's totally fine (unless I'm misreading the spec). Parameters is defined as a JSON object, and from what I recall / understand, a JSON object is mapping keys to values? If that's the case, we should be aok.

[jwforres]

You are right @vaikas-google
I was thinking of JSON document not JSON object when we were talking about this yesterday @derekwaynecarr it's a JSON document that can be an array and still be valid. But the spec does say JSON object so I think we are good to use the env vars format.

[nilebox]

I think we could do something like that, as long as we retain the ability to pass json/yaml literal values

@pmorie @derekwaynecarr the only potential problem I see with this approach is that service broker could expect the secret to be injected anywhere inside JSON/YAML values (not just at the top level).

This is where the difference between environment variables in Pod and parameters in OSB becomes clear: environment variables is the flat list, while OSB Instance parameters is a tree...

Furthemore, if you need to pass a set of credentials for multiple things, we would have to flatten it (by prefixing) or something like that. Example:

parameters:
  master_db:
    username: foo
    password: bar
  slave_db:
    username: f
    password: b
  shared_queue:
    url: http://example.com
    token: 123

While the flattened result would look like

parameters:
  master_db_username: foo
  master_db_password: bar
  slave_db_username: f
  slave_db_password: b
  shared_queue_url: http://example.com
  shared_queue_token: 123

Are we okay with putting a requirement to declare secrets only at the top level? Doesn't it limit the set of service brokers we can support (as this implementation is only a subset of what OSB declares)?

If we actually want to be able to inject secrets in any place inside the parameters JSON/YAML, I would imagine that we need to declare Parameters in spec not as a raw JSON/YAML (nor as a flat map), but as a tree of Parameter objects which could contain child Parameter objects.

Other than that sounds good, I like the pattern used for EnvVars in pods.

[duglin]

I don't think we need to make that distinction. To us it is a flat list. If one value happens to be a tree that's not our concern. In fact, env vars could be a tree too if someone put json into the env var value.

[nilebox]

@duglin sure, but how do you inject a secret inside the tree then?

Or, rather it means that someone has to get a set of secrets, generate a full JSON containing a tree with all the secret values as plain text inside it, and put the result as another secret? Sounds very complicated to me as a user of Service Catalog.

For me it eliminates the value of ability to specify Parameters as raw JSON completely - I have to manually prepare the JSON and create a secret outside without any help from Service Catalog.

Instead, I would prefer to be able to reuse the secrets generated by Service Catalog as a result of creating an Instance in other broker, for example.

I am also not sure how do you distinguish the "string" secret containing JSON from "object" JSON tree stored in it.

[ash2k]

I agree with @nilebox. For example I can imagine an Instance that needs an array (or an object of some shape) of credentials and each item in that array comes from separate Secret. In fact, we want to use this approach in one of the things we are developing and not being able to do this will be unfortunate. I.e. the structure we want to be supported is more flexible than a map.

[ash2k]

To us it is a flat list

But OSB spec says it is an object and objects can have arbitrary structure. And I think that flexibility is really great.

[duglin]

if I'm following it sounds like you want to have a way to specify not just which of the top-level properties are 'normal' and which are 'sensitive', but also be able to specify that all the way down the tree at random spots, right? If so, then while that might be really really flexible, it seems like overkill and way more complex than what's needed.

[duglin]

We have plans to add a way to specify which top-level things are normal vs sensitive - I'm hoping that should be sufficient.

[nilebox]

@duglin I see your point, but to me this limitation is _almost_ the same as just having Parameters field declared as

type Instance struct {
  // Parameters could potentially contain sensitive data, 
  // so let's just put entire JSON blob in Secret
  Parameters *SecretKeySelector
}

I can live with it, but I am not sure if it's the best solution.

[duglin]

Almost, I think you'd want to keep the non-sensitive stuff outside of the secret since I'm told that some systems have a limit on how much they can store in secrets. So I think something more like:

type Instance struct {
  Parameters configMap
  Sensitives *Secret
}

I think this keeps it simple.

[nilebox]

@duglin I am not pushing on it, but just to make it clear what I was proposing in my first comment: all that is needed is just adding support for one more field in ParameterSource:

type ParameterSource struct {
  ConfigMapKeyRef *ConfigMapKeySelector
  SecretKeyRef *SecretKeySelector
  Raw *runtime.RawExtension
  // And now we can support tree with secrets at any level
  Children []Parameter
}

It doesn't sound to me as much more complicated than a solution proposed by @pmorie, but a rather simple extension to it.

I think you'd want to keep the non-sensitive stuff outside of the secret since I'm told that some systems have a limit on how much they can store in secrets.

If we don't want to support secret injection at any level, then I actually prefer your code to the one proposed by @pmorie. At least it makes a clear distinction and doesn't define Parameters object as a flat list (contrary to what OSB spec requires). And yes, it makes the implementation in Service Catalog simple (while making implementation on SC users' side harder, but that's another story).

[ash2k]

@duglin

if I'm following it sounds like you want to have a way to specify not just which of the top-level properties are 'normal' and which are 'sensitive', but also be able to specify that all the way down the tree at random spots, right? If so, then while that might be really really flexible, it seems like overkill and way more complex than what's needed.

Let me explain our use case. Instance parameters get inputs from multiple secrets into Instance-specific places. In fact, even non-sensitive information is output as Secrets in Service Catalog today so all Instance parameters will come from Secrets in our case. Essentially we want to have support for structures like this:
Example from above:

parameters:
  master_db:
    username: foo
    password: bar
  slave_db:
    username: f
    password: b
  shared_queue:
    url: http://example.com
    token: 123

or like this (note that names of fields in secrets do not match parameter names - they may be built by different people, organisations even):

parameters:
  defaults:
    x: a
    y: b
  dependencies:
  - name: master_db
    username: <reference to a secret1/field-user>
    password: <reference to a secret1/field-pass>
  - name: slave_db
    username: <reference to a secret2/field-role>
    password: <reference to a secret2/field-secret>
  - name: shared_queue
    url: http://example.com
    token: <reference to a secret3/field-secret>

and perhaps other forms...

Doesn't look like overkill to me :) I hope this is a valid use case for Service Catalog.

[duglin]

I'm sorry - when I saw secrets as part of this conversation my mind jumped to Credentials - those might have the notion of "normal" vs "sensitive", not "parameters".

Let me ask this... in some of the proposals it includes things like ConfigMapKeyRef *ConfigMapKeySelector, which implies that the data in question is stored outside of the instance. What happens if that configMap is changed? While the new data may not be used because the instance is already created, it means that anyone looking at the data gets the wrong info. It seems to me that this data should be stored within the instance itself so that we can prevent someone from modifying it w/o us knowing it.

Unless those refs are just "write-once/only" and we just use them to populate the InstanceSpec's parameters but they're never used/seen again (except on an update). But even then the data is coped into the InstanceSpec and it could be copied into an embedded ConfigMap or Secret. Can you embed a Secret in another resource?

[nilebox]

@duglin That's a very good question, however I think it's a separate issue. The problem you described applies to both ConfigMap and Secrets. And yes, Secret update doesn't lead to cascade update of Instance, since it was not changed, there is no event (or new revision) to react on.

I think we have two possible ways of handling this:

1. Treat Secrets as immutable and create a new Secret under a new name (and change the secret name in Instance spec). Seems hacky and fragile.

2. Service Catalog could watch on relevant Secret, ConfigMap (and maybe other) objects, and store the last revision of processed Secret in the Instance's metadata. By having this metadata we can always compare it to the current revision of Secret, and decide whether we need to execute an update on Instance (and only after that update the Secret revision in its metadata).
   I can provide an example if it sounds confusing.

Either way, I think that storing revision, hashing contents or any other reliable way should be possible to implement, and that's probably the right way to handle this, instead of making assumptions on immutability of Secrets. But depends on how hard it is to implement, of course.

Also, looks like you want some history (have both "desired" and "actual / last applied" revisions of Secret/ConfigMap). For this I guess we could make duplicate immutable copies of them every time we apply them. Something similar to how Deployments work with rollback support https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#rolling-back-a-deployment

I think we need two separate issues: one for reconciliation loop reacting on Secret/ConfigMap changes and another for history. Reconciliation loop is more important as it at least guarantees that the system state will be eventually consistent.

P.S. What should we do if a Secret or ConfigMap was deleted? Not sure about this.

[pmorie]

@ash2k @duglin @nilebox

TLDR:

While the proposed API does make handling trees a little cumbersome, in the absense of a specific example of a broker that needs secrets composed into nested values, I don't think we should design in the API for it now. We can do something backward compatible later to make it easier if there is a need.

We will definitely not be adding templating in any way to support the nested tree structure; that is basically a non-starter for Kubernetes (many have tried and died trying to do things like this in the past).

Furthemore, if you need to pass a set of credentials for multiple things, we would have to flatten it (by prefixing) or something like that.

...or you could put the blob with nested stuff into a secret. There's no reason that the approach in this issue prevents you from doing that.

Treat Secrets as immutable and create a new Secret under a new name (and change the secret name in Instance spec). Seems hacky and fragile.

That is actually close to what is proposed for deployments (see https://github.com/kubernetes/kubernetes/issues/43112), but we are not going to do that at present.

Secrets (and configmaps) will not be made immutable either, any time soon, and so we will need to make the controller be sensitive to when one of the secrets or configmaps that supplies a parameter value changes.

Can you embed a Secret in another resource?

No

Service Catalog could watch on relevant Secret, ConfigMap (and maybe other) objects, and store the last revision of processed Secret in the Instance's metadata. By having this metadata we can always compare it to the current revision of Secret, and decide whether we need to execute an update on Instance (and only after that update the Secret revision in its metadata).

This is close to what we should do. Specifically, I think we should keep a separate checksum of the payload that we sent for parameters and recalculate it when we get an update event for a secret or configmap we're interested in. That leaves us with two checksums: one for the spec, and one for the parameter blob we've sent over the wire, and allows you to differentiate when the instance's spec vs. the generated parameter payload has changed.

P.S. What should we do if a Secret or ConfigMap was deleted? Not sure about this.

If you cannot form the parameters payload to send to the broker, it's an error. The controller should set the status on the instance accordingly and make an event.

[derekwaynecarr]

@pmorie @nilebox @duglin -- I am not adverse to the idea of having parameter source include nested parameter options. It would actually let us just remove the Raw field entirely and avoid need for templating. I am fine prototyping that in #847

[pmorie]

Yeah, on second thought I would be okay doing nested parameters, but I think we should agree on a maximum nesting level.

[nilebox]

@derekwaynecarr nice, I'll be watching your pull request then.

Minor note: even with nested parameters there are still some missing features in full JSON support:

• JSON arrays: only objects supported in the proposed solution?
• Different types of primitive values: string, integer, float etc: only strings are supported?

I think it's still fine, unless we have a specific service broker which does require these JSON features.

but I think we should agree on a maximum nesting level.

@pmorie too many nesting levels will make spec overcomplicated anyway, so I guess there is an implicit "sanity limit", no need for hard limits :)

[nilebox]

...or you could put the blob with nested stuff into a secret. There's no reason that the approach in this issue prevents you from doing that.

@pmorie This is an option too, but I'm not sure how would you distinguish a _string_ with JSON inside from JSON _object_ there?

i.e. this (1)

{
    "parameters": {
        "fromSecret": "{ \"a\": 1, \"b\": 2 }"
    }
}

vs that (2)

{
    "parameters": {
        "fromSecret": {
            "a": 1,
            "b": 2
        }
    }
}

in Secret anything is a string, so only (1) is possible I suppose. Otherwise we would have to try parsing string as JSON and if succeeded switch to (2) (which again is not necessarily what user wanted).

[derekwaynecarr]

The model proposed has multiple deficiencies:

1. it is not able to specify if a primitive value is a string, number, or bool
2. it is too fine-grained. it leads us down a json object template and value substitution syntax.

Given the lessons learned in prototyping this, I think service-catalog should offer a much more coarse-grained solution. If any parameter is private, the entire envelope of parameters should be private. I also want to ensure that whatever we do is consistent with the direction in sig-auth.

This presents the following options to pursue.

Option 1: service catalog encrypts raw parameters
In this option, the catalog basically does what secrets will do via https://github.com/kubernetes/features/issues/92 as described in https://github.com/kubernetes/community/pull/607

This is a very big hammer IMHO and makes catalog concern itself with key rotation and other things that I would like to avoid in the near term.

Option 2: service catalog supports parameters or secret parameters
For this option, we could support something akin to the following:

type Instance struct {
...

// Parameters is set of properties to be used.
Parameters *runtime.RawExtension
// Referenced value must be valid JSON object.
SecretParameters *v1.SecretKeyRef
...
}

This is basically what @duglin proposed here: https://github.com/kubernetes-incubator/service-catalog/issues/621#issuecomment-302618541

We could let you specify one of the values, or both, and if you provide both, maybe we could merge depending on complexity.

@smarterclayton @liggitt -- can you weigh in here?

[smarterclayton]

We are trying to put a hold on building APIs that reference secrets in the near term, because we are in the midst of refining how secrets should be subdivided for use. The original purpose of secrets was to store data for pods. It has expanded to service account tokens (which creates escalation vectors), ingress certificates (which means ingress is too powerful by default in many cases), and storage class secrets (which are in theory needed for pods). While it helps to coalesce many things into secrets, we are trying in the very short term to settle on a new direction for them that helps subdivide them out. I would prefer not to add new things that need access to secrets in the short term.

[duglin]

@smarterclayton do you have pointer to where we can read-up on the latest discussions on this?

[derekwaynecarr]

ok, so that sounds like a preference for option 1.

which would look more like the following:

type ParameterSpec struct {
  // sensitive, public, etc.  -- this is a string because we are told to avoid bools
  Policy string
  Parameters *runtime.RawExtension
}
type InstanceSpec struct {
  ...
  ParameterSpec ParameterSpec
}
type BindingSpec struct {
  ...
  ParameterSpec ParameterSpec
}

we would then need to encrypt Binding and Instance resources at rest in storage.

our cli commands would need to obfuscate in the future via server side describers based on the parameterSpec.Policy

[smarterclayton]

Last week's sig-auth meeting went into it, see the sig-auth meeting notes (and I think we recorded, at least I hope so).

[jwforres]

This gives a client 3 options for deciding whether to set Policy to sensitive / public / etc

1. Always assume sensitive - less burden on the user but then whats the point in having the flag...
2. Ask the user if any of the parameter information they are filling out is considered sensitive. Prone to a big "oops" from a user when they forget to check that box.
3. Rely on the brokers to define in their parameter schemas whether a particular parameter should be considered sensitive. We want brokers to do this anyway so we can do things like turn text boxes into password inputs. But until the brokers do that we risk exposing people's sensitive information.

Opinions?

[duglin]

just for my own understanding, what are some of the usecases for sending sensitive information to brokers via parameters? Its interesting to me that no one has really complained about this before in the CF world, but perhaps the services you guys have in mind are different than what they've seen in the past.

This https://github.com/kubernetes-incubator/service-catalog/issues/621#issuecomment-302623886 hinted as a usecase but it almost feels like the usecase is wrong, if I'm following it correctly. Couple of things that I didn't grok:
1 - why provide creds for the master/slave? Shouldn't those be things that the service returns to the platform in the binding?
2 - why be so specific about master/slave at all? I would image that the this level QoS should simply be a QoS flag (gold vs silver).
3 - similar question about the queue. I'm assuming that's used between the master/slave - but isn't that an impl detail that the app should be unaware of?

Part of the reason for the SB API, and its abstraction, is to prevent people from having to get into the lower level details of the service.

[nilebox]

@duglin the reason that nobody complained in CF world is most probably​ because it didn't cover dependencies between service instances, and there was no use case for defining a compute service broker.

The major use case we want to support is EC2 service broker, or GCP compute service broker, or any other compute provisioning through Service Catalog / OSB.

Master/slave is just an example (probably a bad one), but the idea is that the application needs credentials to talk to databases, queues (or just other applications) previously provisioned through Service Catalog.

Let's say we have the following service brokers:

• Postgres Service Broker (1)
• SQS queue Service Broker (2)
• Docker image runner service broker (implemented through EC2, GCP compute, Kubernetes, CF whatever) (3)

We want to run a Foobar application, which needs 2 Postgres databases and an SQS queue. Instead of running it as a Pod inside Kubernetes cluster, we decided to run it outside of it. For example, we want to run it in multiple cloud providers: e.g. AWS and Google Cloud.

We have a Docker image, which requires Postgres and SQS credentials injected through environment variables. The way we do it:

• We create instances of Postgres and SQS through (1) and (2)
• We create bindings, fetch the credentials through (1) and (2) and store them as Secrets in Kubernetes
• Now we need to provision an application instance through (3), and we have to include the Secrets from above into Parameters so that they can be injected as environment variables.

It's not the only use case we can think of, but the most obvious one.

[duglin]

Can't you get the same results by just binding (1) and (2) to (3)? Even if you have to create user-provided service instances to (3) for (1) and (2) ?

(We should talk about (1)'s and (2)'s from now on instead of real words, it'll make it it much more interesting :-) )

[derekwaynecarr]

@duglin - our use case is provisioning a compute service on EC2. so +1 to @nilebox examples.

[nilebox]

@duglin not sure if I follow...

How can you provision instance in (3) first without passing environment variables it requires? Also I don't think that creating custom service brokers for (1) and (2) is a good idea. You don't create custom service brokers for binding to Pods, why should you do it for EC2?

The other option is for (3) to talk directly to (1) and (2) to create bindings itself, instead of expecting that someone has already created bindings and will pass the credentials in the instance provisioning request. But it breaks the isolation between service brokers.

I would prefer to keep the service brokers completely decoupled from each other.

All that we actually need to support is a generic way of managing dependencies between services provisioned through Service Catalog (we are working on building this on top of SC), and it requires ability to pass the sensitive data in the requests.

[MHBauer]

Should we add this to the agenda for the next sig meeting? Or have a separate meeting ASAP to discuss?

[nilebox]

@MHBauer whichever works for you, but I am very interested in discussing it sooner rather than later (but also preferably to come up with a proper solution which satisfies everyone in the long term), as this limitation currently blocks us from building a proof of concept.

Implementation is not that urgent, we can hack something around for now, or help @derekwaynecarr with implementing it. The more important is a "design" document to have a shared understanding on the future.

I am also not sure how changes in Secrets mentioned by @smarterclayton affect this, we might want to line up the "secrets injection" design with this rework.

[derekwaynecarr]

can we queue up a meeting w/ @pmorie when he returns from vacation?

On Mon, May 22, 2017 at 6:01 PM, Morgan Bauer [e-mail]
wrote:

Should we add this to the agenda for the next sig meeting? Or have a
separate meeting ASAP to discuss?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes-incubator/service-catalog/issues/621#issuecomment-303232030,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF8dbPFZXfDyJwxytI5hFIf7cRR5fZp1ks5r8gXJgaJpZM4Mv0eF
.

[derekwaynecarr]

i want a path forward determined end of week. i can then implement
whatever we decide after.

On Mon, May 22, 2017 at 6:10 PM, Nail Islamov [e-mail]
wrote:

@MHBauer https://github.com/mhbauer whichever works for you, but I am
very interested in discussing it sooner rather than later (but also
preferably to come up with a proper solution which satisfies everyone in
the long term), as this limitation currently blocks us from building a
proof of concept.

Implementation is not that urgent, we can hack something around for now,
or help @derekwaynecarr https://github.com/derekwaynecarr with
implementing it. The more important is a "design" document to have a shared
understanding on the future.

I am also not sure how changes in Secrets mentioned by @smarterclayton
https://github.com/smarterclayton affect this, we might want to line up
the "secrets injection" design with this rework.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes-incubator/service-catalog/issues/621#issuecomment-303233918,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF8dbLi0uQh8pawjJUnlod5a2ar6VTSNks5r8gfkgaJpZM4Mv0eF
.

[duglin]

@nilebox in those cases what does binding to (3) mean? or do you not bind?

[nilebox]

@duglin I don't think that (3) needs binding support in this example. Just service instance should be enough.

The user of Foobar application will need a URL to talk to it, which could be derived based on the service instance ID, or it could be returned in the last operation endpoint (or operations/{operationId} when we add this endpoint as you mentioned at the meeting).

[duglin]

Also, how much integrate is there between (3) and the platform/kube? I'm assuming (3) isn't some random thing, its probably pretty tightly integrated with the platform/kube, right?

[duglin]

@derekwaynecarr do you plan on skipping the binding parts too?

[nilebox]

@duglin It's probably not a random Docker runner, but a properly implemented Service Broker, but I don't think it has to know about Kubernetes at all.

From a broader perspective, you could consider Service Catalog as just an implementation of OSB marketplace, which can be run as a standalone service without Kubernetes core _at all_.

I don't want to go too deep into this, as it is irrelevant to the issue in my mind. But it illustrates that service brokers are decoupled from Kubernetes by default, unless you want to implement a service broker implementing a Pod runner, haha :)

[duglin]

ok @nilebox spent way too long helping shape my head around this, sorry for being slow, but thanks for the help!

A couple of things....

• this discussion made me realize that there's a big difference between CF and Kube w.r.t. what end users see. In CF a fair amount of this stuff that we have in our model is just not exposed to end users. Which means how stuff is stored internally doesn't really matter to CF. But in Kube everything about how we store our data becomes a UX issue. But that's probably an issue for another day.
• looking back over some of the previous comments, I think a variant of @derekwaynecarr's is one that I like the most:

type Instance struct {
    ...
    Parameters            *runtime.RawExtension
    AlphaSecretParameters *v1.SecretRawExtension
    ...
}

I like this because: 1) it doesn't force people to toggle a flag that makes everything public vs private, 2) people can easily put stuff in one bucket or the other, 3) the buckets are clear and easy to understand, 4) the secret one is "Alpha" so we can change our minds later, 5) it keeps the simple usecases simple, 6) once we agree on the general structure we can then rathole on what "SecretRawExtension" looks like - is it a Secret of some kind of new thing?

[nilebox]

@duglin I think this variant is probably good enough for our usecases. I am keen to hear @pmorie though as he seems to have some strong opinion about this as well.

If at some point Service Catalog will have a need to support service dependency graph with injecting secrets produced by one broker into another, it could be built on top of that (something like PodPresets built for Pods, for example).

I fully​ agree with the point made regarding CF vs Service Catalog (and Kubernetes in general). Our usecase would simply be near to impossible to implement with CloudFoundry.

[nilebox]

@duglin forgot to mention that with your variant we need to be _explicit_ about merging strategy for Parameters and SecretParameters (and clashing naming handling - priority or explosion with validation error), or we can extend OSB spec and introduce a separate secretParameters section from parameters in Instance request.

Another option is to simply allow only one of the two fields to be present, and require another one to be nil.

Other than that it seems to be a perfectly viable solution.

[duglin]

True - we could either block duplicates from being created at all or just define what will happen when they occur.

Adding a "secretParameters" property to the spec might be an interesting option since we're already talking about the option of distinguishing "normal" from "sensitive" credentials in the bindings.

[derekwaynecarr]

@duglin -- the issue with the model you proposed (which is same as my option 2) is we are being told to not use secrets for this purpose anymore.

[derekwaynecarr]

@duglin - unless you are saying we have two runtime.RawExtension fields, and only one of them are obfuscated in the API? At storage, we will have to encrypt the entire thing?

[duglin]

I was kind of thinking that we first focus on what the UX should look like for populating the InstanceSpec, then we can work on how to store/show it. Whether we use secrets or encrypt it ourselves is a secondary discussion.

The diff between what I proposed and your option 2 is that I think you're pointing to a secret, I wasn't proposing that. I was going to suggest that the person give us the data directly and then store it ourselves (someplace).

[nilebox]

we are being told to not use secrets for this purpose anymore

It got me thinking if creating a secret upon creating a Binding is a valid use case then?

PodPreset was designed for injecting secrets into Pods, so it definitely could/should manipulate secrets.

But we have a use case (see above) when we don't necessarily create Pods and PodPresets using a Binding. Which suggests that Service Catalog has its own abstraction as OSB marketplace decoupled from running Pods. And producing a secret on Binding creation seems to be a leaking abstraction then.

@duglin, @pmorie, @derekwaynecarr what do you think?

I understand that current implementation with producing Secrets upon Binding creation and injection into Pods is simple and convenient, and it covers the majority case.
Just wonder if for the sake of "purity" Binding should produce some kind of "general purpose encrypted blob" which could be transformed into Secret by PodPreset controller, for example.
Otherwise our use case will abuse Secrets again - we will need to read Secret and inject it into another Instance (not Pod)

[derekwaynecarr]

@nilebox the secret created as a result of a Binding is fine as that secret is intended to be consumed by pods.

[jwforres]

I disagree from the perspective that the secrets can still be used for the
purpose of injecting information into pods. What if I want to share one
"binding" for many different applications in my project, in that case
creating a Binding with a PodPreset isn't the correct choice as there may
not be one labelSelector that will work for the combination of applications
(i.e. the different Deployments, StatefulSets, etc).

From a UX point of view, this is how we are representing it to end users:
https://cloud.githubusercontent.com/assets/11633780/26425145/d18fec6e-40a1-11e7-99ff-8037bb629369.png

On Wed, May 24, 2017 at 6:40 PM, Nail Islamov [e-mail]
wrote:

we are being told to not use secrets for this purpose anymore

It got me thinking if creating a secret upon creating a Binding is a valid
use case then?

PodPreset was designed for injecting secrets into Pods, so it definitely
could/should manipulate secrets.

But we have a use case (see above) when we don't necessarily create Pods
and PodPresets using a Binding. Which suggests that Service Catalog has its
own abstraction as OSB marketplace decoupled from running Pods. And
producing a secret on Binding creation seems to be a leaking abstraction
then.

@duglin https://github.com/duglin, @pmorie https://github.com/pmorie,
@derekwaynecarr https://github.com/derekwaynecarr what do you think?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes-incubator/service-catalog/issues/621#issuecomment-303872058,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABZk7WYI9kxum7CjhTAuQDkmzrzhqnrmks5r9LHPgaJpZM4Mv0eF
.

[nilebox]

@derekwaynecarr in case of EC2 there are no Pods consuming a Secret. This is uncommon case, but it's still valid. And without an intermediate "general purpose encrypted blob" object, there is no way for us to read sensitive data produced by Binding without abusing Secrets again.

This is not something urgent, just trying to understand the boundaries of Secrets. We can get back to it once Kubernetes has implementation (or guidelines) for storing general purpose sensitive data.

[nilebox]

@jwforres there could be multiple PodPresets referencing a single Secret (produced by a Binding), I don't see a technical limitation for your case there.

[nilebox]

My take on the today's meeting with sig-auth (also see notes):

• it is fine to use Secrets for Instances (and other Service Catalog resources)
• individual get on Secret is fine
• list/watch on Secrets should be avoided (but watch on individual Secret instance is fine)

I think that taking @duglin's approach with using SecretKeySelector is both good enough for now, and is a proper long term solution compatible with common patterns of building services in Kubernetes.

type Instance struct {
    ...
    Parameters            *runtime.RawExtension
    AlphaSecretParameters *v1.SecretKeySelector
    ...
}

We don't have to implement the watch on secret changes and corresponding reconciliation loop syncing instance with service broker right now. IIRC currently Pods depending on a Secret don't get redeployed every time the secrets change - so it's a possible limitation for Instance as well.

Other than mitigating this, I don't see any benefits of embedding the secret parameters and encrypting the entire Instance resource, at least in the long term. Furthemore, we won’t be able to use external secret storage (Vault, KMS) if the secret parameters are embedded into Instance, while Secrets roadmap plan to add this support. The more features we will want out of this, the more it will look like reinventing the Secrets.

[nilebox]

@pmorie @derekwaynecarr if there are no objections to the proposal above, I can try implementing it and raise a pull request to discuss implementation details.

[nilebox]

Side note: it should be just a LocalObjectReference to the Secret as a key-value map of credentials instead of SecretKeySelector, to conform with the way Secret generation is already implemented for Bindings

In other words,

type InstanceSpec struct {
    ...
    Parameters            *runtime.RawExtension
    AlphaSecretParameters *v1.LocalObjectReference
    ...
}

[duglin]

Given the sig-auth call ( https://github.com/kubernetes-incubator/service-catalog/issues/621#issuecomment-305356537 ) is there more discuss w.r.t. using secrets for parameters?

[ash2k]

@nilebox

We don't have to implement the watch on secret changes and corresponding reconciliation loop syncing instance with service broker right now. IIRC currently Pods depending on a Secret don't get redeployed every time the secrets change - so it's a possible limitation for Instance as well.

We don't have to, but IMHO it is the correct "native" behaviour. Deployments will re-create Pods if reference to Secret/Configmap changes AFAIK. The fact, that they don't do the same for update of the contents of those objects is more like a bug to me. But, yes, this can be implemented later. Relevant: https://github.com/kubernetes/kubernetes/issues/22368
And also this issue has discussion about embedding secrets/configmaps.

[nilebox]

Given the sig-auth call ( #621 (comment) ) is there more discuss w.r.t. using secrets for parameters?

@duglin see Paul's comment https://github.com/kubernetes-incubator/service-catalog/pull/957#issuecomment-312100807, there will be one more follow-up session with SIG Auth. Looks like some important people were out of the previous meeting, and they might have a different opinion on this.

[nilebox]

@ash2k not arguing with you, but we need to get the ball rolling first, one small step at a time. IMO there was already too much bikeshedding on this issue, so I just want to concentrate on making an initial design decision everyone is satisfied with.

Hopefully @pmorie will get a green light on using Secrets and we can finally get something released, and we can continue discussing all the possible further improvements afterwards.

[nilebox]

For the record: @pmorie attend the meeting with sig-auth (notes) to confirm that we can use secrets for non-Pod use cases.

• We can use secrets for non-Pod use cases
• We shouldn't use bulk watch an all secrets, individual watch is expected to be introduced in Kubernetes 1.8

[pmorie]

Closed by https://github.com/kubernetes-incubator/service-catalog/pull/1079