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

Extend virt.network_define to create NAT networks #54197

Merged
merged 1 commit into from
Jan 2, 2020

Conversation

cbosdo
Copy link
Contributor

@cbosdo cbosdo commented Aug 13, 2019

What does this PR do?

virt.network_define can already define NAT virtual networks by setting the forward attribute to nat... however libvirt fails because it needs an IP configuration in such a case but virt.network_define doesn't have parameters for it.

The PR adds the missing IP configuration to both virt.network_define module and virt.network_running state to allow properly setting up NAT virtual networks.

What issues does this PR fix or reference?

Previous Behavior

Applying the following state fails with XML error: nat forwarding requested, but no IP address provided for network 'net1' error.

net0:
  virt.network_running:
    - name: net0
    - bridge: net0-br
    - forward: nat
    - autostart: True

New Behavior

Changing the state to the following creates a proper network.

net0:
  virt.network_running:
    - name: net0
    - bridge: net0-br
    - forward: nat
    - ipv4_config:
        cidr: 192.168.44.0/24
        dhcp_ranges:
          - start: 192.168.44.10
            end: 192.168.44.25
          - start: 192.168.44.110
            end: 192.168.44.125
    - autostart: True

The dhcp_ranges part is not strictly required for the NAT network creation and are thus optional, but this is so commonly used that I added them in this PR.

Tests written?

Yes

Commits signed with GPG?

Yes

@cbosdo cbosdo requested a review from a team as a code owner August 13, 2019 14:27
@ghost ghost requested a review from xeacott August 13, 2019 14:27
@cbosdo
Copy link
Contributor Author

cbosdo commented Aug 13, 2019

May be that PR could also be submitted to 2019.2.1 since this typically a bug fix... even though it adds some parameters to the API.

Copy link
Contributor

@xeacott xeacott left a comment

Choose a reason for hiding this comment

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

Looks wonderful @cbosdo thank you for getting this together. Left a comment that should get the rest of the checks passing!

salt/modules/virt.py Outdated Show resolved Hide resolved
@cbosdo
Copy link
Contributor Author

cbosdo commented Aug 14, 2019

for some weird reason, the tests are still showing as not completed after a few hours...

@waynew
Copy link
Contributor

waynew commented Aug 14, 2019

@cbosdo I think that's due to some of the work we're doing on stabilizing our test suite. You can read https://groups.google.com/forum/#!topic/salt-announce/5RJjtDYszx8 for more info, but I'm pretty sure that we temporarily turned off our PR builds last night, operating under a noisy neighbor sort of theory.

Please be patient while we work out the kinks, but also feel free to ping regularly if you don't see any updates happening!

@cbosdo
Copy link
Contributor Author

cbosdo commented Aug 14, 2019

@waynew Thanks for the update on that!

@waynew
Copy link
Contributor

waynew commented Aug 26, 2019

re-run full all

@waynew
Copy link
Contributor

waynew commented Aug 26, 2019

FWIW this looks good to me - I'm not familiar with virt/nat stuff, but I don't see anything otherwise in the code that would concern me!

@cbosdo cbosdo changed the base branch from neon to master October 30, 2019 15:31
@cbosdo
Copy link
Contributor Author

cbosdo commented Oct 30, 2019

Rebased on master

If using virt.network_define with nat network type, then libvirt
complains about missing IP configuration. Allow setting it in both the
virt.network_define module and the virt.network_running state.
@dwoz dwoz merged commit 4ec6117 into saltstack:master Jan 2, 2020
@cbosdo cbosdo deleted the virt-net-nat branch January 6, 2020 07:49
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