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

Salt enforces configuration from /etc/network/interfaces.d/* to /etc/network/interface #40262

Closed
pjediny opened this issue Mar 23, 2017 · 3 comments
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Milestone

Comments

@pjediny
Copy link

pjediny commented Mar 23, 2017

System: Ubuntu 16.04, saltstack 2016.3, but is present in current development branch see debian_ip.py#L1574

Salt reads configuration from /etc/network/interfaces and /etc/network/interfaces.d/* files and when writing configuration to the /etc/network/interfaces it includes interfaces in /etc/network/interfaces.d/* in the file.

This is dangerous behavior, as the files in interfaces.d directory are manually managed, can contain unknown options to the salt module and it can result in deconfigured interfaces in /etc/network/interface overriding any configuration in /etc/network/interfaces.d/*

pjediny pushed a commit to pjediny/salt-formula-linux that referenced this issue Mar 23, 2017
We need to save our manualy managed interfaces to different directory to
workaround salt bug that causes interface deconfiguration in
/etc/network/interfaces for interfaces manualy configured in
/etc/network/interfaces.d

Upstream-Bug: saltstack/salt#40262
@gtmanfred
Copy link
Contributor

Thanks for reporting this.

@garethgreenaway when you get a minute if you could take a look at this concern?

Also @MTecknology I know you have been messing around in debian_ip?

Thanks!
Daniel

Thanks,
Daniel

@gtmanfred gtmanfred added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Mar 23, 2017
@gtmanfred gtmanfred added this to the Blocked milestone Mar 23, 2017
@MTecknology
Copy link
Contributor

I recall, when I was working through the template output issues, this concern popped into my head. The situation I was working with had all of the networking configuration in the same file and it fell off the table.

I can think of a few reasons why someone would want most networking configuration in one file and have a handful of states that manage another networking config file through {file,network}.managed. The workaround confuses me, but the problem doesn't. I definitely agree that this can be dangerous (and potentially destructive).

Thought Dump

  • In states/network.managed:

    • Provide useful doc (Deb opts from modules/debian_ip @ 56-131,426-532 ..give or take)
    • Include a filename argument
    • It needs to pass the arg to ip.get_interface()
    • and ip.get_bond()? .. I'm not following that further
    • More?..
  • In modules/debian_ip:

    • _parse_interfaces() already supports passing a list of file names
    • _parse_interfaces() SHOULD try to get all interfaces
    • _write_file_ifaces() needs to accept the file arg read/write from that file (default /etc/net/if)
    • _write_file_ifaces() needs to pass the arg as a list to _parse_interfaces()

Yikes... that blows up in complexity pretty quickly when you start looking at it. It still seems doable, though. We just need to (by default) only care about /etc/network/interfaces and read no other interface files. If a separate filename is passed, then care only about that filename. When something wants to get all interfaces, that /should/ parse all files.

I haven't even had time to test a feature I already wrote (auto batch) so it's incredibly unlikely I'll be able to take a swing at this in the next ~6 months (or so...).

opnfv-github pushed a commit to opnfv/fuel that referenced this issue Dec 4, 2017
Make sure all missing interfaces/links are up & running
(e.g. br-ex <-> float-to-ex <-> br-floating).
Fix (for saltstack/salt#40262)
into linux formula brought in a weird behaviour with
network/interfaces.u/ items.

Change-Id: Ic13f0ed2063455ae191bbc99920f97c5ecaa61fd
Signed-off-by: Michael Polenchuk <mpolenchuk@mirantis.com>
opnfv-github pushed a commit to opnfv/opnfvdocs that referenced this issue Dec 4, 2017
* Update docs/submodules/fuel from branch 'stable/euphrates'
  - [baremetal] Restart gateway networking service
    
    Make sure all missing interfaces/links are up & running
    (e.g. br-ex <-> float-to-ex <-> br-floating).
    Fix (for saltstack/salt#40262)
    into linux formula brought in a weird behaviour with
    network/interfaces.u/ items.
    
    Change-Id: Ic13f0ed2063455ae191bbc99920f97c5ecaa61fd
    Signed-off-by: Michael Polenchuk <mpolenchuk@mirantis.com>
alexandruavadanii added a commit to alexandruavadanii/salt-formula-linux that referenced this issue Feb 25, 2018
Workaround for Upstream-Bug:
saltstack/salt#40262

Signed-off-by: Alexandru Avadanii <Alexandru.Avadanii@enea.com>
alexandruavadanii added a commit to alexandruavadanii/salt-formula-linux that referenced this issue Feb 25, 2018
Workaround for Upstream-Bug:
saltstack/salt#40262

Signed-off-by: Alexandru Avadanii <Alexandru.Avadanii@enea.com>
opnfv-github pushed a commit to opnfv/opnfvdocs that referenced this issue Feb 26, 2018
* Update docs/submodules/fuel from branch 'master'
  - [ovs/dpdk] Parameterize node-specific compute args
    
    - node-specific parameters (nova pinning, hugepages, dpdk) should be
      configurable via IDF, on a per-node basis;
    - keep default settings for lf-pod2, with and without DPDK,
      override them for virtual deploys via local-virtual1 IDF;
    - leave neutron_tenant_* vars hardcoded for now, as they are required
      on both ctl and cmp nodes - this way we'll deal stricly with
      cmp params, so we can nicely pass them via config.yml to reclass
      per-node (and not per-role), allowing mixed computes later;
    - add compute params for ovs/odl-noha, preparing them for
      deployment on baremetal later.
    
    JIRA: ARMBAND-343
    
    Change-Id: I89a58b9565679ab3882d85f07ae817690ae85c67
    Signed-off-by: Cristina Pauna <cristina.pauna@enea.com>
    
  - [ovs/dpdk] Add opnfv.route_wrapper sls
    
    - fix `route-br-ex` if-up.d script failing when route already exists
      by adding a wrapper around distro's '/sbin/route' binary in
      '/usr/local/sbin/route', exploiting default order in Ubuntu PATH;
    - fix 'br-prv' duplicate entry in 'interfaces.d/ifcfg-br-prv' and
      'interfaces' caused by upstream bug [1];
    - add barrier waiting for all baremetal nodes online before attempting
      reboot, trying to catch rare failures which are undetectable in logs
      as both a succesful reboot and a disconneted minion report 'n/c';
    
    With the above in place, networking service should no longer fail
    to start on cmp nodes w/ DPDK.
    
    [1] saltstack/salt#40262
    
    Change-Id: I6d4895376ce323c14c997e6c9af2ea3eeeee0184
    Signed-off-by: Alexandru Avadanii <Alexandru.Avadanii@enea.com>
opnfv-github pushed a commit to opnfv/fuel that referenced this issue Feb 26, 2018
- fix `route-br-ex` if-up.d script failing when route already exists
  by adding a wrapper around distro's '/sbin/route' binary in
  '/usr/local/sbin/route', exploiting default order in Ubuntu PATH;
- fix 'br-prv' duplicate entry in 'interfaces.d/ifcfg-br-prv' and
  'interfaces' caused by upstream bug [1];
- add barrier waiting for all baremetal nodes online before attempting
  reboot, trying to catch rare failures which are undetectable in logs
  as both a succesful reboot and a disconneted minion report 'n/c';

With the above in place, networking service should no longer fail
to start on cmp nodes w/ DPDK.

[1] saltstack/salt#40262

Change-Id: I6d4895376ce323c14c997e6c9af2ea3eeeee0184
Signed-off-by: Alexandru Avadanii <Alexandru.Avadanii@enea.com>
@MTecknology
Copy link
Contributor

This could be a naive approach, but would it be acceptable to just add some documentation?

Such as...

On Debian-based systems, networking configuration can be specified
in `/etc/network/interfaces` or via included files such as (by default)
`/etc/network/interfaces.d/*`. This can be problematic for configuration
management. It is recommended to use either `file.managed` *or*
`network.managed`.

If using `network.managed`, it can be useful to ensure `interfaces.d/`
is empty. This can be done using:

/etc/network/interfaces.d:
      file.directory:
        - clean: True

Every programatic option I come up requires assumptions and adds a lot of complicated logic. Updating documentation seems like the next best option.

gitebra pushed a commit to gitebra/salt that referenced this issue Aug 31, 2018
* upstream/develop: (101 commits)
  Fix unit test
  Add support to avoid calling refresh_db in opkg.del_repo
  Use a temp file instead of /etc/network/interfaces for unit tests.
  Increase run_script timeout to fix failing test
  Fix git state test
  Fix SaltCloud init in map
  Implement gtmanfred's suggestion
  Don't make a new CloudClient, don't crash on no existing hosts
  Use CloudClient to determine master IP
  Support reading multiple addresses from interface files.
  Support unicode in space-delimited list; fixes unit tests in py2.
  Honor make_master also if master already exists
  Added documentation about debian interfaces.d/*. (Fixes: saltstack#40262)
  Removed python lint.
  Fix tests that use timed_subprocess for py3
  Finished adding support for multiple IP addresses.
  Cleaned up documentation/examples in states.network:
  Added support for -ipaddrs and -ipv6ipaddrs to modules.debian_ip().
  Added support for loopback devices to modules.debian_ip(). (Fixes: saltstack#38672)
  Added a bunch of unit tests for modules.debian_ip.build_interface().
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

No branches or pull requests

3 participants