Skip to content

Commit 9be6dd3

Browse files
author
Cody Maloney
committed
Remove magic global passing of azure and aws storage urls
Previously these were passed by globals when we were doing a release and as arguments when we did a advanced / custom configuration template generation. Now they are always passed as arguments which are injected by release/__init__.py in the case of release for the configuration, and via the normal argumen means otherwise. The magic config global still exists so aws CF template validation can work, although that can definitely have the same pattern done to it in the future. It's more complicated to do however since it needs to ensure the AWS keys that are passed through don't end up inside the produced cluster
1 parent 53444f1 commit 9be6dd3

File tree

7 files changed

+88
-86
lines changed

7 files changed

+88
-86
lines changed

dcos_installer/backend.py

+9-5
Original file line numberDiff line numberDiff line change
@@ -127,13 +127,15 @@ def calculate_base_repository_url(
127127

128128

129129
# Figure out the s3 bucket url from region + bucket + path
130-
# TODO(cmaloney): Allow using a CDN rather than the raw S3 url, which will allow
131-
# us to use this same logic for both the internal / do_create version and the
132-
# user dcos_generate_config.sh option.
133130
def calculate_cloudformation_s3_url(bootstrap_url, config_id):
134131
return '{}/config_id/{}'.format(bootstrap_url, config_id)
135132

136133

134+
# Figure out the s3 bucket url from region + bucket + path
135+
def calculate_cloudformation_s3_url_full(cloudformation_s3_url):
136+
return '{}/cloudformation'.format(cloudformation_s3_url)
137+
138+
137139
def calculate_aws_template_storage_region_name(
138140
aws_template_storage_access_key_id,
139141
aws_template_storage_secret_access_key,
@@ -183,8 +185,9 @@ def calculate_aws_template_storage_region_name(
183185
'must': {
184186
'provider': 'aws',
185187
'cloudformation_s3_url': calculate_cloudformation_s3_url,
188+
'cloudformation_s3_url_full': calculate_cloudformation_s3_url_full,
186189
'bootstrap_url': calculate_base_repository_url,
187-
'reproducible_artifact_path': calculate_reproducible_artifact_path
190+
'reproducible_artifact_path': calculate_reproducible_artifact_path,
188191
},
189192
'conditional': {
190193
'aws_template_upload': {
@@ -208,6 +211,7 @@ def get_aws_advanced_target():
208211
'aws_template_upload',
209212
'aws_template_storage_bucket_path_autocreate',
210213
'cloudformation_s3_url',
214+
'cloudformation_s3_url_full',
211215
'provider',
212216
'bootstrap_url',
213217
'bootstrap_variant',
@@ -269,7 +273,7 @@ def do_aws_cf_configure():
269273
gen_config['bootstrap_url'] = full_config['bootstrap_url']
270274
gen_config['provider'] = full_config['provider']
271275
gen_config['bootstrap_id'] = full_config['bootstrap_id']
272-
gen_config['cloudformation_s3_url'] = full_config['cloudformation_s3_url']
276+
gen_config['cloudformation_s3_url_full'] = full_config['cloudformation_s3_url_full']
273277

274278
# Convert the bootstrap_Variant string we have back to a bootstrap_id as used internally by all
275279
# the tooling (never has empty string, uses None to say "no variant")

gen/__init__.py

+3-2
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,8 @@ def generate(
448448
arguments,
449449
extra_templates=list(),
450450
cc_package_files=list(),
451-
extra_sources=list()):
451+
extra_sources=list(),
452+
extra_targets=list()):
452453
# To maintain the old API where we passed arguments rather than the new name.
453454
user_arguments = arguments
454455
arguments = None
@@ -457,7 +458,7 @@ def generate(
457458
user_arguments, extra_templates, extra_sources)
458459

459460
# TODO(cmaloney): Make it so we only get out the dcosconfig target arguments not all the config target arguments.
460-
resolver = gen.internals.resolve_configuration(sources, targets)
461+
resolver = gen.internals.resolve_configuration(sources, targets + extra_targets)
461462
status = resolver.status_dict
462463

463464
if status['status'] == 'errors':

gen/aws/templates/advanced/zen.json

+4-4
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@
8181
"Infrastructure": {
8282
"Type": "AWS::CloudFormation::Stack",
8383
"Properties": {
84-
"TemplateURL": "{{ cloudformation_full_s3_url }}/infra.json",
84+
"TemplateURL": "{{ cloudformation_s3_url_full }}/infra.json",
8585
"TimeoutInMinutes": "60",
8686
"Parameters": {
8787
"KeyName": {
@@ -109,7 +109,7 @@
109109
"DependsOn": ["Infrastructure"],
110110
"Type": "AWS::CloudFormation::Stack",
111111
"Properties": {
112-
"TemplateURL": "{{ cloudformation_full_s3_url }}/{{ variant_prefix }}{{ os_type }}-advanced-master-{{ num_masters }}.json",
112+
"TemplateURL": "{{ cloudformation_s3_url_full }}/{{ variant_prefix }}{{ os_type }}-advanced-master-{{ num_masters }}.json",
113113
"Parameters": {
114114
"KeyName": {
115115
"Ref": "KeyName"
@@ -160,7 +160,7 @@
160160
"DependsOn": ["Infrastructure", "MasterStack"],
161161
"Type": "AWS::CloudFormation::Stack",
162162
"Properties": {
163-
"TemplateURL": "{{ cloudformation_full_s3_url }}/{{ variant_prefix }}{{ os_type }}-advanced-pub-agent.json",
163+
"TemplateURL": "{{ cloudformation_s3_url_full }}/{{ variant_prefix }}{{ os_type }}-advanced-pub-agent.json",
164164
"Parameters": {
165165
"KeyName": {
166166
"Ref": "KeyName"
@@ -199,7 +199,7 @@
199199
"DependsOn": ["Infrastructure", "MasterStack"],
200200
"Type": "AWS::CloudFormation::Stack",
201201
"Properties": {
202-
"TemplateURL": "{{ cloudformation_full_s3_url }}/{{ variant_prefix }}{{ os_type }}-advanced-priv-agent.json",
202+
"TemplateURL": "{{ cloudformation_s3_url_full }}/{{ variant_prefix }}{{ os_type }}-advanced-priv-agent.json",
203203
"Parameters": {
204204
"KeyName": {
205205
"Ref": "KeyName"

gen/build_deploy/aws.py

+4-32
Original file line numberDiff line numberDiff line change
@@ -264,21 +264,6 @@ def gen_ami_mapping(mappings):
264264
return json.dumps(final, indent=4, sort_keys=True)
265265

266266

267-
def get_cloudformation_s3_url():
268-
assert release._config is not None
269-
# TODO(cmaloney): HACK. Stashing and pulling the config from release/__init__.py
270-
# is definitely not the right way to do this.
271-
272-
if 'options' not in release._config:
273-
raise RuntimeError("No options section in configuration")
274-
275-
if 'cloudformation_s3_url' not in release._config['options']:
276-
raise RuntimeError("No options.cloudformation_s3_url section in configuration")
277-
278-
# TODO(cmaloney): get_session shouldn't live in release.storage
279-
return release._config['options']['cloudformation_s3_url']
280-
281-
282267
def transform(line):
283268
m = AWS_REF_REGEX.search(line)
284269
# no splitting necessary
@@ -388,7 +373,9 @@ def make_advanced_bunch(variant_args, extra_sources, template_name, cc_params):
388373
arguments=variant_args,
389374
extra_templates=extra_templates,
390375
cc_package_files=cc_package_files,
391-
extra_sources=extra_sources + [aws_base_source])
376+
extra_sources=extra_sources + [aws_base_source],
377+
# TODO(cmaloney): Merge this with dcos_installer/backend.py::get_aws_advanced_target()
378+
extra_targets=[gen.internals.Target(variables={'cloudformation_s3_url_full'})])
392379

393380
cloud_config = results.templates['cloud-config.yaml']
394381

@@ -419,21 +406,7 @@ def make_advanced_bunch(variant_args, extra_sources, template_name, cc_params):
419406
return ResultTuple(cloudformation, results)
420407

421408

422-
def get_s3_url_prefix(arguments, reproducible_artifact_path) -> str:
423-
assert reproducible_artifact_path, "reproducible_artifact_path must not be empty"
424-
if 'cloudformation_s3_url' in arguments:
425-
# Caller is `dcos_generate_config.sh --aws-cloudformation`
426-
url = arguments['cloudformation_s3_url'] + '/cloudformation'
427-
return url
428-
else:
429-
# Caller is release create
430-
url = get_cloudformation_s3_url() + '/{}/cloudformation'.format(reproducible_artifact_path)
431-
return url
432-
433-
434409
def gen_advanced_template(arguments, variant_prefix, reproducible_artifact_path, os_type):
435-
cloudformation_full_s3_url = get_s3_url_prefix(arguments, reproducible_artifact_path)
436-
437410
for node_type in ['master', 'priv-agent', 'pub-agent']:
438411
# TODO(cmaloney): This forcibly overwriting arguments might overwrite a user set argument
439412
# without noticing (such as exhibitor_storage_backend)
@@ -470,7 +443,6 @@ def _as_artifact(filename, gen_out):
470443
resource_string("gen", "aws/templates/advanced/zen.json").decode(),
471444
variant_prefix=variant_prefix,
472445
reproducible_artifact_path=reproducible_artifact_path,
473-
cloudformation_full_s3_url=cloudformation_full_s3_url,
474446
**bunch.results.arguments))
475447
else:
476448
local_source.add_must('num_masters', '1')
@@ -571,7 +543,7 @@ def get_button(template_name, s3_url):
571543
button_line = ""
572544
for variant, arguments in variant_arguments.items():
573545
variant_prefix = pkgpanda.util.variant_prefix(variant)
574-
s3_url = get_s3_url_prefix(arguments, reproducible_artifact_path)
546+
s3_url = arguments['cloudformation_s3_url_full']
575547
button_line += region_line_template.format(
576548
region_name=region['name'],
577549
region_id=region['id'],

gen/build_deploy/azure.py

+9-32
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,6 @@
1313
import gen.build_deploy.util as util
1414
import gen.template
1515
import pkgpanda.build
16-
import release
17-
import release.storage
1816
from gen.internals import Source
1917

2018
# TODO(cmaloney): Make it so the template only completes when services are properly up.
@@ -261,24 +259,29 @@ def do_create(tag, build_name, reproducible_artifact_path, commit, variant_argum
261259

262260
yield {
263261
'channel_path': 'azure.html',
264-
'local_content': gen_buttons(build_name, reproducible_artifact_path, tag, commit),
262+
'local_content': gen_buttons(
263+
build_name,
264+
reproducible_artifact_path,
265+
tag,
266+
commit,
267+
next(iter(variant_arguments.values()))['azure_download_url']),
265268
'content_type': 'text/html; charset=utf-8'
266269
}
267270

268271

269-
def gen_buttons(build_name, reproducible_artifact_path, tag, commit):
272+
def gen_buttons(build_name, reproducible_artifact_path, tag, commit, download_url):
270273
'''
271274
Generate the button page, that is, "Deploy a cluster to Azure" page
272275
'''
273276
dcos_urls = [
274277
encode_url_as_param(DOWNLOAD_URL_TEMPLATE.format(
275-
download_url=get_download_url(),
278+
download_url=download_url,
276279
reproducible_artifact_path=reproducible_artifact_path,
277280
arm_template_name='dcos-{}master.azuredeploy.json'.format(x)))
278281
for x in [1, 3, 5]]
279282
acs_urls = [
280283
encode_url_as_param(DOWNLOAD_URL_TEMPLATE.format(
281-
download_url=get_download_url(),
284+
download_url=download_url,
282285
reproducible_artifact_path=reproducible_artifact_path,
283286
arm_template_name='acs-{}master.azuredeploy.json'.format(x)))
284287
for x in [1, 3, 5]]
@@ -298,29 +301,3 @@ def encode_url_as_param(s):
298301
s = s.encode('utf8')
299302
s = urllib.parse.quote_plus(s)
300303
return s
301-
302-
303-
def get_download_url():
304-
assert release._config is not None
305-
# TODO: HACK. Stashing and pulling the config from release/__init__.py
306-
# is definitely not the right way to do this.
307-
# See also gen/build_deploy/aws.py#get_cloudformation_s3_url
308-
309-
if 'storage' not in release._config:
310-
raise RuntimeError("No storage section in configuration")
311-
312-
if 'azure' not in release._config['storage']:
313-
# No azure storage, inject a fake url for now so if people want to use
314-
# the azure templates they know to come look here.
315-
return "https://AZURE NOT CONFIGURED, ADD A storage.azure section to " \
316-
"dcos-release.config.yaml to use the Azure templates"
317-
318-
if 'download_url' not in release._config['storage']['azure']:
319-
raise RuntimeError("No download_url section in azure configuration")
320-
321-
download_url = release._config['storage']['azure']['download_url']
322-
323-
if not download_url.endswith('/'):
324-
raise RuntimeError("Azure download_url must end with a '/'")
325-
326-
return download_url

release/__init__.py

+56-8
Original file line numberDiff line numberDiff line change
@@ -407,12 +407,24 @@ def make_channel_artifacts(metadata):
407407
# Add templates for the default variant.
408408
# Use keyword args to make not matching ordering a loud error around changes.
409409
with logger.scope("Creating {} deploy tools".format(module.__name__)):
410+
# TODO(cmaloney): Cleanup by just having this make and pass another source.
411+
module_specific_variant_arguments = copy.deepcopy(variant_arguments)
412+
for arg_dict in module_specific_variant_arguments.values():
413+
if module.__name__ == 'gen.build_deploy.aws':
414+
arg_dict['cloudformation_s3_url_full'] = metadata['cloudformation_s3_url_full']
415+
elif module.__name__ == 'gen.build_deploy.azure':
416+
arg_dict['azure_download_url'] = metadata['azure_download_url']
417+
elif module.__name__ == 'gen.build_deploy.bash':
418+
pass
419+
else:
420+
raise NotImplementedError("Unknown how to add args to deploy tool: {}".format(module.__name__))
421+
410422
for built_resource in module.do_create(
411423
tag=metadata['tag'],
412424
build_name=metadata['build_name'],
413425
reproducible_artifact_path=metadata['reproducible_artifact_path'],
414426
commit=metadata['commit'],
415-
variant_arguments=variant_arguments,
427+
variant_arguments=module_specific_variant_arguments,
416428
all_bootstraps=metadata["all_bootstraps"]):
417429

418430
assert isinstance(built_resource, dict), built_resource
@@ -543,7 +555,32 @@ def do_build_packages(cache_repository_url):
543555
return result
544556

545557

546-
def set_repository_metadata(repository, metadata, storage_providers, preferred_provider):
558+
def get_azure_download_url(config) -> str:
559+
# TODO: HACK. Stashing and pulling the config from release/__init__.py
560+
# is definitely not the right way to do this.
561+
# See also gen/build_deploy/aws.py#get_cloudformation_s3_url
562+
563+
if 'storage' not in config:
564+
raise RuntimeError("No storage section in configuration")
565+
566+
if 'azure' not in config['storage']:
567+
# No azure storage, inject a fake url for now so if people want to use
568+
# the azure templates they know to come look here.
569+
return "https://AZURE NOT CONFIGURED, ADD A storage.azure section to " \
570+
"dcos-release.config.yaml to use the Azure templates"
571+
572+
if 'download_url' not in config['storage']['azure']:
573+
raise RuntimeError("No download_url section in azure configuration")
574+
575+
download_url = config['storage']['azure']['download_url']
576+
577+
if not download_url.endswith('/'):
578+
raise RuntimeError("Azure download_url must end with a '/'")
579+
580+
return download_url
581+
582+
583+
def set_repository_metadata(repository, metadata, storage_providers, preferred_provider, config) -> None:
547584
metadata['repository_path'] = repository.path_prefix[:-1]
548585
metadata['repository_url'] = preferred_provider.url + repository.path_prefix[:-1]
549586
metadata['build_name'] = repository.path_channel_prefix[:-1]
@@ -552,8 +589,17 @@ def set_repository_metadata(repository, metadata, storage_providers, preferred_p
552589
for name, store in storage_providers.items():
553590
metadata['storage_urls'][name] = store.url
554591

555-
# Explicitly returning none since we modify in place
556-
return None
592+
if 'options' not in config:
593+
raise RuntimeError("No options section in configuration")
594+
595+
if 'cloudformation_s3_url' not in config['options']:
596+
raise RuntimeError("No options.cloudformation_s3_url section in configuration")
597+
598+
# TODO(cmaloney): get_session shouldn't live in release.storage
599+
metadata['cloudformation_s3_url_full'] = config['options']['cloudformation_s3_url'] + \
600+
'/{}/cloudformation'.format(metadata['reproducible_artifact_path'])
601+
602+
metadata['azure_download_url'] = get_azure_download_url(config)
557603

558604

559605
def call_matching_arguments(function, arguments, allow_unused=False):
@@ -710,7 +756,8 @@ def promote(self, src_channel, destination_repository, destination_channel):
710756
self.fetch_key_artifacts(metadata)
711757

712758
repository = Repository(destination_repository, destination_channel, 'commit/{}'.format(metadata['commit']))
713-
set_repository_metadata(repository, metadata, self.__storage_providers, self.__preferred_provider)
759+
set_repository_metadata(
760+
repository, metadata, self.__storage_providers, self.__preferred_provider, self.__config)
714761
assert 'tag' in metadata
715762
del metadata['channel_artifacts']
716763

@@ -757,7 +804,8 @@ def create(self, repository_path, channel, tag):
757804
assert bootstrap_active_packages <= set(info['packages'])
758805

759806
repository = Repository(repository_path, channel, 'commit/{}'.format(metadata['commit']))
760-
set_repository_metadata(repository, metadata, self.__storage_providers, self.__preferred_provider)
807+
set_repository_metadata(
808+
repository, metadata, self.__storage_providers, self.__preferred_provider, self.__config)
761809
metadata['tag'] = tag
762810
assert 'channel_artifacts' not in metadata
763811

@@ -834,8 +882,8 @@ def main():
834882
sys.exit(1)
835883

836884
try:
837-
# TODO(cmaloney): HACK. This is so we can get to the config for aws
838-
# inside gen/build_deploy/aws.py
885+
# TODO(cmaloney): HACK. This is so we can get to the config for aws and azure template
886+
# testing inside gen/build_deploy/{aws,azure}.py
839887
global _config
840888
config = load_config(options.config)
841889
_config = config

release/test_release.py

+3-3
Original file line numberDiff line numberDiff line change
@@ -599,8 +599,6 @@ def mock_make_tar(result_filename, folder):
599599
def test_make_channel_artifacts(monkeypatch):
600600
logging.basicConfig(level=logging.DEBUG)
601601
monkeypatch.setattr('gen.build_deploy.bash.make_installer_docker', mock_make_installer_docker)
602-
monkeypatch.setattr('gen.build_deploy.aws.get_cloudformation_s3_url', mock_get_cf_s3_url)
603-
monkeypatch.setattr('gen.build_deploy.azure.get_download_url', mock_get_azure_download_url)
604602
monkeypatch.setattr('pkgpanda.util.make_tar.__code__', mock_make_tar.__code__)
605603

606604
metadata = {
@@ -651,7 +649,9 @@ def test_make_channel_artifacts(monkeypatch):
651649
'aws': 'https://aws.example.com/',
652650
'azure': 'https://azure.example.com/'
653651
},
654-
'repository_url': 'https://aws.example.com/r_path'
652+
'repository_url': 'https://aws.example.com/r_path',
653+
'cloudformation_s3_url_full': 'https://s3.foobar.com/biz/r_path/channel/commit/sha-1',
654+
'azure_download_url': 'https://azure.example.com'
655655
}
656656

657657
channel_artifacts = release.make_channel_artifacts(metadata)

0 commit comments

Comments
 (0)