cloud-init-dev team mailing list archive

[Dan Watkins <daniel.watkins@xxxxxxxxxxxxx>]

Overall, this looks good, thanks!  I have a few inline comments/questions, but the overall structure/logic seems sound to me.

(Looks like the CI failures are due to linting.)

Diff comments:

> diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
> index 5c017bf..aef5d80 100644
> --- a/cloudinit/sources/DataSourceEc2.py
> +++ b/cloudinit/sources/DataSourceEc2.py
> @@ -536,24 +536,35 @@ def convert_ec2_metadata_network_config(network_md, macs_to_nics=None,
>      @param: fallback_nic: Optionally provide the primary nic interface name.
>         This nic will be guaranteed to minimally have a dhcp4 configuration.
>  
> -    @return A dict of network config version 1 based on the metadata and macs.
> +    @return A dict of network config version 2 based on the metadata and macs.
>      """
> -    netcfg = {'version': 1, 'config': []}
> +    netcfg = {'version': 2, 'ethernets': {}}
>      if not macs_to_nics:
>          macs_to_nics = net.get_interfaces_by_mac()
>      macs_metadata = network_md['interfaces']['macs']
>      for mac, nic_name in macs_to_nics.items():
> +        dev_config = {}
>          nic_metadata = macs_metadata.get(mac)
>          if not nic_metadata:
>              continue  # Not a physical nic represented in metadata
> -        nic_cfg = {'type': 'physical', 'name': nic_name, 'subnets': []}
> -        nic_cfg['mac_address'] = mac
> +        local_ipv4s = nic_metadata.get('local-ipv4s')
>          if (nic_name == fallback_nic or nic_metadata.get('public-ipv4s') or
> -                nic_metadata.get('local-ipv4s')):
> -            nic_cfg['subnets'].append({'type': 'dhcp4'})
> +                local_ipv4s):
> +            dev_config['dhcp4'] = True
> +            if isinstance(local_ipv4s, list) and len(local_ipv4s) > 1:

What else might local_ipv4s be here?

> +                subnet_cidr = nic_metadata.get('subnet-ipv4-cidr-block')
> +                _ip, prefix = subnet_cidr.split('/')

subnet_cidr could be None here.

> +                for secondary_ip in local_ipv4s[1:]:
> +                    if dev_config.get('addresses') is None:

Can this move out of the for loop?

> +                        dev_config['addresses'] = []
> +                    dev_config['addresses'].append(
> +                        '{ip}/{prefix}'.format(ip=secondary_ip, prefix=prefix))
>          if nic_metadata.get('ipv6s'):
> -            nic_cfg['subnets'].append({'type': 'dhcp6'})
> -        netcfg['config'].append(nic_cfg)
> +            dev_config['dhcp6'] = True
> +        dev_config.update({
> +            'match': {'macaddress': nic_metadata.get('mac').lower()},

This will fail if 'mac' isn't present in nic_metadata.

> +            'set-name': nic_name})
> +        netcfg['ethernets'][nic_name] = dev_config
>      return netcfg
>  
>  
> diff --git a/tests/unittests/test_datasource/test_ec2.py b/tests/unittests/test_datasource/test_ec2.py
> index 20d59bf..c99f58f 100644
> --- a/tests/unittests/test_datasource/test_ec2.py
> +++ b/tests/unittests/test_datasource/test_ec2.py
> @@ -112,6 +112,94 @@ DEFAULT_METADATA = {
>      "services": {"domain": "amazonaws.com", "partition": "aws"},
>  }
>  
> +# collected from api version 2016-09-02/ with

Does this comment reference the wrong version?

> +# python3 -c 'import json
> +# from cloudinit.ec2_utils import get_instance_metadata as gm
> +# print(json.dumps(gm("2018-09-24"), indent=1, sort_keys=True))'
> +DEFAULT_METADATA_2018_09_24 = {

Is "DEFAULT_METADATA" accurate?  (I would expect the default metadata to only include a single private IP, but that may be a mistaken assumption!)

> +    "ami-id": "ami-0986c2ac728528ac2",
> +    "ami-launch-index": "0",
> +    "ami-manifest-path": "(unknown)",
> +    "block-device-mapping": {
> +        "ami": "/dev/sda1",
> +        "root": "/dev/sda1"
> +    },
> +    "events": {
> +        "maintenance": {
> +            "history": "[]",
> +            "scheduled": "[]"
> +        }
> +    },
> +    "hostname": "ip-172-31-44-13.us-east-2.compute.internal",
> +    "identity-credentials": {
> +        "ec2": {
> +            "info": {
> +                "AccountId": "329910648901",
> +                "Code": "Success",
> +                "LastUpdated": "2019-07-06T14:22:56Z"
> +            }
> +        }
> +    },
> +    "instance-action": "none",
> +    "instance-id": "i-069e01e8cc43732f8",
> +    "instance-type": "t2.micro",
> +    "local-hostname": "ip-172-31-44-13.us-east-2.compute.internal",
> +    "local-ipv4": "172.31.44.13",
> +    "mac": "0a:07:84:3d:6e:38",
> +    "metrics": {
> +        "vhostmd": "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
> +    },
> +    "network": {
> +        "interfaces": {
> +            "macs": {
> +                "0a:07:84:3d:6e:38": {
> +                    "device-number": "0",
> +                    "interface-id": "eni-0d6335689899ce9cc",
> +                    "ipv4-associations": {
> +                        "18.218.219.181": "172.31.44.13"
> +                    },
> +                    "local-hostname": ("ip-172-31-44-13.us-east-2."
> +                                       "compute.internal"),
> +                    "local-ipv4s": [
> +                        "172.31.44.13",
> +                        "172.31.45.70"
> +                    ],
> +                    "mac": "0a:07:84:3d:6e:38",
> +                    "owner-id": "329910648901",
> +                    "public-hostname": ("ec2-18-218-219-181.us-east-2."
> +                                        "compute.amazonaws.com"),
> +                    "public-ipv4s": "18.218.219.181",
> +                    "security-group-ids": "sg-0c387755222ba8d2e",
> +                    "security-groups": "launch-wizard-4",
> +                    "subnet-id": "subnet-9d7ba0d1",
> +                    "subnet-ipv4-cidr-block": "172.31.32.0/20",
> +                    "vpc-id": "vpc-a07f62c8",
> +                    "vpc-ipv4-cidr-block": "172.31.0.0/16",
> +                    "vpc-ipv4-cidr-blocks": "172.31.0.0/16"
> +                }
> +            }
> +        }
> +    },
> +    "placement": {
> +        "availability-zone": "us-east-2c"
> +    },
> +    "profile": "default-hvm",
> +       "public-hostname": ("ec2-18-218-219-181.us-east-2."
> +                           "compute.amazonaws.com"),
> +    "public-ipv4": "18.218.219.181",
> +    "public-keys": {
> +        "yourkeyname,e": [
> +            "ssh-rsa AAAAW...DZ yourkeyname"
> +        ]
> +    },
> +    "reservation-id": "r-09b4917135cdd33be",
> +    "security-groups": "launch-wizard-4",
> +    "services": {
> +        "domain": "amazonaws.com",
> +        "partition": "aws"
> +    }
> +}
> +
>  
>  def _register_ssh_keys(rfunc, base_url, keys_data):
>      """handle ssh key inconsistencies.


-- 
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/369792
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master.