Fix issue with configservice subscribe command #1223
Conversation
| # If it was a 404 error, than the bucket does not exist. | ||
| error_code = int(e.response['Error']['Code']) | ||
| if error_code == 404: | ||
| bucket_exists = False |
There was a problem hiding this comment.
We have very similar code in our CloudTrail customization. Maybe it's worth making a utility function that can be used across customizations?
There was a problem hiding this comment.
Are we sure this works? In the cloudtrail customization I had to remove the error handler to get the client exceptions to propogate. I would expect we'd need something here as well right?
There was a problem hiding this comment.
@jamesls
I tested it and I have had confirmation it works. I had to disable the error handler as well:
https://github.com/aws/aws-cli/pull/1223/files#diff-9acfa1e377b87026a3322468d0755a93R150
@danielgtaylor
As to consolidating the logic, I could but it is not completely the same logic. I actually think it may be a potential bug in the cloudtrail commands as you handle 404 and 403 errors the same when they are not really the same relative to determining if a bucket exists. Is my concern correct?
Let me know.
There was a problem hiding this comment.
Oh wow, I completely missed L150-152 sorry about that.
|
I think this is a good change, just wanted to mention the similarities between this and CloudTrail. I'm also curious what the errors look like if I select a bucket I don't own and have zero rights to. The bucket won't be created again, but some other command will likely fail, right? The fix in this PR is really only for the case where I can write to the bucket but cannot run a HEAD operation against it. Is a rights check performed server-side? In the CloudTrail customization we put the burden on the user by having them provide one of two mutually exclusive parameters that specify whether to create a new bucket or use an existing one. Now I'm not saying that's the best approach, but it makes the HEAD check defensive rather than part of the core logic of the command. |
|
|
Looks good to me. However I'll let you and Daniel hash out the details about the cloudtrail commonality. I'm neutral on it.
|
Actually tested what happens if you select a bucket with zero rights. The command actually works cleanly through. It looks like AWS Config does not validate whether if you have rights to the bucket when you create the delivery channel. I have yet to figure out where errors propagate when you do not have permissions to a bucket, but I believe it happens when you deliver snapshots of your AWS resources. The status of the delivery will show a fail. That is why I decided to add the print statement on whether a previous bucket is being used or a new one is being created. So users can catch themselves (the resources created/being used is as expected) before errors show up when you check to see if the snapshots were delivered successfully. Given that it may be better, if we used the same style of the cloudtrail commands, but it may be too late to change the API for the commands so that may be the next best option. I will update the PR with what I find. Let me know if you have any other questions. |
|
UPDATE: Actually I just tested it out again, and actually AWS Config does validate if you have permissions to the bucket upon creating a delivery channel. You will get the following error: So I think everything is fine there. |
When checking to see if a bucket exists, we assumed that an exception meant that the bucket did not exist when using HeadBucket. In reality, the error could be a 403. Also expanded the output to inform users if a new bucket/topic is being used or an existing one.
Added a helper function that can be used to determine if a s3 bucket exists or not.
kyleknap
force-pushed
the
subscribe-fix
branch
2 times, most recently
from
e23110b to
0b408ed
Compare
|
I updated the code to consolidate into the helper method |
|
LGTM 🚢-it! |
1 similar comment
1 similar comment
Fix issue with configservice subscribe command
When checking to see if a bucket exists, we assumed that an exception meant that the bucket did not exist when using HeadBucket. In reality, the error could be a 403, and the user wanted to use that bucket but did not have the proper permissions to run a HEAD object on it. Also expanded the output to inform users if a new bucket/topic is being used or an existing one. This helps notify users of what resources they created and/or are using when subscribing to AWS Config.
cc @jamesls @danielgtaylor