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

BUG, ENH, PERF: Debian/Ubuntu network configuration #12216

Closed
10 of 13 tasks
westurner opened this issue Apr 23, 2014 · 22 comments
Closed
10 of 13 tasks

BUG, ENH, PERF: Debian/Ubuntu network configuration #12216

westurner opened this issue Apr 23, 2014 · 22 comments
Labels
Feature new functionality including changes to functionality and code refactors, etc. stale
Milestone

Comments

@westurner
Copy link
Contributor

Initially: "debian_ip ifupdown /etc/network/interfaces options for tunnels, ipv6, and dns-search"

[EDIT]

Documentation

Ubuntu ifupdown Docs

Here are the (ubuntu) docs for /etc/network/interfaces (pkg: ifupdown).

Salt Docs

Sources

Tickets

I don't see any other tickets for e.g. tunnels, ipv6, or dns-search (as utilized by resolvconf) in debian_ip:


[EDIT]

BUG:

  • Unclosed file handles (-> file.open -> with salt.utils.flopen)
  • Don't write /etc/default/networking on Ubuntu >= 12.04
  • * /etc/resolv.conf is overwritten when resolvconf is installed (this may be a documentation issue)

PERF:

  • Simplify /etc/network/interfaces parser logic

TST:

  • Handle 'TODOs'
  • Write tests
  • Further (manual) testing on other platforms
    • I've run this manually on one platform a bunch of times.

ENH:

  • Add (rework/complete) debian ipv6 options
  • Add Adds dns-search option
  • Add wireless-* options
  • Add wpa-* options
  • Add basic seting validations

RLS:

@westurner
Copy link
Contributor Author

... @garethgreenaway et al: is this a unique issue?

@garethgreenaway
Copy link
Contributor

Sorry, not sure what the issue is.

@basepi
Copy link
Contributor

basepi commented Apr 24, 2014

@westurner I'm unsure of what the purpose of this issue is. You linked some docs. What's the feature/bug you're looking to get fixed?

@basepi basepi added this to the Blocked milestone Apr 24, 2014
@westurner
Copy link
Contributor Author

It is not possible to:

  • configure IP tunnels
  • configure IPV6
  • configure dns-search for resolvconf

https://github.com/westurner/salt/tree/debian_network_config

Wes Turner
On Apr 24, 2014 2:26 PM, "Colton Myers" notifications@github.com wrote:

@westurner https://github.com/westurner I'm unsure of what the purpose
of this issue is. You linked some docs. What's the feature/bug you're
looking to get fixed?


Reply to this email directly or view it on GitHubhttps://github.com//issues/12216#issuecomment-41321445
.

@westurner
Copy link
Contributor Author

[EDIT] BUG

Also, there are unclosed file handles

Wes Turner
On Apr 24, 2014 2:26 PM, "Colton Myers" notifications@github.com wrote:

@westurner https://github.com/westurner I'm unsure of what the purpose
of this issue is. You linked some docs. What's the feature/bug you're
looking to get fixed?


Reply to this email directly or view it on GitHubhttps://github.com//issues/12216#issuecomment-41321445
.

westurner added a commit to westurner/salt that referenced this issue Apr 25, 2014
@westurner
Copy link
Contributor Author

From [1], I understand that I can test this by copying my modified debian_ip.py module into /srv/salt/_modules and then calling saltutil.sync_modules with a master/minion setup.

  • Is there developer documentation on testing (changes to; new) modules?
  • Where do I copy this modified template?

[1] http://docs.saltstack.com/en/latest/ref/modules/#modules-are-easy-to-write

@westurner
Copy link
Contributor Author

https://github.com/westurner/salt/compare/debian_network_config

This:

  • Uses a with context manager to ensure that files are closed even in the event of an exception (try/except/finally file.close)
  • Updates the /etc/network/interfaces parser logic (I haven't looked at the ifupdown code, just the man interfaces docs)
  • Renames context -> method (as in the docs)
  • Renames inet_type to addrfam (as in the docs)
  • Uses some of the constants defined at the top of the file
  • Adds ipv6 options
  • Adds dns-search option
  • Adds wireless-* options
  • Adds wpa-* options
  • Adds basic seting validations
  • Has not been tested

About the validations:

  • Wrap salt.utils.validate.net where possible
  • Return (valid: True/False, value: casted/transformed value, errmsg)
  • Named e.g. __ipv4_quad pending a proper placement
  • Could be added to salt.utils.validate.???

@garethgreenaway
Copy link
Contributor

@westurner Some good additions :) I find it easier to run the installer from where ever you're code is, then run a master and a minion on the same host for testing. As you're testing, etc. run the installer and restart the minion.

