Add optional namespace for service in APIRule (#29)

* Initial impl * Refactor and unit tests * Update apirule_types.go * gofmt -s -w . * Update internal/processing/processing_test.go Co-authored-by: Bartosz Chwila <103247439+barchw@users.noreply.github.com> * Update internal/processing/processing_test.go Co-authored-by: Bartosz Chwila <103247439+barchw@users.noreply.github.com> * Update internal/processing/processing_test.go Co-authored-by: Bartosz Chwila <103247439+barchw@users.noreply.github.com> * Better safe than sorry Co-authored-by: Bartosz Chwila <103247439+barchw@users.noreply.github.com>
kyma-project · Sep 28, 2022 · 064be15 · 064be15
1 parent 953625e
commit 064be15
Show file tree

Hide file tree

Showing 19 changed files with 284 additions and 38 deletions.
diff --git a/api/v1alpha1/apiRule_types.go b/api/v1alpha1/apiRule_types.go
@@ -20,7 +20,7 @@ import (
 	runtime "k8s.io/apimachinery/pkg/runtime"
 )
 
-//StatusCode .
+// StatusCode .
 type StatusCode string
 
 const (
@@ -53,7 +53,7 @@ type APIRuleStatus struct {
 	AccessRuleStatus     *APIRuleResourceStatus `json:"accessRuleStatus,omitempty"`
 }
 
-//APIRule is the Schema for the apis ApiRule
+// APIRule is the Schema for the apis ApiRule
 // +kubebuilder:deprecatedversion:warning=v1alpha1 is deprecated as of Kyma 2.5.X
 // +kubebuilder:object:root=true
 // +kubebuilder:subresource:status
@@ -74,7 +74,7 @@ type APIRuleList struct {
 	Items           []APIRule `json:"items"`
 }
 
-//Service .
+// Service .
 type Service struct {
 	// Name of the service
 	Name *string `json:"name"`
@@ -92,7 +92,7 @@ type Service struct {
 	IsExternal *bool `json:"external,omitempty"`
 }
 
-//Rule .
+// Rule .
 type Rule struct {
 	// Path to be exposed
 	// +kubebuilder:validation:Pattern=^([0-9a-zA-Z./*()?!\\_-]+)
@@ -108,7 +108,7 @@ type Rule struct {
 	Mutators []*Mutator `json:"mutators,omitempty"`
 }
 
-//APIRuleResourceStatus .
+// APIRuleResourceStatus .
 type APIRuleResourceStatus struct {
 	Code        StatusCode `json:"code,omitempty"`
 	Description string     `json:"desc,omitempty"`

diff --git a/api/v1alpha1/groupversion_info.go b/api/v1alpha1/groupversion_info.go
@@ -15,8 +15,8 @@ limitations under the License.
 */
 
 // Package v1alpha1 contains API Schema definitions for the gateway v1alpha1 API group
-//+kubebuilder:object:generate=true
-//+groupName=gateway.kyma-project.io
+// +kubebuilder:object:generate=true
+// +groupName=gateway.kyma-project.io
 package v1alpha1
 
 import (

diff --git a/api/v1alpha1/internal_types.go b/api/v1alpha1/internal_types.go
@@ -1,6 +1,6 @@
 package v1alpha1
 
-//JWTAccStrConfig is used to deserialize jwt accessStrategy configuration for the validation purposes
+// JWTAccStrConfig is used to deserialize jwt accessStrategy configuration for the validation purposes
 type JWTAccStrConfig struct {
 	TrustedIssuers []string `json:"trusted_issuers,omitempty"`
 	JWKSUrls       []string `json:"jwks_urls,omitempty"`

diff --git a/api/v1beta1/apirule_types.go b/api/v1beta1/apirule_types.go
@@ -20,7 +20,7 @@ import (
 	runtime "k8s.io/apimachinery/pkg/runtime"
 )
 
-//StatusCode .
+// StatusCode .
 type StatusCode string
 
 const (
@@ -59,7 +59,7 @@ type APIRuleStatus struct {
 	AccessRuleStatus     *APIRuleResourceStatus `json:"accessRuleStatus,omitempty"`
 }
 
-//APIRule is the Schema for the apis ApiRule
+// APIRule is the Schema for the apis ApiRule
 // +kubebuilder:storageversion
 // +kubebuilder:object:root=true
 // +kubebuilder:subresource:status
@@ -80,10 +80,14 @@ type APIRuleList struct {
 	Items           []APIRule `json:"items"`
 }
 
-//Service .
+// Service .
 type Service struct {
 	// Name of the service
 	Name *string `json:"name"`
+	// Namespace of the service, if omitted will default to the APIRule namespace
+	// +kubebuilder:validation:Pattern=^[a-z0-9]([-a-z0-9]*[a-z0-9])?$
+	// +optional
+	Namespace *string `json:"namespace,omitempty"`
 	// Port of the service to expose
 	// +kubebuilder:validation:Minimum=1
 	// +kubebuilder:validation:Maximum=65535
@@ -93,7 +97,7 @@ type Service struct {
 	IsExternal *bool `json:"external,omitempty"`
 }
 
-//Rule .
+// Rule .
 type Rule struct {
 	// Path to be exposed
 	// +kubebuilder:validation:Pattern=^([0-9a-zA-Z./*()?!\\_-]+)
@@ -112,7 +116,7 @@ type Rule struct {
 	Mutators []*Mutator `json:"mutators,omitempty"`
 }
 
-//APIRuleResourceStatus .
+// APIRuleResourceStatus .
 type APIRuleResourceStatus struct {
 	Code        StatusCode `json:"code,omitempty"`
 	Description string     `json:"desc,omitempty"`

diff --git a/api/v1beta1/groupversion_info.go b/api/v1beta1/groupversion_info.go
@@ -15,8 +15,8 @@ limitations under the License.
 */
 
 // Package v1beta1 contains API Schema definitions for the gateway v1beta1 API group
-//+kubebuilder:object:generate=true
-//+groupName=gateway.kyma-project.io
+// +kubebuilder:object:generate=true
+// +groupName=gateway.kyma-project.io
 package v1beta1
 
 import (

diff --git a/api/v1beta1/internal_types.go b/api/v1beta1/internal_types.go
@@ -1,6 +1,6 @@
 package v1beta1
 
-//JWTAccStrConfig is used to deserialize jwt accessStrategy configuration for the validation purposes
+// JWTAccStrConfig is used to deserialize jwt accessStrategy configuration for the validation purposes
 type JWTAccStrConfig struct {
 	TrustedIssuers []string `json:"trusted_issuers,omitempty"`
 	JWKSUrls       []string `json:"jwks_urls,omitempty"`

diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go
diff --git a/config/crd/bases/gateway.kyma-project.io_apirules.yaml b/config/crd/bases/gateway.kyma-project.io_apirules.yaml
@@ -293,6 +293,11 @@ spec:
                         name:
                           description: Name of the service
                           type: string
+                        namespace:
+                          description: Namespace of the service, if omitted will default
+                            to the APIRule namespace
+                          pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$
+                          type: string
                         port:
                           description: Port of the service to expose
                           format: int32
@@ -320,6 +325,11 @@ spec:
                   name:
                     description: Name of the service
                     type: string
+                  namespace:
+                    description: Namespace of the service, if omitted will default
+                      to the APIRule namespace
+                    pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$
+                    type: string
                   port:
                     description: Port of the service to expose
                     format: int32

diff --git a/controllers/apirule_controller.go b/controllers/apirule_controller.go
@@ -140,7 +140,7 @@ func (r *APIRuleReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-//Sets status of APIRule. Accepts an auxilary status code that is used to report VirtualService and AccessRule status.
+// Sets status of APIRule. Accepts an auxilary status code that is used to report VirtualService and AccessRule status.
 func (r *APIRuleReconciler) setStatus(ctx context.Context, api *gatewayv1beta1.APIRule, apiStatus *gatewayv1beta1.APIRuleResourceStatus, auxStatusCode gatewayv1beta1.StatusCode) (ctrl.Result, error) {
 	virtualServiceStatus := &gatewayv1beta1.APIRuleResourceStatus{
 		Code: auxStatusCode,
@@ -151,7 +151,7 @@ func (r *APIRuleReconciler) setStatus(ctx context.Context, api *gatewayv1beta1.A
 	return r.updateStatusOrRetry(ctx, api, apiStatus, virtualServiceStatus, accessRuleStatus)
 }
 
-//Sets status of APIRule in error condition. Accepts an auxilary status code that is used to report VirtualService and AccessRule status.
+// Sets status of APIRule in error condition. Accepts an auxilary status code that is used to report VirtualService and AccessRule status.
 func (r *APIRuleReconciler) setStatusForError(ctx context.Context, api *gatewayv1beta1.APIRule, err error, auxStatusCode gatewayv1beta1.StatusCode) (ctrl.Result, error) {
 	r.Log.Error(err, "Error during reconciliation")
 
@@ -165,7 +165,7 @@ func (r *APIRuleReconciler) setStatusForError(ctx context.Context, api *gatewayv
 	return r.updateStatusOrRetry(ctx, api, generateErrorStatus(err), virtualServiceStatus, accessRuleStatus)
 }
 
-//Updates api status. If there was an error during update, returns the error so that entire reconcile loop is retried. If there is no error, returns a "reconcile success" value.
+// Updates api status. If there was an error during update, returns the error so that entire reconcile loop is retried. If there is no error, returns a "reconcile success" value.
 func (r *APIRuleReconciler) updateStatusOrRetry(ctx context.Context, api *gatewayv1beta1.APIRule, apiStatus, virtualServiceStatus, accessRuleStatus *gatewayv1beta1.APIRuleResourceStatus) (ctrl.Result, error) {
 	_, updateStatusErr := r.updateStatus(ctx, api, apiStatus, virtualServiceStatus, accessRuleStatus)
 	if updateStatusErr != nil {

diff --git a/internal/helpers/namespace.go b/internal/helpers/namespace.go
@@ -0,0 +1,16 @@
+package helpers
+
+import (
+	gatewayv1beta1 "github.com/kyma-incubator/api-gateway/api/v1beta1"
+)
+
+func FindServiceNamespace(api *gatewayv1beta1.APIRule, rule *gatewayv1beta1.Rule) *string {
+	// Fallback direction for the upstream service namespace: Rule.Service > Spec.Service > APIRule
+	if rule != nil && rule.Service != nil && rule.Service.Namespace != nil {
+		return rule.Service.Namespace
+	}
+	if api.Spec.Service != nil && api.Spec.Service.Namespace != nil {
+		return api.Spec.Service.Namespace
+	}
+	return &api.ObjectMeta.Namespace
+}
diff --git a/internal/processing/access_rule_helpers.go b/internal/processing/access_rule_helpers.go
@@ -44,12 +44,14 @@ func generateAccessRuleSpec(api *gatewayv1beta1.APIRule, rule gatewayv1beta1.Rul
 		Authenticators(builders.Authenticators().From(accessStrategies)).
 		Mutators(builders.Mutators().From(rule.Mutators))
 
+	serviceNamespace := helpers.FindServiceNamespace(api, &rule)
+
 	// Use rule level service if it exists
 	if rule.Service != nil {
 		return accessRuleSpec.Upstream(builders.Upstream().
-			URL(fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", *rule.Service.Name, api.ObjectMeta.Namespace, int(*rule.Service.Port)))).Get()
+			URL(fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", *rule.Service.Name, *serviceNamespace, int(*rule.Service.Port)))).Get()
 	}
 	// Otherwise use service defined on APIRule spec level
 	return accessRuleSpec.Upstream(builders.Upstream().
-		URL(fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", *api.Spec.Service.Name, api.ObjectMeta.Namespace, int(*api.Spec.Service.Port)))).Get()
+		URL(fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", *api.Spec.Service.Name, *serviceNamespace, int(*api.Spec.Service.Port)))).Get()
 }
diff --git a/internal/processing/processing_test.go b/internal/processing/processing_test.go
@@ -124,6 +124,44 @@ var _ = Describe("Factory", func() {
 				Expect(len(accessRules)).To(Equal(0))
 			})
 
+			It("should override VS destination host for specified spec level service namespace", func() {
+				strategies := []*gatewayv1beta1.Authenticator{
+					{
+						Handler: &gatewayv1beta1.Handler{
+							Name: "allow",
+						},
+					},
+				}
+
+				allowRule := getRuleFor(apiPath, apiMethods, []*gatewayv1beta1.Mutator{}, strategies)
+				rules := []gatewayv1beta1.Rule{allowRule}
+
+				apiRule := getAPIRuleFor(rules)
+
+				overrideServiceName := "testName"
+				overrideServiceNamespace := "testName-namespace"
+				overrideServicePort := uint32(8080)
+
+				apiRule.Spec.Service = &gatewayv1beta1.Service{
+					Name:      &overrideServiceName,
+					Namespace: &overrideServiceNamespace,
+					Port:      &overrideServicePort,
+				}
+
+				f := NewFactory(nil, ctrl.Log.WithName("test"), oathkeeperSvc, oathkeeperSvcPort, testCors, testAdditionalLabels, defaultDomain)
+
+				desiredState := f.CalculateRequiredState(apiRule)
+				vs := desiredState.virtualService
+				accessRules := desiredState.accessRules
+
+				//verify VS has rule level destination host
+				Expect(len(vs.Spec.Http[0].Route)).To(Equal(1))
+				Expect(vs.Spec.Http[0].Route[0].Destination.Host).To(Equal(overrideServiceName + "." + overrideServiceNamespace + ".svc.cluster.local"))
+
+				//Verify AR does not exist
+				Expect(len(accessRules)).To(Equal(0))
+			})
+
 			It("noop: should override access rule upstream with rule level service", func() {
 				strategies := []*gatewayv1beta1.Authenticator{
 					{
@@ -164,7 +202,49 @@ var _ = Describe("Factory", func() {
 				Expect(accessRules[expectedNoopRuleMatchURL].Spec.Upstream.URL).To(Equal(expectedRuleUpstreamURL))
 			})
 
-			It("allow: should override VS destination host", func() {
+			It("noop: should override access rule upstream with rule level service for specified namespace", func() {
+				strategies := []*gatewayv1beta1.Authenticator{
+					{
+						Handler: &gatewayv1beta1.Handler{
+							Name: "noop",
+						},
+					},
+				}
+
+				overrideServiceName := "testName"
+				overrideServiceNamespace := "testName-namespace"
+				overrideServicePort := uint32(8080)
+
+				service := &gatewayv1beta1.Service{
+					Name:      &overrideServiceName,
+					Namespace: &overrideServiceNamespace,
+					Port:      &overrideServicePort,
+				}
+
+				allowRule := getRuleWithServiceFor(apiPath, apiMethods, []*gatewayv1beta1.Mutator{}, strategies, service)
+				rules := []gatewayv1beta1.Rule{allowRule}
+
+				apiRule := getAPIRuleFor(rules)
+
+				f := NewFactory(nil, ctrl.Log.WithName("test"), oathkeeperSvc, oathkeeperSvcPort, testCors, testAdditionalLabels, defaultDomain)
+
+				desiredState := f.CalculateRequiredState(apiRule)
+				vs := desiredState.virtualService
+				accessRules := desiredState.accessRules
+
+				expectedNoopRuleMatchURL := fmt.Sprintf("<http|https>://%s<%s>", serviceHost, apiPath)
+				expectedRuleUpstreamURL := fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", overrideServiceName, overrideServiceNamespace, overrideServicePort)
+
+				//Verify VS
+				Expect(len(vs.Spec.Http[0].Route)).To(Equal(1))
+				Expect(vs.Spec.Http[0].Route[0].Destination.Host).To(Equal(oathkeeperSvc))
+
+				//Verify AR has rule level upstream
+				Expect(len(accessRules)).To(Equal(1))
+				Expect(accessRules[expectedNoopRuleMatchURL].Spec.Upstream.URL).To(Equal(expectedRuleUpstreamURL))
+			})
+
+			It("allow: should override VS destination host with rule level service", func() {
 				strategies := []*gatewayv1beta1.Authenticator{
 					{
 						Handler: &gatewayv1beta1.Handler{
@@ -200,6 +280,44 @@ var _ = Describe("Factory", func() {
 				Expect(len(accessRules)).To(Equal(0))
 			})
 
+			It("allow: should override VS destination host with rule level service for specified namespace", func() {
+				strategies := []*gatewayv1beta1.Authenticator{
+					{
+						Handler: &gatewayv1beta1.Handler{
+							Name: "allow",
+						},
+					},
+				}
+
+				overrideServiceName := "testName"
+				overrideServiceNamespace := "testName-namespace"
+				overrideServicePort := uint32(8080)
+
+				service := &gatewayv1beta1.Service{
+					Name:      &overrideServiceName,
+					Namespace: &overrideServiceNamespace,
+					Port:      &overrideServicePort,
+				}
+
+				allowRule := getRuleWithServiceFor(apiPath, apiMethods, []*gatewayv1beta1.Mutator{}, strategies, service)
+				rules := []gatewayv1beta1.Rule{allowRule}
+
+				apiRule := getAPIRuleFor(rules)
+
+				f := NewFactory(nil, ctrl.Log.WithName("test"), oathkeeperSvc, oathkeeperSvcPort, testCors, testAdditionalLabels, defaultDomain)
+
+				desiredState := f.CalculateRequiredState(apiRule)
+				vs := desiredState.virtualService
+				accessRules := desiredState.accessRules
+
+				//verify VS has rule level destination host
+				Expect(len(vs.Spec.Http[0].Route)).To(Equal(1))
+				Expect(vs.Spec.Http[0].Route[0].Destination.Host).To(Equal(overrideServiceName + "." + overrideServiceNamespace + ".svc.cluster.local"))
+
+				//Verify AR does not exist
+				Expect(len(accessRules)).To(Equal(0))
+			})
+
 			It("should produce VS and ARs for given paths", func() {
 				noop := []*gatewayv1beta1.Authenticator{
 					{
@@ -889,7 +1007,6 @@ var _ = Describe("Factory", func() {
 
 				apiRule := getAPIRuleFor(rules)
 
-
 				rule := rulev1alpha1.Rule{
 					ObjectMeta: metav1.ObjectMeta{
 						Labels: map[string]string{

diff --git a/internal/processing/virtual_service_helpers.go b/internal/processing/virtual_service_helpers.go
@@ -26,14 +26,16 @@ func (f *Factory) generateVirtualService(api *gatewayv1beta1.APIRule) *networkin
 	for _, rule := range filteredRules {
 		httpRouteBuilder := builders.HTTPRoute()
 		host, port := f.oathkeeperSvc, f.oathkeeperSvcPort
+		serviceNamespace := helpers.FindServiceNamespace(api, &rule)
+
 		if !isSecured(rule) {
 			// Use rule level service if it exists
 			if rule.Service != nil {
-				host = fmt.Sprintf("%s.%s.svc.cluster.local", *rule.Service.Name, api.ObjectMeta.Namespace)
+				host = fmt.Sprintf("%s.%s.svc.cluster.local", *rule.Service.Name, *serviceNamespace)
 				port = *rule.Service.Port
 			} else {
 				// Otherwise use service defined on APIRule spec level
-				host = fmt.Sprintf("%s.%s.svc.cluster.local", *api.Spec.Service.Name, api.ObjectMeta.Namespace)
+				host = fmt.Sprintf("%s.%s.svc.cluster.local", *api.Spec.Service.Name, *serviceNamespace)
 				port = *api.Spec.Service.Port
 			}
 		}