BindInstance.Parameters use unclear

[mhrivnak]

This issue raises a question of what data is intended to be stored where, and potentially will result in changing what data gets stored on the BindInstance.

There are two potential definitions for the word "parameters" in the broker.

1. The set of arguments passed in by an API client to a OSB API endpoint, such as when creating a Service Binding.

2. The set of arguments passed by the broker to an APB at runtime. This generally includes all the parameters from 1 above, and the broker potentially adds others such as "namespace", "cluster", and "_apb_provision_creds".

Which of these is intended to be what is stored on the BindInstance.Parameters field? I think it should probably be just definition 1, because definition 2 seems only related to the running job.

This question came up because we need to compare the parameters provided by a client with any previously-provided parameters for an equivalent request. That comparison currently always fails, because the "previous" parameters include extra broker-added options, while the "new" parameters do not. In other words, we are using definition 2 currently to decide what gets saved on the BindInstance.

https://github.com/openshift/ansible-service-broker/blob/ansible-service-broker-1.1.8-1/pkg/broker/broker.go#L865

[eriknelson]

Good observation, we're more ambiguous with our names in some places than we should be. Often the context matters. (Ex: ServiceInstance. In the broker package, that's the OSB context, and includes param schemas for a catalog response. In the apb package, it's our representation of the instance in the APB space, including the Spec that the Instance was derived from. Two completely different things).

In this case, I'd probably split them into two different maps with explicit names. Something like RequestParameters and APBParameters, where APBParameters starts as a copy of the RequestParameters, and is injected with whatever other specialized _apb_ namespaced vars the broker/APBs require.

[mhrivnak]

For now, given that we have a tangible bug, how crazy would it be to move the comparison logic into a method and have it ignore Parameters that the broker typically adds? Something like this in pkg/apb/types.go:

// BindInstance - Binding Instance describes a completed binding
type BindInstance struct {
    ID           uuid.UUID   `json:"id"`
    ServiceID    uuid.UUID   `json:"service_id"`
    Parameters   *Parameters `json:"parameters"`
}

// UserParameters - returns the Parameters field with any keys and values
// removed that are typically added by the broker itself. The return value
// should represent what a OSB API client provides in a bind request.
func (bi *BindInstance) UserParameters() *Parameters {
    ret := make(Parameters)
    for key, value := range *bi.Parameters {
        switch key {
        // Do not copy keys that are generally added by the broker itself.
        case "cluster", "namespace", "_apb_provision_creds":
            continue
        }   
        ret[key] = value
    }   
    return &ret
}

// IsEqual - Determines if two BindInstances are equal, omitting any Parameters
// that generally get added by the broker itself.
func (bi *BindInstance) IsEqual(newbi *BindInstance) bool {
    if !uuid.Equal(bi.ID, newbi.ID) {
        return false
    }   
    return reflect.DeepEqual(bi.UserParameters(), newbi.UserParameters())
}

[jmrodri]

@mhrivnak I was going to suggest why not ignore anything we added, so the above method seems like a good solution.

[mhrivnak]

Just to follow up, the above workaround is what I ended up doing and is in the code base now.

This still needs better definition though as part of a total evaluation of the bundle contract. I think a reasonable next step is to schedule a brainstorm on that contract and desired changes, which I'm happy to facilitate.

[mhrivnak]

This will be addressed as part of this effort: https://trello.com/c/HTIdIch6/810-propose-changes-to-the-service-bundle-contract-based-on-collected-feedback

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[jmrodri]

/close

[openshift-ci-robot]

@jmrodri: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.