@westurner
Copy link
Contributor Author

@westurner Some good additions :)

Thx!

I find it easier to run the installer from where ever you're code is, then run a master and a minion on the same host for testing. As you're testing, etc. run the installer and restart the minion.

  • As root?
  • I will look into automating these crucial tests

@garethgreenaway
Copy link
Contributor

Yup. both master and minion running as root, installer has to run as root because of file permissions.

@westurner
Copy link
Contributor Author

Got it. I have a packer/vagrant/virtualbox setup that will help with this.
I see the Jenkins setup and tests; is there an easier way with something
like salt-cloud and lxc?

Wes Turner
On Apr 25, 2014 12:16 AM, "garethgreenaway" notifications@github.com
wrote:

Yup. both master and minion running as root, installer has to run as root
because of file permissions.


Reply to this email directly or view it on GitHubhttps://github.com//issues/12216#issuecomment-41359613
.

@basepi basepi modified the milestones: Approved, Blocked Apr 25, 2014
@westurner
Copy link
Contributor Author

@garethgreenaway :

  • When you specify network.system:hostname as a FQDN, is /etc/hostname updated every time?
  • Does network_settings:network.system show a diff of a nonexistant file (/etc/default/networking) every time?

(With Ubuntu 12.04)

@westurner
Copy link
Contributor Author

... [why] does salt.modules.debian_ip.get_network_settings return a single template (when the file doesn't exist on disk)? [1]

[1]

def get_network_settings():

@westurner
Copy link
Contributor Author

resolvconf may or may not be installed on Debian systems.

If resolvconf is not installed, writes to /etc/resolv.conf will persist.

If resolvconf is installed (as it is by default w/ Ubuntu 12.04 (and earlier, IIRC)):

  • writes to /etc/resolv.conf are overwritten
  • /etc/resolv.conf is generated from:
    • scripts in /etc/resolvconf
    • dns-nameservers and dns-search settings in /etc/network/interfaces (and /etc/network/interfaces.d)
    • data pulled from a DHCP server with e.g. dhclient (cached in /var/lib/dhcp/dhclient.${IFNAME}.leases)

Debian & Ubuntu DNS configuration docs references:

@westurner westurner changed the title debian_ip ifupdown /etc/network/interfaces options for tunnels, ipv6, and dns-search Debian/Ubuntu network configuration: BUG, ENH, PERF Apr 26, 2014
westurner added a commit to westurner/salt that referenced this issue Apr 26, 2014
@bbinet
Copy link
Contributor

bbinet commented Jun 4, 2014

what is the status of this work?
these are useful additions: any chance it can be turned into a pull request to be merged?

@westurner
Copy link
Contributor Author

Unfortunately I've been sidetracked and haven't yet learned/written tests
for this sensitive functionality.

If someone has the time and/or resources to get this merged, by all means,
please do.

I've only recently learned about the _modules folder.

Wes Turner
On Jun 4, 2014 3:47 AM, "Bruno Binet" notifications@github.com wrote:

what is the status of this work?
these are useful additions: any chance it can be turned into a pull
request to be merged?


Reply to this email directly or view it on GitHub
#12216 (comment).

@garethgreenaway
Copy link
Contributor

Going to take a look at getting these changes merged in.

@bbinet
Copy link
Contributor

bbinet commented Jul 29, 2014

@garethgreenaway do you think the additions described in #12216 (comment) will be available in Helium release?

@garethgreenaway
Copy link
Contributor

@bbinet these changes are in 2014.7 (Helium) now.

@bbinet
Copy link
Contributor

bbinet commented Jul 29, 2014

Cool! Thanks @garethgreenaway.

@westurner
Copy link
Contributor Author

@garethgreenaway
Any suggestions on how to test the changes in westurner@f98ef01 ?

(There's an RFC at westurner@f98ef01#commitcomment-6131081 )

[EDIT]: I see: garethgreenaway@d7884d5

and https://github.com/saltstack/salt/blob/2014.7/salt/modules/debian_ip.py

Thanks again!

TODO:

  • issue desc < this list
  • Rebase
  • Don't write /etc/default/networking on Ubuntu >= 12.04
  • Handle 'TODOs'
  • Write tests
  • Further (manual) testing on other platforms

@westurner westurner changed the title Debian/Ubuntu network configuration: BUG, ENH, PERF BUG, ENH, PERF: Debian/Ubuntu network configuration Aug 20, 2014
@stale stale bot added the stale label Aug 24, 2017
@stale
Copy link

stale bot commented Aug 24, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

@stale stale bot closed this as completed Aug 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature new functionality including changes to functionality and code refactors, etc. stale
Projects
None yet
Development

No branches or pull requests

4 participants