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

client: Add option to enable hairpinMode on Nomad bridge #15961

Merged
merged 8 commits into from
Feb 2, 2023

Conversation

angrycub
Copy link
Contributor

  • Add bridge_network_hairpin_mode client config setting
  • Add hairpin mode to bridge template; convert to go template
  • add node attribute: nomad.bridge.hairpin_mode
  • Add documentation

client/allocrunner/networking_bridge_linux.go Outdated Show resolved Hide resolved
Comment on lines 39 to 41
require.NotPanics(t, func() { out = buildNomadBridgeNetConfig(*tc.b) })
if tc.name == "bad_name" {
fmt.Println(string(out))
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why print the output for "bad_name" ? and not any others?

and I find myself wanting to assert more than just "doesn't panic" but on the fence about what specifically to suggest short of full-text string matching...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bad_name test was to see if JSON stuffing was a concern. While it appears to be, it will cause the generateAdminChainRule function to break. This PR doesn't actually introduce the JSON stuffing issue and ultimately, bridge names would benefit from some additional validation since they aren't built until they are needed the first time.

Copy link
Contributor Author

@angrycub angrycub Jan 31, 2023

Choose a reason for hiding this comment

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

I removed the test since it doesn't actually test whether or not the go_template rendering breaks, as opposed to making broken JSON. Even in the original code, we used %s inside of quotes rather than leveraging %q for JSON safety. That could be a nice "make it better than it was" item we could add though.

website/content/docs/configuration/client.mdx Outdated Show resolved Hide resolved
Copy link
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

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

I might like to see the value-add of extra unit test coverage to confirm we produce valid json, but this is still quite a good, so LGTM!

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Some minor suggestions, but LGTM!

@@ -184,7 +184,7 @@ func newNetworkConfigurator(log hclog.Logger, alloc *structs.Allocation, config

switch {
case netMode == "bridge":
c, err := newBridgeNetworkConfigurator(log, config.BridgeNetworkName, config.BridgeNetworkAllocSubnet, config.CNIPath, ignorePortMappingHostIP)
c, err := newBridgeNetworkConfigurator(log, config.BridgeNetworkName, config.BridgeNetworkAllocSubnet, config.BridgeNetworkHairpinMode, config.CNIPath, ignorePortMappingHostIP)
Copy link
Contributor

Choose a reason for hiding this comment

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

The list of arguments is getting uncomfortably long, I wouldn't oppose a new config struct the be passed here. Although bridgeNetworkConfiguratorConfig is kind of weird 😬

Another option would be to pass config, but that has the downside of not making it clear which config values are relevant.

@@ -54,7 +58,7 @@ func newBridgeNetworkConfigurator(log hclog.Logger, bridgeName, ipRange, cniPath
b.allocSubnet = defaultNomadAllocSubnet
}

c, err := newCNINetworkConfiguratorWithConf(log, cniPath, bridgeNetworkAllocIfPrefix, ignorePortMappingHostIP, buildNomadBridgeNetConfig(b.bridgeName, b.allocSubnet))
c, err := newCNINetworkConfiguratorWithConf(log, cniPath, bridgeNetworkAllocIfPrefix, ignorePortMappingHostIP, buildNomadBridgeNetConfig(*b))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
c, err := newCNINetworkConfiguratorWithConf(log, cniPath, bridgeNetworkAllocIfPrefix, ignorePortMappingHostIP, buildNomadBridgeNetConfig(*b))
c, err := newCNINetworkConfiguratorWithConf(log, cniPath, bridgeNetworkAllocIfPrefix, ignorePortMappingHostIP, buildNomadBridgeNetConfig(b))

And then you receive a pointer in buildNomadBridgeNetConfig.

Comment on lines 146 to 150
tmpl, err := template.New("cniConf").Parse(nomadCNIConfigTemplate)
if err != nil {
// Panic on error for catching issues in testing
panic(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can move this to the top of file so the template is parse just once at initialization, and the use Must to panic, like:

var nomadBridgeTmpl = template.Must(template.New("cniConf").Parse(nomadCNIConfigTemplate))

Comment on lines 152 to 166
// TODO: Consider exporting these directly from bridgeNetworkConfigurator
// so they aren't repeated in the input struct
type templInput struct {
AllocSubnet string
BridgeName string
HairpinMode bool
CNIAdminChainName string
}

tIn := templInput{
AllocSubnet: b.allocSubnet,
BridgeName: b.bridgeName,
HairpinMode: b.hairpinMode,
CNIAdminChainName: cniAdminChainName,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah cool, so yeah, I think having a config struct passed to bridgeNetworkConfigurator could help here as well.

Comment on lines 173 to 177
err = tmpl.Execute(&out, tIn)
if err != nil {
// Panic on error for catching issues in testing
panic(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs are not very clear in all possible error modes here, so I think it may be better to return err and let the allocrunner handle that? It may be a transient issue that a retry would fix.

Comment on lines +156 to +159
- `bridge_network_hairpin_mode` `(bool: false)` - Specifies if hairpin mode
is enabled on the network bridge created by Nomad for allocations running
with bridge networking mode on this client. You may use the corresponding
node attribute `nomad.bridge.hairpin_mode` in constraints.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `bridge_network_hairpin_mode` `(bool: false)` - Specifies if hairpin mode
is enabled on the network bridge created by Nomad for allocations running
with bridge networking mode on this client. You may use the corresponding
node attribute `nomad.bridge.hairpin_mode` in constraints.
- `bridge_network_hairpin_mode` `(bool: false)` - Specifies if hairpin mode
is enabled on the network bridge created by Nomad for allocations running
with bridge networking mode on this client. You may use the corresponding
node attribute `nomad.bridge.hairpin_mode` in constraints. When hairpin mode
is enabled allocations are able to reach their own IP and port. Changing this
value requires a reboot of the client host to take effect.

I think? 😅

Feel free to modify any of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I think I'd like to omit the additional text so we can get the merge done. Then test the behavior around this on a running system, and then if needed come back and add it in a followup PR. I've added #16023 to track this

Switch back to sprintf implementation to prevent bonus
complexity around error handling. Updated string to use
%q in places where the user can supply a string value
that could break the JSON. Added a valid JSON check to
the test.
@A-Helberg
Copy link

Awesome work!
What release will this be a part of?
I have been using a private build of #13834 will be great to get back to upstream 😄

@tgross tgross added this to the 1.5.0 milestone Feb 13, 2023
@tgross
Copy link
Member

tgross commented Feb 13, 2023

Hi @A-Helberg this shipped in Nomad 1.5.0-beta.1 and will ship in the GA release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants