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.

Sign up for GitHub

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

Fix DNS based CWNPs for network-isolated Clusters #177

Merged
merged 10 commits into from
Apr 22, 2024
Merged

Fix DNS based CWNPs for network-isolated Clusters #177

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
70af9ab
fix: initial dns for proxy
vknabel Mar 22, 2024
e1c9986
Merge branch 'master' into fix-initial-dns
majst01 Mar 22, 2024
ad2c4cc
fix(dns): add port if missing
vknabel Apr 10, 2024
c990a4a
feat: redirect dns and allow firewall proxy dns
vknabel Apr 17, 2024
e02daad
refactor: rendered table dnat
vknabel Apr 17, 2024
e5e4c1f
fix: status was not updated for cwnps
vknabel Apr 17, 2024
daa8199
Update pkg/nftables/networkpolicy.go
vknabel Apr 18, 2024
9d6768a
refactor: use new proxy_dns_servers
vknabel Apr 18, 2024
96cbc32
refactor: StringsJoin in template
vknabel Apr 18, 2024
0243368
chore: upgrade metal-networker to latest release
vknabel Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions controllers/clusterwidenetworkpolicy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func (r *ClusterwideNetworkPolicyReconciler) manageDNSProxy(

if enableDNS && r.DnsProxy == nil {
r.Log.Info("DNS Proxy is initialized")
if r.DnsProxy, err = dns.NewDNSProxy(f.Spec.DNSPort, ctrl.Log.WithName("DNS proxy")); err != nil {
if r.DnsProxy, err = dns.NewDNSProxy(f.Spec.DNSServerAddress, f.Spec.DNSPort, ctrl.Log.WithName("DNS proxy")); err != nil {
return fmt.Errorf("failed to init DNS proxy: %w", err)
}
go r.DnsProxy.Run(ctx)
Expand All @@ -154,7 +154,12 @@ func (r *ClusterwideNetworkPolicyReconciler) manageDNSProxy(

// If proxy is ON, update DNS address(if it's set in spec)
if r.DnsProxy != nil && f.Spec.DNSServerAddress != "" {
if err = r.DnsProxy.UpdateDNSServerAddr(f.Spec.DNSServerAddress); err != nil {
port := 53
if f.Spec.DNSPort != nil {
port = int(*f.Spec.DNSPort)
}
addr := fmt.Sprintf("%s:%d", f.Spec.DNSServerAddress, port)
if err = r.DnsProxy.UpdateDNSServerAddr(addr); err != nil {
return fmt.Errorf("failed to update DNS server address: %w", err)
}
}
Expand Down Expand Up @@ -237,11 +242,6 @@ func (r *ClusterwideNetworkPolicyReconciler) allowedCWNPs(ctx context.Context, c
}

func (r *ClusterwideNetworkPolicyReconciler) updateCWNPState(ctx context.Context, cwnp firewallv1.ClusterwideNetworkPolicy, state firewallv1.PolicyDeploymentState, msg string) error {
// do nothing if message and state already have the desired values
if cwnp.Status.Message == msg && cwnp.Status.State == state {
return nil
}
Comment on lines -240 to -243
Copy link
Contributor

Choose a reason for hiding this comment

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

I will try to investigate the impact on update traffic this will produce as this essentially updates all CWNPs of a cluster very regularly. Maybe it's worth to implement an update that only occurs if necessary (by comparing contents).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a significant difference between requests of this firewall-controller and 2.3.0 for CWNPs in API server dashboards, so I assume we can start doing this.


cwnp.Status.Message = msg
cwnp.Status.State = state

Expand Down
2 changes: 1 addition & 1 deletion pkg/dns/dns_proxy_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func NewDNSProxyHandler(log logr.Logger, cache *DNSCache) *DNSProxyHandler {
log: log.WithName("DNS handler"),
udpClient: udpClient,
tcpClient: tcpClient,
dnsServerAddr: defaultDNSServerAddr,
dnsServerAddr: cache.dnsServerAddr,
updateCache: getUpdateCacheFunc(log, cache),
}
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/dns/dnscache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,12 @@ type DNSCache struct {
ipv6Enabled bool
}

func newDNSCache(ipv4Enabled, ipv6Enabled bool, log logr.Logger) *DNSCache {
func newDNSCache(dns string, ipv4Enabled, ipv6Enabled bool, log logr.Logger) *DNSCache {
return &DNSCache{
log: log,
fqdnToEntry: map[string]cacheEntry{},
setNames: map[string]struct{}{},
dnsServerAddr: defaultDNSServerAddr,
dnsServerAddr: dns,
ipv4Enabled: ipv4Enabled,
ipv6Enabled: ipv6Enabled,
}
Expand Down
11 changes: 9 additions & 2 deletions pkg/dns/dnsproxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,11 @@ type DNSProxy struct {
handler DNSHandler
}

func NewDNSProxy(port *uint, log logr.Logger) (*DNSProxy, error) {
cache := newDNSCache(true, false, log.WithName("DNS cache"))
func NewDNSProxy(dns string, port *uint, log logr.Logger) (*DNSProxy, error) {
if dns == "" {
dns = defaultDNSServerAddr
}
cache := newDNSCache(dns, true, false, log.WithName("DNS cache"))
handler := NewDNSProxyHandler(log, cache)

host, err := getHost()
Expand Down Expand Up @@ -121,6 +124,10 @@ func (p *DNSProxy) IsInitialized() bool {
return p != nil
}

func (p *DNSProxy) CacheAddr() (string, error) {
return getHost()
}

func getHost() (string, error) {
c, err := netconf.New(network.GetLogger(), network.MetalNetworkerConfig)
if err != nil || c == nil {
Expand Down
1 change: 1 addition & 0 deletions pkg/nftables/firewall.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ type FQDNCache interface {
GetSetsForRendering(fqdns []firewallv1.FQDNSelector) (result []dns.RenderIPSet)
GetSetsForFQDN(fqdn firewallv1.FQDNSelector, fqdnSets []firewallv1.IPSet) (result []firewallv1.IPSet)
IsInitialized() bool
CacheAddr() (string, error)
}

// Firewall assembles nftable rules based on k8s entities
Expand Down
15 changes: 15 additions & 0 deletions pkg/nftables/mocks/mock_fqdncache.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions pkg/nftables/networkpolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,19 @@ func clusterwideNetworkPolicyIngressRules(np firewallv1.ClusterwideNetworkPolicy
return uniqueSorted(rules)
}

func clusterwideNetworkPolicyEgressDNSCacheRules(cache FQDNCache, logAcceptedConnections bool) (nftablesRules, error) {
addr, err := cache.CacheAddr()
if err != nil {
return nil, err
}
base := []string{"ip saddr == @cluster_prefixes", fmt.Sprintf("ip daddr { %s }", addr)}
comment := fmt.Sprintf("accept traffic for dns cache")
vknabel marked this conversation as resolved.
Show resolved Hide resolved
return nftablesRules{
assembleDestinationPortRule(base, "tcp", []string{"53"}, logAcceptedConnections, comment+" tcp"),
assembleDestinationPortRule(base, "udp", []string{"53"}, logAcceptedConnections, comment+" udp"),
}, nil
}

func clusterwideNetworkPolicyEgressRules(
cache FQDNCache,
np firewallv1.ClusterwideNetworkPolicy,
Expand Down
15 changes: 15 additions & 0 deletions pkg/nftables/nftables.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,18 @@ table inet firewall {
}
{{- end }}
}
{{- if .AdditionalDNSAddrs }}

# Add additional DNS addresses for dnat redirection for the dns proxy
table inet nat {
set public_dns_servers {
type ipv4_addr
flags interval
auto-merge
elements = {
{{- $sep := " " }}
{{- range .AdditionalDNSAddrs }}{{ $sep }}{{ . }}{{ $sep = ", " }}
vknabel marked this conversation as resolved.
Show resolved Hide resolved
{{- end }} }
}
}
{{- end }}
31 changes: 22 additions & 9 deletions pkg/nftables/rendering.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,13 @@ import (

// firewallRenderingData holds the data available in the nftables template
type firewallRenderingData struct {
ForwardingRules forwardingRules
RateLimitRules nftablesRules
SnatRules nftablesRules
Sets []dns.RenderIPSet
InternalPrefixes string
PrivateVrfID uint
ForwardingRules forwardingRules
RateLimitRules nftablesRules
SnatRules nftablesRules
Sets []dns.RenderIPSet
InternalPrefixes string
PrivateVrfID uint
AdditionalDNSAddrs []string
}

func newFirewallRenderingData(f *Firewall) (*firewallRenderingData, error) {
Expand Down Expand Up @@ -56,13 +57,25 @@ func newFirewallRenderingData(f *Firewall) (*firewallRenderingData, error) {
return &firewallRenderingData{}, err
}

var sets []dns.RenderIPSet
var (
sets []dns.RenderIPSet
dnsAddrs = []string{}
)
if f.cache.IsInitialized() {
sets = f.cache.GetSetsForRendering(f.clusterwideNetworkPolicies.GetFQDNs())
rules, err := clusterwideNetworkPolicyEgressDNSCacheRules(f.cache, f.logAcceptedConnections)
if err != nil {
return &firewallRenderingData{}, err
}
if f.firewall.Spec.DNSServerAddress != "" {
dnsAddrs = append(dnsAddrs, f.firewall.Spec.DNSServerAddress)
}
egress = append(egress, rules...)
}
return &firewallRenderingData{
PrivateVrfID: uint(*f.primaryPrivateNet.Vrf),
InternalPrefixes: strings.Join(f.firewall.Spec.InternalPrefixes, ", "),
AdditionalDNSAddrs: dnsAddrs,
PrivateVrfID: uint(*f.primaryPrivateNet.Vrf),
InternalPrefixes: strings.Join(f.firewall.Spec.InternalPrefixes, ", "),
ForwardingRules: forwardingRules{
Ingress: ingress,
Egress: egress,
Expand Down
11 changes: 6 additions & 5 deletions pkg/nftables/rendering_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,11 @@ func TestFirewallRenderingData_renderString(t *testing.T) {
Egress: []string{"egress rule 1", "egress rule 2"},
Ingress: []string{"ingress rule 1", "ingress rule 2"},
},
InternalPrefixes: "1.2.3.0/24, 2.3.4.0/8",
RateLimitRules: []string{"meta iifname \"eth0\" limit rate over 10 mbytes/second counter name drop_ratelimit drop"},
SnatRules: []string{"ip saddr { 10.0.0.0/8 } oifname \"vlan104009\" counter snat 185.1.2.3 comment \"snat internet\""},
PrivateVrfID: uint(42),
InternalPrefixes: "1.2.3.0/24, 2.3.4.0/8",
RateLimitRules: []string{"meta iifname \"eth0\" limit rate over 10 mbytes/second counter name drop_ratelimit drop"},
SnatRules: []string{"ip saddr { 10.0.0.0/8 } oifname \"vlan104009\" counter snat 185.1.2.3 comment \"snat internet\""},
PrivateVrfID: uint(42),
AdditionalDNSAddrs: []string{"8.9.10.11", "4.5.6.7"},
},
wantErr: false,
},
Expand Down Expand Up @@ -98,7 +99,7 @@ func TestFirewallRenderingData_renderString(t *testing.T) {
rendered, _ := os.ReadFile(path.Join("test_data", tt.name+".nftable.v4"))
want := string(rendered)
if got != want {
t.Errorf("Firewall.renderString() diff: %v", cmp.Diff(got, want))
t.Errorf("Firewall.renderString() diff: %v", cmp.Diff(want, got))
}
})
}
Expand Down
10 changes: 10 additions & 0 deletions pkg/nftables/test_data/more-rules.nftable.v4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,13 @@ table inet firewall {
ip saddr { 10.0.0.0/8 } oifname "vlan104009" counter snat 185.1.2.3 comment "snat internet"
}
}

# Add additional DNS addresses for dnat redirection for the dns proxy
table inet nat {
set public_dns_servers {
Copy link
Contributor

@Gerrit91 Gerrit91 Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To me still the name of this set is a bit misleading because a user might provide a DNS server that is not publicly reachable. Apart from that, my expectation was that the statically defined public DNS servers are not used for resolving queries in case a dedicated DNS server was specified (of course this is problematic as this DNS server needs to resolve the shoots API server DNS but if someone sets this configuration I think it's fine to expect this complex implication).

So, from my side it would be good to duplicate the DNAT rules from the metal-networker. This enables us to remove the DNS proxy related code from the metal-networker and move it the here. Then, we can also implement that only the user-given DNS server is used and fallback to 1.0.0.1, 1.1.1.1, 8.8.4.4, 8.8.8.8 if nothing was given?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the set in metal-stack/metal-networker#106

type ipv4_addr
flags interval
auto-merge
elements = { 8.9.10.11, 4.5.6.7 }
}
}
Loading