From e4b9bffbf851cb8367ab35454d4d655f2f8c83f7 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Mon, 30 Mar 2020 11:19:29 -0700 Subject: [PATCH] Rabbitmq: surface errors from responses (#8619) (#8629) * surface errs from responses * add test * Update builtin/logical/rabbitmq/path_role_create.go Co-Authored-By: Vishal Nayak * improve error message Co-authored-by: Vishal Nayak Co-authored-by: Vishal Nayak --- builtin/logical/rabbitmq/backend_test.go | 33 ++++++ builtin/logical/rabbitmq/path_role_create.go | 108 ++++++++++++++----- 2 files changed, 112 insertions(+), 29 deletions(-) diff --git a/builtin/logical/rabbitmq/backend_test.go b/builtin/logical/rabbitmq/backend_test.go index ac76267f8785..552f5ca704b6 100644 --- a/builtin/logical/rabbitmq/backend_test.go +++ b/builtin/logical/rabbitmq/backend_test.go @@ -97,6 +97,39 @@ func TestBackend_basic(t *testing.T) { } +func TestBackend_returnsErrs(t *testing.T) { + if os.Getenv(logicaltest.TestEnvVar) == "" { + t.Skip(fmt.Sprintf("Acceptance tests skipped unless env '%s' set", logicaltest.TestEnvVar)) + return + } + b, _ := Factory(context.Background(), logical.TestBackendConfig()) + + cleanup, uri, _ := prepareRabbitMQTestContainer(t) + defer cleanup() + + logicaltest.Test(t, logicaltest.TestCase{ + PreCheck: testAccPreCheckFunc(t, uri), + LogicalBackend: b, + Steps: []logicaltest.TestStep{ + testAccStepConfig(t, uri), + { + Operation: logical.CreateOperation, + Path: "roles/web", + Data: map[string]interface{}{ + "tags": testTags, + "vhosts": `{"invalid":{"write": ".*", "read": ".*"}}`, + "vhost_topics": testVHostTopics, + }, + }, + { + Operation: logical.ReadOperation, + Path: "creds/web", + ErrorOk: true, + }, + }, + }) +} + func TestBackend_roleCrud(t *testing.T) { if os.Getenv(logicaltest.TestEnvVar) == "" { t.Skip(fmt.Sprintf("Acceptance tests skipped unless env '%s' set", logicaltest.TestEnvVar)) diff --git a/builtin/logical/rabbitmq/path_role_create.go b/builtin/logical/rabbitmq/path_role_create.go index 71b211f14b80..63c09f6d6d4e 100644 --- a/builtin/logical/rabbitmq/path_role_create.go +++ b/builtin/logical/rabbitmq/path_role_create.go @@ -3,10 +3,9 @@ package rabbitmq import ( "context" "fmt" + "io/ioutil" - "github.com/hashicorp/errwrap" - multierror "github.com/hashicorp/go-multierror" - uuid "github.com/hashicorp/go-uuid" + "github.com/hashicorp/go-uuid" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" rabbithole "github.com/michaelklishin/rabbit-hole" @@ -69,27 +68,63 @@ func (b *backend) pathCredsRead(ctx context.Context, req *logical.Request, d *fr } // Register the generated credentials in the backend, with the RabbitMQ server - if _, err = client.PutUser(username, rabbithole.UserSettings{ + resp, err := client.PutUser(username, rabbithole.UserSettings{ Password: password, Tags: role.Tags, - }); err != nil { + }) + if err != nil { return nil, fmt.Errorf("failed to create a new user with the generated credentials") } + defer func() { + if err := resp.Body.Close(); err != nil { + b.Logger().Error(fmt.Sprintf("unable to close response body: %s", err)) + } + }() + if !isIn200s(resp.StatusCode) { + body, _ := ioutil.ReadAll(resp.Body) + return nil, fmt.Errorf("error creating user %s - %d: %s", username, resp.StatusCode, body) + } + + success := false + defer func() { + if success { + return + } + // Delete the user because it's in an unknown state. + resp, err := client.DeleteUser(username) + if err != nil { + b.Logger().Error(fmt.Sprintf("deleting %s due to permissions being in an unknown state, but failed: %s", username, err)) + } + if !isIn200s(resp.StatusCode) { + body, _ := ioutil.ReadAll(resp.Body) + b.Logger().Error(fmt.Sprintf("deleting %s due to permissions being in an unknown state, but error deleting: %d: %s", username, resp.StatusCode, body)) + } + }() // If the role had vhost permissions specified, assign those permissions // to the created username for respective vhosts. for vhost, permission := range role.VHosts { - if _, err := client.UpdatePermissionsIn(vhost, username, rabbithole.Permissions{ - Configure: permission.Configure, - Write: permission.Write, - Read: permission.Read, - }); err != nil { - outerErr := errwrap.Wrapf(fmt.Sprintf("failed to update permissions to the %q user: {{err}}", username), err) - // Delete the user because it's in an unknown state - if _, rmErr := client.DeleteUser(username); rmErr != nil { - return nil, multierror.Append(errwrap.Wrapf("failed to delete user: {{err}}", rmErr), outerErr) + if err := func() error { + resp, err := client.UpdatePermissionsIn(vhost, username, rabbithole.Permissions{ + Configure: permission.Configure, + Write: permission.Write, + Read: permission.Read, + }) + if err != nil { + return err + } + defer func() { + if err := resp.Body.Close(); err != nil { + b.Logger().Error(fmt.Sprintf("unable to close response body: %s", err)) + } + }() + if !isIn200s(resp.StatusCode) { + body, _ := ioutil.ReadAll(resp.Body) + return fmt.Errorf("error updating vhost permissions for %s - %d: %s", vhost, resp.StatusCode, body) } - return nil, outerErr + return nil + }(); err != nil { + return nil, err } } @@ -97,23 +132,34 @@ func (b *backend) pathCredsRead(ctx context.Context, req *logical.Request, d *fr // to the created username for respective vhosts and exchange. for vhost, permissions := range role.VHostTopics { for exchange, permission := range permissions { - if _, err := client.UpdateTopicPermissionsIn(vhost, username, rabbithole.TopicPermissions{ - Exchange: exchange, - Write: permission.Write, - Read: permission.Read, - }); err != nil { - outerErr := errwrap.Wrapf(fmt.Sprintf("failed to update topic permissions to the %q user: {{err}}", username), err) - // Delete the user because it's in an unknown state - if _, rmErr := client.DeleteUser(username); rmErr != nil { - return nil, multierror.Append(errwrap.Wrapf("failed to delete user: {{err}}", rmErr), outerErr) + if err := func() error { + resp, err := client.UpdateTopicPermissionsIn(vhost, username, rabbithole.TopicPermissions{ + Exchange: exchange, + Write: permission.Write, + Read: permission.Read, + }) + if err != nil { + return err } - return nil, outerErr + defer func() { + if err := resp.Body.Close(); err != nil { + b.Logger().Error(fmt.Sprintf("unable to close response body: %s", err)) + } + }() + if !isIn200s(resp.StatusCode) { + body, _ := ioutil.ReadAll(resp.Body) + return fmt.Errorf("error updating vhost permissions for %s - %d: %s", vhost, resp.StatusCode, body) + } + return nil + }(); err != nil { + return nil, err } } } + success = true // Return the secret - resp := b.Secret(SecretCredsType).Response(map[string]interface{}{ + response := b.Secret(SecretCredsType).Response(map[string]interface{}{ "username": username, "password": password, }, map[string]interface{}{ @@ -127,11 +173,15 @@ func (b *backend) pathCredsRead(ctx context.Context, req *logical.Request, d *fr } if lease != nil { - resp.Secret.TTL = lease.TTL - resp.Secret.MaxTTL = lease.MaxTTL + response.Secret.TTL = lease.TTL + response.Secret.MaxTTL = lease.MaxTTL } - return resp, nil + return response, nil +} + +func isIn200s(respStatus int) bool { + return respStatus >= 200 && respStatus < 300 } const pathRoleCreateReadHelpSyn = `