networking: refactor building nomad bridge config #23772

martisah · 2024-08-08T18:38:25Z

This PR refactors building the Nomad Bridge network configuration JSON, in order to allow for more manipulation of variables within the network configuration.

client/allocrunner/networking_bridge_linux.go

gulducat

this is very promising! some food for thought for ya.

client/allocrunner/bridge_plugin_config.go

client/allocrunner/networking_bridge_linux.go

gulducat · 2024-08-08T19:12:16Z

client/allocrunner/networking_bridge_linux.go

+	if withConsulCNI {
+		CNIconfig.Plugins = append(CNIconfig.Plugins, ConsulCNIBlock{


this feels so much better to me than Sprint-ing the json string

client/allocrunner/networking_bridge_linux.go

gulducat · 2024-08-12T18:01:53Z

client/allocrunner/test_fixtures/bad_input.conflist.json

@@ -0,0 +1,44 @@
+{


to ensure the refactor is no-op, these files were generated by the old code (plus whitespace), and passed prior to the refactor.

gulducat · 2024-08-12T19:30:56Z

Bit of added context for this refactor: Adding IPv6 support to the default Nomad bridge (#14101) will require a little more modification of the CNI conflist than I want to continue fmt.Sprint()ing in a flat string. This refactor will make it much easier to add the IPv6 stuff conditionally if the network device supports it (the bridge plugin errors if you ask it for ipv6 subnet(s) and the host can't do it).

tgross

LGTM to ship. My comments are for discussion and should be considered non-blocking, but the package change feels like we're trying to carve out a CNI package when maybe what we want is a "networking" package?

tgross · 2024-08-12T20:34:22Z

client/allocrunner/cni/bridge.go

+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: BUSL-1.1
+
+package cni


It feels a little weird having a separate package called cni here without it including much of any of the CNI configuration. There's no code isolation happening because everything is exported (has to be because of serialization.) All CNI setup including all the configuration tests are still in the client/allocrunner package. It seems hard to disentangle the "bridge configurator" code from CNI as well.

the initial motivation was to namespace the structs -- I found myself naming them like FirewallCNIPlugin just because they were in the allocrunner package, and I thought if I made a small new package, then they could be named more simply and still make sense. I also just like small, focused packages in general.

but were I to spend more time on this, I probably would move more of the CNI stuff into here, but I figure this is an incremental improvement to be perhaps built on further, later.

regarding the "networking" package comment, a decent chunk of our networking config, especially on linux, is CNI, but I might still like to distinguish the things that are specifically satisfying CNI spec from other more-general networking concerns. i.e. if I really went ham refactoring this area of allocrunner, I might make a "networking" package, then still put a (relatively smaller) "cni" package within there.

but as you might imagine, I am trying not to boil the ocean! 😅

all that said, I could move the new golden-file tests into here, since they are a direct output of this package. I left them where they were because that was the hook into this refactor, but I did create them here in the first place...

I'll also admit to being a little concerned about backports, since I've been intending to go back to 1.6.x for future maintainability, but e.g. consul-cni is 1.8.x only. backports won't be tooooo painful anyway, or I could forego them entirely, but at any rate, it was on my mind when deciding what all to move into this new package.

but as you might imagine, I am trying not to boil the ocean!

Totally reasonable 👍

tgross · 2024-08-12T20:35:17Z

client/allocrunner/cni/bridge_test.go

+func TestConflist_Json(t *testing.T) {
+	conf := &Conflist{
+		CniVersion: "0.0.1",
+		Name:       "test-config",
+		Plugins: []any{
+			Generic{Type: "test-plugin"},
+		},
+	}
+	bts, err := conf.Json()
+	must.NoError(t, err)
+	must.Eq(t, `{
+	"cniVersion": "0.0.1",
+	"name": "test-config",
+	"plugins": [
+		{
+			"type": "test-plugin"
+		}
+	]
+}`, string(bts))
+}


Doesn't this just test the stdlib?

it tests stdlib json, but the main target is the appropriate struct tags to match CNI spec. I was aiming for a minimal test, but this is probably better covered by the more holistic real-world bridge conflists.

client/allocrunner/cni/plugins.go

tgross · 2024-08-12T20:48:07Z

client/allocrunner/test_fixtures/default.conflist.json

+					[
+						{
+							"subnet": ""
+						}
+					]


I think the test was called empty because it wasn't correctly populated as it will be in the caller, which is how you get this bogus range configuration. It might not be a bad idea to have an actually-default test that populates the minimum set of items we'd expect to see populated.

gosh you're right, good catch!

I had half a mind to add validation to protect against this, but just stuck with what was here instead to keep it a true no-op refactor. think I should reach a little further and check for invalid empty values like this?

I think it's fine to keep new invalidation logic out of this refactor PR.

apollo13 · 2024-08-13T10:33:37Z

If the networking config gets touched this massively, would it also be possible to allow users to override it -- I'd really love to use a custom cni configuration for every job without manually adding it to the job

gulducat · 2024-08-13T14:59:26Z

Hi @apollo13! It would certainly be possible to do, and has been suggested before in #14480, but I agree with Luiz's closing comment there that we should be cautious with what the magic "bridge" network string represents. If it could mean anything, then our ability to support our own "bridge" network is undercut drastically, and it's important that we be able to rely on it when designing other features.

I understand that it means a lot of mode = "cni/my-cool-network" in job files, but at least when that is explicit, we (engineers in github issues, and our enterprise support team) know that we're troubleshooting a custom network setup.

tgross · 2024-08-13T15:15:34Z

I'd really love to use a custom cni configuration for every job without manually adding it to the job

Can you dig into what you might be looking for a bit more @apollo13? network.mode = "host" is the default network, so setting network.mode = "bridge" doesn't seem meaningfully different than setting network.mode = "cni/example". The clients need to have the CNI config file of course, but you'd have to configure a "default bridge" similarly too, I'd think?

Co-authored-by: Martina Santangelo <[email protected]>

apollo13 · 2024-08-14T08:33:04Z

Mhm, you are right @tgross. I forgot about host -- I always set bridge so in my mind this was the default. I guess part of the motiviation is wanting to run connect-enabled jobs with my custom bridge. Though that should be possible if we were to remove the current limitations and checks around mode="bridge" and also allow cni/ there. Not sure though how that would work with the modifications needed for the transparent proxy though.

On a sidenote, namespaces have enabled_task_drivers/disabled_task_drivers. Would you accept a PR adding enabled_network_modes/disable_network_modes?

tgross · 2024-08-14T12:57:02Z

On a sidenote, namespaces have enabled_task_drivers/disabled_task_drivers. Would you accept a PR adding enabled_network_modes/disable_network_modes?

I like where you're going with that. Full disclosure: we're sort of getting a lot of internal pressure to add make any kind of feature like Enterprise, but given that we've already got the nearly-identical task driver restrictions in CE I don't see why we couldn't accept that PR. Especially if it came from the community. 😁

apollo13 · 2024-08-14T14:02:00Z

Full disclosure: we're sort of getting a lot of internal pressure to add make any kind of feature like Enterprise

It is so sad that a company like HashiCorp cannot figure out a good balance where both sides benefit. But I guess the writing was on the wall with the IPO. Feels like HashiCorp (well not you Tim and your colleagues but rather upper management) really just wants to make nomad uninteresting. What I don't get though: If you make the community offering bad enough that noone uses it, who will then advocate for enterprise at their work etc? But what do I know, all I know is that once I turn to k8s because I can't get my stuff done with nomad anymore (using our own fork anyways) then all my (granted small ones and not so many currently) contributions to nomad will stop as well -- is that a winning situation for anyone? :/ (sorry for the rant, I am not trying to shit on anyone, quite the opposite, I care deeply about nomad)

I don't see why we couldn't accept that PR. Especially if it came from the community. 😁

Thank you, will see what I can put up and then mention you on the PR. I just wanted to ask first after the bad turnout of #10355.

vercel bot deployed to Preview – nomad-ui August 8, 2024 18:39 View deployment

martisah commented Aug 8, 2024

View reviewed changes

client/allocrunner/networking_bridge_linux.go Outdated Show resolved Hide resolved

gulducat reviewed Aug 8, 2024

View reviewed changes

vercel bot deployed to Preview – nomad-ui August 8, 2024 20:14 View deployment

vercel bot deployed to Preview – nomad-ui August 9, 2024 16:07 View deployment

vercel bot deployed to Preview – nomad-ui August 12, 2024 17:54 View deployment

gulducat force-pushed the refactor-bridge-template branch from 3a66fa6 to 420bd98 Compare August 12, 2024 17:59

vercel bot deployed to Preview – nomad-ui August 12, 2024 18:00 View deployment

gulducat reviewed Aug 12, 2024

View reviewed changes

gulducat force-pushed the refactor-bridge-template branch from 420bd98 to c94cb74 Compare August 12, 2024 18:29

vercel bot deployed to Preview – nomad-ui August 12, 2024 18:30 View deployment

gulducat marked this pull request as ready for review August 12, 2024 19:18

gulducat changed the title ~~bridge: refactor building Nomad bridge network config~~ networking: refactor building nomad bridge config Aug 12, 2024

gulducat requested review from tgross and jrasell August 12, 2024 19:19

gulducat added theme/networking backport/ent/1.6.x+ent Changes are backported to 1.6.x+ent backport/ent/1.7.x+ent Changes are backported to 1.7.x+ent backport/1.8.x backport to 1.8.x release line labels Aug 12, 2024

tgross approved these changes Aug 12, 2024

View reviewed changes

networking: refactor building nomad bridge config

47b0116

Co-authored-by: Martina Santangelo <[email protected]>

gulducat force-pushed the refactor-bridge-template branch from c94cb74 to 47b0116 Compare August 13, 2024 16:26

vercel bot deployed to Preview – nomad-ui August 13, 2024 16:28 View deployment

gulducat merged commit 73ce56b into main Aug 14, 2024
19 checks passed

gulducat deleted the refactor-bridge-template branch August 14, 2024 17:43

hc-github-team-nomad-core mentioned this pull request Aug 14, 2024

Backport of networking: refactor building nomad bridge config into release/1.8.x #23812

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

networking: refactor building nomad bridge config #23772

networking: refactor building nomad bridge config #23772

martisah commented Aug 8, 2024

gulducat left a comment

gulducat Aug 8, 2024 •

edited

Loading

gulducat Aug 12, 2024

gulducat commented Aug 12, 2024

tgross left a comment

tgross Aug 12, 2024

gulducat Aug 13, 2024

gulducat Aug 13, 2024

tgross Aug 13, 2024

tgross Aug 12, 2024

gulducat Aug 13, 2024

tgross Aug 12, 2024

gulducat Aug 13, 2024

gulducat Aug 13, 2024

tgross Aug 13, 2024

apollo13 commented Aug 13, 2024

gulducat commented Aug 13, 2024

tgross commented Aug 13, 2024

apollo13 commented Aug 14, 2024

tgross commented Aug 14, 2024 •

edited

Loading

apollo13 commented Aug 14, 2024

		if withConsulCNI {
		CNIconfig.Plugins = append(CNIconfig.Plugins, ConsulCNIBlock{

networking: refactor building nomad bridge config #23772

networking: refactor building nomad bridge config #23772

Conversation

martisah commented Aug 8, 2024

gulducat left a comment

Choose a reason for hiding this comment

gulducat Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gulducat commented Aug 12, 2024

tgross left a 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

apollo13 commented Aug 13, 2024

gulducat commented Aug 13, 2024

tgross commented Aug 13, 2024

apollo13 commented Aug 14, 2024

tgross commented Aug 14, 2024 • edited Loading

apollo13 commented Aug 14, 2024

gulducat Aug 8, 2024 •

edited

Loading

tgross commented Aug 14, 2024 •

edited

Loading