Revised proposal for initial secret parameter support

[pmorie]

This is based on design discussions in a SIG meeting on July 18, 2017.

We discussed the following as guiding factors for the design of parameter handling:

1. We do not need to solve every form of secret parameters now
2. We should design for additional future parameter APIs
3. We should try to capture the 80% use-cases for beta

We discussed the following as making up the 80%:

1. Send parameters based on a single secret key
2. Build a parameters dictionary from all the key-value pairs in a secret

API changes

The following API implements the (1) and (2) parts of the 80% solution:

type InstanceSpec struct {  
  // other fields omitted

  Parameters *Parameters `json:"parameters,omitempty"`
}

type BindingSpec struct {
  // other fields omitted

  Parameters *Parameters `json:"parameters,omitempty"`
}

type Parameters struct {
  Secret *SecretParameters `json:"secret,omitempty"`
}

type SecretParameters {
  // Reference to a secret in the namespace.
  Ref LocalObjectReference `json:"ref"`

  // if provided, the parameters will be set from the json or yaml
  // contained in this secret key.  If unset, the parameters will
  // be created as a dictionary from all keys in the secret.
  Key *string `json:"key,omitempty"`
}

There should be a validation which ensures ultimately ensures that one and only one field of Parameters is set. Future extensions can be implemented as new pointer fields of Parameters.

Example: all parameters from a single secret key:

apiVersion: servicecatalog.k8s.io/v1alpha1
kind: Binding
metadata:
  name: test-binding
  namespace: test-ns
spec:
  instanceRef:
    name: test-instance
  secretName: my-secret
  parameters:
    secret:
      ref: my-parameters
      key: body

Example: all parameters from every key in a secret:

apiVersion: servicecatalog.k8s.io/v1alpha1
kind: Binding
metadata:
  name: test-binding
  namespace: test-ns
spec:
  instanceRef:
    name: test-instance
  secretName: my-secret
  parameters:
    secret:
      ref: my-parameters

Future extensions

We can add future methods of creating parameters to the Parameters struct, like so:

type Parameters struct {
  Secret *SecretParameters `json:"secret,omitempty"`
  SomeOtherMethod *SomeOtherParameters `json:"someOther,omitempty"`
}

[pmorie]

Since #1028 already has some of this functionality, and @nilebox has expressed interest in finishing this feature, I suggest that we repurpose #1028 to implement this API.

[nilebox]

We can add future methods of creating parameters

Thinking, if SecretParameters should be just merged with Parameters type then. Also, I would split 2 ways of secret parameters into independent exclusive fields. In other words:

type Parameters struct {
  // Reference to a secret in the namespace.
  // The parameters will be created as a dictionary from all keys in the secret.
  SecretRef *LocalObjectReference
  // Reference to a secret key in the namespace.
  // The parameters will be set from the JSON contained in this secret key
  SecretKeyRef *SecretKeySelector

  // Any other methods also go there, for example:
  Raw *runtime.RawExtension
  ConfigMapRef *LocalObjectReference
  ConfigMapKeyRef *ConfigMapKeySelector
}

[nilebox]

@pmorie we will need to update the design.md doc file with the section about parameters. Do you see this as part of https://github.com/kubernetes-incubator/service-catalog/issues/1028 pull request, or as a separate one? I would prefer to keep it separate to avoid doc bikeshedding blocking the code merge.

[nilebox]

@pmorie @arschles @vaikas-google @kibbles-n-bytes @spadgett Please have a look at the completely new alternative proposal which came out of our discussion with @duglin by trying to address some of his concerns:

• list support (ability to specify separate fields pointing to different sources)
• multiple sources support with merging strategy
• easy to use for most (simple) use cases

The example of the InstanceSpec (see gist)

