Add support for DNSNameResolver #4045

arkadeepsen · 2023-12-09T16:31:36Z

This PR adds the support for DNSNameResolver in ovnk as per the enhancement proposal openshift/enhancements#1335.

This feature is enabled via a new flag enable-dns-name-resolver. However, enable-egress-firewall flag should also be set for this feature to be enabled.

cluster-manager watches for events related to EgressFirewall objects and creates/deletes the DNSNameResolver objects based on the events.

ovnkube-controller watches DNSNameResolver to get the latest IP addresses associated with a DNS name. resolver is initialized instead ofegressFirewallDNS and it is used to create/destroy/update address sets related to DNS names.

If the enable-dns-name-resolver is not enabled, then the behavior of ovnk doesn't change.

coveralls · 2023-12-09T16:56:24Z

coverage: 52.689% (+0.3%) from 52.418%
when pulling 6788da1 on arkadeepsen:dnsnameresolver
into 9bf6a48 on ovn-org:master.

npinaeva

Thanks for this work @arkadeepsen !
Did a first round, feel free to ping me for more info/help on these comments :)

go-controller/pkg/clustermanager/clustermanager.go

go-controller/pkg/clustermanager/dnsnameresolver/controller.go

go-controller/pkg/ovn/egressfirewall_resolver.go

go-controller/pkg/clustermanager/dnsnameresolver_test.go

go-controller/pkg/kube/kube.go

go-controller/pkg/ovn/egressfirewall_resolver.go

go-controller/pkg/util/util.go

arkadeepsen · 2024-02-01T07:03:21Z

@npinaeva I have made the suggested changes. PTAL.

npinaeva

nice update! some more comments :)

go-controller/pkg/kube/kube.go

go-controller/pkg/clustermanager/dnsnameresolver/controller.go

go-controller/pkg/ovn/default_network_controller.go

go-controller/pkg/ovn/dnsnameresolver.go

netlify · 2024-03-26T18:43:05Z

✅ Deploy Preview for subtle-torrone-bb0c84 ready!

Name	Link
🔨 Latest commit	`c9a2169`
🔍 Latest deploy log	https://app.netlify.com/sites/subtle-torrone-bb0c84/deploys/662936d36165a50009a01690
😎 Deploy Preview	https://deploy-preview-4045--subtle-torrone-bb0c84.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

go-controller/pkg/clustermanager/dnsnameresolver/controller.go

go-controller/pkg/clustermanager/dnsnameresolver/resolver_info.go

go-controller/pkg/factory/factory.go

go-controller/pkg/kube/kube.go

npinaeva

getting better every time!

go-controller/pkg/factory/factory.go

go-controller/pkg/util/util.go

npinaeva

nice progress!

go-controller/pkg/ovn/dns-name-resolver/dns_name_resolver.go

go-controller/pkg/ovn/dnsnameresolver.go

go-controller/pkg/ovn/egressfirewall.go

go-controller/pkg/clustermanager/dnsnameresolver/resolver_info.go

npinaeva · 2024-04-11T14:26:44Z

go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go

+				Err: fmt.Errorf("error adding DNS name %s: addressSetFactory is nil", request.DNSName),
+			}
+		}
+		asIndex := GetEgressFirewallDNSAddrSetDbIDs(request.DNSName, extEgDNS.controllerName)


note to self: we may need to rename addrset owner to DNSName instead of egress firewall at some point

go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go

go-controller/pkg/ovn/egressfirewall.go

go-controller/pkg/ovn/default_network_controller.go

go-controller/pkg/ovn/dns-name-resolver/egressfirewall_dns.go

go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go

go-controller/pkg/clustermanager/dnsnameresolver/controller.go

npinaeva

interface looks great!

npinaeva

Looks like we use underscores for folder names, so I think we should rename ovn/dns-name-resolver folder.
Also, it may be a good time to start splitting this into commits, I am thinking we should have separate commits for

vendor bump
crd change
cluster manager side
everything else

go-controller/pkg/ovn/dns-name-resolver/dnsnameresolver-controller.go

go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go

go-controller/pkg/clustermanager/dnsnameresolver/controller.go

go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go

go-controller/pkg/util/dnsnameresolver.go

go-controller/pkg/ovn/egressfirewall_test.go

go-controller/pkg/ovn/dns-name-resolver/dnsnameresolver-controller.go

go-controller/pkg/ovn/egressfirewall_test.go

go-controller/pkg/ovn/egressfirewall.go

go-controller/pkg/ovn/egressfirewall_test.go

npinaeva · 2024-04-22T16:58:12Z

btw #4251 got merged

arkadeepsen · 2024-04-23T05:48:14Z

btw #4251 got merged

@npinaeva updated code accordingly

go-controller/pkg/clustermanager/dnsnameresolver/controller_test.go

go-controller/pkg/ovn/dns_name_resolver/dns.go

go-controller/pkg/ovn/dns_name_resolver/dns_test.go

go-controller/pkg/ovn/default_network_controller.go

go-controller/pkg/ovn/dns_name_resolver/external_dns.go

go-controller/pkg/ovn/egressfirewall.go

go-controller/pkg/ovn/dns_name_resolver/dns.go

go-controller/pkg/ovn/dns_name_resolver/external_dns_test.go

go-controller/pkg/ovn/egressfirewall_test.go

npinaeva

thank you for patience and a great contribution @arkadeepsen!

arkadeepsen · 2024-05-07T04:12:15Z

thank you for patience and a great contribution @arkadeepsen!

Thanks @npinaeva for providing your feedback and suggestions to the PR.

trozet

Can we add e2es to make sure this new feature works?

trozet · 2024-05-08T02:54:07Z

go-controller/pkg/clustermanager/dnsnameresolver/controller.go

