openshift/ansible-service-broker

Supplying the same UUID for a service instance and binding returns 400

jmrodri opened this issue · 3 comments

jmrodri commented

Bug:

When the same UUID is passed in for the service instance and the binding the initial binding will fail with a 400 Bad Request. The logs will show:

time="2018-08-27T20:00:19Z" level=debug msg="Creating k8s apiserver"            
time="2018-08-27T20:00:19Z" level=info msg="Listening on https://[::]:1338"        
time="2018-08-27T20:00:19Z" level=info msg="Ansible Service Broker Starting"       
time="2018-08-27T20:03:19Z" level=debug msg="Unable to find originating origin header"
time="2018-08-27T20:03:19Z" level=info msg="Request: \"PUT /osb/v2/service_instances/6fbf3e48-aa26-11e8-a9d1-0242ac11000b/service_bindings/6fbf3e48-aa26-11e8-a9d1-0242ac11000b?accepts_incomplete=true HTTP/1.1\\r\\nHost: broker-automation-broker.172.17.0.1.nip.io\\r\\nAccept: application/json\\r\\nContent-Length: 191\\r\\nContent-Type: application/json\\r\\nForwarded: for=10.13.57.144;host=broker-automation-broker.172.17.0.1.nip.io;proto=https\\r\\nUser-Agent: curl/7.59.0\\r\\nX-Forwarded-For: 10.13.57.144\\r\\nX-Forwarded-Host: broker-automation-broker.172.17.0.1.nip.io\\r\\nX-Forwarded-Port: 443\\r\\nX-Forwarded-Proto: https\\r\\n\\r\\n{\\n  \\\"plan_id\\\": \\\"7f4a5e35e4af2beb70076e72fab0b7ff\\\",\\n  \\\"service_id\\\": \\\"1dda1477cace09730bd8ed7a6505607e\\\",\\n  \\\"context\\\": \\\"blog-project\\\",\\n  \\\"app_guid\\\":\\\"\\\",\\n  \\\"bind_resource\\\":{},\\n  \\\"parameters\\\": {}\\n}\""
time="2018-08-27T20:03:19Z" level=debug msg="get service instance: 6fbf3e48-aa26-11e8-a9d1-0242ac11000b"
time="2018-08-27T20:03:19Z" level=debug msg="get spec: 1dda1477cace09730bd8ed7a6505607e"
time="2018-08-27T20:03:19Z" level=debug msg="Auto Escalate has been set to true, we are escalating permissions"
time="2018-08-27T20:03:19Z" level=debug msg="Injecting PlanID as parameter: { _apb_plan_id: dev }"
time="2018-08-27T20:03:19Z" level=debug msg="Injecting ServiceClassID as parameter: { _apb_service_class_id: 1dda1477cace09730bd8ed7a6505607e }"
time="2018-08-27T20:03:19Z" level=debug msg="Injecting ServiceInstanceID as parameter: { _apb_service_instance_id: 6fbf3e48-aa26-11e8-a9d1-0242ac11000b }"
time="2018-08-27T20:03:19Z" level=debug msg="Injecting lastRequestingUserKey as parameter: { _apb_last_requesting_user:  }"  
time="2018-08-27T20:03:19Z" level=debug msg="Injecting ServiceBindingID as parameter: { _apb_service_binding_id: 6fbf3e48-aa26-11e8-a9d1-0242ac11000b }"
time="2018-08-27T20:03:19Z" level=debug msg="Found secret with name 6fbf3e48-aa26-11e8-a9d1-0242ac11000b\n"
time="2018-08-27T20:03:19Z" level=debug msg="get binding instance: 6fbf3e48-aa26-11e8-a9d1-0242ac11000b"
time="2018-08-27T20:03:19Z" level=debug msg="set binding instance: 6fbf3e48-aa26-11e8-a9d1-0242ac11000b"
time="2018-08-27T20:03:19Z" level=warning msg="Broker configured to *NOT* launch and run APB bind"
time="2018-08-27T20:03:19Z" level=error msg="Unable to create secret '6fbf3e48-aa26-11e8-a9d1-0242ac11000b' into namespace 'automation-broker'"
time="2018-08-27T20:03:19Z" level=error msg="unable to save extracted credentials - secrets \"6fbf3e48-aa26-11e8-a9d1-0242ac11000b\" already exists"
time="2018-08-27T20:03:19Z" level=error msg="Unable to create new binding extracted creds from provision creds - secrets \"6fbf3e48-aa26-11e8-a9d1-0242ac11000b\" already exists"
172.17.0.1 - - [27/Aug/2018:20:03:19 +0000] "PUT /osb/v2/service_instances/6fbf3e48-aa26-11e8-a9d1-0242ac11000b/service_bindings/6fbf3e48-aa26-11e8-a9d1-0242ac11000b?accepts_incomplete=true HTTP/1.1" 400 87
time="2018-08-27T20:04:24Z" level=debug msg="Unable to find originating origin header"
time="2018-08-27T20:04:24Z" level=info msg="Request: \"PUT /osb/v2/service_instances/6fbf3e48-aa26-11e8-a9d1-0242ac11000b/service_bindings/6fbf3e48-aa26-11e8-a9d1-0242ac11000b?accepts_incomplete=true HTTP/1.1\\r\\nHost: broker-automation-broker.172.17.0.1.nip.io\\r\\nAccept: application/json\\r\\nContent-Length: 191\\r\\nContent-Type: application/json\\r\\nForwarded: for=10.13.57.144;host=broker-automation-broker.172.17.0.1.nip.io;proto=https\\r\\nUser-Agent: curl/7.59.0\\r\\nX-Forwarded-For: 10.13.57.144\\r\\nX-Forwarded-Host: broker-automation-broker.172.17.0.1.nip.io\\r\\nX-Forwarded-Port: 443\\r\\nX-Forwarded-Proto: https\\r\\n\\r\\n{\\n  \\\"plan_id\\\": \\\"7f4a5e35e4af2beb70076e72fab0b7ff\\\",\\n  \\\"service_id\\\": \\\"1dda1477cace09730bd8ed7a6505607e\\\",\\n  \\\"context\\\": \\\"blog-project\\\",\\n  \\\"app_guid\\\":\\\"\\\",\\n  \\\"bind_resource\\\":{},\\n  \\\"parameters\\\": {}\\n}\""

