-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SAM] Makes S3 Bucket parameter optional and creates bucket automatically in the deployment region #3040
[SAM] Makes S3 Bucket parameter optional and creates bucket automatically in the deployment region #3040
Changes from 14 commits
f51bcb2
cc7794e
3d399b2
c11c21b
191b3a2
68ac314
c294e59
a645a77
9cde45e
e5e55d3
671b3db
34080bf
39c37d3
7cca0fb
548407d
34b34a0
63e5b20
8403b76
277e7a3
00fe5c8
aab4cb1
e96d568
fcb21ca
3661e42
ad77c0f
37d1e22
d552634
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,19 +11,18 @@ | |
# ANY KIND, either express or implied. See the License for the specific | ||
# language governing permissions and limitations under the License. | ||
|
||
import os | ||
import json | ||
import logging | ||
import os | ||
import sys | ||
|
||
import json | ||
|
||
from botocore.client import Config | ||
|
||
from awscli.customizations.cloudformation import exceptions | ||
from awscli.customizations.cloudformation.artifact_exporter import Template | ||
from awscli.customizations.cloudformation.yamlhelper import yaml_dump | ||
from awscli.customizations.cloudformation import exceptions | ||
from awscli.customizations.commands import BasicCommand | ||
from awscli.customizations.s3uploader import S3Uploader | ||
from botocore.client import Config | ||
from botocore.exceptions import ClientError | ||
|
||
LOG = logging.getLogger(__name__) | ||
|
||
|
@@ -40,6 +39,10 @@ class PackageCommand(BasicCommand): | |
"--stack-name <YOUR STACK NAME>" | ||
"\n") | ||
|
||
MSG_PACKAGE_S3_BUCKET_CREATION = ( | ||
"Bucket {bucket} doesn't exist.\n" | ||
"Creating s3://{bucket} at {region} region.\n") | ||
|
||
NAME = "package" | ||
|
||
DESCRIPTION = BasicCommand.FROM_FILE("cloudformation", | ||
|
@@ -57,7 +60,7 @@ class PackageCommand(BasicCommand): | |
|
||
{ | ||
'name': 's3-bucket', | ||
'required': True, | ||
'required': False, | ||
'help_text': ( | ||
'The name of the S3 bucket where this command uploads' | ||
' the artifacts that are referenced in your template.' | ||
|
@@ -112,19 +115,70 @@ class PackageCommand(BasicCommand): | |
} | ||
] | ||
|
||
def _get_bucket_region(self, s3_bucket, s3_client): | ||
s3_loc = s3_client.get_bucket_location( | ||
Bucket=s3_bucket).get("LocationConstraint", "us-east-1") | ||
return s3_loc.get | ||
|
||
def _run_main(self, parsed_args, parsed_globals): | ||
region = parsed_globals.region if parsed_globals.region else "us-east-1" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we error out if region is not supplied? Last thing we want is surprises. If they are using AWS CLI, they have most likely set the region. So this will be raising the error for the 1% case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That’s a good idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was our initial surprise when hacking that during the hackathon - If you don't supply "--region" during the package command "parsed_globals.region" will be "None" and we were expecting to be whatever was set in the AWS CLI - That's the reason we had this one-liner if. What's set in the AWS CLI like regions for a profile (default, lab, etc.) only seems to work when you initiate a connection with a service (self._session <- contains a dict of regions set in a profile and likely use those in the absence of one). Given that we need the region set as a parameter to run some additional logic we have two options here:
Implemented 1st option for now and commit to follow Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heitor, I see your point. But give how buckets are global, this might do more harm than good. @jamesls Do you know of any option to do Option-2 above (reliably get the Region set in their creds chain)? |
||
s3_client = self._session.create_client( | ||
"s3", | ||
config=Config(signature_version='s3v4'), | ||
region_name=parsed_globals.region, | ||
region_name=region, | ||
verify=parsed_globals.verify_ssl) | ||
|
||
template_path = parsed_args.template_file | ||
if not os.path.isfile(template_path): | ||
raise exceptions.InvalidTemplatePathError( | ||
template_path=template_path) | ||
template_path=template_path) | ||
|
||
if (parsed_args.s3_bucket is not None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bracket not needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
bucket = parsed_args.s3_bucket | ||
s3_bucket_region = self._get_bucket_region(bucket, s3_client) | ||
if not s3_bucket_region == region: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's an exact word comparison and has to be "==" as "is not" isn't exactly the same here and can lead to surprised and make tests to fail. |
||
raise exceptions.PackageFailedRegionMismatchError( | ||
bucket_region=s3_bucket_region, | ||
deploy_region=region | ||
) | ||
else: | ||
sts_client = self._session.create_client( | ||
"sts", | ||
config=Config(signature_version='s3v4'), | ||
verify=parsed_globals.verify_ssl | ||
) | ||
account = sts_client.get_caller_identity().get('Account', "") | ||
bucket = "sam-{region}-{account}".format( | ||
account=account, | ||
region=region | ||
) | ||
|
||
bucket = parsed_args.s3_bucket | ||
# Check if SAM deployment bucket already exists otherwise create it | ||
try: | ||
s3_client.head_bucket(Bucket=bucket) | ||
except ClientError as e: | ||
if e.response["Error"]["Code"] == "404": | ||
sys.stdout.write( | ||
self.MSG_PACKAGE_S3_BUCKET_CREATION.format( | ||
bucket=bucket, region=region)) | ||
|
||
_s3_params = { | ||
"all_regions": { | ||
"Bucket": bucket | ||
}, | ||
"us_standard": { | ||
"Bucket": bucket, | ||
"CreateBucketConfiguration": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand. I thought the location constraint was for non-us-standard regions. Am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right - Fixed. |
||
"LocationConstraint": region | ||
} | ||
} | ||
} | ||
|
||
# Create bucket in specified region or else use us-east-1 | ||
if parsed_globals.region: | ||
s3_client.create_bucket(**_s3_params['all_regions']) | ||
else: | ||
s3_client.create_bucket(**_s3_params['us_standard']) | ||
|
||
self.s3_uploader = S3Uploader(s3_client, | ||
bucket, | ||
|
@@ -142,8 +196,8 @@ def _run_main(self, parsed_args, parsed_globals): | |
|
||
if output_file: | ||
msg = self.MSG_PACKAGED_TEMPLATE_WRITTEN.format( | ||
output_file_name=output_file, | ||
output_file_path=os.path.abspath(output_file)) | ||
output_file_name=output_file, | ||
output_file_path=os.path.abspath(output_file)) | ||
sys.stdout.write(msg) | ||
|
||
sys.stdout.flush() | ||
|
@@ -154,7 +208,8 @@ def _export(self, template_path, use_json): | |
exported_template = template.export() | ||
|
||
if use_json: | ||
exported_str = json.dumps(exported_template, indent=4, ensure_ascii=False) | ||
exported_str = json.dumps( | ||
exported_template, indent=4, ensure_ascii=False) | ||
else: | ||
exported_str = yaml_dump(exported_template) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why another
.get
? Isn'ts3_loc
just a string?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It allows the function to be mocked for the tests easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanath is right.. This code seems incomplete, I'm download the latest source code and will send another commit after checking tests, etc. That part should've been:
instead of: