Add context timeout for checkaccess requests and fix metrics (#350)

* Send correct code in SAR status Add Checkaccess latency metrics Fix checkaccess counters Signed-off-by: Anumita <ansheno@microsoft.com> * added context timeout for checkaccess Add metrics for discover resources Signed-off-by: Anumita <ansheno@microsoft.com> * add tests for context timeout Signed-off-by: Anumita <ansheno@microsoft.com> * add client request id Signed-off-by: Anumita <ansheno@microsoft.com> * remove unnecessary metrics Signed-off-by: Anumita <ansheno@microsoft.com> * modify log line Signed-off-by: Anumita <ansheno@microsoft.com> Signed-off-by: Anumita <ansheno@microsoft.com> Co-authored-by: Krupesh <krdhruva@microsoft.com>
kubeguard · Jan 23, 2023 · 09ab161 · 09ab161
1 parent e26284d
commit 09ab161
Show file tree

Hide file tree

Showing 17 changed files with 514 additions and 141 deletions.
diff --git a/authz/providers/azure/azure.go b/authz/providers/azure/azure.go
@@ -16,6 +16,7 @@ limitations under the License.
 package azure
 
 import (
+	"net/http"
 	"strings"
 	"sync"
 
@@ -24,6 +25,7 @@ import (
 	authzOpts "go.kubeguard.dev/guard/authz/providers/azure/options"
 	"go.kubeguard.dev/guard/authz/providers/azure/rbac"
 	azureutils "go.kubeguard.dev/guard/util/azure"
+	errutils "go.kubeguard.dev/guard/util/error"
 
 	"github.com/Azure/go-autorest/autorest/azure"
 	"github.com/pkg/errors"
@@ -78,7 +80,7 @@ func newAuthzClient(opts authzOpts.Options, authopts auth.Options, operationsMap
 
 func (s Authorizer) Check(request *authzv1.SubjectAccessReviewSpec, store authz.Store) (*authzv1.SubjectAccessReviewStatus, error) {
 	if request == nil {
-		return nil, errors.New("subject access review is nil")
+		return nil, errutils.WithCode(errors.New("subject access review is nil"), http.StatusBadRequest)
 	}
 
 	// check if user is system accounts
@@ -118,15 +120,21 @@ func (s Authorizer) Check(request *authzv1.SubjectAccessReviewSpec, store authz.
 	}
 
 	if s.rbacClient.IsTokenExpired() {
-		_ = s.rbacClient.RefreshToken()
+		if err := s.rbacClient.RefreshToken(); err != nil {
+			return nil, errutils.WithCode(err, http.StatusInternalServerError)
+		}
 	}
 
 	response, err := s.rbacClient.CheckAccess(request)
 	if err == nil {
 		klog.V(5).Infof(response.Reason)
 		_ = s.rbacClient.SetResultInCache(request, response.Allowed, store)
 	} else {
-		err = errors.Errorf(rbac.CheckAccessErrorFormat, err)
+		code := http.StatusInternalServerError
+		if v, ok := err.(errutils.HttpStatusCode); ok {
+			code = v.Code()
+		}
+		err = errutils.WithCode(errors.Errorf(rbac.CheckAccessErrorFormat, err), code)
 	}
 
 	return response, err

diff --git a/authz/providers/azure/azure_test.go b/authz/providers/azure/azure_test.go
@@ -29,6 +29,7 @@ import (
 	authzOpts "go.kubeguard.dev/guard/authz/providers/azure/options"
 	"go.kubeguard.dev/guard/authz/providers/azure/rbac"
 	azureutils "go.kubeguard.dev/guard/util/azure"
+	errutils "go.kubeguard.dev/guard/util/error"
 
 	"github.com/appscode/pat"
 	"github.com/stretchr/testify/assert"
@@ -71,7 +72,7 @@ func clientSetup(serverUrl, mode string) (*Authorizer, error) {
 	return c, nil
 }
 
-func serverSetup(loginResp, checkaccessResp string, loginStatus, checkaccessStatus int) (*httptest.Server, error) {
+func serverSetup(loginResp, checkaccessResp string, loginStatus, checkaccessStatus int, sleepFor time.Duration) (*httptest.Server, error) {
 	listener, err := net.Listen("tcp", "127.0.0.1:")
 	if err != nil {
 		return nil, err
@@ -85,6 +86,7 @@ func serverSetup(loginResp, checkaccessResp string, loginStatus, checkaccessStat
 	}))
 
 	m.Post("/arm/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		time.Sleep(sleepFor)
 		w.WriteHeader(checkaccessStatus)
 		_, _ = w.Write([]byte(checkaccessResp))
 	}))
@@ -98,8 +100,8 @@ func serverSetup(loginResp, checkaccessResp string, loginStatus, checkaccessStat
 	return srv, nil
 }
 
-func getServerAndClient(t *testing.T, loginResp, checkaccessResp string, checkaccessStatus int) (*httptest.Server, *Authorizer, authz.Store) {
-	srv, err := serverSetup(loginResp, checkaccessResp, http.StatusOK, checkaccessStatus)
+func getServerAndClient(t *testing.T, loginResp, checkaccessResp string, checkaccessStatus int, sleepFor time.Duration) (*httptest.Server, *Authorizer, authz.Store) {
+	srv, err := serverSetup(loginResp, checkaccessResp, http.StatusOK, checkaccessStatus, sleepFor)
 	if err != nil {
 		t.Fatalf("Error when creating server, reason: %v", err)
 	}
@@ -129,7 +131,7 @@ func TestCheck(t *testing.T) {
 		"actionId":"Microsoft.Kubernetes/connectedClusters/pods/delete",
 		"isDataAction":true,"roleAssignment":null,"denyAssignment":null,"timeToLiveInMs":300000}]`
 
-		srv, client, store := getServerAndClient(t, loginResp, validBody, http.StatusOK)
+		srv, client, store := getServerAndClient(t, loginResp, validBody, http.StatusOK, 1*time.Second)
 		defer srv.Close()
 		defer store.Close()
 
@@ -146,11 +148,14 @@ func TestCheck(t *testing.T) {
 		assert.NotNil(t, resp)
 		assert.Equal(t, resp.Allowed, true)
 		assert.Equal(t, resp.Denied, false)
+		if v, ok := err.(errutils.HttpStatusCode); ok {
+			assert.Equal(t, v.Code(), http.StatusOK)
+		}
 	})
 
 	t.Run("unsuccessful request", func(t *testing.T) {
 		validBody := `""`
-		srv, client, store := getServerAndClient(t, loginResp, validBody, http.StatusInternalServerError)
+		srv, client, store := getServerAndClient(t, loginResp, validBody, http.StatusInternalServerError, 1*time.Second)
 		defer srv.Close()
 		defer store.Close()
 
@@ -166,5 +171,31 @@ func TestCheck(t *testing.T) {
 		assert.Nilf(t, resp, "response should be nil")
 		assert.NotNilf(t, err, "should get error")
 		assert.Contains(t, err.Error(), "Error occured during authorization check")
+		if v, ok := err.(errutils.HttpStatusCode); ok {
+			assert.Equal(t, v.Code(), http.StatusInternalServerError)
+		}
+	})
+
+	t.Run("context timeout request", func(t *testing.T) {
+		validBody := `""`
+		srv, client, store := getServerAndClient(t, loginResp, validBody, http.StatusInternalServerError, 25*time.Second)
+		defer srv.Close()
+		defer store.Close()
+
+		request := &authzv1.SubjectAccessReviewSpec{
+			User: "beta@bing.com",
+			ResourceAttributes: &authzv1.ResourceAttributes{
+				Namespace: "dev", Group: "", Resource: "pods",
+				Subresource: "status", Version: "v1", Name: "test", Verb: "delete",
+			}, Extra: map[string]authzv1.ExtraValue{"oid": {"00000000-0000-0000-0000-000000000000"}},
+		}
+
+		resp, err := client.Check(request, store)
+		assert.Nilf(t, resp, "response should be nil")
+		assert.NotNilf(t, err, "should get error")
+		assert.Contains(t, err.Error(), "Checkaccess requests have timed out")
+		if v, ok := err.(errutils.HttpStatusCode); ok {
+			assert.Equal(t, v.Code(), http.StatusInternalServerError)
+		}
 	})
 }
diff --git a/authz/providers/azure/rbac/checkaccessreqhelper.go b/authz/providers/azure/rbac/checkaccessreqhelper.go
@@ -18,10 +18,12 @@ package rbac
 import (
 	"encoding/json"
 	"fmt"
+	"net/http"
 	"path"
 	"strings"
 
 	azureutils "go.kubeguard.dev/guard/util/azure"
+	errutils "go.kubeguard.dev/guard/util/error"
 
 	"github.com/google/uuid"
 	"github.com/pkg/errors"
@@ -494,16 +496,16 @@ func prepareCheckAccessRequestBody(req *authzv1.SubjectAccessReviewSpec, cluster
 		val := oid.String()
 		userOid = val[1 : len(val)-1]
 		if !isValidUUID(userOid) {
-			return nil, errors.New("oid info sent from authentication module is not valid")
+			return nil, errutils.WithCode(errors.New("oid info sent from authentication module is not valid"), http.StatusBadRequest)
 		}
 	} else {
-		return nil, errors.New("oid info not sent from authentication module")
+		return nil, errutils.WithCode(errors.New("oid info not sent from authentication module"), http.StatusBadRequest)
 	}
 	groups := getValidSecurityGroups(req.Groups)
 	username = req.User
 	actions, err := getDataActions(req, clusterType, operationsMap)
 	if err != nil {
-		return nil, errors.Wrap(err, "Error while creating list of dataactions for check access call")
+		return nil, errutils.WithCode(errors.Wrap(err, "Error while creating list of dataactions for check access call"), http.StatusInternalServerError)
 	}
 	var checkAccessReqs []*CheckAccessRequest
 	for i := 0; i < len(actions); i += ActionBatchCount {
@@ -547,7 +549,7 @@ func ConvertCheckAccessResponse(body []byte) (*authzv1.SubjectAccessReviewStatus
 	err := json.Unmarshal(body, &response)
 	if err != nil {
 		klog.V(10).Infof("Failed to parse checkacccess response. Error:%s", err.Error())
-		return nil, errors.Wrap(err, "Error in unmarshalling check access response.")
+		return nil, errutils.WithCode(errors.Wrap(err, "Error in unmarshalling check access response."), http.StatusInternalServerError)
 	}
 
 	deniedResultFound := slices.IndexFunc(response, func(a AuthorizationDecision) bool { return strings.ToLower(a.Decision) != Allowed })