+// objects in the cluster.
+type Controller struct {
+	lock             sync.Mutex
+	ocpNetworkClient ocpnetworkclientset.Interface


so does this feature work without openshift? I think there should be some update to our docs to talk about this new feature.

I added the doc explaining how to use the feature.

trozet · 2024-05-08T03:27:49Z

go-controller/pkg/libovsdb/util/address_set.go

+	// names and mark them as unreferenced.
+	addrSetReferenced := map[string]bool{}
+	predicate := libovsdbops.GetPredicate[*nbdb.AddressSet](predicateIDs, func(item *nbdb.AddressSet) bool {
+		fmt.Printf("Address Set: %s\n", item.Name)


should remove this

trozet · 2024-05-08T03:29:02Z

go-controller/pkg/libovsdb/util/address_set.go

+	// Set addrSetReferenced[addrSetName] = true if referencing acl exists.
+	_, err = libovsdbops.FindACLsWithPredicate(nbClient, func(item *nbdb.ACL) bool {
+		for addrSetName := range addrSetReferenced {
+			fmt.Printf("ACL: %s\n", item.Match)


trozet · 2024-05-08T03:29:31Z

go-controller/pkg/libovsdb/util/address_set.go

+	// acl then delete it.
+	ops := []ovsdb.Operation{}
+	for addrSetName, isReferenced := range addrSetReferenced {
+		fmt.Printf("Address Set: %s, is referenced: %t\n", addrSetName, isReferenced)


trozet · 2024-05-08T03:30:25Z

go-controller/pkg/libovsdb/util/address_set.go

+	// Delete the stale address sets.
+	_, err = libovsdbops.TransactAndCheck(nbClient, ops)
+	if err != nil {
+		return fmt.Errorf("failed to trasact db ops: %v", err)


"failed to transact db ops to delete address sets..."

Updated the error message.

npinaeva · 2024-05-08T07:34:01Z

Can we add e2es to make sure this new feature works?

that is planned as a separate PR, as it requires some more work to add dnsresolver to the kind cluster here https://github.com/openshift/coredns-ocp-dnsnameresolver. The feature will need only the operator provided by ^ to work. In openshift it is deployed as a part of cluster-dns-operator, but we need some more time to make it work with kind.

Signed-off-by: arkadeepsen <arsen@redhat.com>

…upport wildcard DNS name Signed-off-by: arkadeepsen <arsen@redhat.com>

Signed-off-by: arkadeepsen <arsen@redhat.com>

npinaeva · 2024-05-08T15:37:29Z

docs/features/network-security-controls/dns-name-resolution.md

+The DNS name resolver should be configured in the following components for it to work
+in a cluster:
+
+### [DNSNameResolver operator](https://github.com/openshift/coredns-ocp-dnsnameresolver/tree/main/operator)


I think we will automate this in our kind.sh script, so maybe we can omit this section for now

Do we not need these details if someone wants to deploy in any other cluster apart from kind?

I think its a good idea to keep it for now until we have automated install. We also have the helm install now so we will need to make sure it installs with kind and helm. @flavio-fernandes fyi

arkadeepsen requested review from trozet, dcbw, girishmg and jcaamano as code owners December 9, 2023 16:31

arkadeepsen mentioned this pull request Dec 9, 2023

CFE-888: Enable DNSNameResolver feature-gate openshift/cluster-network-operator#2131

Merged

arkadeepsen requested a review from tssurya as a code owner December 21, 2023 14:55

arkadeepsen force-pushed the dnsnameresolver branch from d7d701d to 9bd6ecb Compare December 21, 2023 15:03

npinaeva reviewed Jan 11, 2024

View reviewed changes

arkadeepsen force-pushed the dnsnameresolver branch from 9bd6ecb to ce53e84 Compare January 12, 2024 10:34

arkadeepsen force-pushed the dnsnameresolver branch from 3b3d8f8 to 8d4dcf2 Compare January 30, 2024 12:31

npinaeva reviewed Feb 13, 2024

View reviewed changes

tssurya added feature/egress-firewall All issues related to egress firewall kind/feature All issues/PRs that are new features labels Mar 14, 2024

tssurya added this to the v1.0.0 milestone Mar 14, 2024

arkadeepsen force-pushed the dnsnameresolver branch from c3b6f9b to ffbc8c6 Compare March 26, 2024 18:42

arkadeepsen force-pushed the dnsnameresolver branch from ffbc8c6 to 6ccaadb Compare March 26, 2024 18:56

npinaeva reviewed Apr 4, 2024

View reviewed changes

go-controller/pkg/factory/factory.go Outdated Show resolved Hide resolved

go-controller/pkg/factory/factory.go Outdated Show resolved Hide resolved

go-controller/pkg/factory/factory.go Outdated Show resolved Hide resolved

go-controller/pkg/util/util.go Outdated Show resolved Hide resolved

arkadeepsen force-pushed the dnsnameresolver branch from 6ccaadb to abb6f51 Compare April 11, 2024 08:26

npinaeva reviewed Apr 11, 2024

View reviewed changes

arkadeepsen force-pushed the dnsnameresolver branch from abb6f51 to 99eea14 Compare April 15, 2024 07:22

npinaeva reviewed Apr 15, 2024

View reviewed changes

arkadeepsen force-pushed the dnsnameresolver branch from 99eea14 to 7607777 Compare April 16, 2024 15:25

npinaeva reviewed Apr 17, 2024

View reviewed changes

arkadeepsen force-pushed the dnsnameresolver branch from 1675617 to 1a3edd1 Compare April 23, 2024 05:43

arkadeepsen force-pushed the dnsnameresolver branch 2 times, most recently from e3888c0 to cecb24c Compare April 23, 2024 21:57

npinaeva reviewed Apr 24, 2024

View reviewed changes

arkadeepsen force-pushed the dnsnameresolver branch 3 times, most recently from e38a75c to c9a2169 Compare April 24, 2024 16:44

npinaeva reviewed Apr 25, 2024

View reviewed changes

go-controller/pkg/ovn/default_network_controller.go Show resolved Hide resolved

arkadeepsen force-pushed the dnsnameresolver branch from c9a2169 to a3fe52a Compare April 29, 2024 19:34

npinaeva reviewed May 1, 2024

View reviewed changes

go-controller/pkg/ovn/dns_name_resolver/external_dns.go Outdated Show resolved Hide resolved

arkadeepsen force-pushed the dnsnameresolver branch from a3fe52a to 90bbe2f Compare May 3, 2024 18:54

arkadeepsen requested a review from a team as a code owner May 3, 2024 18:54

npinaeva reviewed May 6, 2024

View reviewed changes

arkadeepsen force-pushed the dnsnameresolver branch from 90bbe2f to 6788da1 Compare May 6, 2024 15:02

npinaeva previously approved these changes May 6, 2024

View reviewed changes

trozet requested changes May 8, 2024

View reviewed changes

arkadeepsen added 5 commits May 8, 2024 16:31

Bump openshift/api and openshift/client-go versions

ff43983

Signed-off-by: arkadeepsen <arsen@redhat.com>

Update kubebuilder validation of DNSName field of EgressFirewall to s…

37d4ace

…upport wildcard DNS name Signed-off-by: arkadeepsen <arsen@redhat.com>

Add support for DNSNameResolver in clustermanager

b1c70fa

Signed-off-by: arkadeepsen <arsen@redhat.com>

Add support for DNSNameResolver in ovnkube-controller

bac9a79

Signed-off-by: arkadeepsen <arsen@redhat.com>

Add doc for DNSNameResolver

0f7ff0f

Signed-off-by: arkadeepsen <arsen@redhat.com>

arkadeepsen dismissed npinaeva’s stale review via 0f7ff0f May 8, 2024 15:22

arkadeepsen force-pushed the dnsnameresolver branch from 6788da1 to 0f7ff0f Compare May 8, 2024 15:22

npinaeva reviewed May 8, 2024

View reviewed changes

trozet approved these changes May 8, 2024

View reviewed changes

trozet merged commit cc2006c into ovn-org:master May 8, 2024
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for DNSNameResolver #4045

Add support for DNSNameResolver #4045

arkadeepsen commented Dec 9, 2023 •

edited

Loading

coveralls commented Dec 9, 2023 •

edited

Loading

npinaeva left a comment

arkadeepsen commented Feb 1, 2024

npinaeva left a comment

netlify bot commented Mar 26, 2024 •

edited

Loading

npinaeva left a comment

npinaeva left a comment

npinaeva Apr 11, 2024

npinaeva left a comment

npinaeva left a comment

npinaeva commented Apr 22, 2024

arkadeepsen commented Apr 23, 2024

npinaeva left a comment

arkadeepsen commented May 7, 2024

trozet left a comment

trozet May 8, 2024

arkadeepsen May 8, 2024

trozet May 8, 2024

arkadeepsen May 8, 2024

trozet May 8, 2024

arkadeepsen May 8, 2024

trozet May 8, 2024

arkadeepsen May 8, 2024

trozet May 8, 2024

arkadeepsen May 8, 2024

npinaeva commented May 8, 2024

npinaeva May 8, 2024

arkadeepsen May 8, 2024

trozet May 8, 2024

Add support for DNSNameResolver #4045

Add support for DNSNameResolver #4045

Conversation

arkadeepsen commented Dec 9, 2023 • edited Loading

coveralls commented Dec 9, 2023 • edited Loading

npinaeva left a comment

Choose a reason for hiding this comment

arkadeepsen commented Feb 1, 2024

npinaeva left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 26, 2024 • edited Loading

✅ Deploy Preview for subtle-torrone-bb0c84 ready!

npinaeva left a comment

Choose a reason for hiding this comment

npinaeva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npinaeva left a comment

Choose a reason for hiding this comment

npinaeva left a comment

Choose a reason for hiding this comment

npinaeva commented Apr 22, 2024

arkadeepsen commented Apr 23, 2024

npinaeva left a comment

Choose a reason for hiding this comment

arkadeepsen commented May 7, 2024

trozet 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

npinaeva commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkadeepsen commented Dec 9, 2023 •

edited

Loading

coveralls commented Dec 9, 2023 •

edited

Loading

netlify bot commented Mar 26, 2024 •

edited

Loading