Disable ec2 mirror for non aws instances by lucasmoura · Pull Request #390 · canonical/cloud-init

lucasmoura · 2020-05-25T20:45:58Z

For versions before 20.2, we allowed the use of ec2 mirrors if the datasource availability_zone matches one of the ec2 regions. We are now updating that behavior to allow allow the use of ec2 mirrors on ec2 instances or if the user directly passes an an ec2 mirror url through #cloud-config apt directives.

LP: #1456277

blackboxsw

I think we may need to revisit this and have a discussion.

I'm actually not certain that this bug is valid anymore.

the %(ec2_region)s mirror lines should have failed to be included on any cloud platform unless their region names happened to be something like eu-west-[0-9][a-z]

because we would have raised a KeyError (and continued instead of appending)

That said, I think we can still cleanup the platform_type matching using your platform_type == "ec2" and then provide such ec2_region and region keys.
One other thing I'm thinking about is that ubuntu cloudimage for azure adds the following custom config
primary:
- http://azure.archive.ubuntu.com/ubuntu/

so I wonder if it's worth considering adding a %(platform)s.archive.ubuntu.com as well as adapting the original %(ec2_region)s.%(platform)s.archive.ubuntu,com

blackboxsw · 2020-06-08T21:54:21Z

        # list of mirrors to try to resolve
-        mirror = util.search_for_mirror(mcfg.get("search", None))
+        search_mirrors = None
+        params = {'REGION': cloud.datasource.region}


I think I'd like to reject this 'feature' to provide REGION magic template string in user cloud-config. I'd like to support one-way to do things, and that's with jinja templates which already provide this feature. We should instead add a documentation example to this json schema definition that shows how to do this

Without a magic REGION template string , a user can override this on their instance by providing the following cloud-config:

#template: jinja #cloud-config apt: primary: - arches: [default] search: - http://{{ region }}.ec2.archive.ubuntu.com/ubuntu/

blackboxsw · 2020-06-08T22:31:01Z

+            if (ALLOW_EC2_MIRRORS_ON_NON_AWS_INSTANCE_TYPES
+                    and data_source.platform_type == "ec2"):
+                subst['ec2_region'] = "%s" % ec2_region
+            elif not ALLOW_EC2_MIRRORS_ON_NON_AWS_INSTANCE_TYPES:


This elif is incorrect isn't it? We only want to set subst[ec2_region'] if any([ data_source.platform_type == "ec2", ALLOW_EC2_MIRRORS_ON_NON_AWS_INSTANCE_TYPES] ?

Yes, it is incorrect 🤦 I will update the logic to fix this

blackboxsw · 2020-06-08T22:32:24Z

@@ -845,7 +848,13 @@ def _get_package_mirror_info(mirror_info, data_source=None,
        # ec2 availability zones are named cc-direction-[0-9][a-d] (us-east-1b)
        # the region is us-east-1. so region = az[0:-1]
        if _EC2_AZ_RE.match(data_source.availability_zone):


I think maybe we can just drop this re.match and instead just specificall check data_source.platform_type == "ec2".

But wouldn't that interfere with past release ? We would like to have past releases still being able to user ec2 mirrors, right ?

You are right, darn backward compatibility.

github-actions · 2020-06-23T00:12:07Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging powersj, and he will ensure that someone takes a look soon.

(If the pull request is closed, please do feel free to reopen it if you wish to continue working on it.)

lucasmoura · 2020-06-26T18:28:35Z

@blackboxsw I have update the PR with the suggested changes. There is just some things that I have some doubts about:

I have verified that when we use a jinja template file as you stated, we get the parsed version on the apt_configure module when we start evaluating the mirrors. But where do you think we should put an example of this change ? Maybe we can add it as an example on the apt_configure module schema ?
I know this is not related to this PR, but just to make sure, we we call this method do we parse the user-data and apply it at the same time or am I missing something ?
I have not yet addressed the distro url in this update, but I do think this is a good idea. Shall I add this to this PR or another one ?

blackboxsw

Thank @lucasmoura minor text update request (or something similar to the suggestion). Also can you please update the description of the PR to the commit message you would like to see when we squash merge this branch?

blackboxsw · 2020-06-30T17:35:28Z

@@ -845,7 +848,13 @@ def _get_package_mirror_info(mirror_info, data_source=None,
        # ec2 availability zones are named cc-direction-[0-9][a-d] (us-east-1b)
        # the region is us-east-1. so region = az[0:-1]
        if _EC2_AZ_RE.match(data_source.availability_zone):


You are right, darn backward compatibility.

blackboxsw · 2020-06-30T17:49:47Z

+the use of ec2 mirrors if the user region matches one
+of the possible aws regions. We are now updating that to only
+the use of ec2 mirror for aws instances, unless the user specifically
+sets that on the userdata config.


Suggested change

the use of ec2 mirrors if the user region matches one

of the possible aws regions. We are now updating that to only

the use of ec2 mirror for aws instances, unless the user specifically

sets that on the userdata config.

the use of ec2 mirrors if the datasource availability_zone format

matches one of the possible aws ec2 regions. After the 20.2 release, we

no longer publish ec2 region mirror urls on non-AWS cloud platforms.

Besides feature_overrides.py, users can override this by providing

#cloud-config apt directives.

lucasmoura · 2020-06-30T19:21:48Z

@blackboxsw Done, I have updated both descriptions

blackboxsw

Thanks @lucasmoura

blackboxsw self-assigned this May 26, 2020

blackboxsw requested changes Jun 8, 2020

View reviewed changes

github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Jun 23, 2020

lucasmoura force-pushed the ec2-mirror branch 2 times, most recently from 6d59994 to 0cf79cd Compare June 26, 2020 18:10

blackboxsw removed the stale-pr Pull request is stale; will be auto-closed soon label Jun 30, 2020

blackboxsw requested changes Jun 30, 2020

View reviewed changes

lucasmoura force-pushed the ec2-mirror branch from 0cf79cd to 72de439 Compare June 30, 2020 19:17

lucasmoura requested a review from blackboxsw June 30, 2020 19:21

Lucas Moura added 4 commits June 30, 2020 16:22

Create feature flag to allow ec2 mirrors

ad07554

Allow mirrors to be rendered in apt_configure

b547c6e

Update ec2 mirror parsing

44e65a4

Update aws ec2 feature flag description

33eb435

lucasmoura force-pushed the ec2-mirror branch from 72de439 to 33eb435 Compare June 30, 2020 19:22

blackboxsw approved these changes Jun 30, 2020

View reviewed changes

Merge branch 'master' into ec2-mirror

4d9a1d6

blackboxsw merged commit 3fcdacc into canonical:master Jun 30, 2020

raharper mentioned this pull request Jul 10, 2020

Ubuntu/devel #486

Merged

This was referenced May 10, 2023

cloud-init searches for ec2 mirrors regardless of what cloud its on #2549

Closed

#include fails silently. #3069

Closed

cloud-init status broken in groovy lxd containers #3758

Closed

Release 20.3 #3774

Closed

-the use of ec2 mirrors if the user region matches one
-of the possible aws regions. We are now updating that to only
-the use of ec2 mirror for aws instances, unless the user specifically
-sets that on the userdata config.
+the use of ec2 mirrors if the datasource availability_zone format
+matches one of the possible aws ec2 regions. After the 20.2 release, we
+no longer publish ec2 region mirror urls on non-AWS cloud platforms.
+Besides feature_overrides.py, users can override this by providing
+#cloud-config apt directives.

Conversation

lucasmoura commented May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 23, 2020

Uh oh!

lucasmoura commented Jun 26, 2020

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasmoura commented Jun 30, 2020

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lucasmoura commented May 25, 2020 •

edited

Loading