Skip to content

net: Fix static routes to host in eni renderer#668

Merged
TheRealFalcon merged 6 commits into
canonical:masterfrom
tnt-dev:fix-static-route-to-host-eni-render
Jan 13, 2021
Merged

net: Fix static routes to host in eni renderer#668
TheRealFalcon merged 6 commits into
canonical:masterfrom
tnt-dev:fix-static-route-to-host-eni-render

Conversation

@tnt-dev
Copy link
Copy Markdown
Contributor

@tnt-dev tnt-dev commented Nov 16, 2020

Route '-net' parameter is incompatible with /32 IPv4 networks without 'netmask' part so we have to use '-host' in that case.

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

Route '-net' parameter is incompatible with /32 IPv4 addresses so we
have to use '-host' in that case.
Copy link
Copy Markdown
Collaborator

@igalic igalic left a comment

Choose a reason for hiding this comment

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

is this equally broken for IPv6?

@tnt-dev
Copy link
Copy Markdown
Contributor Author

tnt-dev commented Nov 26, 2020

No, in IPv6 mode route works well without [-host|-net] part:

$ route -A inet6 add 2a00:1450:400e:80d::200e/128 gw $GW_IP
$ route -A inet6 add 2000::/3 gw 2a02:6ea0:c007::1337 gw $GW_IP

@github-actions
Copy link
Copy Markdown

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 mitechie, 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.)

@github-actions github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Dec 11, 2020
@igalic
Copy link
Copy Markdown
Collaborator

igalic commented Dec 11, 2020

@mitechie this doesn't seem to be on anyone's radar (but that's understandable, given the current SRU work)

@mitechie mitechie removed the stale-pr Pull request is stale; will be auto-closed soon label Dec 11, 2020
@mitechie
Copy link
Copy Markdown
Contributor

Thanks, it's on the board of things to review/get landed after the release. We'll get caught up on it. Thanks Mina!

@TheRealFalcon TheRealFalcon self-assigned this Jan 4, 2021
@TheRealFalcon
Copy link
Copy Markdown
Contributor

Hey @tnt-dev , thanks for the contribution!

I noticed you added yourself to the CLA signers file, but have you signed the actual CLA too? I'm not seeing your information there. If you did, can you provide the date that you signed it?

We have also recently introduced integration testing into cloud-init. As part of this PR, would you be able to add an integration test for this change? test_gh626.py also uses a network config and would probably be the best example to base your test on.

tnt-dev added a commit to tnt-dev/cloud-init that referenced this pull request Jan 11, 2021
@tnt-dev tnt-dev force-pushed the fix-static-route-to-host-eni-render branch from 487277e to a032850 Compare January 11, 2021 16:00
@tnt-dev
Copy link
Copy Markdown
Contributor Author

tnt-dev commented Jan 11, 2021

CLA is signed, integration test has been added.

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

This update looks good. Just two more minor comments inline and then I think it'll be good to land.

Comment thread tests/integration_tests/bugs/test_gh668.py Outdated
Comment thread tests/integration_tests/bugs/test_gh668.py
@tnt-dev
Copy link
Copy Markdown
Contributor Author

tnt-dev commented Jan 13, 2021

Yes, I have removed SRU pytest decorator and have added a short comment about the source of the original problem.

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

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.

4 participants