Skip to content
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

fix(ec2): Ensure metadata exists before configuring PBR #5287

Merged
merged 2 commits into from
May 23, 2024

Conversation

TheRealFalcon
Copy link
Member

@TheRealFalcon TheRealFalcon commented May 13, 2024

Draft as currently untested.

Proposed Commit Message

fix(ec2): Ensure metadata exists before configuring PBR

Fixes GH-5283

Additional Context

#5283

Test Steps

I reproduced the issue on a multi-nic IPv6 only system, then created an AMI with this fix and launched using only the AMI as the difference and no longer see the exception and cannot connect as expected.

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

Copy link
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @TheRealFalcon, for tackling this one. The approach LGTM.

Comment on lines +982 to +988
if not (subnet_prefix_routes and ips):
LOG.debug(
"Not enough IMDS information to configure policy routing "
"for IPv%s",
"4" if is_ipv4 else "6",
)
return
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this is fairly straight-forward a fix. Can we add a test to assert we don't call dhcp_discovery when no subnet_ipv4_cidr_blocks are present in nic_metadata?

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing the network-config attached to an ipv6-only subnet, we can see the following config which shows the key subnet-ipv4-cidr-block is absent for those network interfaces:

   "network": {
    "interfaces": {
     "macs": {
      "02:6b:df:a2:4b:2b": {
       "device-number": "1",
       "interface-id": "eni-0669816d0cf6067c8",
       "ipv6s": "2600:1f16:67f:f201:8d2e:4d1f:9e80:4ab9",
       "local-hostname": "i-0951b6d0b66337a34.us-east-2.compute.internal",
       "mac": "02:6b:df:a2:4b:2b",
       "owner-id": "483410185795",
       "security-group-ids": "sg-0bf34e5c3cde1d8cb",
       "security-groups": "default",
       "subnet-id": "subnet-0903f279682c660fa",
       "subnet-ipv6-cidr-blocks": "2600:1f16:67f:f201:0:0:0:0/64",
       "vpc-id": "vpc-0ac1befb8c824a484",
       "vpc-ipv4-cidr-block": "192.168.0.0/20",
       "vpc-ipv4-cidr-blocks": "192.168.0.0/20",
       "vpc-ipv6-cidr-blocks": "2600:1f16:67f:f200:0:0:0:0/56"
      },
      "02:7c:03:b8:5c:af": {
       "device-number": "0",
       "interface-id": "eni-0f3cddb84c16e140a",
       "ipv6s": "2600:1f16:67f:f201:6613:29a2:dbf7:2f1f",
       "local-hostname": "i-0951b6d0b66337a34.us-east-2.compute.internal",
       "mac": "02:7c:03:b8:5c:af",
       "owner-id": "483410185795",
       "security-group-ids": "sg-0bf34e5c3cde1d8cb",
       "security-groups": "default",
       "subnet-id": "subnet-0903f279682c660fa",
       "subnet-ipv6-cidr-blocks": "2600:1f16:67f:f201:0:0:0:0/64",
       "vpc-id": "vpc-0ac1befb8c824a484",
       "vpc-ipv4-cidr-block": "192.168.0.0/20",
       "vpc-ipv4-cidr-blocks": "192.168.0.0/20",
       "vpc-ipv6-cidr-blocks": "2600:1f16:67f:f200:0:0:0:0/56"
      }
     }
    }
   },

I agree this changeset it correct and defensive in the face of single-stack ipv6 support and will avoid tracebacks as mentioned in the original issue. It'd be good to get a little unittest coverage of this case given that pycloudlib doesn't support simple ipv6-only vpc/subnet creation for us at the moment.

@TheRealFalcon
Copy link
Member Author

Unit test added

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the test coverage here and the network-config seen from an ipv6-only subnet to confirm we properly configure w/out traceback.

@TheRealFalcon TheRealFalcon merged commit 587ac9c into canonical:main May 23, 2024
27 of 29 checks passed
@TheRealFalcon TheRealFalcon deleted the fix-pbr branch May 23, 2024 20:30
TheRealFalcon added a commit to TheRealFalcon/cloud-init that referenced this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants