fb_networkmanager: new cookbook to manage NetworkManager #127

jaymzh · 2020-05-13T01:54:24Z

This cookbook aims to manage NetworkManager in the most controlled
way possible. It might-could-should be sent over to CPE cookbooks,
I can redirect over there if preferred.

While, much to my dismay, some people do run NM on servers, and this
will work for that, it was built to handle this client machine case.

NetworkManager treats its config files as databases and can change them,
so they can't be handled simply as templates - more finesse is needed.

However, write-once doesn't provide the level of management that
FB-API-style cookbooks expect. This marries the two. All desired
settings are managed 100%, and NetworkManager is allowed to make
additive settings (such as "seen-bssids" or "password" [sic]) so that it
can continue to function for users.

It is immune to the format changes that NM can make - spacing and
ordering changes will not cause the file to be updated any more than
ignorable settings. When updating the file, care is taken to stick
close to NM's style.

I considered making blacklist/whitelists of fields that NM could
modify, but in practice since we merge in the user's config, the user's
config is effectively a blacklist, and whitelist would likely be far
too fragile.

It has full docs, including on it's Migration Feature which will be
useful for anyone who had their own Chef to do this but wants to
migrate.

It also comes with pretty good test coverage.

davide125

I'm fine with having this in the repo, but I'd like @michel-slm to review it as well.

cookbooks/fb_networkmanager/attributes/default.rb

cookbooks/fb_networkmanager/metadata.rb

cookbooks/fb_networkmanager/attributes/default.rb

cookbooks/fb_networkmanager/libraries/default.rb

cookbooks/fb_networkmanager/libraries/resource.rb

cookbooks/fb_networkmanager/recipes/default.rb

cookbooks/fb_networkmanager/resources/system_connections.rb

cookbooks/fb_networkmanager/spec/default_spec.rb

cookbooks/fb_networkmanager/spec/resource_spec.rb

michel-slm · 2020-05-13T16:20:51Z

Also adding @jgoguen who writes the original CPE network manager cookbook.

michel-slm · 2020-05-13T19:26:01Z

re: testing on Fedora - that can probably wait until after this review is done, and we then try switching to it and send further PRs to fix any issue we find.

cookbooks/fb_networkmanager/attributes/default.rb

cookbooks/fb_networkmanager/libraries/resource.rb

jgoguen · 2020-05-13T22:57:28Z

cookbooks/fb_networkmanager/libraries/default.rb

+        content.each_pair do |sect, opts|
+          doc.section(sect, :option_sep => '=') do |section|
+            opts.each_pair do |opt, val|
+              v = [val].flatten.join(',')


NetworkManager connection files (but not NetworkManager.conf, because consistency!) use semicolons for multi-value separators, and requires a trailing semicolon for multi-value fields. Using a semicolon in a value makes IniParse wrap the value in quotes, which breaks NM parsing the file.

When I wrote a NetworkManager cookbook, I ended up having to manually generate the not-quite-INI text myself. You can imagine how impressed I was (still am) about having to do that when NM could have just used the proper INI format.

Can you give me an example to test? I had eap=peap; and dropped it to eap=peap and it worked fine.

@jgoguen I don't think the trailing semicolon is necessary, and here's some data:

[phil@ldx-vulturus ~]$ sudo grep ^eap /etc/NetworkManager/system-connections/fb_networkmanager_vicarious_wifi_hq eap=peap [phil@ldx-vulturus ~]$ nmcli c show 6f1f4851-15a3-321f-beec-fbfc873c9f85 | grep 802-1x.eap 802-1x.eap: peap

vs

root@ldx-vulturus:~# sudo grep ^eap /etc/NetworkManager/system-connections/fb_networkmanager_vicarious_wifi_hq eap=peap; root@ldx-vulturus:~# nmcli c reload root@ldx-vulturus:~# nmcli c show 6f1f4851-15a3-321f-beec-fbfc873c9f85 | grep 802-1x.eap 802-1x.eap: peap

They're identical.

Interesting, what version of NetworkManager? I have 1.20 and even for connections manually created fresh it's still forcing semicolon-separate lists with a trailing semicolon.

I have 1.22 on another laptop, I can check what it does later tonight.

I wonder then if this INI-incompatible change is the new format. I created new connections on both 1.20 and 1.22, both forced trailing semicolons for multi-value fiends. It would work if I remove the trailing semicolon, if I restart NetworkManager itself not just reload connections, but as soon as anything modified the connection file it put the trailing semicolons back.

Where it uses semicolons as list item separators, I wonder if IniParse would see the first value and then discard the rest treating it as a comment…

certainly it does like to add them back. I just didn't find any problem to not having them.

Well, I'm going to assume there was something borked on my system. So as long as this can output semicolon-delimited lists for the connection files and comma-delimited lists for NetworkManager.conf it's good.

It doesn't today. I'll add that and tests. Thanks!

oh huh, I didn't get to this... I'll come back to this

jgoguen · 2020-05-13T23:07:38Z

cookbooks/fb_networkmanager/spec/default_spec.rb

+
+  # we don't test to make sure IniParse does what IniParse does,
+  # instead we teste the things we're doing custom...
+  context '#to_ini' do


Add a test for array fields. If you call FB::NetworkManager.to_ini on this hash:

{ 'connection' => { 'permissions' => %w{user:jgoguen:}, }, '802-1x' => { 'identity' => 'jgoguen', 'eap' => %w{tls}, }, }

I expect this to be the generated config file:

[connection] permissions=user:jgoguen:; [802-1x] identity=jgoguen eap=tls;

permissions and eap are two fields that NetworkManager requires be multi-value, including the trailing semicolon after the last value.

define doesn't work? it doesn't complain for sure, and it connects...

In NM 1.20 and 1.22, what I found in my testing is that if I remove trailing semicolons I consistently have to restart NM for it to work with the changed connection file, but adding a trailing semicolon (which NM will do itself when it tries to update a multi-value field) works just by reloading the connections. So if we write out a connection file that removes a trailing semicolon, based on my experience we'll need to restart the entire NetworkManager daemon instead of just reloading the configs.

I will experiment with this.

jgoguen · 2020-05-13T23:13:56Z

cookbooks/fb_networkmanager/spec/resource_spec.rb

+    let(:from_contents) do
+      <<~EOS
+      [oldsection]
+      key=value
+
+      [section1]
+      existing=stuff
+      optionA=previous_value
+
+      [section2]
+      blue=red
+      EOS
+    end


Add another test case where there's multi-value fields in the contents. This content:

[oldsection] key=value foo=bar;baz; [section1] existing=stuff optionA=previous_value optionB=red;orange;yellow;

must generate this hash:

{ 'oldsection' => { 'key' => 'value', 'foo' => [ 'bar', 'baz', ], }, 'section1' => { 'existing' => 'stuff', 'optionA' => 'previous_value', 'optionB' => [ 'red', 'orange', 'yellow', ], }, }

cookbooks/fb_networkmanager/libraries/resource.rb

jgoguen · 2020-05-13T23:25:39Z

cookbooks/fb_networkmanager/recipes/default.rb

+vpnhome = '/var/lib/openvpn/chroot'
+FB::Users.initialize_group(node, 'nm-openvpn')
+node.default['fb_users']['users']['nm-openvpn'] = {
+  'gid' => 'nm-openvpn',
+  # If /var/lib/openvpn doesn't exist yet, we can't create it in time
+  # but we want to create the user, so for the first run, create it
+  # with /tmp
+  'home' => ::File.directory?(vpnhome) ? vpnhome : '/tmp',
+  'shell' => '/usr/sbin/nologin',
+  'action' => :add,
+}
+
+packages = %w{
+  network-manager
+  openvpn
+  network-manager-openvpn-gnome
+}


Is it assured we definitely want OpenVPN always configured? I'd prefer making VPN optional and allow choosing either/both of at least OpenVPN and openconnect.

This doesn't force anyone to use openvpn. It just happens to setup openvpn only because I didn't have a test-case for openconnect. It's not mutually-exclusive afaik?

Ah, good catch @jgoguen. Yes, we should gate this behind an attribute and make it optional. VPN setup is totally fine here, but we shouldn't do it by default, and we should write in a way that can support multiple backends (even if you only implement openvpn for now).

I'm just installing the right packages, you can make NM use whatever backened you want in the connection. We can install all the backends. This doesn't configure any VPNs... it just makes it available.

But if I'm never going to use OpenVPN, I need openconnect, there's no point to having it installed. If we gate it behind an attribute we can allow administrators to better control what gets installed.

As soon as we're going to have multiple VPN backends installed by Chef (plus any sanity work, like making sure the user is created) it may be better if we instead have a series of includes:

include_recipe 'fb_networkmanager::openvpn include_recipe 'fb_networkmanager::openconnect

Each one can have an early return based on something like node['fb_networkmanager']['enable'] && node['fb_networkmanager'][vpntype]['enable']

Oh, yes you're right. It would need to be a set of resources (or a resource called multiple times with different actions, which just no).

@davide125 any strong opinion? I'd still like to have some gate for these but not strongly enough to demand that much extra work and testing for little gain.

You have a gate, manage_packages. :)

I'm not convinced we should be installing openvpn (and openconnect or vpnc or whatever for that matter) itself here; a bunch of these will happily drop off system services, and can have interesting side effects if not setup properly. I think the ideal state would be that we have fb_openvpn / fb_openconnect / etc to manage them properly, and that fb_networkmanager just uses their API and takes care of the n-m specific bits (in this case, the user and the plugin package). I do realize this is a fuckton of work though, and it doesn't make sense to block this PR on it.

How about this: we move the openvpn stuff to a fb_networkmanager::openvpn, and we just don't include it in the default recipe. In the README, we document that if you want openvpn you should include. This is akin to how we handle some of the more corner cases in fb_systemd iirc.

except that doesn't make sense for fb_networkmanater to use an fb_openvpn API, because it's not configuring it statically. NetworkManager internally uses those things, but it needs the packages installed.

We can certainly make extra recipes for them and that's fine, but it's literally Just The Packages, to be clear.

I still contend we should split it out. If we had a fb_openvpn, the way I see it fb_networkmanager::openvpn would just include fb_openvpn::packages and then install its thing. For now, I think it'd be fine to install both openvpn and the other package directly in a fb_networkmanager::openvpn. N-M can support like 5 or 6 different type of VPNs, some with fairly complex and invasive dependencies. We shouldn't force the user to have them all installed by default.

This cookbook aims to manage NetworkManager in the most controlled way possible. It might-could-should be sent over to CPE cookbooks, I can redirect over there if preferred. While, much to my dismay, some people do run NM on servers, and this will work for that, it was built to handle this client machine case. NetworkManager treats its config files as databases and can change them, so they can't be handled simply as templates - more finesse is needed. However, write-once doesn't provide the level of management that FB-API-style cookbooks expect. This marries the two. All desired settings are managed 100%, and NetworkManager is allowed to make additive settings (such as "seen-bssids" or "password" [sic]) so that it can continue to function for users. It is immune to the format changes that NM can make - spacing and ordering changes will not cause the file to be updated any more than ignorable settings. When updating the file, care is taken to stick close to NM's style. I considered making blacklist/whitelists of fields that NM could modify, but in practice since we merge in the user's config, the user's config is effectively a blacklist, and whitelist would likely be far too fragile. It has full docs, including on it's Migration Feature which will be useful for anyone who had their own Chef to do this but wants to migrate. It also comes with pretty good test coverage. Signed-off-by: Phil Dibowitz <[email protected]>

davide125 · 2020-12-10T23:04:14Z

Overall logic lgtm, but I'd like @jgoguen to do another pass

jgoguen

Overall LGTM. I'd like to see the VPN parts split out, but setting manage_packages to false is reasonable enough in the meantime.

jgoguen · 2020-12-11T04:47:08Z

cookbooks/fb_networkmanager/README.md

+
+The rendered config here will set `connection.autoconnect-priority` to 100
+if there is no value for it found in the existing file, but will use the value
+in the file if it exists. The logic here is quote simple:


Suggested change

in the file if it exists. The logic here is quote simple:

in the file if it exists. The logic here is quite simple:

jgoguen · 2020-12-11T05:02:00Z

cookbooks/fb_networkmanager/recipes/default.rb

+if node['fb_users']
+  vpnhome = '/var/lib/openvpn/chroot'
+  FB::Users.initialize_group(node, 'nm-openvpn')
+  node.default['fb_users']['users']['nm-openvpn'] = {
+    'gid' => 'nm-openvpn',
+    # If /var/lib/openvpn doesn't exist yet, we can't create it in time
+    # but we want to create the user, so for the first run, create it
+    # with /tmp
+    'home' => ::File.directory?(vpnhome) ? vpnhome : '/tmp',
+    'shell' => '/usr/sbin/nologin',
+    'action' => :add,
+  }
+end
+
+packages = %w{
+  network-manager
+  openvpn
+  network-manager-openvpn-gnome
+}
+
+package packages do
+  only_if do
+    node['fb_networkmanager']['enable'] &&
+      node['fb_networkmanager']['manage_packages']
+  end
+  action :upgrade
+end


