CFE-4322: Changed cf-remote spawn with AWS to query for AMI from known owners #85

craigcomstock · 2024-04-01T17:28:05Z

Before we hard-coded AMI and had to update them fairly often and add them when we supported a new platform.

Here we change to querying for AMI based on known trusted owner IDs.

This should make it so that new platforms are automatically available and any updates will be in-place automatically as well.

Ticket: CFE-4322
Changelog: title

craigcomstock · 2024-04-01T17:39:54Z

cf_remote/commands.py

-    print("Available platforms:")
-    for key in sorted(cloud_data.aws_platforms.keys()):
+    print("Available platforms (image criteria):")
+    for key in sorted(cloud_data.aws_image_criteria.keys()):


list platforms isn't quite descriptive anymore as we don't have an explicit list for AWS (I don't know anything (yet) about GCP).

new output of cf-remote spawn --list-platforms

Available platforms (image criteria): centos debian debian-9 macos rhel suse ubuntu ubuntu-16 windows windows-2008 windows-2012 windows-2016 windows-2019

Old output:

$ cf-remote spawn --list-platforms Available platforms: centos-5-x32 centos-5-x64 centos-6-x64 centos-7-x64 debian-10-x64 debian-11-arm64 debian-11-x64 debian-12-x64 debian-4-x32 debian-4-x64 debian-5-x32 debian-5-x64 debian-6-x32 debian-6-x64 debian-7-x32 debian-7-x64 debian-8-x64 debian-9-x64 rhel-5-x64 rhel-6-x64 rhel-7-x64 rhel-8-x64 rhel-9-x64 ubuntu-12-04-x32 ubuntu-12-04-x64 ubuntu-14-04-x32 ubuntu-14-04-x64 ubuntu-16-04-x64 ubuntu-18-04-x64 ubuntu-20-04-x64 ubuntu-22-04-arm64 ubuntu-22-04-x64 windows-2012-x64 windows-2016-x64 windows-2019-x64

Yes, I tested several of the older platforms and pretty sure their AMIs are dead. But I will double-check those today.

adjusted slightly for more information since we don't explicitly have a list anymore to describe the format of the "platform" argument.

Platform images are queried based on the platform name, version and architecture. The form of platform specified is: <platform_name>[-<version>][-<architecture>]. e.g. debian, debian-12 or debian-12-x64 Ubuntu version can be just major (20) or major+minor (20-04) Architecture can either be x64 or arm64 Available platforms: centos debian debian-9 rhel suse ubuntu ubuntu-16 windows windows-2008 windows-2012 windows-2016 windows-2019

craigcomstock · 2024-04-01T19:10:24Z

working on some nice debug logging from my development print statements:

[INFO] Set the log level to DEBUG
Spawning VMs...[DEBUG] Looking for AWS AMI for platform_name 'debian'
[DEBUG] Parsed platform_version '' from platform_name 'debian'
[DEBUG] Determined architecture to be 'x86_64'
[DEBUG] Determined image criteria: {'owner_id': '136693071363', 'name_pattern': 'debian-{version}*', 'user': 'admin', 'architecture': 'x86_64', 'version': ''}
[DEBUG] Selected image <NodeImage: id=ami-0fa1b1734dd573e80, name=debian-11-backports-amd64-20240211-1654, driver=Amazon EC2  ...>
Spawning new platform 'debian' VM in AWS (AMI: ami-0fa1b1734dd573e80, size=t2.xlarge) False
.DONE
[DEBUG] Directory already exists: '/Users/craig/.cfengine/cf-remote'
Details about the spawned VMs can be found in /Users/craig/.cfengine/cf-remote/cloud_state.json
Destroying all hosts

craigcomstock · 2024-04-01T19:25:09Z

Need to clean up commits and add some more docs/changes notices like:

with this change you can configure a different region other than eu-west-1 and cf-remote will most likely "just work", only tested in us-east-2
some platforms, like windows, are locked down to eu-west-1 currently due to our custom images we use with SSH server installed/configured

craigcomstock · 2024-04-01T19:28:59Z

An example of "non default region" just working with windows:

nothing: cf-remote --log-level=debug spawn --count 1 --platform windows --role client --name test
[INFO] Set the log level to DEBUG
Spawning VMs...[DEBUG] Looking for AWS AMI for platform_name 'windows'
[DEBUG] Parsed platform_version '*' from platform_name 'windows'
[DEBUG] Determined architecture to be 'x86_64'
[DEBUG] Determined image criteria: {'meta': 'Note that typically we rely on custom pre-configured windows imimages with ssh installed and pre-populated public keys so an image spawned from this criteria will not come with ssh built-in and ready to go.', 'owner_id': '801119661308', 'name_pattern': 'Windows_Server-{version}-English-Core-Base*', 'user': 'Administrator', 'architecture': 'x86_64', 'version': '*'}
[DEBUG] Selected image <NodeImage: id=ami-0a43a3af91e0c8d41, name=Windows_Server-2022-English-Core-Base-2024.03.13, driver=Amazon EC2  ...>
Spawning new platform 'windows' VM in AWS (AMI: ami-0a43a3af91e0c8d41, size=t2.micro) False
.DONE

nickanderson

Looking forward to it. I probably shouldn't be the one to judge the code though.

craigcomstock · 2024-04-01T19:40:07Z

commits squashed, fully ready for review :)

