Handle IPPolicy CRD state transitions in a safer way (#260)

* Handle IPPolicy CRD state transitions in a safer way

* Address review feedback

Update tests and dry-up adding ip policy rules updates

api/v1alpha1/ippolicy_types.go

@@ -34,6 +34,7 @@ type IPPolicyRule struct {
 	// +kubebuilder:validation:Required
 	CIDR string `json:"cidr,omitempty"`
 	// +kubebuilder:validation:Required
+	// +kubebuilder:validation:Enum=allow;deny
 	Action string `json:"action,omitempty"`
 }
 

internal/controllers/ippolicy_controller.go

@@ -179,19 +179,33 @@ func (r *IPPolicyReconciler) createOrUpdateIPPolicyRules(ctx context.Context, po
 	if err != nil {
 		return err
 	}
-	diff := newIPPolicyDiff(remoteRules, policy.Spec.Rules)
-	for _, rule := range diff.needCreate {
-		rule.IPPolicyID = policy.Status.ID
-		_, err := r.IPPolicyRulesClient.Create(ctx, rule)
-		if err != nil {
-			return err
+	iter := newIPPolicyDiff(policy.Status.ID, remoteRules, policy.Spec.Rules)
+
+	for iter.Next() {
+		for _, d := range iter.NeedsDelete() {
+			r.Log.V(3).Info("Deleting IP Policy Rule", "id", d.ID, "policy.id", policy.Status.ID, "cidr", d.CIDR, "action", d.Action)
+			if err := r.IPPolicyRulesClient.Delete(ctx, d.ID); err != nil {
+				return err
+			}
+			r.Log.V(3).Info("Deleted IP Policy Rule", "id", d.ID)
 		}
-	}
 
-	for _, rule := range diff.needDelete {
-		err := r.IPPolicyRulesClient.Delete(ctx, rule.ID)
-		if err != nil {
-			return err
+		for _, c := range iter.NeedsCreate() {
+			r.Log.V(3).Info("Creating IP Policy Rule", "policy.id", policy.Status.ID, "cidr", c.CIDR, "action", c.Action)
+			rule, err := r.IPPolicyRulesClient.Create(ctx, c)
+			if err != nil {
+				return err
+			}
+			r.Log.V(3).Info("Created IP Policy Rule", "id", rule.ID, "policy.id", policy.Status.ID, "cidr", rule.CIDR, "action", rule.Action)
+		}
+
+		for _, u := range iter.NeedsUpdate() {
+			r.Log.V(3).Info("Updating IP Policy Rule", "id", u.ID, "policy.id", policy.Status.ID, "cidr", u.CIDR, "metadata", u.Metadata, "description", u.Description)
+			rule, err := r.IPPolicyRulesClient.Update(ctx, u)
+			if err != nil {
+				return err
+			}
+			r.Log.V(3).Info("Updated IP Policy Rule", "id", rule.ID, "policy.id", policy.Status.ID)
 		}
 	}
 
@@ -213,71 +227,177 @@ func (r *IPPolicyReconciler) getRemotePolicyRules(ctx context.Context, policyID
 	return rules, iter.Err()
 }
 
+// IPPolicyDiff represents the diff between the remote and spec rules for an IPPolicy.
+// From the ngrok docs:
+//
+//	"IP Restrictions allow you to attach one or more IP policies to the route.
+//	 If multiple IP policies are attached, a connection will be allowed only if
+//	 its source IP matches at least one policy with an 'allow' action and
+//	 does not match any policy with a 'deny' action."
+//
+// This provides an iterator of the rules that need to be created,updated, and deleted in order to update the remote securely.
 type IPPolicyDiff struct {
-	needCreate []*ngrok.IPPolicyRuleCreate
-	needDelete []*ngrok.IPPolicyRule
+	idx int
+
+	policyID string
+
+	remoteDeny  map[string]*ngrok.IPPolicyRule
+	remoteAllow map[string]*ngrok.IPPolicyRule
+	specDeny    map[string]ingressv1alpha1.IPPolicyRule
+	specAllow   map[string]ingressv1alpha1.IPPolicyRule
+
+	creates []*ngrok.IPPolicyRuleCreate
+	deletes []*ngrok.IPPolicyRule
+	updates []*ngrok.IPPolicyRuleUpdate
 }
 
-func newIPPolicyDiff(remote []*ngrok.IPPolicyRule, spec []ingressv1alpha1.IPPolicyRule) *IPPolicyDiff {
-	remoteDeny := make(map[string]*ngrok.IPPolicyRule)
-	remoteAllow := make(map[string]*ngrok.IPPolicyRule)
-	specDeny := make(map[string]ingressv1alpha1.IPPolicyRule)
-	specAllow := make(map[string]ingressv1alpha1.IPPolicyRule)
+func newIPPolicyDiff(policyID string, remote []*ngrok.IPPolicyRule, spec []ingressv1alpha1.IPPolicyRule) *IPPolicyDiff {
+	diff := &IPPolicyDiff{
+		policyID:    policyID,
+		remoteDeny:  make(map[string]*ngrok.IPPolicyRule),
+		remoteAllow: make(map[string]*ngrok.IPPolicyRule),
+		specDeny:    make(map[string]ingressv1alpha1.IPPolicyRule),
+		specAllow:   make(map[string]ingressv1alpha1.IPPolicyRule),
+	}
 
+	// Group the remote rules by their CIDR
 	for _, rule := range remote {
 		if rule.Action == IPPolicyRuleActionDeny {
-			remoteDeny[rule.CIDR] = rule
+			diff.remoteDeny[rule.CIDR] = rule
 		} else {
-			remoteAllow[rule.CIDR] = rule
+			diff.remoteAllow[rule.CIDR] = rule
 		}
 	}
 
+	// Group the spec rules by their CIDR
 	for _, rule := range spec {
 		if rule.Action == IPPolicyRuleActionDeny {
-			specDeny[rule.CIDR] = rule
+			diff.specDeny[rule.CIDR] = rule
 		} else {
-			specAllow[rule.CIDR] = rule
+			diff.specAllow[rule.CIDR] = rule
 		}
 	}
 
-	diff := &IPPolicyDiff{
-		needCreate: make([]*ngrok.IPPolicyRuleCreate, 0),
-		needDelete: make([]*ngrok.IPPolicyRule, 0),
-	}
+	return diff
+}
 
-	for cidr, specRule := range specAllow {
-		if _, ok := remoteAllow[cidr]; !ok {
-			diff.needCreate = append(diff.needCreate, &ngrok.IPPolicyRuleCreate{
-				Action:      pointer.String(IPPolicyRuleActionAllow),
-				Description: specRule.Description,
-				Metadata:    specRule.Metadata,
-				CIDR:        cidr,
-			})
-		}
-	}
+func (d *IPPolicyDiff) Next() bool {
+	defer func() { d.idx++ }()
+
+	// Reset the diff
+	d.creates = make([]*ngrok.IPPolicyRuleCreate, 0)
+	d.deletes = make([]*ngrok.IPPolicyRule, 0)
+	d.updates = make([]*ngrok.IPPolicyRuleUpdate, 0)
 
-	for cidr, specRule := range specDeny {
-		if _, ok := remoteDeny[cidr]; !ok {
-			diff.needCreate = append(diff.needCreate, &ngrok.IPPolicyRuleCreate{
-				Action:      pointer.String(IPPolicyRuleActionDeny),
-				Description: specRule.Description,
-				Metadata:    specRule.Metadata,
-				CIDR:        cidr,
-			})
+	switch d.idx {
+	case 0: // Create all new deny rules that don't exist in the remote with a matching CIDR.
+		for cidr, rule := range d.specDeny {
+			if !d.existsInRemote(cidr) {
+				d.creates = append(d.creates, d.createRule(rule))
+			}
+		}
+		return true
+	case 1: // Delete any allow rules with matching CIDRs that will be changing to deny rules. Then create the deny rules.
+		for cidr, rule := range d.specDeny {
+			if _, ok := d.remoteAllow[cidr]; ok {
+				d.deletes = append(d.deletes, d.remoteAllow[cidr])
+				d.creates = append(d.creates, d.createRule(rule))
+			}
 		}
+		return true
+	case 2: // Delete any deny rules with matching CIDRs that will be changing to allow rules. Then create the allow rules.
+		for cidr, rule := range d.specAllow {
+			if _, ok := d.remoteDeny[cidr]; ok {
+				d.deletes = append(d.deletes, d.remoteDeny[cidr])
+				d.creates = append(d.creates, d.createRule(rule))
+			}
+		}
+		return true
+	case 3: // Create all new allow rules that don't exist in the remote with a matching CIDR.
+		for cidr, rule := range d.specAllow {
+			if !d.existsInRemote(cidr) {
+				d.creates = append(d.creates, d.createRule(rule))
+			}
+		}
+		return true
+	case 4: // Delete any remaining rules that are not in the spec.
+		for cidr, rule := range d.remoteAllow {
+			if !d.existsInSpec(cidr) {
+				d.deletes = append(d.deletes, rule)
+			}
+		}
+		for cidr, rule := range d.remoteDeny {
+			if !d.existsInSpec(cidr) {
+				d.deletes = append(d.deletes, rule)
+			}
+		}
+		return true
+	case 5: // Update any rules that exist in the spec and remote but have only different metadata/description.
+		for cidr, rule := range d.specAllow {
+			if remoteRule, ok := d.remoteAllow[cidr]; ok {
+				d.addUpdateIfNeeded(rule, remoteRule)
+			}
+		}
+		for cidr, rule := range d.specDeny {
+			if remoteRule, ok := d.remoteDeny[cidr]; ok {
+				d.addUpdateIfNeeded(rule, remoteRule)
+			}
+		}
+		return true
+	default:
 	}
 
-	for cidr, rule := range remoteAllow {
-		if _, ok := specAllow[cidr]; !ok {
-			diff.needDelete = append(diff.needDelete, rule)
-		}
+	return false
+}
+
+func (d *IPPolicyDiff) NeedsCreate() []*ngrok.IPPolicyRuleCreate {
+	return d.creates
+}
+
+func (d *IPPolicyDiff) NeedsDelete() []*ngrok.IPPolicyRule {
+	return d.deletes
+}
+
+func (d *IPPolicyDiff) NeedsUpdate() []*ngrok.IPPolicyRuleUpdate {
+	return d.updates
+}
+
+// existsInSpec returns true if the CIDR exists in the spec for either an allow or deny rule.
+func (d *IPPolicyDiff) existsInSpec(cidr string) bool {
+	_, okDeny := d.specDeny[cidr]
+	_, okAllow := d.specAllow[cidr]
+	return okDeny || okAllow
+}
+
+// existsInRemote returns true if the CIDR exists in the remote for either an allow or deny rule.
+func (d *IPPolicyDiff) existsInRemote(cidr string) bool {
+	_, okDeny := d.remoteDeny[cidr]
+	_, okAllow := d.remoteAllow[cidr]
+	return okDeny || okAllow
+}
+
+func (d *IPPolicyDiff) createRule(rule ingressv1alpha1.IPPolicyRule) *ngrok.IPPolicyRuleCreate {
+	return &ngrok.IPPolicyRuleCreate{
+		IPPolicyID:  d.policyID,
+		CIDR:        rule.CIDR,
+		Action:      pointer.String(rule.Action),
+		Metadata:    rule.Metadata,
+		Description: rule.Description,
 	}
+}
 
-	for cidr, rule := range remoteDeny {
-		if _, ok := specDeny[cidr]; !ok {
-			diff.needDelete = append(diff.needDelete, rule)
-		}
+func (d *IPPolicyDiff) addUpdateIfNeeded(rule ingressv1alpha1.IPPolicyRule, remoteRule *ngrok.IPPolicyRule) {
+	updatedNeeded := rule.CIDR == remoteRule.CIDR &&
+		rule.Action == remoteRule.Action &&
+		(rule.Metadata != remoteRule.Metadata || rule.Description != remoteRule.Description)
+	if !updatedNeeded {
+		return
 	}
 
-	return diff
+	d.updates = append(d.updates, &ngrok.IPPolicyRuleUpdate{
+		ID:          remoteRule.ID,
+		Metadata:    pointer.String(rule.Metadata),
+		Description: pointer.String(rule.Description),
+		CIDR:        pointer.String(rule.CIDR),
+	})
 }

internal/controllers/ippolicy_controllers_test.go

@@ -0,0 +1,75 @@
+package controllers
+
+import (
+	"testing"
+
+	ingressv1alpha1 "github.com/ngrok/kubernetes-ingress-controller/api/v1alpha1"
+	"github.com/ngrok/ngrok-api-go/v5"
+	"github.com/stretchr/testify/assert"
+	"k8s.io/utils/pointer"
+)
+
+func TestIPPolicyDiff(t *testing.T) {
+	remoteRules := []*ngrok.IPPolicyRule{
+		{ID: "1", CIDR: "192.168.1.0/25", Action: IPPolicyRuleActionAllow, Description: "a"},    // 2. Rule changed from allow to deny
+		{ID: "2", CIDR: "192.168.128.0/25", Action: IPPolicyRuleActionDeny, Description: "aa"},  // 3. Rule changed from deny to allow
+		{ID: "3", CIDR: "172.16.0.0/16", Action: IPPolicyRuleActionAllow, Description: "aaa"},   // 5. Allow Rule that will no longer exist
+		{ID: "4", CIDR: "172.17.0.0/16", Action: IPPolicyRuleActionDeny, Description: "aaaa"},   // 5. Deny Rule that will no longer exist
+		{ID: "5", CIDR: "172.19.0.0/16", Action: IPPolicyRuleActionAllow, Description: "aaaaa"}, // 6. Just changing description
+	}
+	changedDescriptionRule := ingressv1alpha1.IPPolicyRule{CIDR: "172.19.0.0/16", Action: IPPolicyRuleActionAllow}
+	changedDescriptionRule.Description = "b"
+
+	specRules := []ingressv1alpha1.IPPolicyRule{
+		{CIDR: "192.168.1.0/25", Action: IPPolicyRuleActionDeny},    // 2. Rule changed from allow to deny
+		{CIDR: "192.168.128.0/25", Action: IPPolicyRuleActionAllow}, // 3. Rule changed from deny to allow
+		{CIDR: "10.0.0.0/8", Action: IPPolicyRuleActionDeny},        // 1. New Rule to be denied
+		{CIDR: "172.18.0.0/16", Action: IPPolicyRuleActionAllow},    // 4. New Rule to be allowed
+		changedDescriptionRule, // 6. Just changing description
+	}
+
+	diff := newIPPolicyDiff("test", remoteRules, specRules)
+
+	assert.True(t, diff.Next())
+	assert.Empty(t, diff.NeedsDelete())
+	assert.Empty(t, diff.NeedsUpdate())
+	assert.Equal(t, []*ngrok.IPPolicyRuleCreate{
+		{IPPolicyID: "test", CIDR: specRules[2].CIDR, Action: pointer.String(IPPolicyRuleActionDeny)}},
+		diff.NeedsCreate(),
+	)
+
+	assert.True(t, diff.Next())
+	assert.Empty(t, diff.NeedsUpdate())
+	assert.Equal(t, []*ngrok.IPPolicyRule{remoteRules[0]}, diff.NeedsDelete())
+	assert.Equal(t, []*ngrok.IPPolicyRuleCreate{
+		{IPPolicyID: "test", CIDR: specRules[0].CIDR, Action: pointer.String(IPPolicyRuleActionDeny)},
+	}, diff.NeedsCreate())
+
+	assert.True(t, diff.Next())
+	assert.Empty(t, diff.NeedsUpdate())
+	assert.Equal(t, []*ngrok.IPPolicyRule{remoteRules[1]}, diff.NeedsDelete())
+	assert.Equal(t, []*ngrok.IPPolicyRuleCreate{
+		{IPPolicyID: "test", CIDR: specRules[1].CIDR, Action: pointer.String(IPPolicyRuleActionAllow)},
+	}, diff.NeedsCreate())
+
+	assert.True(t, diff.Next())
+	assert.Empty(t, diff.NeedsUpdate())
+	assert.Equal(t, []*ngrok.IPPolicyRule{}, diff.NeedsDelete())
+	assert.Equal(t, []*ngrok.IPPolicyRuleCreate{
+		{IPPolicyID: "test", CIDR: specRules[3].CIDR, Action: pointer.String(IPPolicyRuleActionAllow)},
+	}, diff.NeedsCreate())
+
+	assert.True(t, diff.Next())
+	assert.Empty(t, diff.NeedsUpdate())
+	assert.Equal(t, []*ngrok.IPPolicyRule{remoteRules[2], remoteRules[3]}, diff.NeedsDelete())
+	assert.Equal(t, []*ngrok.IPPolicyRuleCreate{}, diff.NeedsCreate())
+
+	assert.True(t, diff.Next())
+	assert.Empty(t, diff.NeedsDelete())
+	assert.Empty(t, diff.NeedsCreate())
+	assert.Equal(t, []*ngrok.IPPolicyRuleUpdate{
+		{ID: "5", CIDR: pointer.String("172.19.0.0/16"), Description: pointer.String("b"), Metadata: pointer.String("")},
+	}, diff.NeedsUpdate())
+
+	assert.False(t, diff.Next())
+}