Add Reconciled Instance state

[duglin]

Currently, the Instance resource has the following properties that are settable by users:

• PlanName
• Parameters
• ParametersFrom

There is no way for a user to determine what the actual values are for these properties - meaning, what does the Broker think they are. For example, a user could set PlanName to a value but the Broker may reject the request for some reason. While the error condition will (should) appear in the Instances.Status.Conditions property, the user doesn't have a way to retrieve the actual value since the Spec shows the invalid value.

Proposal

• add something to Instance.Status to hold the "reconciled" version of these properties.

The exact shape of these properties is an impl detail but two options are: a) inlined

type InstanceStatus struct {
  ...
  ReconciledGeneration        int
  ReconciledPlan              string
  ReconciledParameters        *runtime.RawExtension 
  ReconciledParametersFrom    []ParametersFromSource
}

and b) via a reference

type InstanceStatus struct {
  ...
  ReconciledSpec *SpecSnapshot
}

type SpecSnapshot struct { 
  Generation        int
  PlanName          string
  Parameters        *runtime.RawExtension 
  ParametersFrom    []ParametersFromSource
}

[pmorie]

I endorse this, as it is the product of an offline discussion @duglin and I had.

[pmorie]

Note, this also implies that when the controller performs an async operation, it has to track the information about that snapshot of the spec:

type InstanceStatus struct {
  // other fields omitted

  // AsyncOperationSpec is the snapshot the current async operation is for.
  AsyncOperationSpec *SpecSnapshot
}

However, we also have AsyncOpInProgress. In order to avoid having conflicting elements in status, we might want to consider dropping AsyncOpInProgress.

[duglin]

Based on https://github.com/kubernetes-incubator/service-catalog/issues/1095 its not clear to me if we should use "Observed" or "Reconciled" as the prefix - YUGE decision! :-) But we should try for consistency.

[duglin]

at f2f we agreed to adding Status.ReconciledSpec (* to struct - option b) and will open a separate issue for the async/in-progress Spec tracking.

[duglin]

See: #1150 and #1149 for related issues

[kibbles-n-bytes]

As per our face-to-face discussion, we have consensus that Reconciled**Generation** represents the last completed reconciliation attempt by the controller, whether that attempt was successful or not. However, the proposed Reconciled**Spec** represents the current successful state of the world, and is not updated on failed reconciliation attempts.

Since the usage of the prefix "Reconciled" in these two situations are at odds (one reflecting attempted, one representing accepted), I think we should rename ReconciledSpec to something else. My proposed name would be CurrentSpec.

[duglin]

+1 to a rename to avoid confusion. CurrentSpec is better but "current" could be thought of as "this Instance's", which of course its not. Perhaps "ExternalSpec" since we have "ExternalID" already and "External" in these cases refers to what the broker's view of things are.

[kibbles-n-bytes]

ExternalSpec seems fine to me.

I'm worried about the ParametersFrom section in the SpecSnapshot. If the secrets mutate, there'd be no way to make that obvious to the user. It's important that we don't surface the data in the secret, however. I think it'd be important to at least list the keys of those secret parameters; what we could do is store Parameters with the entire merged parameter payload, except for secret parameters their value could be redacted.

[duglin]

Unless we create a copy of the secrets just for the purpose of having a static thing to point to, I'm not sure what else we can do.

[kibbles-n-bytes]

@duglin What about copying the keys, but not the data? So we could at least tell the user "we sent the following keys with secret data".

[pmorie]

We cannot copy the secret data and store it anywhere in a non-secret resource.

I like @kibbles-n-bytes' idea here: https://github.com/kubernetes-incubator/service-catalog/issues/1147#issuecomment-325787055

Going to write up a specific proposal now.

[duglin]

I think this all depends on what we intend this data to be used for.

If its for the user to know what the Broker has then we're stuck because we can't store the secret data here - and I include in that the key names because even seeing those could be a security hole. The best I think we can do is create a new copy of the secret and point to that, but I actually don't think that is worth it right now - until we get a solid usecase for it.

If its to be able to do some kind of alternative spec vs status diff (which is what (Reconcilded)Generation is for) then we could just store a hash of the data. But I don't think we have a usecase for this either.

Net: I would suggest we save a pointer to the original secret and just tell people the data it points to might not match what the broker has.

[pmorie]

The key names are fine, the secret parameter values are not fine

[pmorie]

Proposal

For ServiceInstance, we will add two fields to the status:

• InProgressProperties: a record of what properties we're currently performing an operation on at the broker
• ExternalProperties: a record of what properties the broker knows about for this instance

The InProgressProperties field is necessary to ensure we correctly capture the checksum of the properties sent to the broker. While the spec cannot be mutated while an operation is in progress, secrets can be changed out of band.

type ServiceInstanceStatus struct {
    // other fields omitted

    // InProgressProperties is the properties state for which there is a
    // provision or update action in progress.
    InProgressProperties *ServiceInstancePropertiesState

    // ExternalProperties is the properites state which the broker knows
    // about.
    ExternalProperties *ServiceInstancePropertiesState
}

// ServiceInstancePropertiesState is the state of a ServiceInstance that
// the ServiceBroker knows about.
type ServiceInstancePropertiesState struct {
    // PlanName is the name of the plan that the broker knows this
    // ServiceInstance to be on.
    PlanName string `json:"planName"`

    // Parameters is a blob of the parameters and their values that the broker
    // knows about for this ServiceInstance.  If a parameter was sourced from
    // a secret, its value will be "<redacted>" in this blob.
    Parameters runtime.RawExtension `json:"parameters"`

    // ParametersChecksum is the checksum of the parameters that were sent.
    ParameterChecksum string `json:"parameterChecksum,omitempty"`

    // UserInfo is information about the user that made the request.
    UserInfo UserInfo `json:"userInfo"`
}

For ServiceInstanceCredential, we'll use the same pattern:

type ServiceInstanceCredentialSpec struct {
    // other fields omitted

    // InProgressProperties is the properties state for which there is a bind
    // operation in progress.
    InProgressProperties *ServiceInstanceCredentialPropertiesState

    // ExternalProperties is the properties state that the broker knows
    // about for this ServiceInstanceCredential.
    ExternalProperties *ServiceInstanceCredentialPropertiesState
}

// ServiceInstanceCredentialPropertiesState is the state of a
// ServiceInstanceCredential that the ServiceBroker knows about.
type ServiceInstanceCredentialPropertiesState struct {
    // Parameters is a blob of the parameters and their values that the broker
    // knows about for this ServiceInstanceCredential.  If a parameter was
    // sourced from a secret, its value will be "<redacted>" in this blob.
    Parameters runtime.RawExtension `json:"parameters"`

    // ParametersChecksum is the checksum of the parameters that were sent.
    ParameterChecksum string `json:"parameterChecksum,omitempty"`

    // UserInfo is information about the user that made the request.
    UserInfo UserInfo `json:"userInfo"`
}

[duglin]

I don't understand the purpose behind the InProgress properties, or why we need the checksum. Whether the values of the secrets change or not any retry logic is going to go back to the secret to get the values so why does saving the keys or the checksum help? And, btw, exposing secret keys is a security hole because it now tells a hacker what keys to try to hack -they now only need to focus on figuring out the value and not the key in their attack. 1/2 way done.

[pmorie]

Precedent in k8s is that secret keys are not secret. Values of secret keys are. Example: pods store coordinates for secret keys but are not an escalating resource because they don't expose the values of those keys. Since there is no way to get a secret without getting the entire payload, exposing a secret key does not make a resource escalating.

I believe that we need a checksum so that we can detect changes in parameter values to see whether we have to send the parameters for an instance update. We could eliminate the checksum field for now because we don't currently support updates, and come back to that problem once we're at the point where we have to implement it. I think showing the plan and parameters is sufficient for now.

[duglin]

I forgot about the update case despite us talking about it earlier - geez! So checksum makes sense now and even though we're not using it yet I'm ok with it being there.

re: secret key names.... gotcha, but not happy about it :-(

[pmorie]

Updated this comment to also contain user information, per the September 5 SIG call.