craigcomstock · 2024-04-01T19:46:21Z

Another thing this change is nice for while we still use explicit AMIs elsewhere is to find the latest AMI for a platform:

cf-remote spawn --platform ubuntu-20-04-x64 --count 1 --role client --name test
Spawning VMs...Spawning new platform 'ubuntu-20-04-x64' VM in AWS (AMI: ami-005c1d2266900dc31, size=t2.micro)
.DONE
Waiting for VMs to get IP addresses...DONE
Details about the spawned VMs can be found in /Users/craig/.cfengine/cf-remote/cloud_state.json

larsewi

This will be a nice improvement 🚀

cf_remote/spawn.py

tests/aws-spawn-test.sh

nickanderson · 2024-04-02T15:46:56Z

@craigcomstock probably demo.sh in core should be updated with the release of this change: https://github.com/cfengine/core/blob/f47517a53ec5e84bfabb28761233453798b93b1b/docs/cf_remote_demo.sh

tests/test_spawn.py

Before we hard-coded AMI and had to update them fairly often and add them when we supported a new platform. Here we change to querying for AMI based on known trusted owner IDs. This should make it so that new platforms are automatically available and any updates will be in-place automatically as well. This change should also enable users to specify other regions and have most tplatforms "just work". One note: specific Windows AMI are only available in eu-west-1 Ticket: CFE-4322 Changelog: title Co-authored-by: Lars Erik Wik <[email protected]>

craigcomstock · 2024-04-02T16:20:25Z

@craigcomstock probably demo.sh in core should be updated with the release of this change: https://github.com/cfengine/core/blob/f47517a53ec5e84bfabb28761233453798b93b1b/docs/cf_remote_demo.sh

spawning worked fine for me but... centos-7 ssh didn't work so will check that out separately, shouldn't be an issue with cf-remote I don't think.

craigcomstock · 2024-04-02T16:27:33Z

@craigcomstock probably demo.sh in core should be updated with the release of this change: https://github.com/cfengine/core/blob/f47517a53ec5e84bfabb28761233453798b93b1b/docs/cf_remote_demo.sh

spawning worked fine for me but... centos-7 ssh didn't work so will check that out separately, shouldn't be an issue with cf-remote I don't think.

Yep, a problem on my end, probably keys. I rotated my keys in AWS and all was fine. centos-7 did take some TIME to be ready. 🤔

vpodzime

Late to the party, but still a couple code-quality comments.

vpodzime · 2024-04-03T09:39:03Z

cf_remote/spawn.py

