From bda6b56ce0a915f47e5ca5719efbeed32268f01d Mon Sep 17 00:00:00 2001 From: Karsten Brusch Date: Thu, 1 Dec 2022 21:37:31 +0100 Subject: [PATCH 1/5] adding aaaa lookups annotation --- README.md | 3 + controllers/fqdnnetworkpolicy_controller.go | 64 +++++++++++---------- 2 files changed, 37 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index f6b3cd3..2a685da 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,9 @@ By default, the NetworkPolicy associated with a FQDNNetworkPolicy gets deleted w To prevent this behavior, set the `fqdnnetworkpolicies.networking.gke.io/delete-policy` annotation to `abandon` on the NetworkPolicy. +There might be scenarios where IPv6 AAAA lookups are not desired or supported in the resulting NetworkPolicy. +To skip AAAA loopups set the `fqdnnetworkpolicies.networking.gke.io/aaaa-lookups` annotation to `skip` + ## Limitations There are a few functional limitations to FQDNNetworkPolicies: diff --git a/controllers/fqdnnetworkpolicy_controller.go b/controllers/fqdnnetworkpolicy_controller.go index b17d1cf..acd6c39 100644 --- a/controllers/fqdnnetworkpolicy_controller.go +++ b/controllers/fqdnnetworkpolicy_controller.go @@ -45,6 +45,7 @@ type FQDNNetworkPolicyReconciler struct { var ( ownerAnnotation = "fqdnnetworkpolicies.networking.gke.io/owned-by" deletePolicyAnnotation = "fqdnnetworkpolicies.networking.gke.io/delete-policy" + AaaaLookupsAnnotation = "fqdnnetworkpolicies.networking.gke.io/aaaa-lookups" finalizerName = "finalizer.fqdnnetworkpolicies.networking.gke.io" // TODO make retry configurable retry = time.Second * time.Duration(10) @@ -465,36 +466,39 @@ func (r *FQDNNetworkPolicyReconciler) getNetworkPolicyEgressRules(ctx context.Co } } } - - // AAAA records - m6 := new(dns.Msg) - m6.SetQuestion(f, dns.TypeAAAA) - - // TODO: We're always using the first nameserver. Should we do - // something different? Note from Jens: - // by default only if options rotate is set in resolv.conf - // they are rotated. Otherwise the first is used, after a (5s) - // timeout the next etc. So this is not too bad for now. - r6, _, err := c.Exchange(m6, "["+ns[0]+"]:53") - if err != nil { - log.Error(err, "unable to resolve "+f) - continue - } - if len(r6.Answer) == 0 { - log.V(1).Info("could not find AAAA record for " + f) - } - for _, ans := range r6.Answer { - if t, ok := ans.(*dns.AAAA); ok { - // Adding a peer per answer - peers = append(peers, networking.NetworkPolicyPeer{ - IPBlock: &networking.IPBlock{CIDR: t.AAAA.String() + "/128"}}) - // We want the next sync for the FQDNNetworkPolicy to happen - // just after the TTL of the DNS record has expired. - // Because a single FQDNNetworkPolicy may have different DNS - // records with different TTLs, we pick the lowest one - // and resynchronise after that. - if ans.Header().Ttl < nextSync { - nextSync = ans.Header().Ttl + if fqdnNetworkPolicy.Annotations[AaaaLookupsAnnotation] == "skip" { + log.Info("NetworkPolicy has AAAA lookups policy set to skip, not resolving AAAA records") + } else { + // AAAA records + m6 := new(dns.Msg) + m6.SetQuestion(f, dns.TypeAAAA) + + // TODO: We're always using the first nameserver. Should we do + // something different? Note from Jens: + // by default only if options rotate is set in resolv.conf + // they are rotated. Otherwise the first is used, after a (5s) + // timeout the next etc. So this is not too bad for now. + r6, _, err := c.Exchange(m6, "["+ns[0]+"]:53") + if err != nil { + log.Error(err, "unable to resolve "+f) + continue + } + if len(r6.Answer) == 0 { + log.V(1).Info("could not find AAAA record for " + f) + } + for _, ans := range r6.Answer { + if t, ok := ans.(*dns.AAAA); ok { + // Adding a peer per answer + peers = append(peers, networking.NetworkPolicyPeer{ + IPBlock: &networking.IPBlock{CIDR: t.AAAA.String() + "/128"}}) + // We want the next sync for the FQDNNetworkPolicy to happen + // just after the TTL of the DNS record has expired. + // Because a single FQDNNetworkPolicy may have different DNS + // records with different TTLs, we pick the lowest one + // and resynchronise after that. + if ans.Header().Ttl < nextSync { + nextSync = ans.Header().Ttl + } } } } From aac9591b8351754090ff24eb80f296146d96505a Mon Sep 17 00:00:00 2001 From: Karsten Brusch Date: Thu, 1 Dec 2022 21:39:20 +0100 Subject: [PATCH 2/5] fix typo --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 2a685da..adde538 100644 --- a/README.md +++ b/README.md @@ -57,7 +57,7 @@ To prevent this behavior, set the `fqdnnetworkpolicies.networking.gke.io/delete- NetworkPolicy. There might be scenarios where IPv6 AAAA lookups are not desired or supported in the resulting NetworkPolicy. -To skip AAAA loopups set the `fqdnnetworkpolicies.networking.gke.io/aaaa-lookups` annotation to `skip` +To skip AAAA lookups set the `fqdnnetworkpolicies.networking.gke.io/aaaa-lookups` annotation to `skip` ## Limitations From 8b4c02c69aba6d631089920afef9599cb6587db0 Mon Sep 17 00:00:00 2001 From: Karsten Brusch Date: Thu, 1 Dec 2022 21:45:20 +0100 Subject: [PATCH 3/5] comments and precise logging --- controllers/fqdnnetworkpolicy_controller.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/controllers/fqdnnetworkpolicy_controller.go b/controllers/fqdnnetworkpolicy_controller.go index acd6c39..c7df984 100644 --- a/controllers/fqdnnetworkpolicy_controller.go +++ b/controllers/fqdnnetworkpolicy_controller.go @@ -466,8 +466,9 @@ func (r *FQDNNetworkPolicyReconciler) getNetworkPolicyEgressRules(ctx context.Co } } } + // check for AAAA lookups skip annotation if fqdnNetworkPolicy.Annotations[AaaaLookupsAnnotation] == "skip" { - log.Info("NetworkPolicy has AAAA lookups policy set to skip, not resolving AAAA records") + log.Info("FQDNNetworkPolicy has AAAA lookups policy set to skip, not resolving AAAA records") } else { // AAAA records m6 := new(dns.Msg) From f349a424f8d4eb22009bb89636cda2e0f09d0fc0 Mon Sep 17 00:00:00 2001 From: Karsten Brusch Date: Fri, 2 Dec 2022 08:07:51 +0100 Subject: [PATCH 4/5] adding test --- api/v1alpha3/fqdnnetworkpolicy_testing.go | 4 + ...etworkpolicy_valid_aaaalookupsskipped.yaml | 32 ++++++++ .../fqdnnetworkpolicy_controller_test.go | 78 +++++++++++++++++++ 3 files changed, 114 insertions(+) create mode 100644 config/samples/networking_v1alpha3_fqdnnetworkpolicy_valid_aaaalookupsskipped.yaml diff --git a/api/v1alpha3/fqdnnetworkpolicy_testing.go b/api/v1alpha3/fqdnnetworkpolicy_testing.go index 35d7832..ac190c4 100644 --- a/api/v1alpha3/fqdnnetworkpolicy_testing.go +++ b/api/v1alpha3/fqdnnetworkpolicy_testing.go @@ -65,6 +65,10 @@ func (r *FQDNNetworkPolicy) GetValidNonExistentFQDNResource() *FQDNNetworkPolicy return r.LoadResource("./config/samples/networking_v1alpha3_fqdnnetworkpolicy_valid_nonexistentfqdn.yaml") } +func (r *FQDNNetworkPolicy) GetValidAaaaLookupsSkippedResource() *FQDNNetworkPolicy { + return r.LoadResource("./config/samples/networking_v1alpha3_fqdnnetworkpolicy_valid_aaaalookupsskipped.yaml") +} + func (r *FQDNNetworkPolicy) GetInvalidResource() *FQDNNetworkPolicy { return r.LoadResource("./config/samples/networking_v1alpha3_fqdnnetworkpolicy_invalid.yaml") } diff --git a/config/samples/networking_v1alpha3_fqdnnetworkpolicy_valid_aaaalookupsskipped.yaml b/config/samples/networking_v1alpha3_fqdnnetworkpolicy_valid_aaaalookupsskipped.yaml new file mode 100644 index 0000000..4515fa1 --- /dev/null +++ b/config/samples/networking_v1alpha3_fqdnnetworkpolicy_valid_aaaalookupsskipped.yaml @@ -0,0 +1,32 @@ +# Copyright 2022 Google LLC. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: networking.gke.io/v1alpha3 +kind: FQDNNetworkPolicy +metadata: + name: fqdnnetworkpolicy-valid + annotations: + fqdnnetworkpolicies.networking.gke.io/aaaa-lookups: "skip" +spec: + podSelector: {} + policyTypes: + - Egress + egress: + - to: + - fqdns: + - github.com + - gitlab.com + ports: + - port: 443 + protocol: TCP diff --git a/controllers/fqdnnetworkpolicy_controller_test.go b/controllers/fqdnnetworkpolicy_controller_test.go index b258abb..17ff107 100644 --- a/controllers/fqdnnetworkpolicy_controller_test.go +++ b/controllers/fqdnnetworkpolicy_controller_test.go @@ -350,6 +350,84 @@ var _ = Describe("FQDNNetworkPolicy controller", func() { Expect(k8sClient.Delete(ctx, &networkPolicy)).Should(Succeed()) }) }) + Context("when the NetworkPolicy has the aaaa-lookups annotation set to skip", func() { + ctx := context.Background() + fqdnNetworkPolicy := getFQDNNetworkPolicy("context5", "default") + fqdnNetworkPolicy.GetValidAaaaLookupsSkippedResource() + + nn := types.NamespacedName{ + Namespace: fqdnNetworkPolicy.Namespace, + Name: fqdnNetworkPolicy.Name, + } + It("Shouldn't lookup AAAA records", func() { + Expect(k8sClient.Create(ctx, &fqdnNetworkPolicy)).Should(Succeed()) + Eventually(func() error { + networkPolicy := networking.NetworkPolicy{} + return k8sClient.Get(ctx, nn, &networkPolicy) + }).Should(Succeed()) + + // check only ipv4 adresses are present + Eventually(func() error { + r := net.Resolver{} + // computing the expected IPs in the NetworkPolicy + // from the FQDNs in the FQDNNetworkPolicy + // We use a different lib for resolving than the one in the main code + expectedIPs := []string{} + for _, fer := range fqdnNetworkPolicy.Spec.Egress { + for _, to := range fer.To { + for _, fqdn := range to.FQDNs { + ip4s, err := r.LookupIP(ctx, "ip4", fqdn) + if err != nil { + continuing := false + derr, ok := err.(*net.DNSError) + if ok && derr.IsNotFound { + continuing = true + } + aerr, ok := err.(*net.AddrError) + if ok && aerr.Err == "no suitable address found" { + continuing = true + } + if !continuing { + return err + } + } + for _, ip := range ip4s { + expectedIPs = append(expectedIPs, ip.String()+"/32") + } + } + } + } + // Getting the NetworkPolicy + networkPolicy := networking.NetworkPolicy{} + err := k8sClient.Get(ctx, nn, &networkPolicy) + if err != nil { + return err + } + if len(networkPolicy.Spec.PolicyTypes) != 1 || + networkPolicy.Spec.PolicyTypes[0] != networking.PolicyTypeEgress { + return errors.New("Unexpected PolicyType: " + fmt.Sprintf("%v", networkPolicy.Spec.PolicyTypes) + + ". Expected PolicyType: [Egress]") + } + total := 0 + for _, egressRule := range networkPolicy.Spec.Egress { + // checking that every CIDR in the NetworkPolicy + // is in the expect list of IPs + total += len(egressRule.To) + for _, to := range egressRule.To { + // removing the /32 at the end of the CIDR + if !containsString(expectedIPs, string(to.IPBlock.CIDR)) { + return errors.New("Unexpected IP in NetworkPolicy: " + string(to.IPBlock.CIDR) + + ". Expected IPs: " + fmt.Sprint(expectedIPs)) + } + } + } + if total != len(expectedIPs) { + return errors.New("Some expected IPs are not present in the NetworkPolicy") + } + return nil + }).Should(Succeed()) + }) + }) }) }) From 7b095fedc3d35d5c4f9eda52b54d37c2965db3cd Mon Sep 17 00:00:00 2001 From: Karsten Brusch Date: Fri, 2 Dec 2022 08:08:17 +0100 Subject: [PATCH 5/5] apply suggestions from PR --- README.md | 3 +-- controllers/fqdnnetworkpolicy_controller.go | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index adde538..f322416 100644 --- a/README.md +++ b/README.md @@ -56,8 +56,7 @@ By default, the NetworkPolicy associated with a FQDNNetworkPolicy gets deleted w To prevent this behavior, set the `fqdnnetworkpolicies.networking.gke.io/delete-policy` annotation to `abandon` on the NetworkPolicy. -There might be scenarios where IPv6 AAAA lookups are not desired or supported in the resulting NetworkPolicy. -To skip AAAA lookups set the `fqdnnetworkpolicies.networking.gke.io/aaaa-lookups` annotation to `skip` +You can disable AAAA lookups for an FQDNNetworkPolicy by setting the `fqdnnetworkpolicies.networking.gke.io/aaaa-lookups` annotation to `skip`. The resulting NetworkPolicy will not contain any IPv6 addresses. ## Limitations diff --git a/controllers/fqdnnetworkpolicy_controller.go b/controllers/fqdnnetworkpolicy_controller.go index c7df984..96bb4a8 100644 --- a/controllers/fqdnnetworkpolicy_controller.go +++ b/controllers/fqdnnetworkpolicy_controller.go @@ -45,7 +45,7 @@ type FQDNNetworkPolicyReconciler struct { var ( ownerAnnotation = "fqdnnetworkpolicies.networking.gke.io/owned-by" deletePolicyAnnotation = "fqdnnetworkpolicies.networking.gke.io/delete-policy" - AaaaLookupsAnnotation = "fqdnnetworkpolicies.networking.gke.io/aaaa-lookups" + aaaaLookupsAnnotation = "fqdnnetworkpolicies.networking.gke.io/aaaa-lookups" finalizerName = "finalizer.fqdnnetworkpolicies.networking.gke.io" // TODO make retry configurable retry = time.Second * time.Duration(10) @@ -467,7 +467,7 @@ func (r *FQDNNetworkPolicyReconciler) getNetworkPolicyEgressRules(ctx context.Co } } // check for AAAA lookups skip annotation - if fqdnNetworkPolicy.Annotations[AaaaLookupsAnnotation] == "skip" { + if fqdnNetworkPolicy.Annotations[aaaaLookupsAnnotation] == "skip" { log.Info("FQDNNetworkPolicy has AAAA lookups policy set to skip, not resolving AAAA records") } else { // AAAA records