I'd still like to see the VPN parts split out into a separate cookbook, but for now setting node.default['fb_networkmanager']['manage_packages'] to false is a reasonable enough workaround.

It definitely doesn't make sense in another cookbook. It's all network-manager configs. you don't configure vpn, you configure network-manager and it may or may not use backends.

I can make enable_openvpn_packages if we want, or make it a separate recipe... but I think a config value seems sane. Then I can something like

package "network manager packages" do only_if do node['fb_networkmanager']['enable'] && node['fb_networkmanager']['manage_packages'] end package_names lazy { p = %w{network-manager} if node['fb_networkmanager']['enable_openvpn_packages'] p += %w{openvpn network-manager-openvpn-gnome} end p } action :upgrade end

I don't love it, but if people feel strongly.

cookbooks/fb_networkmanager/resources/system_connections.rb

jaymzh · 2020-12-11T06:02:54Z

I missed the "lists are different in the two files" and the trailing semi-colon thing, will do those soon.

facebook-github-bot added the CLA Signed label May 13, 2020

davide125 requested changes May 13, 2020

View reviewed changes

davide125 reviewed May 13, 2020

View reviewed changes

cookbooks/fb_networkmanager/attributes/default.rb Outdated Show resolved Hide resolved

jgoguen suggested changes May 13, 2020

View reviewed changes

jaymzh force-pushed the fb_networkmanager branch from ebc6365 to c6a2408 Compare July 2, 2020 18:48

jaymzh added 4 commits December 9, 2020 13:01

code review

15cf94f

review bits

1f84c31

add defaults feature

c332057

jaymzh force-pushed the fb_networkmanager branch from d5c158e to c332057 Compare December 9, 2020 21:01

docs for defaults

b644633

jaymzh requested review from davide125 and jgoguen December 9, 2020 21:18

jgoguen approved these changes Dec 11, 2020

View reviewed changes

fix doc typos

aaf6487

sjpark0605 requested a review from joshuamiller01 as a code owner May 17, 2024 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fb_networkmanager: new cookbook to manage NetworkManager #127

fb_networkmanager: new cookbook to manage NetworkManager #127

jaymzh commented May 13, 2020

davide125 left a comment

michel-slm commented May 13, 2020

michel-slm commented May 13, 2020

jgoguen May 13, 2020

jaymzh May 14, 2020

jaymzh May 14, 2020 •

edited

Loading

jgoguen May 14, 2020

jaymzh May 14, 2020

jgoguen May 14, 2020

jaymzh May 14, 2020

jgoguen May 15, 2020

jaymzh May 15, 2020

jaymzh Dec 11, 2020

jgoguen May 13, 2020

jaymzh May 14, 2020

jgoguen May 14, 2020

jaymzh May 14, 2020

jgoguen May 13, 2020

jgoguen May 13, 2020

jaymzh May 14, 2020

davide125 May 14, 2020

jaymzh May 14, 2020

jgoguen May 14, 2020

jgoguen May 15, 2020

jaymzh May 15, 2020

davide125 May 15, 2020

jaymzh May 15, 2020

davide125 Dec 10, 2020

davide125 commented Dec 10, 2020

jgoguen left a comment

jgoguen Dec 11, 2020

jgoguen Dec 11, 2020

jaymzh Dec 11, 2020 •

edited

Loading

jaymzh commented Dec 11, 2020

	in the file if it exists. The logic here is quote simple:
	in the file if it exists. The logic here is quite simple:

fb_networkmanager: new cookbook to manage NetworkManager #127

Are you sure you want to change the base?

fb_networkmanager: new cookbook to manage NetworkManager #127

Conversation

jaymzh commented May 13, 2020

davide125 left a comment

Choose a reason for hiding this comment

michel-slm commented May 13, 2020

michel-slm commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymzh May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davide125 commented Dec 10, 2020

jgoguen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymzh Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

jaymzh commented Dec 11, 2020

jaymzh May 14, 2020 •

edited

Loading

jaymzh Dec 11, 2020 •

edited

Loading