Merge pull request #80234 from tallclair/kubelet-authz

Cleanup kubelet authz tests & make explicit

pkg/kubelet/server/BUILD

@@ -82,7 +82,6 @@ go_test(
         "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/httpstream:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy:go_default_library",
-        "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/authenticator:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",

pkg/kubelet/server/auth_test.go

@@ -16,7 +16,15 @@ limitations under the License.
 
 package server
 
-import "testing"
+import (
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apiserver/pkg/authentication/user"
+	"k8s.io/apiserver/pkg/authorization/authorizer"
+)
 
 func TestIsSubPath(t *testing.T) {
 	testcases := map[string]struct {
@@ -51,3 +59,96 @@ func TestIsSubPath(t *testing.T) {
 		}
 	}
 }
+
+func TestGetRequestAttributes(t *testing.T) {
+	for _, test := range AuthzTestCases() {
+		t.Run(test.Method+":"+test.Path, func(t *testing.T) {
+			getter := NewNodeAuthorizerAttributesGetter(authzTestNodeName)
+
+			req, err := http.NewRequest(test.Method, "https://localhost:1234"+test.Path, nil)
+			require.NoError(t, err)
+			attrs := getter.GetRequestAttributes(AuthzTestUser(), req)
+
+			test.AssertAttributes(t, attrs)
+		})
+	}
+}
+
+const (
+	authzTestNodeName = "test"
+	authzTestUserName = "phibby"
+)
+
+type AuthzTestCase struct {
+	Method, Path string
+
+	ExpectedVerb, ExpectedSubresource string
+}
+
+func (a *AuthzTestCase) AssertAttributes(t *testing.T, attrs authorizer.Attributes) {
+	expectedAttributes := authorizer.AttributesRecord{
+		User:            AuthzTestUser(),
+		APIGroup:        "",
+		APIVersion:      "v1",
+		Verb:            a.ExpectedVerb,
+		Resource:        "nodes",
+		Name:            authzTestNodeName,
+		Subresource:     a.ExpectedSubresource,
+		ResourceRequest: true,
+		Path:            a.Path,
+	}
+
+	assert.Equal(t, expectedAttributes, attrs)
+}
+
+func AuthzTestUser() user.Info {
+	return &user.DefaultInfo{Name: authzTestUserName}
+}
+
+func AuthzTestCases() []AuthzTestCase {
+	// Path -> ExpectedSubresource
+	testPaths := map[string]string{
+		"/attach/{podNamespace}/{podID}/{containerName}":       "proxy",
+		"/attach/{podNamespace}/{podID}/{uid}/{containerName}": "proxy",
+		"/configz": "proxy",
+		"/containerLogs/{podNamespace}/{podID}/{containerName}": "proxy",
+		"/cri/":                    "proxy",
+		"/cri/foo":                 "proxy",
+		"/debug/flags/v":           "proxy",
+		"/debug/pprof/{subpath:*}": "proxy",
+		"/exec/{podNamespace}/{podID}/{containerName}":       "proxy",
+		"/exec/{podNamespace}/{podID}/{uid}/{containerName}": "proxy",
+		"/healthz":                            "proxy",
+		"/healthz/log":                        "proxy",
+		"/healthz/ping":                       "proxy",
+		"/healthz/syncloop":                   "proxy",
+		"/logs/":                              "log",
+		"/logs/{logpath:*}":                   "log",
+		"/metrics":                            "metrics",
+		"/metrics/cadvisor":                   "metrics",
+		"/metrics/probes":                     "metrics",
+		"/metrics/resource/v1alpha1":          "metrics",
+		"/pods/":                              "proxy",
+		"/portForward/{podNamespace}/{podID}": "proxy",
+		"/portForward/{podNamespace}/{podID}/{uid}":         "proxy",
+		"/run/{podNamespace}/{podID}/{containerName}":       "proxy",
+		"/run/{podNamespace}/{podID}/{uid}/{containerName}": "proxy",
+		"/runningpods/":    "proxy",
+		"/spec/":           "spec",
+		"/stats/":          "stats",
+		"/stats/container": "stats",
+		"/stats/summary":   "stats",
+		"/stats/{namespace}/{podName}/{uid}/{containerName}": "stats",
+		"/stats/{podName}/{containerName}":                   "stats",
+	}
+	testCases := []AuthzTestCase{}
+	for path, subresource := range testPaths {
+		testCases = append(testCases,
+			AuthzTestCase{"POST", path, "create", subresource},
+			AuthzTestCase{"GET", path, "get", subresource},
+			AuthzTestCase{"PUT", path, "update", subresource},
+			AuthzTestCase{"PATCH", path, "patch", subresource},
+			AuthzTestCase{"DELETE", path, "delete", subresource})
+	}
+	return testCases
+}

pkg/kubelet/server/server_test.go

@@ -43,7 +43,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/httpstream"
 	"k8s.io/apimachinery/pkg/util/httpstream/spdy"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
@@ -682,91 +681,57 @@ type authTestCase struct {
 	Path   string
 }
 
-func TestAuthFilters(t *testing.T) {
+// Ensure all registered handlers & services have an associated testcase.
+func TestAuthzCoverage(t *testing.T) {
 	fw := newServerTest()
 	defer fw.testHTTPServer.Close()
 
-	testcases := []authTestCase{}
-
-	// This is a sanity check that the Handle->HandleWithFilter() delegation is working
-	// Ideally, these would move to registered web services and this list would get shorter
-	expectedPaths := []string{"/healthz", "/metrics", "/metrics/cadvisor"}
-	paths := sets.NewString(fw.serverUnderTest.restfulCont.RegisteredHandlePaths()...)
-	for _, expectedPath := range expectedPaths {
-		if !paths.Has(expectedPath) {
-			t.Errorf("Expected registered handle path %s was missing", expectedPath)
-		}
-	}
+	// method:path -> has coverage
+	expectedCases := map[string]bool{}
 
 	// Test all the non-web-service handlers
 	for _, path := range fw.serverUnderTest.restfulCont.RegisteredHandlePaths() {
-		testcases = append(testcases, authTestCase{"GET", path})
-		testcases = append(testcases, authTestCase{"POST", path})
-		// Test subpaths for directory handlers
-		if strings.HasSuffix(path, "/") {
-			testcases = append(testcases, authTestCase{"GET", path + "foo"})
-			testcases = append(testcases, authTestCase{"POST", path + "foo"})
-		}
+		expectedCases["GET:"+path] = false
+		expectedCases["POST:"+path] = false
 	}
 
 	// Test all the generated web-service paths
 	for _, ws := range fw.serverUnderTest.restfulCont.RegisteredWebServices() {
 		for _, r := range ws.Routes() {
-			testcases = append(testcases, authTestCase{r.Method, r.Path})
+			expectedCases[r.Method+":"+r.Path] = false
 		}
 	}
 
-	methodToAPIVerb := map[string]string{"GET": "get", "POST": "create", "PUT": "update"}
-	pathToSubresource := func(path string) string {
-		switch {
-		// Cases for subpaths we expect specific subresources for
-		case isSubpath(path, statsPath):
-			return "stats"
-		case isSubpath(path, specPath):
-			return "spec"
-		case isSubpath(path, logsPath):
-			return "log"
-		case isSubpath(path, metricsPath):
-			return "metrics"
-
-		// Cases for subpaths we expect to map to the "proxy" subresource
-		case isSubpath(path, "/attach"),
-			isSubpath(path, "/configz"),
-			isSubpath(path, "/containerLogs"),
-			isSubpath(path, "/debug"),
-			isSubpath(path, "/exec"),
-			isSubpath(path, "/healthz"),
-			isSubpath(path, "/pods"),
-			isSubpath(path, "/portForward"),
-			isSubpath(path, "/run"),
-			isSubpath(path, "/runningpods"),
-			isSubpath(path, "/cri"):
-			return "proxy"
-
-		default:
-			panic(fmt.Errorf(`unexpected kubelet API path %s.
-The kubelet API has likely registered a handler for a new path.
-If the new path has a use case for partitioned authorization when requested from the kubelet API,
-add a specific subresource for it in auth.go#GetRequestAttributes() and in TestAuthFilters().
-Otherwise, add it to the expected list of paths that map to the "proxy" subresource in TestAuthFilters()`, path))
+	// This is a sanity check that the Handle->HandleWithFilter() delegation is working
+	// Ideally, these would move to registered web services and this list would get shorter
+	expectedPaths := []string{"/healthz", "/metrics", "/metrics/cadvisor"}
+	for _, expectedPath := range expectedPaths {
+		if _, expected := expectedCases["GET:"+expectedPath]; !expected {
+			t.Errorf("Expected registered handle path %s was missing", expectedPath)
 		}
 	}
-	attributesGetter := NewNodeAuthorizerAttributesGetter(types.NodeName("test"))
 
-	for _, tc := range testcases {
+	for _, tc := range AuthzTestCases() {
+		expectedCases[tc.Method+":"+tc.Path] = true
+	}
+
+	for tc, found := range expectedCases {
+		if !found {
+			t.Errorf("Missing authz test case for %s", tc)
+		}
+	}
+}
+
+func TestAuthFilters(t *testing.T) {
+	fw := newServerTest()
+	defer fw.testHTTPServer.Close()
+
+	attributesGetter := NewNodeAuthorizerAttributesGetter(authzTestNodeName)
+
+	for _, tc := range AuthzTestCases() {
+		t.Run(tc.Method+":"+tc.Path, func(t *testing.T) {
 			var (
-			expectedUser       = &user.DefaultInfo{Name: "test"}
-			expectedAttributes = authorizer.AttributesRecord{
-				User:            expectedUser,
-				APIGroup:        "",
-				APIVersion:      "v1",
-				Verb:            methodToAPIVerb[tc.Method],
-				Resource:        "nodes",
-				Name:            "test",
-				Subresource:     pathToSubresource(tc.Path),
-				ResourceRequest: true,
-				Path:            tc.Path,
-			}
+				expectedUser = AuthzTestUser()
 
 				calledAuthenticate = false
 				calledAuthorize    = false
@@ -779,47 +744,27 @@ Otherwise, add it to the expected list of paths that map to the "proxy" subresou
 			}
 			fw.fakeAuth.attributesFunc = func(u user.Info, req *http.Request) authorizer.Attributes {
 				calledAttributes = true
-			if u != expectedUser {
-				t.Fatalf("%s: expected user %v, got %v", tc.Path, expectedUser, u)
-			}
+				require.Equal(t, expectedUser, u)
 				return attributesGetter.GetRequestAttributes(u, req)
 			}
 			fw.fakeAuth.authorizeFunc = func(a authorizer.Attributes) (decision authorizer.Decision, reason string, err error) {
 				calledAuthorize = true
-			if a != expectedAttributes {
-				t.Fatalf("%s: expected attributes\n\t%#v\ngot\n\t%#v", tc.Path, expectedAttributes, a)
-			}
+				tc.AssertAttributes(t, a)
 				return authorizer.DecisionNoOpinion, "", nil
 			}
 
 			req, err := http.NewRequest(tc.Method, fw.testHTTPServer.URL+tc.Path, nil)
-		if err != nil {
-			t.Errorf("%s: unexpected error: %v", tc.Path, err)
-			continue
-		}
-		resp, err := http.DefaultClient.Do(req)
-		if err != nil {
-			t.Errorf("%s: unexpected error: %v", tc.Path, err)
-			continue
-		}
-		defer resp.Body.Close()
-		if resp.StatusCode != http.StatusForbidden {
-			t.Errorf("%s: unexpected status code %d", tc.Path, resp.StatusCode)
-			continue
-		}
+			require.NoError(t, err)
 
-		if !calledAuthenticate {
-			t.Errorf("%s: Authenticate was not called", tc.Path)
-			continue
-		}
-		if !calledAttributes {
-			t.Errorf("%s: Attributes were not called", tc.Path)
-			continue
-		}
-		if !calledAuthorize {
-			t.Errorf("%s: Authorize was not called", tc.Path)
-			continue
-		}
+			resp, err := http.DefaultClient.Do(req)
+			require.NoError(t, err)
+			defer resp.Body.Close()
+
+			assert.Equal(t, http.StatusForbidden, resp.StatusCode)
+			assert.True(t, calledAuthenticate, "Authenticate was not called")
+			assert.True(t, calledAttributes, "Attributes were not called")
+			assert.True(t, calledAuthorize, "Authorize was not called")
+		})
 	}
 }