spec:
  serviceClassName: user-provided-service
  planName: default
  parameters:
  - name: username               # easy name/value pair case
    value: root
  - name: password               # pull value from a secret's key
    valueFrom:
      secretKeyRef:
        name: mySecret1
        key: passwd
  - name: size                   # pull value from a configMap key
    valueFrom:
      configMapKeyRef:
        name: myConfigMap0
        key: length
  parametersFrom:
  - configMapRef:
      name: myConfigMap1           # pull in all the CM's key/value's   (string/string)
  - secretRef:
      name: mySecret2              # pull in all the secret key/value's (string/string)
  - configMapKeyBlobRef:
      name: myConfigMap2
      key: credentials             # "credentials" is a JSON object so we take it verbatim and we merge with the above
  - secretKeyBlobRef:
      name: mySecret3
      key: creds                   # "creds" is a JSON object so we take it verbatim and merge with the the above

As you can see, it's consistent with the way envFrom and env.valueFrom work in the Pod spec:

apiVersion: v1
kind: Pod
metadata:
  name: my-pod
spec:
  containers:
  - name: test-container
    image: nginxdemos/hello
    env:
    - name: USERNAME
      value: root
    - name: PASSWORD
      valueFrom:
        secretKeyRef:
          name: mySecret1
          key: passwd
    - name: SIZE
      valueFrom:
        configMapKeyRef:
          name: myConfigMap0
          key: length
    envFrom:
    - configMapRef:
        name: myConfigMap1
    - secretRef:
        name: mySecret2

• there is no analogy for secretKeyBlobRef and configMapKeyBlobRef of course, but the rest maps one-to-one
• envFrom items have prefix attribute, which we might want to leverage as well (for conflict resolution)

[nilebox]

One of the limitations the proposal above has is poor support for multi-level JSON. Only configMapKeyBlobRef and secretKeyBlobRef support them, all the other options are treated as a string.

The easy addition fixing this problem is an extra optional field type with possible values of string/json (with string value as a default). For example,

...
    - name: textField
      value: qwerty # string by default
    - name: meta
      type: json # the string value will be deserialized and passed as JSON object
      valueFrom:
        secretKeyRef:
          name: mySecret1
          key: jsonBlob
...

With this addition we don't need special cases like secretKeyBlobRef, just type+secretKeyRef combination is enough.

In the future we can add support for other valid JSON property types (number, array etc)

[duglin]

@nilebox in that example why would we need to treat it as json instead of just a string that happens to look like json? If our merge strategy is really simple (something like we detect conflicts at the top-most level and error), then we wouldn't really need to care that "meta" is json instead of a string would we?

[nilebox]

we wouldn't really need to care that "meta" is json instead of a string would we

@duglin yes we would. It's not about the conflict resolution, it's about being able to pass a value to the broker either as a string, or as a json object, and let the user explicitly make a choice.

for example, if you have a structure like this:

{
  "textField": "textValue",
  "meta": {
    "attributes": {
      "a": "b",
      "c": "d"
    },
    "labels": [
      "one",
      "two"
    ]
  }
}

, you should also be able to send something like this:

{
  "textField": "textValue",
  "meta": "{\"labels\": [\"one\", \"two\"]}"
}

and it's up to the user to decide.

[duglin]

if I'm following, there are two cases - one where the value is a string that looks like json and one where the value is an actual json blob and needs to treated as such, right?

Do these cover it?

- name: meta1
  value: "{\"labels\": [\"one\", \"two\"]}"      # string
- name: meta2
  valueFrom:
    configMapKeyRef:
      name: myConfigMap
      key: myMeta                          # string
- name: meta3
  valueFrom:
    configMapBlobRef:
      name: myConfigMap
      key: myMetaJson                  # json

I think that's consistent with how we're using it in the other spots because I'm viewing xxxBlobRefs as a way of saying "use the json over here".

I'm hoping someone comes up with a better name than "BlobRef" :-)

[nilebox]

@duglin yes, it covers it, but I like type attribute more. It's more flexible, explicit, and extensible for support of other outputs in the future (int/float, array etc).

You can even do this for example:

- name: meta1
  type: json
  value: "{\"labels\": [\"one\", \"two\"]}"      # looks like string, but will be passed as JSON object

[duglin]

@nilebox and I chatted off-line and I see the flexibility of the "type" flag - and since its optional (and we'll default to "string") it seems ok for now. I just wonder where the line will be where we say "if you really want that much flexibility just give us the entire json because we're not a JSON templating language thingy".

[pmorie]

I'm closing this because #1075 is more representative of discussion so far and up to date.