-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Cross stack subnet stuff #1512
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
base: firewall-types
Are you sure you want to change the base?
Cross stack subnet stuff #1512
Conversation
hostmap.go
Outdated
| } | ||
|
|
||
| func (i *HostInfo) buildNetworks(networks, unsafeNetworks []netip.Prefix) { | ||
| func (i *HostInfo) buildNetworks(myVpnNetworksTable *bart.Lite, networks, unsafeNetworks []netip.Prefix) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep thinking this would be better as buildNetworks(myVpnNetworksTable *bart.Lite, crt cert.Certificate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I don't like passing whole objects around when I only need a member or two
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less to type and screw up, they are the literal Networks() and UnsafeNetworks() from the certificate, anything else would be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhhh ordering bc they're the same type. Smart.
There's a school of thought that would tell us to define a VPNNetwork and UnsafeNetwork type, but, I don't wanna.
Will change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this change actually is part of #1509, I forgot to target this PR at that one to reflect that, updated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah crap #1509 doesn't include the removal of filteredNetworks
| if !lh.myVpnNetworksTable.Contains(addr) { | ||
| return nil, util.NewContextualError("lighthouse host is not in our networks, invalid", m{"vpnAddr": addr, "networks": lh.myVpnNetworks}, nil) | ||
| lh.l.WithFields(m{"vpnAddr": addr, "networks": lh.myVpnNetworks}). | ||
| Warn("lighthouse host is not in our networks, this might be a mistake") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be beneficial to explain a little more about the mistake, maybe lighthouse host is not in our networks, only lighthouse control traffic will work, you will not be able to ping this lighthouse. or something. Same with the static host map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah good idea
replaces #1506