Support allowedURLs (#496)

<!-- Thank you for contributing! Please make sure that your code changes
are covered with tests. In case of new features or big changes remember
to adjust the documentation.

In case of an existing issue, reference it using one of the following:

closes: #ISSUE
related: #ISSUE

How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->

## What
*Describe what the change is solving*

Add regex validation to allowedURLs

Add conversion functions and tests

Create method to allow/deny BoundEndpoint projection

Deny BoundEndpoints based on allowedURLs

## How
*Describe the solution*

## Breaking Changes
*Are there any breaking changes in this PR?*
No.

Author: hjkatz

api/bindings/v1alpha1/boundendpoint_types.go

@@ -34,6 +34,11 @@ import (
 
 // BoundEndpointSpec defines the desired state of BoundEndpoint
 type BoundEndpointSpec struct {
+	// Allowed is a flag that determines if the BoundEndpoint is allowed to be projected into the cluster
+	// This is controlled by the KubernetesOperator CRD .spec.allowedURLs field
+	// +kubebuilder:validation:Required
+	Allowed bool `json:"allowed"`
+
 	// EndpointURI is the unique identifier
 	// representing the BoundEndpoint + its Endpoints
 	// Format: <scheme>://<service>.<namespace>:<port>
@@ -125,7 +130,7 @@ type BindingEndpoint struct {
 	v6.Ref `json:",inline"`
 
 	// +kubebuilder:validation:Required
-	// +kube:validation:Enum=provisioning;bound;error;unknown
+	// +kube:validation:Enum=provisioning;bound;denied;error;unknown
 	// +kubebuilder:default="unknown"
 	Status BindingEndpointStatus `json:"status"`
 
@@ -143,12 +148,13 @@ type BindingEndpoint struct {
 
 // BindingEndpointStatus is an enum that represents the status of a BindingEndpoint
 // TODO(https://github.com/ngrok-private/ngrok/issues/32666)
-// +kubebuilder:validation:Enum=unknown;provisioning;bound;error
+// +kubebuilder:validation:Enum=unknown;provisioning;denied;bound;error
 type BindingEndpointStatus string
 
 const (
 	StatusUnknown      BindingEndpointStatus = "unknown"
 	StatusProvisioning BindingEndpointStatus = "provisioning"
+	StatusDenied       BindingEndpointStatus = "denied"
 	StatusBound        BindingEndpointStatus = "bound"
 	StatusError        BindingEndpointStatus = "error"
 )

api/ngrok/v1alpha1/kubernetesoperator_types.go

@@ -57,10 +57,14 @@ type KubernetesOperatorBinding struct {
 	Name string `json:"name,omitempty"`
 
 	// AllowedURLs is a list of URI patterns ([scheme://]<service-name>.<namespace-name>) thet determine which BoundEndpoints are allowed to be created by the operator
-	// TODO(hkatz) We are only implementing `*` for now
-	// Support more patterns in the future, see product spec
+	// You may specify a wildcard for:
+	//   - All endpoints: `*`
+	//   - All services in a namespace: `*.namespace`
+	//   - All namespaces: `*.*`
+	//   - Named service in all namespaces: `service.*`
+	// See: https://regex101.com/r/APbE3G/4
 	// +kubebuilder:validation:Required
-	// +kubebuilder:validation:items:Pattern=`^[*]$`
+	// +kubebuilder:validation:items:Pattern=`^(([*]|(https?|tls|tcp)://)?([*]|([*]|[a-z]([-a-z0-9]{0,61}[a-z0-9])?)[.]([*]|[a-z]([-a-z0-9]{0,61}[a-z0-9])?)))$`
 	AllowedURLs []string `json:"allowedURLs,omitempty"`
 
 	// The public ingress endpoint for this Kubernetes Operator

cmd/api/main.go

@@ -533,6 +533,7 @@ func enableBindingsFeatureSet(_ context.Context, opts managerOpts, mgr ctrl.Mana
 		Recorder:                     mgr.GetEventRecorderFor("endpoint-binding-poller"),
 		Namespace:                    opts.namespace,
 		KubernetesOperatorConfigName: opts.releaseName,
+		AllowedURLs:                  opts.bindings.allowedURLs,
 		TargetServiceAnnotations:     targetServiceAnnotations,
 		TargetServiceLabels:          targetServiceLabels,
 		PollingInterval:              10 * time.Second,
@@ -584,6 +585,8 @@ func createKubernetesOperator(ctx context.Context, client client.Client, opts ma
 			}
 		}
 		k8sOperator.Spec.EnabledFeatures = features
+
+		setupLog.Info("created KubernetesOperator", "name", k8sOperator.Name, "namespace", k8sOperator.Namespace, "op", fmt.Sprintf("%+v", k8sOperator.Spec.Binding))
 		return nil
 	})
 	return err

helm/ngrok-operator/templates/crds/bindings.k8s.ngrok.com_boundendpoints.yaml

@@ -222,6 +222,19 @@ tests:
   - contains:
       path: spec.template.spec.containers[0].args
       content: --zap-log-level=error
+- it: Should pass bindings allowed urls if set
+  set:
+    bindings:
+      enabled: true
+      allowedURLs:
+      - test.example
+      - http://*
+  template: controller-deployment.yaml
+  documentIndex: 0 # Document 0 is the deployment since its the first template
+  asserts:
+  - contains:
+      path: spec.template.spec.containers[0].args
+      content: --bindings-allowed-urls=test.example,http://*
 - it: Should pass log format argument if set
   set:
     log:

helm/ngrok-operator/templates/crds/ngrok.k8s.ngrok.com_kubernetesoperators.yaml

@@ -281,7 +281,7 @@ bindings:
   name: ""
   description: "Created by ngrok-operator"
   allowedURLs:
-  - "*" # TODO(hkatz) only supporting this for now, implement others later
+  - "*"
   serviceAnnotations: {}
   serviceLabels: {}
   ingressEndpoint: "kubernetes-binding-ingress.ngrok.io:443"

helm/ngrok-operator/tests/controller-deployment_test.yaml

@@ -65,6 +65,7 @@ const (
 	NgrokErrorUpstreamServiceCreateFailed = "ERR_NGROK_0001"
 	NgrokErrorTargetServiceCreateFailed   = "ERR_NGROK_0002"
 	NgrokErrorFailedToBind                = "ERR_NGROK_003"
+	NgrokErrorNotAllowed                  = "ERR_NGROK_004"
 )
 
 var (
@@ -161,8 +162,10 @@ func (r *BoundEndpointReconciler) SetupWithManager(mgr ctrl.Manager) error {
 // - Upstream Service in the ngrok-op namespace pointed at the Pod Forwarders
 func (r *BoundEndpointReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	cr := &bindingsv1alpha1.BoundEndpoint{}
-	if ctrlErr, err := r.controller.Reconcile(ctx, req, cr); err != nil {
-		return ctrlErr, err
+	var res ctrl.Result
+	var err error
+	if res, err = r.controller.Reconcile(ctx, req, cr); err != nil {
+		return res, err
 	}
 
 	// update ngrok api resource status on upsert
@@ -179,6 +182,11 @@ func (r *BoundEndpointReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 func (r *BoundEndpointReconciler) create(ctx context.Context, cr *bindingsv1alpha1.BoundEndpoint) error {
 	targetService, upstreamService := r.convertBoundEndpointToServices(cr)
 
+	// binding is not allowed to be created
+	if !cr.Spec.Allowed {
+		return r.denyBoundEndpoint(ctx, cr)
+	}
+
 	if err := r.createUpstreamService(ctx, cr, upstreamService); err != nil {
 		return r.controller.ReconcileStatus(ctx, cr, err)
 	}
@@ -259,6 +267,15 @@ func (r *BoundEndpointReconciler) createUpstreamService(ctx context.Context, own
 func (r *BoundEndpointReconciler) update(ctx context.Context, cr *bindingsv1alpha1.BoundEndpoint) error {
 	log := ctrl.LoggerFrom(ctx)
 
+	// binding is not allowed
+	if !cr.Spec.Allowed {
+		if err := r.deleteBoundEndpointServices(ctx, cr); err != nil {
+			return r.controller.ReconcileStatus(ctx, cr, err)
+		}
+
+		return r.denyBoundEndpoint(ctx, cr)
+	}
+
 	desiredTargetService, desiredUpstreamService := r.convertBoundEndpointToServices(cr)
 
 	var existingTargetService v1.Service
@@ -333,22 +350,40 @@ func (r *BoundEndpointReconciler) update(ctx context.Context, cr *bindingsv1alph
 }
 
 func (r *BoundEndpointReconciler) delete(ctx context.Context, cr *bindingsv1alpha1.BoundEndpoint) error {
+	return r.deleteBoundEndpointServices(ctx, cr)
+}
+
+// deleteBoundEndpointServices deletes the Target and Upstream Services for the BoundEndpoint
+func (r *BoundEndpointReconciler) deleteBoundEndpointServices(ctx context.Context, cr *bindingsv1alpha1.BoundEndpoint) error {
 	log := ctrl.LoggerFrom(ctx)
 
 	targetService, upstreamService := r.convertBoundEndpointToServices(cr)
-	if err := r.Client.Delete(ctx, targetService); err != nil {
+
+	targetNamespace := &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: targetService.Namespace}}
+	if err := r.Client.Get(ctx, types.NamespacedName{Name: targetNamespace.Name}, targetNamespace); err != nil {
 		if client.IgnoreNotFound(err) == nil {
-			return nil
+			// fallthrough, no Target Service to delete
 		} else {
-			r.Recorder.Event(cr, v1.EventTypeWarning, "Delete", "Failed to delete Target Service")
-			log.Error(err, "Failed to delete Target Service")
+			log.Error(err, "Failed to get Target Namespace")
 			return err
 		}
+	} else {
+		// Target Namespace exists, try to delete the Target Service
+
+		if err := r.Client.Delete(ctx, targetService); err != nil {
+			if client.IgnoreNotFound(err) == nil {
+				return nil
+			} else {
+				r.Recorder.Event(cr, v1.EventTypeWarning, "Delete", "Failed to delete Target Service")
+				log.Error(err, "Failed to delete Target Service")
+				return err
+			}
+		}
 	}
 
 	if err := r.Client.Delete(ctx, upstreamService); err != nil {
 		if client.IgnoreNotFound(err) == nil {
-			return nil
+			// fallthrough, nothing to do
 		} else {
 			r.Recorder.Event(cr, v1.EventTypeWarning, "Delete", "Failed to delete Upstream Service")
 			log.Error(err, "Failed to delete Upstream Service")
@@ -555,6 +590,7 @@ func (r *BoundEndpointReconciler) tryToBindEndpoint(ctx context.Context, boundEn
 				bindErr = err
 			} else {
 				// success case: we dialed and closed the connection
+				bindErr = nil
 				break
 			}
 		}
@@ -567,13 +603,15 @@ func (r *BoundEndpointReconciler) tryToBindEndpoint(ctx context.Context, boundEn
 	var desired *bindingsv1alpha1.BindingEndpoint
 	if bindErr != nil {
 		// error
+		log.Error(bindErr, "Failed to bind BoundEndpoint, moving to error")
 		desired = &bindingsv1alpha1.BindingEndpoint{
 			Status:       bindingsv1alpha1.StatusError,
 			ErrorCode:    NgrokErrorFailedToBind,
 			ErrorMessage: fmt.Sprintf("Failed to bind BoundEndpoint: %s", bindErr),
 		}
 	} else {
 		// success
+		log.Info("Bound BoundEndpoint successfully, moving to bound")
 		desired = &bindingsv1alpha1.BindingEndpoint{
 			Status:       bindingsv1alpha1.StatusBound,
 			ErrorCode:    "",
@@ -586,3 +624,17 @@ func (r *BoundEndpointReconciler) tryToBindEndpoint(ctx context.Context, boundEn
 	setEndpointsStatus(boundEndpoint, desired)
 	return bindErr
 }
+
+// denyBoundEndpoint sets the status of the BoundEndpoint to denied
+func (r *BoundEndpointReconciler) denyBoundEndpoint(ctx context.Context, boundEndpoint *bindingsv1alpha1.BoundEndpoint) error {
+	reason := "Endpoint URI is not allowed by KubernetesOperator allowedURLs configuration"
+
+	setEndpointsStatus(boundEndpoint, &bindingsv1alpha1.BindingEndpoint{
+		Status:       bindingsv1alpha1.StatusDenied,
+		ErrorCode:    NgrokErrorNotAllowed,
+		ErrorMessage: reason,
+	})
+
+	r.Recorder.Event(boundEndpoint, v1.EventTypeWarning, "Denied", reason)
+	return r.controller.ReconcileStatus(ctx, boundEndpoint, nil)
+}