From 554bb8b4b6850f6ebcb34428e9afabfa5d639bc1 Mon Sep 17 00:00:00 2001 From: Harrison Katz Date: Thu, 23 Jan 2025 16:50:33 -0500 Subject: [PATCH] Add ngrok enriched errors (#555) * Add ngrok enriched errors * Fix staticcheck --- api/bindings/v1alpha1/boundendpoint_types.go | 1 - ...bindings.k8s.ngrok.com_boundendpoints.yaml | 5 +-- internal/controller/base_controller.go | 7 +++ .../bindings/boundendpoint_controller.go | 43 +++++++++++------- .../bindings/boundendpoint_controller_test.go | 2 +- .../bindings/boundendpoint_poller_test.go | 16 +++---- .../ngrok/kubernetesoperator_controller.go | 6 ++- internal/ngrokapi/enriched_errors.go | 45 +++++++++++++++++++ 8 files changed, 96 insertions(+), 29 deletions(-) create mode 100644 internal/ngrokapi/enriched_errors.go diff --git a/api/bindings/v1alpha1/boundendpoint_types.go b/api/bindings/v1alpha1/boundendpoint_types.go index d7d09801..cab277d6 100644 --- a/api/bindings/v1alpha1/boundendpoint_types.go +++ b/api/bindings/v1alpha1/boundendpoint_types.go @@ -132,7 +132,6 @@ type BindingEndpoint struct { // ErrorCode is the ngrok API error code if the status is error // +kubebuilder:validation:Optional // +kubebuilder:validation:Pattern=`^ERR_NGROK_\d+$` - // TODO(hkatz) Define error codes and implement in the API ErrorCode string `json:"errorCode,omitempty"` // ErrorMessage is a free-form error message if the status is error diff --git a/helm/ngrok-operator/templates/crds/bindings.k8s.ngrok.com_boundendpoints.yaml b/helm/ngrok-operator/templates/crds/bindings.k8s.ngrok.com_boundendpoints.yaml index fe9b05ee..1e259182 100644 --- a/helm/ngrok-operator/templates/crds/bindings.k8s.ngrok.com_boundendpoints.yaml +++ b/helm/ngrok-operator/templates/crds/bindings.k8s.ngrok.com_boundendpoints.yaml @@ -153,9 +153,8 @@ spec: in the ngrok API that is attached to the kubernetes operator binding properties: errorCode: - description: |- - ErrorCode is the ngrok API error code if the status is error - TODO(hkatz) Define error codes and implement in the API + description: ErrorCode is the ngrok API error code if the status + is error pattern: ^ERR_NGROK_\d+$ type: string errorMessage: diff --git a/internal/controller/base_controller.go b/internal/controller/base_controller.go index 0575244e..2fb47f2b 100644 --- a/internal/controller/base_controller.go +++ b/internal/controller/base_controller.go @@ -9,6 +9,7 @@ import ( "github.com/go-logr/logr" "github.com/ngrok/ngrok-api-go/v7" + "github.com/ngrok/ngrok-operator/internal/ngrokapi" "github.com/ngrok/ngrok-operator/internal/util" v1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/record" @@ -163,6 +164,12 @@ func CtrlResultForErr(err error) (ctrl.Result, error) { return ctrl.Result{RequeueAfter: time.Minute}, nil case nerr.StatusCode == http.StatusNotFound: return ctrl.Result{}, err + case ngrok.IsErrorCode(nerr, ngrokapi.NgrokOpErrFailedToCreateCSR.Code): + return ctrl.Result{RequeueAfter: 30 * time.Second}, nerr + case ngrok.IsErrorCode(nerr, ngrokapi.NgrokOpErrFailedToCreateUpstreamService.Code, ngrokapi.NgrokOpErrFailedToCreateTargetService.Code): + return ctrl.Result{RequeueAfter: 1 * time.Minute}, nerr + case ngrok.IsErrorCode(nerr, ngrokapi.NgrokOpErrEndpointDenied.Code): + return ctrl.Result{}, nil // do not retry, endpoint poller will take care of this default: // the rest are client errors, we don't retry by default return ctrl.Result{}, nil diff --git a/internal/controller/bindings/boundendpoint_controller.go b/internal/controller/bindings/boundendpoint_controller.go index 235bb710..c51966dc 100644 --- a/internal/controller/bindings/boundendpoint_controller.go +++ b/internal/controller/bindings/boundendpoint_controller.go @@ -44,8 +44,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/go-logr/logr" + "github.com/ngrok/ngrok-api-go/v7" bindingsv1alpha1 "github.com/ngrok/ngrok-operator/api/bindings/v1alpha1" "github.com/ngrok/ngrok-operator/internal/controller" + "github.com/ngrok/ngrok-operator/internal/ngrokapi" "github.com/ngrok/ngrok-operator/internal/util" ) @@ -61,12 +63,6 @@ const ( // Used for indexing BoundEndpoints by their target namespace. Not an actual // field on the BoundEndpoint object. BoundEndpointTargetNamespacePath = ".spec.targetNamespace" - - // TODO(hkatz) ngrok-error-codes - NgrokErrorUpstreamServiceCreateFailed = "ERR_NGROK_0001" - NgrokErrorTargetServiceCreateFailed = "ERR_NGROK_0002" - NgrokErrorFailedToBind = "ERR_NGROK_003" - NgrokErrorNotAllowed = "ERR_NGROK_004" ) var ( @@ -223,13 +219,15 @@ func (r *BoundEndpointReconciler) createTargetService(ctx context.Context, owner r.Recorder.Event(owner, v1.EventTypeWarning, "Created", "Failed to create Target Service") log.Error(err, "Failed to create Target Service") + ngrokErr := ngrokapi.NewNgrokError(err, ngrokapi.NgrokOpErrFailedToCreateTargetService, "Failed to create Target Service") + setEndpointsStatus(owner, &bindingsv1alpha1.BindingEndpoint{ Status: bindingsv1alpha1.StatusError, - ErrorCode: NgrokErrorTargetServiceCreateFailed, - ErrorMessage: fmt.Sprintf("Failed to create Target Service: %s", err), + ErrorCode: ngrokErr.ErrorCode, + ErrorMessage: ngrokErr.Error(), }) - return err + return ngrokErr } r.Recorder.Event(service, v1.EventTypeNormal, "Created", "Created Target Service") @@ -245,13 +243,15 @@ func (r *BoundEndpointReconciler) createUpstreamService(ctx context.Context, own r.Recorder.Event(owner, v1.EventTypeWarning, "Created", "Failed to create Upstream Service") log.Error(err, "Failed to create Upstream Service") + ngrokErr := ngrokapi.NewNgrokError(err, ngrokapi.NgrokOpErrFailedToCreateUpstreamService, "Failed to create Upstream Service") + setEndpointsStatus(owner, &bindingsv1alpha1.BindingEndpoint{ Status: bindingsv1alpha1.StatusError, - ErrorCode: NgrokErrorUpstreamServiceCreateFailed, - ErrorMessage: fmt.Sprintf("Failed to create Upstream Service: %s", err), + ErrorCode: ngrokErr.ErrorCode, + ErrorMessage: ngrokErr.Error(), }) - return err + return ngrokErr } r.Recorder.Event(service, v1.EventTypeNormal, "Created", "Created Upstream Service") @@ -589,13 +589,15 @@ func (r *BoundEndpointReconciler) tryToBindEndpoint(ctx context.Context, boundEn // update statuses var desired *bindingsv1alpha1.BindingEndpoint + var ngrokErr *ngrok.Error if bindErr != nil { // error log.Error(bindErr, "Failed to bind BoundEndpoint, moving to error") + ngrokErr = ngrokapi.NewNgrokError(bindErr, ngrokapi.NgrokOpErrFailedToConnectServices, "Failed to bind BoundEndpoint") desired = &bindingsv1alpha1.BindingEndpoint{ Status: bindingsv1alpha1.StatusError, - ErrorCode: NgrokErrorFailedToBind, - ErrorMessage: fmt.Sprintf("Failed to bind BoundEndpoint: %s", bindErr), + ErrorCode: ngrokErr.ErrorCode, + ErrorMessage: ngrokErr.Error(), } } else { // success @@ -610,5 +612,16 @@ func (r *BoundEndpointReconciler) tryToBindEndpoint(ctx context.Context, boundEn // set status setEndpointsStatus(boundEndpoint, desired) - return bindErr + + // Why do we check for nil here and return nil explicitly instead of just `return ngrokErr`? + // Because *ngrok.Error meets the interface of error means that error is actually (T=*ngrok.Error, V=nil) + // so outer function signature is (error) and not (*ngrok.Error) + // and an interface _pointing_ to nil is != to nil itself + // therefore *ngrok.Error.(error) is _always_ != nil + // + // See: https://go.dev/doc/faq#nil_error + if ngrokErr != nil { + return ngrokErr + } + return nil } diff --git a/internal/controller/bindings/boundendpoint_controller_test.go b/internal/controller/bindings/boundendpoint_controller_test.go index f525484d..e05c2fac 100644 --- a/internal/controller/bindings/boundendpoint_controller_test.go +++ b/internal/controller/bindings/boundendpoint_controller_test.go @@ -140,7 +140,7 @@ func Test_setEndpointsStatus(t *testing.T) { desired: &bindingsv1alpha1.BindingEndpoint{ Status: bindingsv1alpha1.StatusError, ErrorCode: "ERR_NGROK_1234", - ErrorMessage: "Exampl Error Message", + ErrorMessage: "Example Error Message", }, }, } diff --git a/internal/controller/bindings/boundendpoint_poller_test.go b/internal/controller/bindings/boundendpoint_poller_test.go index 39cdcdf5..4861f49d 100644 --- a/internal/controller/bindings/boundendpoint_poller_test.go +++ b/internal/controller/bindings/boundendpoint_poller_test.go @@ -5,7 +5,7 @@ import ( "testing" "github.com/go-logr/logr" - v6 "github.com/ngrok/ngrok-api-go/v7" + "github.com/ngrok/ngrok-api-go/v7" bindingsv1alpha1 "github.com/ngrok/ngrok-operator/api/bindings/v1alpha1" "github.com/ngrok/ngrok-operator/internal/ngrokapi" "github.com/stretchr/testify/assert" @@ -184,7 +184,7 @@ func Test_BoundEndpointPoller_boundEndpointNeedsUpdate(t *testing.T) { HashedName: hashURI(uriExample1), Endpoints: []bindingsv1alpha1.BindingEndpoint{ { - Ref: v6.Ref{ID: "ep_abc123", URI: "example-uri"}, + Ref: ngrok.Ref{ID: "ep_abc123", URI: "example-uri"}, Status: bindingsv1alpha1.StatusProvisioning, ErrorCode: "", ErrorMessage: "", @@ -218,7 +218,7 @@ func Test_BoundEndpointPoller_boundEndpointNeedsUpdate(t *testing.T) { HashedName: hashURI(uriExample1), Endpoints: []bindingsv1alpha1.BindingEndpoint{ { - Ref: v6.Ref{ID: "ep_abc123", URI: "example-uri"}, + Ref: ngrok.Ref{ID: "ep_abc123", URI: "example-uri"}, Status: bindingsv1alpha1.StatusProvisioning, ErrorCode: "", ErrorMessage: "", @@ -248,7 +248,7 @@ func Test_BoundEndpointPoller_boundEndpointNeedsUpdate(t *testing.T) { HashedName: hashURI(uriExample1), Endpoints: []bindingsv1alpha1.BindingEndpoint{ { - Ref: v6.Ref{ID: "ep_abc123", URI: "example-uri"}, + Ref: ngrok.Ref{ID: "ep_abc123", URI: "example-uri"}, Status: bindingsv1alpha1.StatusProvisioning, ErrorCode: "", ErrorMessage: "", @@ -275,7 +275,7 @@ func Test_BoundEndpointPoller_boundEndpointNeedsUpdate(t *testing.T) { HashedName: hashURI(uriExample2), Endpoints: []bindingsv1alpha1.BindingEndpoint{ { - Ref: v6.Ref{ID: "ep_def456", URI: "example-uri"}, + Ref: ngrok.Ref{ID: "ep_def456", URI: "example-uri"}, Status: bindingsv1alpha1.StatusProvisioning, ErrorCode: "", ErrorMessage: "", @@ -301,19 +301,19 @@ func Test_BoundEndpointPoller_boundEndpointNeedsUpdate(t *testing.T) { HashedName: hashURI(uriExample2), Endpoints: []bindingsv1alpha1.BindingEndpoint{ { - Ref: v6.Ref{ID: "ep_def456", URI: "example-uri"}, + Ref: ngrok.Ref{ID: "ep_def456", URI: "example-uri"}, Status: bindingsv1alpha1.StatusProvisioning, ErrorCode: "", ErrorMessage: "", }, { - Ref: v6.Ref{ID: "ep_xyz999", URI: "example-uri"}, + Ref: ngrok.Ref{ID: "ep_xyz999", URI: "example-uri"}, Status: bindingsv1alpha1.StatusProvisioning, ErrorCode: "", ErrorMessage: "", }, { - Ref: v6.Ref{ID: "ep_www000", URI: "example-uri"}, + Ref: ngrok.Ref{ID: "ep_www000", URI: "example-uri"}, Status: bindingsv1alpha1.StatusProvisioning, ErrorCode: "", ErrorMessage: "", diff --git a/internal/controller/ngrok/kubernetesoperator_controller.go b/internal/controller/ngrok/kubernetesoperator_controller.go index 1f809c60..ddf53f1b 100644 --- a/internal/controller/ngrok/kubernetesoperator_controller.go +++ b/internal/controller/ngrok/kubernetesoperator_controller.go @@ -62,6 +62,10 @@ var featureMap = map[string]string{ ngrokv1alpha1.KubernetesOperatorFeatureGateway: "Gateway", } +const ( + NgrokErrorFailedToCreateCSR = "ERR_NGROK_20006" +) + // KubernetesOperatorReconciler reconciles a KubernetesOperator object type KubernetesOperatorReconciler struct { client.Client @@ -149,7 +153,7 @@ func (r *KubernetesOperatorReconciler) create(ctx context.Context, ko *ngrokv1al if bindingsEnabled { tlsSecret, err := r.findOrCreateTLSSecret(ctx, ko) if err != nil { - return err + return ngrokapi.NewNgrokError(err, ngrokapi.NgrokOpErrFailedToCreateCSR, "failed to create TLS secret for CSR") } createParams.Binding = &ngrok.KubernetesOperatorBindingCreate{ diff --git a/internal/ngrokapi/enriched_errors.go b/internal/ngrokapi/enriched_errors.go new file mode 100644 index 00000000..8495bfd6 --- /dev/null +++ b/internal/ngrokapi/enriched_errors.go @@ -0,0 +1,45 @@ +package ngrokapi + +import ( + "fmt" + "net/http" + + "github.com/ngrok/ngrok-api-go/v7" +) + +// Enriched Errors utilities to help interact with ngrok.Error and ngrok.IsErrorCode() returned by the ngrok API +// For right now these are all manually defined + +type EnrichedError struct { + name string + Code int + statusCode int32 +} + +// list of supported ngrok error codes +var ( + NgrokOpErrInternalServerError = EnrichedError{"ERR_NGROK_20000", 20000, http.StatusInternalServerError} + NgrokOpErrConfigurationError = EnrichedError{"ERR_NGROK_20001", 20001, http.StatusBadRequest} + NgrokOpErrFailedToCreateUpstreamService = EnrichedError{"ERR_NGROK_20002", 20002, http.StatusServiceUnavailable} + NgrokOpErrFailedToCreateTargetService = EnrichedError{"ERR_NGROK_20003", 20003, http.StatusServiceUnavailable} + NgrokOpErrFailedToConnectServices = EnrichedError{"ERR_NGROK_20004", 20004, http.StatusServiceUnavailable} + NgrokOpErrEndpointDenied = EnrichedError{"ERR_NGROK_20005", 20005, http.StatusForbidden} + NgrokOpErrFailedToCreateCSR = EnrichedError{"ERR_NGROK_20006", 20006, http.StatusInternalServerError} +) + +func NewNgrokError(origErr error, ee EnrichedError, msg string) *ngrok.Error { + if ngrokErr, ok := origErr.(*ngrok.Error); ok { + // already have an ngrok.Error + // overwrite the message and return + return &ngrok.Error{ + Msg: ngrokErr.Msg, + Details: ngrokErr.Details, + } + } + + return &ngrok.Error{ + Msg: fmt.Sprintf("%s: %s", msg, origErr), + ErrorCode: ee.name, + StatusCode: ee.statusCode, + } +}