From 5339c0c72c2cc6cf04b985223009cc03865db3b7 Mon Sep 17 00:00:00 2001
From: John Gardiner Myers <jgmyers@proofpoint.com>
Date: Fri, 11 Aug 2023 20:53:26 -0700
Subject: [PATCH] Fix reconciliation of provider-specific properties

---
 plan/plan.go      | 21 ++++++++---------
 plan/plan_test.go | 58 ++++++++++++++++++++++++++++-------------------
 2 files changed, 45 insertions(+), 34 deletions(-)

diff --git a/plan/plan.go b/plan/plan.go
index 5f9ef0cc51..4dc9517363 100644
--- a/plan/plan.go
+++ b/plan/plan.go
@@ -207,22 +207,21 @@ func shouldUpdateTTL(desired, current *endpoint.Endpoint) bool {
 func (p *Plan) shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint) bool {
 	desiredProperties := map[string]endpoint.ProviderSpecificProperty{}
 
-	if desired.ProviderSpecific != nil {
-		for _, d := range desired.ProviderSpecific {
-			desiredProperties[d.Name] = d
-		}
+	for _, d := range desired.ProviderSpecific {
+		desiredProperties[d.Name] = d
 	}
-	if current.ProviderSpecific != nil {
-		for _, c := range current.ProviderSpecific {
-			if d, ok := desiredProperties[c.Name]; ok {
-				return c.Value != d.Value
-			} else {
-				return c.Value != ""
+	for _, c := range current.ProviderSpecific {
+		if d, ok := desiredProperties[c.Name]; ok {
+			if c.Value != d.Value {
+				return true
 			}
+			delete(desiredProperties, c.Name)
+		} else {
+			return true
 		}
 	}
 
-	return false
+	return len(desiredProperties) > 0
 }
 
 // filterRecordsForPlan removes records that are not relevant to the planner.
diff --git a/plan/plan_test.go b/plan/plan_test.go
index e012aea52a..8776bcfc7a 100644
--- a/plan/plan_test.go
+++ b/plan/plan_test.go
@@ -155,6 +155,10 @@ func (suite *PlanTestSuite) SetupTest() {
 			endpoint.ResourceLabelKey: "ingress/default/bar-127",
 		},
 		ProviderSpecific: endpoint.ProviderSpecific{
+			endpoint.ProviderSpecificProperty{
+				Name:  "alias",
+				Value: "false",
+			},
 			endpoint.ProviderSpecificProperty{
 				Name:  "external-dns.alpha.kubernetes.io/cloudflare-proxied",
 				Value: "true",
@@ -173,6 +177,10 @@ func (suite *PlanTestSuite) SetupTest() {
 				Name:  "external-dns.alpha.kubernetes.io/cloudflare-proxied",
 				Value: "false",
 			},
+			endpoint.ProviderSpecificProperty{
+				Name:  "alias",
+				Value: "false",
+			},
 		},
 	}
 	suite.bar127AWithProviderSpecificUnset = &endpoint.Endpoint{
@@ -182,7 +190,12 @@ func (suite *PlanTestSuite) SetupTest() {
 		Labels: map[string]string{
 			endpoint.ResourceLabelKey: "ingress/default/bar-127",
 		},
-		ProviderSpecific: endpoint.ProviderSpecific{},
+		ProviderSpecific: endpoint.ProviderSpecific{
+			endpoint.ProviderSpecificProperty{
+				Name:  "alias",
+				Value: "false",
+			},
+		},
 	}
 	suite.bar192A = &endpoint.Endpoint{
 		DNSName:    "bar",
@@ -364,28 +377,27 @@ func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificRemoval() {
 	validateEntries(suite.T(), changes.Delete, expectedDelete)
 }
 
-// todo: this is currently an expect-fail
-//func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificAddition() {
-//	current := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificUnset}
-//	desired := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificTrue}
-//	expectedCreate := []*endpoint.Endpoint{}
-//	expectedUpdateOld := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificUnset}
-//	expectedUpdateNew := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificTrue}
-//	expectedDelete := []*endpoint.Endpoint{}
-//
-//	p := &Plan{
-//		Policies:       []Policy{&SyncPolicy{}},
-//		Current:        current,
-//		Desired:        desired,
-//		ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME},
-//	}
-//
-//	changes := p.Calculate().Changes
-//	validateEntries(suite.T(), changes.Create, expectedCreate)
-//	validateEntries(suite.T(), changes.UpdateNew, expectedUpdateNew)
-//	validateEntries(suite.T(), changes.UpdateOld, expectedUpdateOld)
-//	validateEntries(suite.T(), changes.Delete, expectedDelete)
-//}
+func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificAddition() {
+	current := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificUnset}
+	desired := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificTrue}
+	expectedCreate := []*endpoint.Endpoint{}
+	expectedUpdateOld := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificUnset}
+	expectedUpdateNew := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificTrue}
+	expectedDelete := []*endpoint.Endpoint{}
+
+	p := &Plan{
+		Policies:       []Policy{&SyncPolicy{}},
+		Current:        current,
+		Desired:        desired,
+		ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME},
+	}
+
+	changes := p.Calculate().Changes
+	validateEntries(suite.T(), changes.Create, expectedCreate)
+	validateEntries(suite.T(), changes.UpdateNew, expectedUpdateNew)
+	validateEntries(suite.T(), changes.UpdateOld, expectedUpdateOld)
+	validateEntries(suite.T(), changes.Delete, expectedDelete)
+}
 
 func (suite *PlanTestSuite) TestSyncSecondRoundWithOwnerInherited() {
 	current := []*endpoint.Endpoint{suite.fooV1Cname}