+            platform_version = ""
+    else:
+        platform_version = (
+            platform_name.count("-") > 0 and platform_name.split("-")[1] or "*"


I'd prefer using value1 if condition else value2 here instead of relying on logical operators.

vpodzime · 2024-04-03T09:39:56Z

cf_remote/spawn.py

+# image for that platform and version.
+def _get_image_criteria(platform_name):
+    log.debug("Looking for AWS AMI for platform_name '%s'" % (platform_name))
+    platform = platform_name.split("-")[0]


Please add an example platform_name string here. Otherwise the code below is quite confusing.

Also, looks like platform_name should be .split("-") and then the individual components should be "named" (assigned to properly named variables).

vpodzime · 2024-04-03T09:41:34Z

cf_remote/spawn.py

+    platform_with_major_version = "-".join(platform_name.split("-")[0:2])
+    architecture = platform_name.split("-")[-1]
+    # architecture should be either x64 or arm64
+    if not (architecture == "x64" or architecture == "arm64"):


architecture not in ("x64", "arm64")

vpodzime · 2024-04-03T09:42:21Z

cf_remote/spawn.py

+        ex_filters={
+            "name": criteria["name_pattern"].format(version=criteria["version"]),
+            "architecture": criteria["architecture"],
+            "virtualization-type": "hvm",


Is the virtualization-type really needed? Why should we care?

vpodzime · 2024-04-03T09:43:13Z

cf_remote/spawn.py

+        raise ValueError("No images found for criteria: %s" % (criteria))
+    selected = sorted(candidates, key=lambda x: x.extra["creation_date"], reverse=True)[
+        0
+    ]


WTH is this formatting about?

vpodzime · 2024-04-03T09:44:13Z

cf_remote/spawn.py

-        },
+    criteria = _get_image_criteria(platform)
+    architecture = criteria["architecture"] or aws_defaults["architecture"]
+    sizes = criteria.get("sizes") or aws_defaults["sizes"]


.get("sizes", aws_defaults["sizes"]). Or could the former be and empty string/list/...?

vpodzime · 2024-04-03T09:44:21Z

cf_remote/spawn.py

+    sizes = criteria.get("sizes") or aws_defaults["sizes"]
+    small = sizes[architecture]["size"]
+    large = sizes[architecture]["xlsize"]
+    if size == None:


size is None

vpodzime · 2024-04-03T09:44:42Z

cf_remote/spawn.py

+    criteria = _get_image_criteria(platform)
+    architecture = criteria["architecture"] or aws_defaults["architecture"]
+    sizes = criteria.get("sizes") or aws_defaults["sizes"]
+    small = sizes[architecture]["size"]


.get("size")?

vpodzime · 2024-04-03T09:45:05Z

cf_remote/spawn.py

+    small = sizes[architecture]["size"]
+    large = sizes[architecture]["xlsize"]
+    if size == None:
+        size = (large or small) if (role == "hub") else (small or large)


Nice! 😁 👍

vpodzime · 2024-04-03T09:45:46Z

tests/aws-spawn-test.sh

+trap cleanup EXIT
+
+# this is a fairly exhaustive test and will take some time
+# spawn all reasonable "platform" specifications


And it's also not very cheap. 😉

craigcomstock marked this pull request as draft April 1, 2024 17:28

craigcomstock commented Apr 1, 2024

View reviewed changes

craigcomstock marked this pull request as ready for review April 1, 2024 19:21

craigcomstock requested review from larsewi and nickanderson April 1, 2024 19:21

nickanderson reviewed Apr 1, 2024

View reviewed changes

craigcomstock force-pushed the CFE-4322 branch from fbbe20b to 1ee5324 Compare April 1, 2024 19:39

craigcomstock force-pushed the CFE-4322 branch from 1ee5324 to 9f92a80 Compare April 1, 2024 19:47

larsewi requested changes Apr 2, 2024

View reviewed changes

larsewi requested a review from olehermanse April 2, 2024 13:33

craigcomstock force-pushed the CFE-4322 branch 2 times, most recently from a1ebef9 to 44dd56a Compare April 2, 2024 14:50

craigcomstock requested review from nickanderson and larsewi April 2, 2024 14:50

craigcomstock force-pushed the CFE-4322 branch from 44dd56a to 6e43a2e Compare April 2, 2024 15:38

larsewi reviewed Apr 2, 2024

View reviewed changes

tests/test_spawn.py Outdated Show resolved Hide resolved

craigcomstock force-pushed the CFE-4322 branch from fd3b0f4 to 261a5dd Compare April 2, 2024 16:04

larsewi approved these changes Apr 2, 2024

View reviewed changes

craigcomstock merged commit 734973d into cfengine:master Apr 2, 2024
5 checks passed

craigcomstock deleted the CFE-4322 branch April 2, 2024 16:27

vpodzime reviewed Apr 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CFE-4322: Changed cf-remote spawn with AWS to query for AMI from known owners #85

CFE-4322: Changed cf-remote spawn with AWS to query for AMI from known owners #85

craigcomstock commented Apr 1, 2024

craigcomstock Apr 1, 2024 •

edited

Loading

nickanderson Apr 1, 2024

craigcomstock Apr 2, 2024

craigcomstock Apr 2, 2024

craigcomstock commented Apr 1, 2024 •

edited

Loading

craigcomstock commented Apr 1, 2024

craigcomstock commented Apr 1, 2024

nickanderson left a comment

craigcomstock commented Apr 1, 2024

craigcomstock commented Apr 1, 2024

larsewi left a comment

nickanderson commented Apr 2, 2024

craigcomstock commented Apr 2, 2024

craigcomstock commented Apr 2, 2024

vpodzime left a comment

vpodzime Apr 3, 2024

vpodzime Apr 3, 2024

vpodzime Apr 3, 2024

vpodzime Apr 3, 2024

vpodzime Apr 3, 2024

vpodzime Apr 3, 2024

vpodzime Apr 3, 2024

vpodzime Apr 3, 2024

vpodzime Apr 3, 2024

vpodzime Apr 3, 2024

vpodzime Apr 3, 2024

CFE-4322: Changed cf-remote spawn with AWS to query for AMI from known owners #85

CFE-4322: Changed cf-remote spawn with AWS to query for AMI from known owners #85

Conversation

craigcomstock commented Apr 1, 2024

craigcomstock Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigcomstock commented Apr 1, 2024 • edited Loading

craigcomstock commented Apr 1, 2024

craigcomstock commented Apr 1, 2024

nickanderson left a comment

Choose a reason for hiding this comment

craigcomstock commented Apr 1, 2024

craigcomstock commented Apr 1, 2024

larsewi left a comment

Choose a reason for hiding this comment

nickanderson commented Apr 2, 2024

craigcomstock commented Apr 2, 2024

craigcomstock commented Apr 2, 2024

vpodzime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigcomstock Apr 1, 2024 •

edited

Loading

craigcomstock commented Apr 1, 2024 •

edited

Loading