Cause

The cause is the APB creates a secret with credentials during the provision call. The secret is located in the broker's namespace using the same UUID as the service instance. When the initial bind call is invoked it determines there is no binding, so attempts to create the secret with the credentials on it. The secret will use the binding UUID, which as you know was identical to the service instance UUID in our example. When the broker attempts to create a secret with the same IDs, we get an already exists error from the oc and k8s client. This is an edge case as under normal operation: svcat or OKD webui a different UUID will be used for the binding.

jmrodri commented

Solutions

There are a few ways to address this:

  1. Disallow duplicate ids for service instances and bindings, fail early from the handler
if serviceinstance.id == binding.id:
    return 400
  1. Update SaveExtractedCredentialSecret and UpdateExtractedCredentialSecret methods in clients/kubernetes.go of bundle-lib to handle the AlreadyExists and Conflict errors during the storing of the secrets. We do similar solution in the crd/dao.go of the broker.

Here is a start, this is not complete yet as I decided to put the bug aside and create this issue instead.

// SaveExtractedCredentialSecret - Save the extCreds as a secret                
func (k KubernetesClient) SaveExtractedCredentialSecret(instanceID, ns string,  
    extCreds map[string]interface{}, labels map[string]string) error {          
                                                                                
    b, err := json.Marshal(extCreds)                                            
    if err != nil {                                                             
        log.Errorf("Unable to marshal credentials - %v", err)                   
        return err                                                              
    }                                                                           
                                                                                
    data := map[string][]byte{credentialsKey: b}                                
    s := &apiv1.Secret{                                                         
        ObjectMeta: metav1.ObjectMeta{                                          
            Name:   instanceID,                                                 
            Labels: labels,                                                     
        },
        Data: data,
    }
    _, err = k.Client.CoreV1().Secrets(ns).Create(s)
    // UPDATE THE ERROR HANDLING
    // pseudo code below
    if err != nil && err != AlreadyExists {
        // Attempt to update the secret
         _, err = k.Client.CoreV1().Secrets(ns).Update(s)
        if err != nil && err != Conflict {
            // update returned an error that was not Conflict, we don't know
            // how to deal with this, returning
            log.Errorf("error message")
            return err
        }
    } else if err != nil {
        // create returned an error that was not AlreadyExists, we don't know
        // how to deal with this, returning
        log.Errorf("error message")
        return err
    }
        
    /* OLD error code
    if err != nil {
        log.Errorf("Unable to create secret '%v' into namespace '%v'", instanceID, ns)
        return err
    }
    */
    return nil
}
  1. Update the ext_creds.go methods to look at the errors return from the runtime methods. For example, SetExtractedCredentials could look at the errors currently returned by the above runtime method and handle it at that layer. In this scenario, we wouldn't touch the client methods
// SetExtractedCredentials - Will set new credentials for an id.
// Please use this method with caution.
func SetExtractedCredentials(id string, creds *ExtractedCredentials) error {
    err := runtime.Provider.CreateExtractedCredential(id, clusterConfig.Namespace, creds.Credentials, nil)
    if err != nil && err != AlreadyExists {
        // Attempt to update the secret
         err := runtime.Provider.UpdateExtractedCredential(id, clusterConfig.Namespace, creds.Credentials, nil)
        if err != nil && err != Conflict {
            // update returned an error that was not Conflict, we don't know
            // how to deal with this, returning
            log.Errorf("error message")
            return err
        }
    } else if err != nil {
        // create returned an error that was not AlreadyExists, we don't know
        // how to deal with this, returning
        log.Errorf("error message")
        return err
    }
    return nil
}
shawn-hurley commented

Disallow duplicate ids for service instances and bindings, fail early from the handler

I suggest that we do this if we have to. If someone uses a duplicate ID I would suggest that the name (Universal Unique ID) means that the client is generating the ID wrong.

The same failure could happen if you use the same ID as a completely different service. I would suggest that we return the error in the scenario by failing at whatever level that is IMO.

jmrodri commented

Fixed by #1081