HCP Packer Buckets: Change UpsertBucket to call GetBucket #13059
Conversation
JenGoldstrich
force-pushed
the
hpr-1772-upsert-bucket-get-first
branch
4 times, most recently
from
a18699a
to
22a8fd2
Compare
…evel role based authentication, as CreateBucket uses project level auth
JenGoldstrich
force-pushed
the
hpr-1772-upsert-bucket-get-first
branch
from
22a8fd2
to
046c8f1
Compare
JenGoldstrich
changed the title
| _, err := c.Packer.PackerServiceGetBucket(getParams, nil) | ||
| if err != nil { | ||
| if CheckErrorCode(err, codes.NotFound) { | ||
| _, err = c.CreateBucket(ctx, bucketName, bucketDescription, bucketLabels) |
There was a problem hiding this comment.
Question, is the error message that gets displayed to the user if they don't have permission to create a bucket descriptive enough for the user to determine why creation failed?
I don't know that I ever checked the error message and wonder if we need to add a call to action so that a user knows what to do when it fails. You don't need to account for it in this PR but I am curious if there is any changes to the error messaging we need to account for.
There was a problem hiding this comment.
I think returning the raw API error is fine as it's what we currently returned, after this change if a user doesn't have access to create a bucket we'll return an error that looks like this
failed to initialize bucket "blah": [PUT
/packer/2023-01-01/organizations/{location.organization_id}/projects/{location.project_id}/buckets][403]
PackerService_CreateBucket default &{Code:7 Details:[] Message:Not Authorized}
This is the same error that was returned currently where we call CreateBucket first, so I think this will give users enough information to operate on it, I'd wait and see if a user runs into a pain point figuring this out before improving this experience personally as I do think the API error contains the relevant information. I could see a future where we first check what type of principal the user is using and return a more detailed error message though.
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
This PR changes the API calls made by the internal function
UpsertBucket, previously this function would call CreateBucket and then if that returned a AlreadyExists/Conflict status code it would call UpdateBucket to set the bucket labels and description on the bucket. This lead to failures with bucket level service principals because the principal would be scoped to only access the existing bucket, and so Packer would error out because of making an unauthorized request.To handle this I have changed UpsertBucket to call GetBucket first, if GetBucket 404s the method then calls Create, if GetBucket succeeds it calls UpdateBucket, and if GetBucket fails with a non 404 status code Packer exits with the error from GetBucket.