Create secrets automatically if secrets passed in ServiceInstance/Binding parameter blocks (automated parametersFrom)

[wryun]

#194 and #195 can be useful, but expose the secret in the service instance object. Surprisingly, this is usually ok - most binding outputs are not truly 'secret'. But obviously not always, and the only way to not expose the secret is to use parametersFrom.

To make 'secret references' truly useful, then, it would be good to automatically generate secrets to allow parameter passing. Possible approaches:

1. automatically detect when a 'secret reference' is inside a parameter, and convert that parameter to a secret

• has the advantage that it's impossible to inadvertently leak secrets, but doesn't expose the 'non-secret' parts of the binding outputs (the majority) AND means that one can't easily use a generic templating language (imagining future changes...), since you need to apply the template yourself to know where the secrets are in the JSON structure. It also means that if you have a single non-secret 'secret' in a big parameter block, the whole lot will be converted to a secret

2. allow one to mark a particular parameter as 'secret' (e.g. interpreting an '!' at the end of the parameter as indicating it should be converted to a secret)

• opposite pros/cons cf (1)

Both of these approaches mean that it's possible to easily pass secrets into the midst of a complex JSON object, which Service Catalog makes difficult.

Downsides to this:

• we're building in more Service Catalog knowledge into Smith (but that ship has sailed with secret updates?)
• in reality, I think this belongs in Service Catalog (ability to have secret refs inside parameter block vs. parametersFrom)
• although this allows passing secrets outside the top-level, one must make the whole top-level parameter secret if any sub property is secret, which means that large blocks of the JSON can be 'infected' by a secret (even worse in (1))

Nevertheless, I think this is worth doing, as it makes a bunch of things easier (no need for complex secret generating plugins) and gets us much closer to schema based validation (no 'magical' set of variables coming from parametersFrom).

[wryun]

New proposal (thanks for discussing and holding off on lunch, @ash2k !):

References are currently made up of two parts, dependsOn (essentially just to make the typing clearer and processing more trivial) and the reference itself (which contains all the information about the reference). We should instead invert this relationship, so that all the information (jsonpath, secret, example, etc.) is concentrated in the dependsOn, where there is room for structured data, and the reference itself becomes as empty as possible. This cleans up not only 'how to make things optionally secret', but also gets rid of the messy example syntax.

i.e. something like:

  - name: binding1
    dependsOn:
    - instance1
    spec:
      object:
        apiVersion: servicecatalog.k8s.io/v1beta1
        kind: ServiceBinding
        metadata:
          name: binding1
        spec:
          instanceRef:
            name: "{{instance1#metadata.name#"exampleName"}}"
          secretName: secret1  

Becomes:

  - name: binding1
    references:
      instanceRef:
        resource: instance1
        path: metadata.name
        example: instanceName
        secret: false
    spec:
      object:
        apiVersion: servicecatalog.k8s.io/v1beta1
        kind: ServiceBinding
        metadata:
          name: binding1
        spec:
          instanceRef:
            name: "!{instanceRef}"
          secretName: secret1  

Note that escaping can be done by repetition of '!' (i.e. !{x} is a reference to x, and !!{x} is the raw string !{x}, etc.). We could make this a list instead of a map to be more kube like, but it fits very well with the concept of a map... (i.e. instanceRef -> blah).

Another option here would be to remove the reference from the parameter entirely, and instead specify an 'output path' in the reference. This would mean we would never have invalid typed data (i.e. string where expecting an object), but would make the templates considerably more opaque.

[wryun]

Ok, the thing I didn't properly consider was that we sometimes used dependsOn in the absence of actual references. Not sure how essential this is - we can always force it by having an 'empty' Reference (i.e. just the resource and nothing else...). Thoughts?

[ash2k]

Add an attribute "unused: true" and error if not set to true but not used?

[wryun]

A bit messy, though. At the moment I'm not checking usages at all... not sure if I should or not. Maybe it's always an error if you specify a Path and don't use it? (not as explicit, of course...)

[ash2k]

I think we should check if a reference is used, otherwise it may be hurting concurrency and... it's just junk that should be cleaned up.
I like the idea with empty/optional path. 👍

[wryun]

Now that we have the reference refactoring, I think this is as simple as allowing complex references into secrets so that we can 'autowire' them appropriately (and get rid of crazy secret plugins...).

See also: #263

EDIT: though this would require the autowiring functions to know about the outputs. I'm... surprisingly ok with this.