Add DNSPolicy Routing Strategy #629

mikenairn · 2023-10-18T12:00:37Z

Adds a routing strategy field to the DNSPolicy spec that determines how the policy with generate endpoints for any created DNSRecords.

Two routing strategies are allowed, simple and loadbalanced. Simple will creates a single DNS record (A or CNAME) for each listener/hostname with all ip/hostnames as targets. LoadBalanced works as before by creating a more complex record structure with CNAMES and A records using Geo and Weighted routing strategies to achieve loadbalancing functionality.

The strategy field is currently marked as immutable and it should not be changed after initial DNSPolicy creation.

closes #655

maleck13 · 2023-10-19T10:24:15Z

want to merge this to single-cluster branch?
I almost have a pr now for a flag to turn off OCM integration

… IPAddress MGC types.

…name and IPAddress MGC types.

maleck13 · 2023-11-08T14:57:32Z

pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go

@@ -205,11 +203,14 @@ func getClusterGatewayAddresses(gw *gatewayv1beta1.Gateway) map[string][]gateway
 			}
 		}

-		gatewayAddresses = append(gatewayAddresses, gatewayv1beta1.GatewayAddress{
+		if _, ok := clusterAddrs[cluster]; !ok {


yeah this is what I expected nice!

maleck13 · 2023-11-08T15:40:24Z

This is looking good to me, we need to wait on @makslion PR to allow setting things like geo and weight in a single cluster (IE to something custom like what we did with ManagedCLuster)?

mikenairn · 2023-11-08T16:04:16Z

This is looking good to me, we need to wait on @makslion PR to allow setting things like geo and weight in a single cluster (IE to something custom like what we did with ManagedCLuster)?

It should already be possible to do this by adding the same labels to the gateway itself in a single cluster context . Admittedly there is no test for it, but would be fairly easy to add now.

mikenairn · 2023-11-08T18:13:41Z

pkg/apis/v1alpha1/dnspolicy_types.go

+	// +kubebuilder:validation:Enum=simple;loadbalanced
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="RoutingStrategy is immutable"
+	// +kubebuilder:default=loadbalanced
+	RoutingStrategy RoutingStrategy `json:"routingStrategy"`


routingStrategy is required, but having the default means you don't have to specify it and will default to "loadbalanced" (The current behaviour)

pkg/controllers/dnspolicy/dns_helper.go

bundle/manifests/kuadrant.io_dnspolicies.yaml

maleck13 · 2023-11-10T08:53:25Z

pkg/controllers/dnspolicy/dns_helper.go

+
+// getSimpleEndpoints returns the endpoints for the given MultiClusterGatewayTarget using the simple routing strategy
+
+func (dh *dnsHelper) getSimpleEndpoints(mcgTarget *dns.MultiClusterGatewayTarget, hostname string, currentEndpoints map[string]*v1alpha1.Endpoint) []*v1alpha1.Endpoint {


might want to change the name of this type? Bit confusing when we are also working in single cluster

I assume you are talking about MultiClusterGatewayTarget and yes it needs renamed. Won't be in this PR though.

maleck13 · 2023-11-10T09:25:17Z

test/policy_integration/dnspolicy_controller_test.go

+						))
+				}, TestTimeoutMedium, TestRetryIntervalMedium, ctx).Should(Succeed())
+			})
+


This is a pretty great test, but there is a lot going on in this file now. Perhaps as a follow up we could think about whether we should split this test in someway to make it easier to read / mantain? IE single vs multi or something

yes, its far too long now, and a lot more could be added so we need to look at how we want these to be separated out. Probably at the very least a separate test file for multi/single and simple/loadbalanced contexts.

maleck13

Happy to merge this, some minor comments.

Adds a routingStrategy field to the DNSPolicy spec that determines how the policy with generate endpoints for any created DNSRecords. Two strategies are allowed, `simple` and `loadbalanced`. Simple will create a single DNS record (A or CNAME) for each listener/hostname with all ip/hostnames as targets. LoadBalanced works as before by creating a more complex record structure with CNAMES and A records using Geo and Weighted routing strategies to achieve loadbalancing functionality. The routingStrategy field is currently marked as immutable and it should not be changed after initial DNSPolicy creation.

Update DNSPolicy test to include contexts for routing strategy + single and multi cluster status. Rename builders to <ResourceName>Builder e.g. GatewayBuilder, TLSPolicyBuilder etc... Remove references to "placed" where possible Rename most constants to make them more consistent with what they are.

Inadvertently added here https://github.com/Kuadrant/multicluster-gateway-controller/pull/412/files#diff-162ceed42032f121fd671b5884a611158c00f31e292b7602e48d5671ca7fe01fR25

openshift-ci · 2023-11-10T09:48:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maleck13, mikenairn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [maleck13,mikenairn]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

maleck13 · 2023-11-10T09:48:25Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress label Oct 18, 2023

mikenairn had a problem deploying to e2e-internal October 18, 2023 12:00 — with GitHub Actions Failure

openshift-ci bot added the approved label Oct 18, 2023

mikenairn added a commit that referenced this pull request Oct 19, 2023

fix(#629): Ensure address type is set correctly for both Hostname and…

f4d4399

… IPAddress MGC types.

mikenairn added a commit to mikenairn/multi-cluster-traffic-controller that referenced this pull request Oct 19, 2023

fix(Kuadrant#629): Ensure address type is set correctly for both Host…

d53d848

…name and IPAddress MGC types.

mikenairn had a problem deploying to e2e-internal October 19, 2023 15:30 — with GitHub Actions Failure

mikenairn added a commit that referenced this pull request Oct 19, 2023

bundle(#629): make bundle

3f0de51

mikenairn changed the title ~~Add DNSPolicy Strategy~~ Add DNSPolicy Routing Strategy Nov 6, 2023

mikenairn force-pushed the simple_dnspolicy_strategy branch from d53d848 to 66668bd Compare November 6, 2023 13:11

mikenairn had a problem deploying to e2e-internal November 6, 2023 13:11 — with GitHub Actions Failure

mikenairn temporarily deployed to e2e-internal November 6, 2023 17:13 — with GitHub Actions Inactive

mikenairn force-pushed the simple_dnspolicy_strategy branch from 83916f0 to 69ff542 Compare November 6, 2023 18:16

mikenairn temporarily deployed to e2e-internal November 6, 2023 18:16 — with GitHub Actions Inactive

mikenairn temporarily deployed to e2e-internal November 7, 2023 12:34 — with GitHub Actions Inactive

mikenairn force-pushed the simple_dnspolicy_strategy branch from 0ccee81 to b00c61c Compare November 8, 2023 10:45

mikenairn temporarily deployed to e2e-internal November 8, 2023 10:45 — with GitHub Actions Inactive

mikenairn force-pushed the simple_dnspolicy_strategy branch from b00c61c to 206a8ae Compare November 8, 2023 11:00

mikenairn temporarily deployed to e2e-internal November 8, 2023 11:00 — with GitHub Actions Inactive

mikenairn force-pushed the simple_dnspolicy_strategy branch from 206a8ae to cee6e58 Compare November 8, 2023 14:20

mikenairn temporarily deployed to e2e-internal November 8, 2023 14:20 — with GitHub Actions Inactive

mikenairn marked this pull request as ready for review November 8, 2023 14:23

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 8, 2023

maksymvavilov mentioned this pull request Nov 8, 2023

Update DNSPolicy reconciler to function with non mgc gateways (e.g. istio) #496

Closed

2 tasks

maleck13 reviewed Nov 8, 2023

View reviewed changes

mikenairn force-pushed the simple_dnspolicy_strategy branch from cee6e58 to db29367 Compare November 8, 2023 17:45

mikenairn temporarily deployed to e2e-internal November 8, 2023 17:45 — with GitHub Actions Inactive

mikenairn commented Nov 8, 2023

View reviewed changes

mikenairn force-pushed the simple_dnspolicy_strategy branch from db29367 to 02c20cf Compare November 9, 2023 13:15

mikenairn temporarily deployed to e2e-internal November 9, 2023 13:16 — with GitHub Actions Inactive

mikenairn temporarily deployed to e2e-internal November 9, 2023 13:49 — with GitHub Actions Inactive

mikenairn force-pushed the simple_dnspolicy_strategy branch from 62b95d7 to 163eb6d Compare November 9, 2023 18:34

mikenairn had a problem deploying to e2e-internal November 9, 2023 18:34 — with GitHub Actions Failure

mikenairn temporarily deployed to e2e-internal November 9, 2023 20:09 — with GitHub Actions Inactive

mikenairn force-pushed the simple_dnspolicy_strategy branch from 163eb6d to 7cbe345 Compare November 10, 2023 08:19

mikenairn temporarily deployed to e2e-internal November 10, 2023 08:19 — with GitHub Actions Inactive

mikenairn commented Nov 10, 2023

View reviewed changes

pkg/controllers/dnspolicy/dns_helper.go Show resolved Hide resolved

maleck13 reviewed Nov 10, 2023

View reviewed changes

bundle/manifests/kuadrant.io_dnspolicies.yaml Outdated Show resolved Hide resolved

maleck13 reviewed Nov 10, 2023

View reviewed changes

maleck13 approved these changes Nov 10, 2023

View reviewed changes

mikenairn added 3 commits November 10, 2023 09:42

Remove authorino direct dependency

4744c83

Inadvertently added here https://github.com/Kuadrant/multicluster-gateway-controller/pull/412/files#diff-162ceed42032f121fd671b5884a611158c00f31e292b7602e48d5671ca7fe01fR25

mikenairn force-pushed the simple_dnspolicy_strategy branch from 7cbe345 to 4744c83 Compare November 10, 2023 09:42

mikenairn temporarily deployed to e2e-internal November 10, 2023 09:42 — with GitHub Actions Inactive

maleck13 approved these changes Nov 10, 2023

View reviewed changes

openshift-ci bot assigned maleck13 Nov 10, 2023

openshift-ci bot added the lgtm label Nov 10, 2023

openshift-merge-bot bot merged commit 910af6e into Kuadrant:main Nov 10, 2023
9 checks passed

mikenairn deleted the simple_dnspolicy_strategy branch November 10, 2023 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DNSPolicy Routing Strategy #629

Add DNSPolicy Routing Strategy #629

mikenairn commented Oct 18, 2023 •

edited

Loading

maleck13 commented Oct 19, 2023

maleck13 Nov 8, 2023

maleck13 commented Nov 8, 2023

mikenairn commented Nov 8, 2023

mikenairn Nov 8, 2023

maleck13 Nov 10, 2023

mikenairn Nov 10, 2023

maleck13 Nov 10, 2023

mikenairn Nov 10, 2023

maleck13 left a comment

openshift-ci bot commented Nov 10, 2023

maleck13 commented Nov 10, 2023


		// getSimpleEndpoints returns the endpoints for the given MultiClusterGatewayTarget using the simple routing strategy

		func (dh dnsHelper) getSimpleEndpoints(mcgTarget dns.MultiClusterGatewayTarget, hostname string, currentEndpoints map[string]v1alpha1.Endpoint) []v1alpha1.Endpoint {

Add DNSPolicy Routing Strategy #629

Add DNSPolicy Routing Strategy #629

Conversation

mikenairn commented Oct 18, 2023 • edited Loading

maleck13 commented Oct 19, 2023

maleck13 Nov 8, 2023

Choose a reason for hiding this comment

maleck13 commented Nov 8, 2023

mikenairn commented Nov 8, 2023

mikenairn Nov 8, 2023

Choose a reason for hiding this comment

maleck13 Nov 10, 2023

Choose a reason for hiding this comment

mikenairn Nov 10, 2023

Choose a reason for hiding this comment

maleck13 Nov 10, 2023

Choose a reason for hiding this comment

mikenairn Nov 10, 2023

Choose a reason for hiding this comment

maleck13 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 10, 2023

maleck13 commented Nov 10, 2023

mikenairn commented Oct 18, 2023 •

edited

Loading