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

Allow key configuration for [ip6]gre[tap] tunnels (LP: #1966476) #274

Merged
merged 4 commits into from
Jul 12, 2022

Conversation

slyon
Copy link
Collaborator

@slyon slyon commented Apr 29, 2022

Description

The following example should configure a GRE tunnel between 172.16.4.2 and 172.16.1.2, with key 33490175. The effect should be like manually configured with: sudo ip tunnel add test1 mode gre local 172.16.4.2 remote 172.16.1.2 key 33490175.

However, the following Netplan configuration fails under Ubuntu 22.04 (latest development version, March 25, 2022):

network:
  version: 2
  renderer: networkd
  tunnels:
    gre1-255-255:
      mode: gre
      ttl: 255
      local: 172.16.4.2
      remote: 172.16.1.2
      key: 33490175
      addresses:
        - 172.20.82.94/30

Result of "sudo netplan generate": Error in network definition: gre1-255-255: 'input-key' is not required for this tunnel type

Some investigation into the sources of Netplan (src/validation.c -> https://github.com/canonical/netplan/blob/main/src/validation.c):
It seems that the cases NETPLAN_TUNNEL_MODE_GRE and NETPLAN_TUNNEL_MODE_IP6GRE are missing for systemd-networkd. According to systemd/systemd#12144, systemd-networkd should support configuration with keys since ca. 2 years.

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad. LP#1966476

@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2022

Codecov Report

Merging #274 (30f9572) into main (6a47c70) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head 30f9572 differs from pull request most recent head 9e091de. Consider uploading reports for the commit 9e091de to get more accurate results

@@            Coverage Diff             @@
##             main     #274      +/-   ##
==========================================
- Coverage   99.19%   99.07%   -0.12%     
==========================================
  Files          61       61              
  Lines       11206    10862     -344     
==========================================
- Hits        11116    10762     -354     
- Misses         90      100      +10     
Impacted Files Coverage Δ
src/validation.c 99.61% <ø> (ø)
tests/generator/test_tunnels.py 100.00% <100.00%> (ø)
src/abi_compat.c 83.33% <0.00%> (-16.67%) ⬇️
src/parse.c 99.82% <0.00%> (-0.02%) ⬇️
src/nm.c 100.00% <0.00%> (ø)
src/dbus.c 100.00% <0.00%> (ø)
src/util.c 100.00% <0.00%> (ø)
src/error.c 100.00% <0.00%> (ø)
src/types.c 100.00% <0.00%> (ø)
src/netplan.c 100.00% <0.00%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d4deb6...9e091de. Read the comment docs.

@dreibh
Copy link

dreibh commented May 19, 2022

The solution is working successfully for configuring a GRE tunnel with key.

@slyon
Copy link
Collaborator Author

slyon commented May 23, 2022

Thank you very much for the feedback/confirmation. I still need to find some time to polish up this PR (maybe add an integration tests) and get it landed.

@slyon slyon force-pushed the slyon/tunnel-key-ip6gre-lp1966476 branch from 30f9572 to 0046f0a Compare June 15, 2022 09:48
@slyon slyon marked this pull request as ready for review June 15, 2022 09:48
@slyon
Copy link
Collaborator Author

slyon commented Jun 15, 2022

I cleaned this PR up, added some integration tests (autopkgtest) and rebased on top of the current origin/main branch.
Thans again for testing @dreibh! Would you mind doing a code review as well?

@slyon slyon requested a review from schopin-pro July 4, 2022 08:40
Copy link
Contributor

@schopin-pro schopin-pro left a comment

Choose a reason for hiding this comment

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

LGTM :)

Small nitpick: adding the tests in a commit before adding the implementation means that you'll have a broken test suite in your history. This triggers my inner Git neat freak ;-)

@slyon slyon force-pushed the slyon/tunnel-key-ip6gre-lp1966476 branch from 243830a to 9e091de Compare July 12, 2022 12:49
@slyon
Copy link
Collaborator Author

slyon commented Jul 12, 2022

Thanks for the review! I rebased on top of origin/master (and re-ordered the git history while on it, to satisfy Simon's inner Git neat freak). Merging.

@slyon slyon merged commit f3b9f7f into main Jul 12, 2022
@slyon slyon deleted the slyon/tunnel-key-ip6gre-lp1966476 branch July 12, 2022 13:23
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