From f89dbeaf59760a860d35e480eb91d2ae0022d596 Mon Sep 17 00:00:00 2001 From: Bartosz Chwila <103247439+barchw@users.noreply.github.com> Date: Fri, 30 Dec 2022 14:53:28 +0100 Subject: [PATCH] Cherrypick periodic reconciliation to rel 1.2 (#152) * Run reconcilation periodically (#147) * Requeue after ENV * Don't requeue configMap * Reconcile every time * Requeue faster after error * After fix * Add nil check * Don't run reconcilation again after delete * Move code * Print delete timestamp * Negate deletion timestamp * Print delete timestamp * Print delete timestamp * Don't requeue on delettion * Validate on handler change * Go fmt revert * Add the problems * Add the problems * Switch cases * Unit tests * Add reconciliation duration parameter and periodic reconciliation test (#151) * Update apirule_controller.go * Update apirule_controller.go * Update apirule_controller.go * Reconcilation period setup * Add test for reconcilation period * Actually configure from params * Fix typo --- .../api_controller_integration_test.go | 268 ++++++++++++++++++ .../suite_test.go | 188 ++++++++++++ controllers/apirule_controller.go | 89 ++++-- controllers/suite_test.go | 9 +- internal/processing/istio/jwt_validator.go | 25 ++ .../processing/istio/jwt_validator_test.go | 20 ++ internal/processing/ory/jwt_validator.go | 18 ++ internal/processing/ory/jwt_validator_test.go | 25 +- internal/validation/validate.go | 7 + main.go | 7 + 10 files changed, 625 insertions(+), 31 deletions(-) create mode 100644 controllers/api_controlller_reconciliation_test/api_controller_integration_test.go create mode 100644 controllers/api_controlller_reconciliation_test/suite_test.go diff --git a/controllers/api_controlller_reconciliation_test/api_controller_integration_test.go b/controllers/api_controlller_reconciliation_test/api_controller_integration_test.go new file mode 100644 index 000000000..5e219a1ac --- /dev/null +++ b/controllers/api_controlller_reconciliation_test/api_controller_integration_test.go @@ -0,0 +1,268 @@ +package reconciliation_test + +import ( + "context" + "fmt" + "math/rand" + "time" + + "github.com/kyma-incubator/api-gateway/internal/helpers" + istioint "github.com/kyma-incubator/api-gateway/internal/types/istio" + + "encoding/json" + + gatewayv1beta1 "github.com/kyma-incubator/api-gateway/api/v1beta1" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/controller-runtime/pkg/source" +) + +const ( + timeout = time.Second * 10 + + kind = "APIRule" + testGatewayURL = "kyma-system/kyma-gateway" + testOathkeeperSvcURL = "oathkeeper.kyma-system.svc.cluster.local" + testOathkeeperPort uint32 = 1234 + testNamespace = "atgo-system" + testNameBase = "test" + testIDLength = 5 +) + +var _ = Describe("APIRule Controller Reconciliation", func() { + const testServiceName = "httpbin" + const testServicePort uint32 = 443 + var testIssuer = "https://oauth2.example.com/" + + BeforeEach(func() { + // We configure `ory` in ConfigMap as the default for all tests + cm := testConfigMap(helpers.JWT_HANDLER_ORY) + err := c.Update(context.TODO(), cm) + if apierrors.IsInvalid(err) { + Fail(fmt.Sprintf("failed to update configmap, got an invalid object error: %v", err)) + } + Expect(err).NotTo(HaveOccurred()) + }) + + It("Should update the apirule with error after the ConfigMap was changed to different one", func() { + Context("From Ory to Istio", func() { + // given + apiRuleName := generateTestName(testNameBase, testIDLength) + testServiceHost := fmt.Sprintf("httpbin-%s.kyma.local", apiRuleName) + + rule := testRule("/img", []string{"GET"}, nil, testOryJWTHandler(testIssuer, []string{"test-scope"})) + apiRule := testInstance(apiRuleName, testNamespace, testServiceName, testServiceHost, testServicePort, []gatewayv1beta1.Rule{rule}) + + By("Create ApiRule with Rule using JWT handler") + err := c.Create(context.TODO(), apiRule) + Expect(err).NotTo(HaveOccurred()) + defer func() { + err := c.Delete(context.TODO(), apiRule) + Expect(err).NotTo(HaveOccurred()) + }() + + initialStateReq := reconcile.Request{NamespacedName: types.NamespacedName{Name: apiRuleName, Namespace: testNamespace}} + Eventually(requests, timeout).Should(Receive(Equal(initialStateReq))) + + // when + By("Setting JWT handler config map to istio") + cm := testConfigMap("istio") + err = c.Update(context.TODO(), cm) + Expect(err).NotTo(HaveOccurred()) + + cmRequest := reconcile.Request{NamespacedName: types.NamespacedName{Name: cm.Name, Namespace: cm.Namespace}} + Eventually(requests, timeout).Should(Receive(Equal(cmRequest))) + + updateApiRuleReq := reconcile.Request{NamespacedName: types.NamespacedName{Name: apiRuleName, Namespace: testNamespace}} + Eventually(requests, timeout).Should(Receive(Equal(updateApiRuleReq))) + + // then + + expectedApiRule := gatewayv1beta1.APIRule{} + err = c.Get(context.TODO(), client.ObjectKey{Name: apiRuleName, Namespace: testNamespace}, &expectedApiRule) + Expect(err).NotTo(HaveOccurred()) + + Expect(expectedApiRule.Status.APIRuleStatus.Code).To(Equal(gatewayv1beta1.StatusError)) + }) + + Context("From Istio to Ory", func() { + // given + By("Setting JWT handler config map to istio") + cm := testConfigMap("istio") + err := c.Update(context.TODO(), cm) + Expect(err).NotTo(HaveOccurred()) + + apiRuleName := generateTestName(testNameBase, testIDLength) + testServiceHost := fmt.Sprintf("httpbin-%s.kyma.local", apiRuleName) + + rule := testRule("/img", []string{"GET"}, nil, testIstioJWTHandler(testIssuer, "https://example.com/well-known/.jwks")) + apiRule := testInstance(apiRuleName, testNamespace, testServiceName, testServiceHost, testServicePort, []gatewayv1beta1.Rule{rule}) + + By("Create ApiRule with Rule using JWT handler") + err = c.Create(context.TODO(), apiRule) + Expect(err).NotTo(HaveOccurred()) + defer func() { + err := c.Delete(context.TODO(), apiRule) + Expect(err).NotTo(HaveOccurred()) + }() + + initialStateReq := reconcile.Request{NamespacedName: types.NamespacedName{Name: apiRuleName, Namespace: testNamespace}} + Eventually(requests, timeout).Should(Receive(Equal(initialStateReq))) + + // when + By("Setting JWT handler config map to ory") + cm = testConfigMap("ory") + err = c.Update(context.TODO(), cm) + Expect(err).NotTo(HaveOccurred()) + + cmRequest := reconcile.Request{NamespacedName: types.NamespacedName{Name: cm.Name, Namespace: cm.Namespace}} + Eventually(requests, timeout).Should(Receive(Equal(cmRequest))) + + updateApiRuleReq := reconcile.Request{NamespacedName: types.NamespacedName{Name: apiRuleName, Namespace: testNamespace}} + Eventually(requests, timeout).Should(Receive(Equal(updateApiRuleReq))) + + // then + + expectedApiRule := gatewayv1beta1.APIRule{} + err = c.Get(context.TODO(), client.ObjectKey{Name: apiRuleName, Namespace: testNamespace}, &expectedApiRule) + Expect(err).NotTo(HaveOccurred()) + + Expect(expectedApiRule.Status.APIRuleStatus.Code).To(Equal(gatewayv1beta1.StatusError)) + }) + }) +}) + +func add(mgr manager.Manager, r reconcile.Reconciler) error { + c, err := controller.New("api-gateway-controller", mgr, controller.Options{Reconciler: r}) + if err != nil { + return err + } + + err = c.Watch(&source.Kind{Type: &gatewayv1beta1.APIRule{}}, &handler.EnqueueRequestForObject{}) + if err != nil { + return err + } + + err = c.Watch(&source.Kind{Type: &corev1.ConfigMap{}}, &handler.EnqueueRequestForObject{}) + if err != nil { + return err + } + + return nil +} + +func toCSVList(input []string) string { + if len(input) == 0 { + return "" + } + + res := `"` + input[0] + `"` + + for i := 1; i < len(input); i++ { + res = res + "," + `"` + input[i] + `"` + } + + return res +} + +func testRule(path string, methods []string, mutators []*gatewayv1beta1.Mutator, handler *gatewayv1beta1.Handler) gatewayv1beta1.Rule { + return gatewayv1beta1.Rule{ + Path: path, + Methods: methods, + Mutators: mutators, + AccessStrategies: []*gatewayv1beta1.Authenticator{ + { + Handler: handler, + }, + }, + } +} + +func testInstance(name, namespace, serviceName, serviceHost string, servicePort uint32, rules []gatewayv1beta1.Rule) *gatewayv1beta1.APIRule { + var gateway = testGatewayURL + + return &gatewayv1beta1.APIRule{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: gatewayv1beta1.APIRuleSpec{ + Host: &serviceHost, + Gateway: &gateway, + Service: &gatewayv1beta1.Service{ + Name: &serviceName, + Port: &servicePort, + }, + Rules: rules, + }, + } +} + +func testConfigMap(jwtHandler string) *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: helpers.CM_NAME, + Namespace: helpers.CM_NS, + }, + Data: map[string]string{ + helpers.CM_KEY: fmt.Sprintf("jwtHandler: %s", jwtHandler), + }, + } +} + +func testOryJWTHandler(issuer string, scopes []string) *gatewayv1beta1.Handler { + + configJSON := fmt.Sprintf(`{ + "trusted_issuers": ["%s"], + "jwks": [], + "required_scope": [%s] + }`, issuer, toCSVList(scopes)) + + return &gatewayv1beta1.Handler{ + Name: "jwt", + Config: &runtime.RawExtension{ + Raw: []byte(configJSON), + }, + } +} + +func testIstioJWTHandler(issuer string, jwksUri string) *gatewayv1beta1.Handler { + bytes, err := json.Marshal(istioint.JwtConfig{ + Authentications: []istioint.JwtAuth{ + { + Issuer: issuer, + JwksUri: jwksUri, + }, + }, + }) + Expect(err).To(BeNil()) + return &gatewayv1beta1.Handler{ + Name: "jwt", + Config: &runtime.RawExtension{ + Raw: bytes, + }, + } +} + +func generateTestName(name string, length int) string { + + rand.Seed(time.Now().UnixNano()) + + letterRunes := []rune("abcdefghijklmnopqrstuvwxyz") + + b := make([]rune, length) + for i := range b { + b[i] = letterRunes[rand.Intn(len(letterRunes))] + } + return name + "-" + string(b) +} diff --git a/controllers/api_controlller_reconciliation_test/suite_test.go b/controllers/api_controlller_reconciliation_test/suite_test.go new file mode 100644 index 000000000..6e3e187b5 --- /dev/null +++ b/controllers/api_controlller_reconciliation_test/suite_test.go @@ -0,0 +1,188 @@ +package reconciliation_test + +import ( + "context" + "fmt" + "path/filepath" + "testing" + "time" + + rulev1alpha1 "github.com/ory/oathkeeper-maester/api/v1alpha1" + "istio.io/api/networking/v1beta1" + networkingv1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1" + securityv1beta1 "istio.io/client-go/pkg/apis/security/v1beta1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/envtest/printer" + + gatewayv1beta1 "github.com/kyma-incubator/api-gateway/api/v1beta1" + "github.com/kyma-incubator/api-gateway/controllers" + "github.com/kyma-incubator/api-gateway/internal/helpers" + "github.com/kyma-incubator/api-gateway/internal/processing" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + // +kubebuilder:scaffold:imports +) + +var ( + cfg *rest.Config + k8sClient client.Client + testEnv *envtest.Environment + requests chan reconcile.Request + c client.Client + ctx context.Context + cancel context.CancelFunc + + TestAllowOrigins = []*v1beta1.StringMatch{{MatchType: &v1beta1.StringMatch_Regex{Regex: ".*"}}} + TestAllowMethods = []string{"GET", "POST", "PUT", "DELETE"} + TestAllowHeaders = []string{"header1", "header2"} +) + +func TestAPIGatewayReconciliation(t *testing.T) { + RegisterFailHandler(Fail) + + RunSpecsWithDefaultAndCustomReporters(t, + "Reconciliation Suite", + []Reporter{printer.NewlineReporter{}, printer.NewProwReporter("api-gateway-controller-reconciliation")}) +} + +var _ = BeforeSuite(func(done Done) { + logf.SetLogger(zap.New(zap.UseDevMode(true), zap.WriteTo(GinkgoWriter))) + ctx, cancel = context.WithCancel(context.TODO()) + + By("bootstrapping test environment for reconciliation") + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases"), filepath.Join("..", "..", "hack")}, + } + + var err error + cfg, err = testEnv.Start() + Expect(err).ToNot(HaveOccurred()) + Expect(cfg).ToNot(BeNil()) + + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) + Expect(err).ToNot(HaveOccurred()) + Expect(k8sClient).ToNot(BeNil()) + + s := runtime.NewScheme() + + err = gatewayv1beta1.AddToScheme(s) + Expect(err).NotTo(HaveOccurred()) + + err = rulev1alpha1.AddToScheme(s) + Expect(err).NotTo(HaveOccurred()) + + err = networkingv1beta1.AddToScheme(s) + Expect(err).NotTo(HaveOccurred()) + + err = securityv1beta1.AddToScheme(s) + Expect(err).NotTo(HaveOccurred()) + + err = corev1.AddToScheme(s) + Expect(err).NotTo(HaveOccurred()) + + mgr, err := manager.New(cfg, manager.Options{Scheme: s, MetricsBindAddress: "0"}) + Expect(err).NotTo(HaveOccurred()) + + c, err = client.New(cfg, client.Options{Scheme: s}) + Expect(err).NotTo(HaveOccurred()) + + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: testNamespace}, + Spec: corev1.NamespaceSpec{}, + } + err = c.Create(context.TODO(), ns) + Expect(err).NotTo(HaveOccurred()) + + nsKyma := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: helpers.CM_NS}, + Spec: corev1.NamespaceSpec{}, + } + err = c.Create(context.TODO(), nsKyma) + Expect(err).NotTo(HaveOccurred()) + + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: helpers.CM_NAME, + Namespace: helpers.CM_NS, + }, + Data: map[string]string{ + helpers.CM_KEY: fmt.Sprintf("jwtHandler: %s", helpers.JWT_HANDLER_ORY), + }, + } + err = c.Create(context.TODO(), cm) + Expect(err).NotTo(HaveOccurred()) + + apiReconciler := &controllers.APIRuleReconciler{ + Client: mgr.GetClient(), + Log: ctrl.Log.WithName("controllers").WithName("Api"), + OathkeeperSvc: testOathkeeperSvcURL, + OathkeeperSvcPort: testOathkeeperPort, + DomainAllowList: []string{"bar", "kyma.local"}, + CorsConfig: &processing.CorsConfig{ + AllowOrigins: TestAllowOrigins, + AllowMethods: TestAllowMethods, + AllowHeaders: TestAllowHeaders, + }, + GeneratedObjectsLabels: map[string]string{}, + Config: &helpers.Config{}, + + // Run the suite with period that won't interfere with tests + ReconcilePeriod: time.Second, + OnErrorReconcilePeriod: time.Second, + } + Expect(err).NotTo(HaveOccurred()) + + var recFn reconcile.Reconciler + recFn, requests = SetupTestReconcile(apiReconciler) + Expect(add(mgr, recFn)).To(Succeed()) + + go func() { + defer GinkgoRecover() + err = mgr.Start(ctx) + Expect(err).ToNot(HaveOccurred(), "failed to run manager") + }() + + close(done) +}, 60) + +var _ = AfterSuite(func() { + /* + Provided solution for timeout issue waiting for kubeapiserver + https://github.com/kubernetes-sigs/controller-runtime/issues/1571#issuecomment-1005575071 + */ + cancel() + By("tearing down the test environment,but I do nothing here.") + err := testEnv.Stop() + // Set 4 with random + if err != nil { + time.Sleep(4 * time.Second) + } + err = testEnv.Stop() + Expect(err).NotTo(HaveOccurred()) + +}) + +// SetupTestReconcile returns a reconcile.Reconcile implementation that delegates to inner and +// writes the request to requests after Reconcile is finished. +func SetupTestReconcile(inner reconcile.Reconciler) (reconcile.Reconciler, chan reconcile.Request) { + requests := make(chan reconcile.Request) + fn := reconcile.Func(func(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { + result, err := inner.Reconcile(ctx, req) + requests <- req + return result, err + }) + return fn, requests +} diff --git a/controllers/apirule_controller.go b/controllers/apirule_controller.go index a4da0c803..0212271dd 100644 --- a/controllers/apirule_controller.go +++ b/controllers/apirule_controller.go @@ -58,11 +58,15 @@ type APIRuleReconciler struct { DefaultDomainName string Scheme *runtime.Scheme Config *helpers.Config + ReconcilePeriod time.Duration + OnErrorReconcilePeriod time.Duration } const ( - CONFIGMAP_NAME = "api-gateway-config" - CONFIGMAP_NS = "kyma-system" + CONFIGMAP_NAME = "api-gateway-config" + CONFIGMAP_NS = "kyma-system" + DEFAULT_RECONCILIATION_PERIOD = 30 * time.Minute + ERROR_RECONCILIATION_PERIOD = time.Minute ) type configMapPredicate struct { @@ -107,7 +111,7 @@ func (p configMapPredicate) Generic(e event.GenericEvent) bool { //+kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch;delete func (r *APIRuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - r.Log.Info("Starting reconcilation") + r.Log.Info("Starting reconciliation") validator := validation.APIRule{ ServiceBlockList: r.ServiceBlockList, @@ -116,6 +120,7 @@ func (r *APIRuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct DefaultDomainName: r.DefaultDomainName, } r.Log.Info("Checking if it's ConfigMap reconciliation") + r.Log.Info("Reconciling for", "namespacedName", req.NamespacedName.String()) isCMReconcile := (req.NamespacedName.String() == types.NamespacedName{Namespace: helpers.CM_NS, Name: helpers.CM_NAME}.String()) if isCMReconcile || r.Config.JWTHandler == "" { r.Log.Info("Starting ConfigMap reconciliation") @@ -136,7 +141,7 @@ func (r *APIRuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct failuresJson, _ := json.Marshal(configValidationFailures) r.Log.Error(err, fmt.Sprintf(`Config validation failure {"controller": "Api", "failures": %s}`, string(failuresJson))) } - return doneReconcile() + return doneReconcileNoRequeue() } } r.Log.Info("Starting ApiRule reconciliation") @@ -144,16 +149,19 @@ func (r *APIRuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct err := r.Client.Get(ctx, req.NamespacedName, apiRule) if err != nil { - r.Log.Error(err, "Error getting ApiRule") if apierrs.IsNotFound(err) { //There is no APIRule. Nothing to process, dependent objects will be garbage-collected. - return doneReconcile() + r.Log.Info("Finishing reconciliation after ApiRule was deleted") + return doneReconcileNoRequeue() } + r.Log.Error(err, "Error getting ApiRule") + //Nothing is yet processed: StatusSkipped status := processing.GetStatusForError(&r.Log, err, gatewayv1beta1.StatusSkipped) return r.updateStatusOrRetry(ctx, apiRule, status) } + r.Log.Info("Validating ApiRule config") configValidationFailures := validator.ValidateConfig(r.Config) if len(configValidationFailures) > 0 { @@ -167,25 +175,23 @@ func (r *APIRuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct r.Log.Info("Validating if not status update or temporary annotation set") if apiRule.Generation != apiRule.Status.ObservedGeneration { r.Log.Info("not a status update") - c := processing.ReconciliationConfig{ - OathkeeperSvc: r.OathkeeperSvc, - OathkeeperSvcPort: r.OathkeeperSvcPort, - CorsConfig: r.CorsConfig, - AdditionalLabels: r.GeneratedObjectsLabels, - DefaultDomainName: r.DefaultDomainName, - ServiceBlockList: r.ServiceBlockList, - DomainAllowList: r.DomainAllowList, - HostBlockList: r.HostBlockList, - } - - cmd := r.getReconciliation(c) - r.Log.Info("Process reconcile") - status := processing.Reconcile(ctx, r.Client, &r.Log, cmd, apiRule) - r.Log.Info("Update status or retry") - return r.updateStatusOrRetry(ctx, apiRule, status) } - r.Log.Info("Finishing reconciliation") - return doneReconcile() + c := processing.ReconciliationConfig{ + OathkeeperSvc: r.OathkeeperSvc, + OathkeeperSvcPort: r.OathkeeperSvcPort, + CorsConfig: r.CorsConfig, + AdditionalLabels: r.GeneratedObjectsLabels, + DefaultDomainName: r.DefaultDomainName, + ServiceBlockList: r.ServiceBlockList, + DomainAllowList: r.DomainAllowList, + HostBlockList: r.HostBlockList, + } + + cmd := r.getReconciliation(c) + r.Log.Info("Process reconcile") + status := processing.Reconcile(ctx, r.Client, &r.Log, cmd, apiRule) + r.Log.Info("Update status or retry") + return r.updateStatusOrRetry(ctx, apiRule, status) } func (r *APIRuleReconciler) getReconciliation(config processing.ReconciliationConfig) processing.ReconciliationCommand { @@ -211,14 +217,43 @@ func (r *APIRuleReconciler) updateStatusOrRetry(ctx context.Context, api *gatewa if updateStatusErr != nil { return retryReconcile(updateStatusErr) //controller retries to set the correct status eventually. } - //Fail fast: If status is updated, users are informed about the problem. We don't need to reconcile again. - return doneReconcile() + + // If error happened during reconciliation (e.g. VirtualService conflict) requeue for reconciliation earlier + if statusHasError(status) { + return doneReconcileErrorRequeue(r.OnErrorReconcilePeriod) + } + + return doneReconcileDefaultRequeue(r.ReconcilePeriod) +} + +func statusHasError(status processing.ReconciliationStatus) bool { + return status.ApiRuleStatus.Code == gatewayv1beta1.StatusError || + status.AccessRuleStatus.Code == gatewayv1beta1.StatusError || + status.VirtualServiceStatus.Code == gatewayv1beta1.StatusError || + status.AuthorizationPolicyStatus.Code == gatewayv1beta1.StatusError || + status.RequestAuthenticationStatus.Code == gatewayv1beta1.StatusError } -func doneReconcile() (ctrl.Result, error) { +func doneReconcileNoRequeue() (ctrl.Result, error) { return ctrl.Result{}, nil } +func doneReconcileDefaultRequeue(reconcilerPeriod time.Duration) (ctrl.Result, error) { + after := DEFAULT_RECONCILIATION_PERIOD + if reconcilerPeriod != 0 { + after = reconcilerPeriod + } + return ctrl.Result{RequeueAfter: after}, nil +} + +func doneReconcileErrorRequeue(errorReconcilerPeriod time.Duration) (ctrl.Result, error) { + after := ERROR_RECONCILIATION_PERIOD + if errorReconcilerPeriod != 0 { + after = errorReconcilerPeriod + } + return ctrl.Result{RequeueAfter: after}, nil +} + func retryReconcile(err error) (ctrl.Result, error) { return reconcile.Result{Requeue: true}, err } diff --git a/controllers/suite_test.go b/controllers/suite_test.go index e85504868..c648f309f 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -13,7 +13,6 @@ import ( securityv1beta1 "istio.io/client-go/pkg/apis/security/v1beta1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/envtest/printer" @@ -101,14 +100,14 @@ var _ = BeforeSuite(func(done Done) { Expect(err).NotTo(HaveOccurred()) ns := &corev1.Namespace{ - ObjectMeta: v1.ObjectMeta{Name: testNamespace}, + ObjectMeta: metav1.ObjectMeta{Name: testNamespace}, Spec: corev1.NamespaceSpec{}, } err = c.Create(context.TODO(), ns) Expect(err).NotTo(HaveOccurred()) nsKyma := &corev1.Namespace{ - ObjectMeta: v1.ObjectMeta{Name: helpers.CM_NS}, + ObjectMeta: metav1.ObjectMeta{Name: helpers.CM_NS}, Spec: corev1.NamespaceSpec{}, } err = c.Create(context.TODO(), nsKyma) @@ -139,6 +138,10 @@ var _ = BeforeSuite(func(done Done) { }, GeneratedObjectsLabels: map[string]string{}, Config: &helpers.Config{}, + + // Run the suite with period that won't interfere with tests + ReconcilePeriod: time.Hour * 24, + OnErrorReconcilePeriod: time.Hour * 24, } Expect(err).NotTo(HaveOccurred()) diff --git a/internal/processing/istio/jwt_validator.go b/internal/processing/istio/jwt_validator.go index 90cf240cb..bcc710d20 100644 --- a/internal/processing/istio/jwt_validator.go +++ b/internal/processing/istio/jwt_validator.go @@ -6,6 +6,7 @@ import ( gatewayv1beta1 "github.com/kyma-incubator/api-gateway/api/v1beta1" istiojwt "github.com/kyma-incubator/api-gateway/internal/types/istio" + oryjwt "github.com/kyma-incubator/api-gateway/internal/types/ory" "github.com/kyma-incubator/api-gateway/internal/validation" ) @@ -26,6 +27,8 @@ func (o *handlerValidator) Validate(attributePath string, handler *gatewayv1beta return problems } + problems = append(problems, checkForOryConfig(attributePath, handler)...) + for i, auth := range template.Authentications { invalidIssuer, err := validation.IsInvalidURL(auth.Issuer) if invalidIssuer { @@ -54,3 +57,25 @@ func (o *handlerValidator) Validate(attributePath string, handler *gatewayv1beta return problems } + +func checkForOryConfig(attributePath string, handler *gatewayv1beta1.Handler) (problems []validation.Failure) { + var template oryjwt.JWTAccStrConfig + err := json.Unmarshal(handler.Config.Raw, &template) + if err != nil { + return []validation.Failure{{AttributePath: attributePath + ".config", Message: "Can't read json: " + err.Error()}} + } + + if len(template.JWKSUrls) > 0 { + problems = append(problems, validation.Failure{AttributePath: attributePath + ".config" + ".jwks_urls", Message: "Configuration for jwks_urls is not supported with Istio handler"}) + } + + if len(template.RequiredScopes) > 0 { + problems = append(problems, validation.Failure{AttributePath: attributePath + ".config" + ".required_scopes", Message: "Configuration for required_scopes is not supported with Istio handler"}) + } + + if len(template.TrustedIssuers) > 0 { + problems = append(problems, validation.Failure{AttributePath: attributePath + ".config" + ".trusted_issuers", Message: "Configuration for trusted_issuers is not supported with Istio handler"}) + } + + return problems +} diff --git a/internal/processing/istio/jwt_validator_test.go b/internal/processing/istio/jwt_validator_test.go index 709c479d9..63e1700a9 100644 --- a/internal/processing/istio/jwt_validator_test.go +++ b/internal/processing/istio/jwt_validator_test.go @@ -5,6 +5,7 @@ import ( gatewayv1beta1 "github.com/kyma-incubator/api-gateway/api/v1beta1" istiojwt "github.com/kyma-incubator/api-gateway/internal/types/istio" + "github.com/kyma-incubator/api-gateway/internal/types/ory" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/runtime" @@ -88,6 +89,16 @@ var _ = Describe("JWT Validator", func() { Expect(problems[0].AttributePath).To(Equal("some.attribute.config")) Expect(problems[0].Message).To(Equal("Can't read json: invalid character '/' looking for beginning of value")) }) + + It("Should fail for config with Ory JWT configuration", func() { + handler := &gatewayv1beta1.Handler{Name: "jwt", Config: testURLJWTOryConfig("https://issuer.test/.well-known/jwks.json", "https://issuer.test/")} + + //when + problems := (&handlerValidator{}).Validate("some.attribute", handler) + + //then + Expect(problems).To(Not(BeEmpty())) + }) }) func emptyJWTIstioConfig() *runtime.RawExtension { @@ -119,6 +130,15 @@ func testURLJWTIstioConfig(JWKSUrl string, trustedIssuer string) *runtime.RawExt }) } +func testURLJWTOryConfig(JWKSUrls string, trustedIssuers string) *runtime.RawExtension { + return getRawConfig( + &ory.JWTAccStrConfig{ + JWKSUrls: []string{JWKSUrls}, + TrustedIssuers: []string{trustedIssuers}, + RequiredScopes: []string{"atgo"}, + }) +} + func getRawConfig(config any) *runtime.RawExtension { bytes, err := json.Marshal(config) Expect(err).To(BeNil()) diff --git a/internal/processing/ory/jwt_validator.go b/internal/processing/ory/jwt_validator.go index 8036e6981..80c524289 100644 --- a/internal/processing/ory/jwt_validator.go +++ b/internal/processing/ory/jwt_validator.go @@ -5,6 +5,7 @@ import ( "fmt" gatewayv1beta1 "github.com/kyma-incubator/api-gateway/api/v1beta1" + istiojwt "github.com/kyma-incubator/api-gateway/internal/types/istio" "github.com/kyma-incubator/api-gateway/internal/types/ory" "github.com/kyma-incubator/api-gateway/internal/validation" ) @@ -19,12 +20,15 @@ func (o *handlerValidator) Validate(attributePath string, handler *gatewayv1beta problems = append(problems, validation.Failure{AttributePath: attributePath + ".config", Message: "supplied config cannot be empty"}) return problems } + err := json.Unmarshal(handler.Config.Raw, &template) if err != nil { problems = append(problems, validation.Failure{AttributePath: attributePath + ".config", Message: "Can't read json: " + err.Error()}) return problems } + problems = append(problems, checkForIstioConfig(attributePath, handler)...) + // The https:// configuration for TrustedIssuers is not necessary in terms of security best practices, // however it is part of "secure by default" configuration, as this is the most common use case for iss claim. // If we want to allow some weaker configurations, we should have a dedicated configuration which allows that. @@ -60,3 +64,17 @@ func (o *handlerValidator) Validate(attributePath string, handler *gatewayv1beta return problems } + +func checkForIstioConfig(attributePath string, handler *gatewayv1beta1.Handler) (problems []validation.Failure) { + var template istiojwt.JwtConfig + err := json.Unmarshal(handler.Config.Raw, &template) + if err != nil { + return []validation.Failure{{AttributePath: attributePath + ".config", Message: "Can't read json: " + err.Error()}} + } + + if len(template.Authentications) > 0 { + return []validation.Failure{{AttributePath: attributePath + ".config" + ".authentications", Message: "Configuration for authentications is not supported with Ory handler"}} + } + + return problems +} diff --git a/internal/processing/ory/jwt_validator_test.go b/internal/processing/ory/jwt_validator_test.go index f9faf63c8..1a7ed3ee9 100644 --- a/internal/processing/ory/jwt_validator_test.go +++ b/internal/processing/ory/jwt_validator_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" gatewayv1beta1 "github.com/kyma-incubator/api-gateway/api/v1beta1" + istiojwt "github.com/kyma-incubator/api-gateway/internal/types/istio" "github.com/kyma-incubator/api-gateway/internal/types/ory" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -99,6 +100,16 @@ var _ = Describe("JWT Validator", func() { //then Expect(problems).To(HaveLen(0)) }) + + It("Should fail for config with Istio JWT configuration", func() { + handler := &gatewayv1beta1.Handler{Name: "jwt", Config: testURLJWTIstioConfig("https://issuer.test/.well-known/jwks.json", "https://issuer.test/")} + + //when + problems := (&handlerValidator{}).Validate("some.attribute", handler) + + //then + Expect(problems).To(Not(BeEmpty())) + }) }) func emptyConfig() *runtime.RawExtension { @@ -124,7 +135,19 @@ func testURLJWTConfig(JWKSUrls string, trustedIssuers string) *runtime.RawExtens }) } -func getRawConfig(config *ory.JWTAccStrConfig) *runtime.RawExtension { +func testURLJWTIstioConfig(JWKSUrl string, trustedIssuer string) *runtime.RawExtension { + return getRawConfig( + istiojwt.JwtConfig{ + Authentications: []istiojwt.JwtAuth{ + { + Issuer: trustedIssuer, + JwksUri: JWKSUrl, + }, + }, + }) +} + +func getRawConfig(config any) *runtime.RawExtension { bytes, err := json.Marshal(config) Expect(err).To(BeNil()) return &runtime.RawExtension{ diff --git a/internal/validation/validate.go b/internal/validation/validate.go index d42521517..b107bd4a1 100644 --- a/internal/validation/validate.go +++ b/internal/validation/validate.go @@ -77,6 +77,13 @@ func (v *APIRule) ValidateConfig(config *helpers.Config) []Failure { func (v *APIRule) validateHost(attributePath string, vsList networkingv1beta1.VirtualServiceList, api *gatewayv1beta1.APIRule) []Failure { var problems []Failure + if api.Spec.Host == nil { + problems = append(problems, Failure{ + AttributePath: attributePath, + Message: "Host was nil", + }) + return problems + } host := *api.Spec.Host if !helpers.HostIncludesDomain(*api.Spec.Host) { diff --git a/main.go b/main.go index 1c6a341e5..903412cc9 100644 --- a/main.go +++ b/main.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "strings" + "time" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. @@ -86,6 +87,8 @@ func main() { var domainName string var corsAllowOrigins, corsAllowMethods, corsAllowHeaders string var generatedObjectsLabels string + var reconciliationPeriod uint + var errorReconciliationPeriod uint const blockListedSubdomains string = "api" @@ -102,6 +105,8 @@ func main() { flag.StringVar(&corsAllowMethods, "cors-allow-methods", "GET,POST,PUT,DELETE", "list of allowed methods") flag.StringVar(&corsAllowHeaders, "cors-allow-headers", "Authorization,Content-Type,*", "list of allowed headers") flag.StringVar(&generatedObjectsLabels, "generated-objects-labels", "", "Comma-separated list of key=value pairs used to label generated objects") + flag.UintVar(&reconciliationPeriod, "reconciliation-period", 0, "Default reconciliation period when no error happened in the previous run [s]") + flag.UintVar(&errorReconciliationPeriod, "error-reconciliation-period", 0, "Reconciliation period after an error happened in the previous run (e.g. VirtualService confict) [s]") flag.Parse() @@ -175,6 +180,8 @@ func main() { GeneratedObjectsLabels: additionalLabels, Scheme: mgr.GetScheme(), Config: &helpers.Config{}, + ReconcilePeriod: time.Duration(reconciliationPeriod) * time.Second, + OnErrorReconcilePeriod: time.Duration(errorReconciliationPeriod) * time.Second, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "APIRule") os.Exit(1)