Processing restructure (kyma-project#99)

* Move processing logic into separate processors for VirtualService and AccessRule * Make object processors more independent * Rename functions * Move apply out of processors * Renamed function * Add todos that were mentioned in the issue * Inline one-liner methods * Add reconciliation commands for ory and istio * Add support for virtual service creation based on handler * Restructure modules * Move AccessRuleProcessor to ory module * Change visibility of types and remove old TODOs * Restructure virtual service functions * Remove TODO as this is not relevant any longer * Add ory virtual service tests * Move test utils * Add access rule processor tests * Add virtual service processor tests * Add reconciliation test * Add pragmatic approach to remove flakiness of tests * Add remove flakiness of tests * Fix flaky test and remove dummy feature flag implementation * Remove commented code * Use context, client and logger always from params to avoid confusion and remove getters from interface * Update internal/processing/internal/test/test_utils.go Co-authored-by: Bartosz Chwila <103247439+barchw@users.noreply.github.com> * Add abstraction for access rule as it needs to be different created * Provide logger instance as a pointer * Remove unused code * Add stronger typing for Action * Add comments for interfaces and functions * Add comments for interfaces and functions Co-authored-by: Bartosz Chwila <103247439+barchw@users.noreply.github.com>
barchw · Dec 23, 2022 · dbf7257 · dbf7257
1 parent 74b4f28
commit dbf7257
Show file tree

Hide file tree

Showing 6 changed files with 523 additions and 1 deletion.
diff --git a/go.mod b/go.mod
@@ -78,7 +78,7 @@ require (
 	google.golang.org/protobuf v1.28.0 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
-	gopkg.in/yaml.v2 v2.4.0
+	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 	k8s.io/api v0.26.0
 	k8s.io/apiextensions-apiserver v0.25.4

diff --git a/internal/processing/access_rule_processor.go b/internal/processing/access_rule_processor.go
@@ -0,0 +1,105 @@
+package processing
+
+import (
+	"context"
+	"fmt"
+	gatewayv1beta1 "github.com/kyma-incubator/api-gateway/api/v1beta1"
+	rulev1alpha1 "github.com/ory/oathkeeper-maester/api/v1alpha1"
+	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+// AccessRuleProcessor is the generic processor that handles the Ory Rules in the reconciliation of API Rule.
+type AccessRuleProcessor struct {
+	Creator AccessRuleCreator
+}
+
+// AccessRuleCreator provides the creation of Rules using the configuration in the given APIRule.
+// The key of the map is expected to be unique and comparable with the
+type AccessRuleCreator interface {
+	Create(api *gatewayv1beta1.APIRule) map[string]*rulev1alpha1.Rule
+}
+
+func (r AccessRuleProcessor) EvaluateReconciliation(ctx context.Context, client ctrlclient.Client, apiRule *gatewayv1beta1.APIRule) ([]*ObjectChange, error) {
+	desired := r.getDesiredState(apiRule)
+	actual, err := r.getActualState(ctx, client, apiRule)
+	if err != nil {
+		return make([]*ObjectChange, 0), err
+	}
+
+	c := r.getObjectChanges(desired, actual)
+
+	return c, nil
+}
+
+func (r AccessRuleProcessor) getObjectChanges(desiredRules map[string]*rulev1alpha1.Rule, actualRules map[string]*rulev1alpha1.Rule) []*ObjectChange {
+	arChanges := make(map[string]*ObjectChange)
+
+	for path, rule := range desiredRules {
+
+		if actualRules[path] != nil {
+			actualRules[path].Spec = rule.Spec
+			arChanges[path] = NewObjectUpdateAction(actualRules[path])
+		} else {
+			arChanges[path] = NewObjectCreateAction(rule)
+		}
+
+	}
+
+	for path, rule := range actualRules {
+		if desiredRules[path] == nil {
+			arChanges[path] = NewObjectDeleteAction(rule)
+		}
+	}
+
+	arChangesToApply := make([]*ObjectChange, 0, len(arChanges))
+
+	for _, applyCommand := range arChanges {
+		arChangesToApply = append(arChangesToApply, applyCommand)
+	}
+
+	return arChangesToApply
+}
+
+func (r AccessRuleProcessor) getDesiredState(api *gatewayv1beta1.APIRule) map[string]*rulev1alpha1.Rule {
+	return r.Creator.Create(api)
+}
+
+func (r AccessRuleProcessor) getActualState(ctx context.Context, client ctrlclient.Client, api *gatewayv1beta1.APIRule) (map[string]*rulev1alpha1.Rule, error) {
+	labels := GetOwnerLabels(api)
+
+	var arList rulev1alpha1.RuleList
+	if err := client.List(ctx, &arList, ctrlclient.MatchingLabels(labels)); err != nil {
+		return nil, err
+	}
+
+	accessRules := make(map[string]*rulev1alpha1.Rule)
+	pathDuplicates := HasPathDuplicates(api.Spec.Rules)
+
+	for i := range arList.Items {
+		obj := arList.Items[i]
+		accessRules[SetAccessRuleKey(pathDuplicates, obj)] = &obj
+	}
+
+	return accessRules, nil
+}
+
+func SetAccessRuleKey(hasPathDuplicates bool, rule rulev1alpha1.Rule) string {
+
+	if hasPathDuplicates {
+		return fmt.Sprintf("%s:%s", rule.Spec.Match.URL, rule.Spec.Match.Methods)
+	}
+
+	return rule.Spec.Match.URL
+}
+
+func HasPathDuplicates(rules []gatewayv1beta1.Rule) bool {
+	duplicates := map[string]bool{}
+	for _, rule := range rules {
+		if duplicates[rule.Path] {
+			return true
+		}
+		duplicates[rule.Path] = true
+	}
+
+	return false
+}
diff --git a/internal/processing/access_rule_processor_test.go b/internal/processing/access_rule_processor_test.go
@@ -0,0 +1,274 @@
+package processing_test
+
+import (
+	"context"
+	"fmt"
+	gatewayv1beta1 "github.com/kyma-incubator/api-gateway/api/v1beta1"
+	"github.com/kyma-incubator/api-gateway/internal/builders"
+	"github.com/kyma-incubator/api-gateway/internal/processing"
+	. "github.com/kyma-incubator/api-gateway/internal/processing/internal/test"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	. "github.com/onsi/gomega/gstruct"
+	rulev1alpha1 "github.com/ory/oathkeeper-maester/api/v1alpha1"
+	networkingv1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+var _ = Describe("Access Rule Processor", func() {
+	It("should create access rule when no exists", func() {
+		// given
+		apiRule := &gatewayv1beta1.APIRule{}
+
+		processor := processing.AccessRuleProcessor{
+			Creator: mockCreator{
+				createMock: func() map[string]*rulev1alpha1.Rule {
+					return map[string]*rulev1alpha1.Rule{
+						"<http|https>://myService.myDomain.com<path>": builders.AccessRule().Spec(
+							builders.AccessRuleSpec().Match(
+								builders.Match().URL("<http|https>://myService.myDomain.com<path>"))).Get(),
+					}
+				},
+			},
+		}
+
+		// when
+		result, err := processor.EvaluateReconciliation(context.TODO(), GetEmptyFakeClient(), apiRule)
+
+		// then
+		Expect(err).To(BeNil())
+		Expect(result).To(HaveLen(1))
+		Expect(result[0].Action.String()).To(Equal("create"))
+	})
+
+	It("should update access rule when path exists", func() {
+		// given
+		noop := []*gatewayv1beta1.Authenticator{
+			{
+				Handler: &gatewayv1beta1.Handler{
+					Name: "noop",
+				},
+			},
+		}
+
+		noopRule := GetRuleFor("path", ApiMethods, []*gatewayv1beta1.Mutator{}, noop)
+		rules := []gatewayv1beta1.Rule{noopRule}
+
+		apiRule := GetAPIRuleFor(rules)
+
+		rule := rulev1alpha1.Rule{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					processing.OwnerLabelv1alpha1: fmt.Sprintf("%s.%s", apiRule.ObjectMeta.Name, apiRule.ObjectMeta.Namespace),
+				},
+			},
+			Spec: rulev1alpha1.RuleSpec{
+				Match: &rulev1alpha1.Match{
+					URL: fmt.Sprintf("<http|https>://%s<%s>", ServiceHost, "path"),
+				},
+			},
+		}
+
+		vs := networkingv1beta1.VirtualService{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					processing.OwnerLabelv1alpha1: fmt.Sprintf("%s.%s", apiRule.ObjectMeta.Name, apiRule.ObjectMeta.Namespace),
+				},
+			},
+		}
+
+		scheme := runtime.NewScheme()
+		err := rulev1alpha1.AddToScheme(scheme)
+		Expect(err).NotTo(HaveOccurred())
+		err = networkingv1beta1.AddToScheme(scheme)
+		Expect(err).NotTo(HaveOccurred())
+		err = gatewayv1beta1.AddToScheme(scheme)
+		Expect(err).NotTo(HaveOccurred())
+
+		client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&rule, &vs).Build()
+		processor := processing.AccessRuleProcessor{
+			Creator: mockCreator{
+				createMock: func() map[string]*rulev1alpha1.Rule {
+					return map[string]*rulev1alpha1.Rule{
+						"<http|https>://myService.myDomain.com<path>": builders.AccessRule().Spec(
+							builders.AccessRuleSpec().Match(
+								builders.Match().URL("<http|https>://myService.myDomain.com<path>"))).Get(),
+					}
+				},
+			},
+		}
+
+		// when
+		result, err := processor.EvaluateReconciliation(context.TODO(), client, apiRule)
+
+		// then
+		Expect(err).To(BeNil())
+		Expect(result).To(HaveLen(1))
+		Expect(result[0].Action.String()).To(Equal("update"))
+	})
+
+	It("should delete access rule", func() {
+		// given
+		noop := []*gatewayv1beta1.Authenticator{
+			{
+				Handler: &gatewayv1beta1.Handler{
+					Name: "noop",
+				},
+			},
+		}
+
+		noopRule := GetRuleFor("same", ApiMethods, []*gatewayv1beta1.Mutator{}, noop)
+		rules := []gatewayv1beta1.Rule{noopRule}
+
+		apiRule := GetAPIRuleFor(rules)
+
+		rule := rulev1alpha1.Rule{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					processing.OwnerLabelv1alpha1: fmt.Sprintf("%s.%s", apiRule.ObjectMeta.Name, apiRule.ObjectMeta.Namespace),
+				},
+			},
+			Spec: rulev1alpha1.RuleSpec{
+				Match: &rulev1alpha1.Match{
+					URL: fmt.Sprintf("<http|https>://%s<%s>", ServiceHost, "path"),
+				},
+			},
+		}
+
+		vs := networkingv1beta1.VirtualService{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					processing.OwnerLabelv1alpha1: fmt.Sprintf("%s.%s", apiRule.ObjectMeta.Name, apiRule.ObjectMeta.Namespace),
+				},
+			},
+		}
+
+		scheme := runtime.NewScheme()
+		err := rulev1alpha1.AddToScheme(scheme)
+		Expect(err).NotTo(HaveOccurred())
+		err = networkingv1beta1.AddToScheme(scheme)
+		Expect(err).NotTo(HaveOccurred())
+		err = gatewayv1beta1.AddToScheme(scheme)
+		Expect(err).NotTo(HaveOccurred())
+
+		client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&rule, &vs).Build()
+		processor := processing.AccessRuleProcessor{
+			Creator: mockCreator{
+				createMock: func() map[string]*rulev1alpha1.Rule {
+					return map[string]*rulev1alpha1.Rule{}
+				},
+			},
+		}
+
+		// when
+		result, err := processor.EvaluateReconciliation(context.TODO(), client, apiRule)
+
+		// then
+		Expect(err).To(BeNil())
+		Expect(result).To(HaveLen(1))
+		Expect(result[0].Action.String()).To(Equal("delete"))
+	})
+
+	When("rule exists and and rule path is different", func() {
+		It("should create new rule and delete old rule", func() {
+			// given
+			noop := []*gatewayv1beta1.Authenticator{
+				{
+					Handler: &gatewayv1beta1.Handler{
+						Name: "noop",
+					},
+				},
+			}
+
+			noopRule := GetRuleFor("newPath", ApiMethods, []*gatewayv1beta1.Mutator{}, noop)
+			rules := []gatewayv1beta1.Rule{noopRule}
+
+			apiRule := GetAPIRuleFor(rules)
+
+			rule := rulev1alpha1.Rule{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						processing.OwnerLabelv1alpha1: fmt.Sprintf("%s.%s", apiRule.ObjectMeta.Name, apiRule.ObjectMeta.Namespace),
+					},
+				},
+				Spec: rulev1alpha1.RuleSpec{
+					Match: &rulev1alpha1.Match{
+						URL: fmt.Sprintf("<http|https>://%s<%s>", ServiceHost, "oldPath"),
+					},
+				},
+			}
+
+			vs := networkingv1beta1.VirtualService{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						processing.OwnerLabelv1alpha1: fmt.Sprintf("%s.%s", apiRule.ObjectMeta.Name, apiRule.ObjectMeta.Namespace),
+					},
+				},
+			}
+
+			scheme := runtime.NewScheme()
+			err := rulev1alpha1.AddToScheme(scheme)
+			Expect(err).NotTo(HaveOccurred())
+			err = networkingv1beta1.AddToScheme(scheme)
+			Expect(err).NotTo(HaveOccurred())
+			err = gatewayv1beta1.AddToScheme(scheme)
+			Expect(err).NotTo(HaveOccurred())
+
+			client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&rule, &vs).Build()
+			processor := processing.AccessRuleProcessor{
+				Creator: mockCreator{
+					createMock: func() map[string]*rulev1alpha1.Rule {
+						return map[string]*rulev1alpha1.Rule{
+							"<http|https>://myService.myDomain.com<newPath>": builders.AccessRule().Spec(
+								builders.AccessRuleSpec().Match(
+									builders.Match().URL("<http|https>://myService.myDomain.com<newPath>"))).Get(),
+						}
+					},
+				},
+			}
+
+			// when
+			result, err := processor.EvaluateReconciliation(context.TODO(), client, apiRule)
+
+			// then
+			Expect(err).To(BeNil())
+			Expect(result).To(HaveLen(2))
+
+			createResultMatcher := PointTo(MatchFields(IgnoreExtras, Fields{
+				"Action": WithTransform(actionToString, Equal("create")),
+				"Obj": PointTo(MatchFields(IgnoreExtras, Fields{
+					"Spec": MatchFields(IgnoreExtras, Fields{
+						"Match": PointTo(MatchFields(IgnoreExtras, Fields{
+							"URL": Equal("<http|https>://myService.myDomain.com<newPath>"),
+						})),
+					}),
+				})),
+			}))
+
+			deleteResultMatcher := PointTo(MatchFields(IgnoreExtras, Fields{
+				"Action": WithTransform(actionToString, Equal("delete")),
+				"Obj": PointTo(MatchFields(IgnoreExtras, Fields{
+					"Spec": MatchFields(IgnoreExtras, Fields{
+						"Match": PointTo(MatchFields(IgnoreExtras, Fields{
+							"URL": Equal("<http|https>://myService.myDomain.com<oldPath>"),
+						})),
+					}),
+				})),
+			}))
+
+			Expect(result).To(ContainElements(createResultMatcher, deleteResultMatcher))
+		})
+	})
+})
+
+type mockCreator struct {
+	createMock func() map[string]*rulev1alpha1.Rule
+}
+
+func (r mockCreator) Create(_ *gatewayv1beta1.APIRule) map[string]*rulev1alpha1.Rule {
+	return r.createMock()
+}
+
+var actionToString = func(a processing.Action) string { return a.String() }