Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix error message in readonly approver #376

Merged
merged 1 commit into from
Oct 17, 2022
Merged

Fix error message in readonly approver #376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/gcp-controller-manager/kubelet_readonly_csr_approver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ func (approver *kubeletReadonlyCSRApprover) handle(ctx context.Context, csr *cap

switch {
case response.err != nil:
klog.Errorf("validating CSR %q: %v", csr.Name, err)
return fmt.Errorf("validating CSR %q: %v", csr.Name, err)
klog.Errorf("validating CSR %q failed: validator %q has error %v", csr.Name, validator.name, response.err)
return fmt.Errorf("validating CSR %q failed: validator %q has error %v", csr.Name, validator.name, response.err)
case response.result == false:
klog.Infof("validator %q: denied CSR %q", validator.name, csr.Name)
recordValidatorMetric(csrmetrics.ApprovalStatusDeny)
Expand Down
168 changes: 118 additions & 50 deletions cmd/gcp-controller-manager/kubelet_readonly_csr_approver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ func TestKubeletReadonlyApprover(t *testing.T) {
testCases := []struct {
name string
sarAllowed, podAnnotationAllowed, usageAllowed, autopilotEnabled bool
sarWantError, podAnnotationWantError bool
expectCondition capi.CertificateSigningRequestCondition
expectError error
}{
{
name: "success when all valid",
Expand All @@ -48,11 +50,9 @@ func TestKubeletReadonlyApprover(t *testing.T) {
},
},
{
name: "success when all valid but annotation bypassed",
sarAllowed: true,
podAnnotationAllowed: false,
usageAllowed: true,
autopilotEnabled: false,
name: "success when all valid but annotation bypassed",
sarAllowed: true,
usageAllowed: true,
expectCondition: capi.CertificateSigningRequestCondition{
Type: capi.CertificateApproved,
Reason: "AutoApproved",
Expand All @@ -61,11 +61,8 @@ func TestKubeletReadonlyApprover(t *testing.T) {
},
},
{
name: "fail by all validation fail",
sarAllowed: false,
podAnnotationAllowed: false,
usageAllowed: false,
autopilotEnabled: true,
name: "fail by all validation fail",
autopilotEnabled: true,
expectCondition: capi.CertificateSigningRequestCondition{
Type: capi.CertificateDenied,
Reason: "AutoDenied",
Expand All @@ -75,7 +72,6 @@ func TestKubeletReadonlyApprover(t *testing.T) {
},
{
name: "fail by sar validation fail",
sarAllowed: false,
podAnnotationAllowed: true,
usageAllowed: true,
autopilotEnabled: true,
Expand All @@ -87,11 +83,10 @@ func TestKubeletReadonlyApprover(t *testing.T) {
},
},
{
name: "fail by pod annotation validation fail",
sarAllowed: true,
podAnnotationAllowed: false,
usageAllowed: true,
autopilotEnabled: true,
name: "fail by pod annotation validation fail",
sarAllowed: true,
usageAllowed: true,
autopilotEnabled: true,
expectCondition: capi.CertificateSigningRequestCondition{
Type: capi.CertificateDenied,
Reason: "AutoDenied",
Expand All @@ -103,7 +98,6 @@ func TestKubeletReadonlyApprover(t *testing.T) {
name: "fail by usage validation fail",
sarAllowed: true,
podAnnotationAllowed: true,
usageAllowed: false,
autopilotEnabled: true,
expectCondition: capi.CertificateSigningRequestCondition{
Type: capi.CertificateDenied,
Expand All @@ -112,22 +106,58 @@ func TestKubeletReadonlyApprover(t *testing.T) {
Status: v1.ConditionTrue,
},
},
{
name: "fail by sar validation error",
sarAllowed: true,
podAnnotationAllowed: true,
usageAllowed: true,
autopilotEnabled: true,
sarWantError: true,
expectError: fmt.Errorf("validating CSR \"fail by sar validation error\" failed: validator \"rbac validator\" has error failed to get subject access review"),
expectCondition: capi.CertificateSigningRequestCondition{
Type: capi.CertificateDenied,
Reason: "AutoDenied",
Message: "csr usage any is not allowed, allowed usages: [\"key encipherment\",\"digital signature\",\"client auth\"]",
Status: v1.ConditionTrue,
},
},
{
name: "fail by pod annotation validation error",
sarAllowed: true,
podAnnotationAllowed: true,
usageAllowed: true,
autopilotEnabled: true,
podAnnotationWantError: true,
expectError: fmt.Errorf("validating CSR \"fail by pod annotation validation error\" failed: validator \"pod annotation validator\" has error get pod test failed: failed to get pod"),
expectCondition: capi.CertificateSigningRequestCondition{
Type: capi.CertificateDenied,
Reason: "AutoDenied",
Message: "csr usage any is not allowed, allowed usages: [\"key encipherment\",\"digital signature\",\"client auth\"]",
Status: v1.ConditionTrue,
},
},
}

for _, testCase := range testCases {
t.Run(fmt.Sprint(testCase.name), func(t *testing.T) {
request := generateTestRequest(t, testCase.sarAllowed, testCase.podAnnotationAllowed, testCase.usageAllowed, testCase.autopilotEnabled)
request := generateTestRequest(t, testCase.sarAllowed, testCase.podAnnotationAllowed, testCase.usageAllowed, testCase.autopilotEnabled,
testCase.sarWantError, testCase.podAnnotationWantError)
request.csr.Name = testCase.name
request.csr.Spec.SignerName = kubeletReadonlyCSRSignerName
approver := newKubeletReadonlyCSRApprover(request.controllerContext)
*autopilotEnabled = testCase.autopilotEnabled
err := approver.handle(request.context, request.csr)
if err != nil {
t.Fatalf("error when handle approver, error: %v", err)
}
if diff := cmp.Diff(request.csr.Status.Conditions[0], testCase.expectCondition, cmp.Comparer(func(x, y capi.CertificateSigningRequestCondition) bool {
return x.Type == y.Type && x.Reason == y.Reason && x.Message == y.Message && x.Status == y.Status
})); diff != "" {
t.Fatalf("condition don't match, diff -want +got\n%s", diff)
switch {
case testCase.expectError == nil && err == nil:
if diff := cmp.Diff(request.csr.Status.Conditions[0], testCase.expectCondition, cmp.Comparer(func(x, y capi.CertificateSigningRequestCondition) bool {
return x.Type == y.Type && x.Reason == y.Reason && x.Message == y.Message && x.Status == y.Status
})); diff != "" {
t.Fatalf("condition don't match, diff -want +got\n%s", diff)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"condition doesn't match"

}
case testCase.expectError != nil && err == nil ||
err != nil && testCase.expectError == nil ||
err.Error() != testCase.expectError.Error():
t.Fatalf("error not match, got: %v, expect: %v", err, testCase.expectError)
}
})
}
Expand Down Expand Up @@ -162,6 +192,7 @@ func TestValidateCSRUsage(t *testing.T) {

testCases := []struct {
name string
wantError bool
request kubeletReadonlyCSRRequest
expectResult kubeletReadonlyCSRResponse
}{
Expand Down Expand Up @@ -203,6 +234,7 @@ func TestValidateCSRUsage(t *testing.T) {
if allowed {
testCases = append(testCases, struct {
name string
wantError bool
request kubeletReadonlyCSRRequest
expectResult kubeletReadonlyCSRResponse
}{
Expand All @@ -219,6 +251,7 @@ func TestValidateCSRUsage(t *testing.T) {
} else {
testCases = append(testCases, struct {
name string
wantError bool
request kubeletReadonlyCSRRequest
expectResult kubeletReadonlyCSRResponse
}{
Expand Down Expand Up @@ -250,6 +283,7 @@ func TestValidatePodAnnotation(t *testing.T) {
annotation string
autopilotEnabled bool
podName string
wantError bool
expectResult kubeletReadonlyCSRResponse
}{
{
Expand All @@ -274,23 +308,31 @@ func TestValidatePodAnnotation(t *testing.T) {
},
},
{
name: "not auto pilot cluster",
podName: "test-default",
autopilotEnabled: false,
name: "not auto pilot cluster",
podName: "test-autopilot-not-enabled",
expectResult: kubeletReadonlyCSRResponse{
result: true,
err: nil,
message: "Bypassing annotation validation because cluster is not a GKE Autopilot cluster.",
},
},
{
name: "error when get pod",
podName: "test-get-pod-fail",
wantError: true,
annotation: kubeletAPILimitedReaderAnnotationKey,
autopilotEnabled: true,
expectResult: kubeletReadonlyCSRResponse{
err: fmt.Errorf("get pod test-get-pod-fail failed: failed to get pod"),
message: "get pod test-get-pod-fail failed: failed to get pod",
},
},
}
for _, testCase := range testCases {
t.Run(fmt.Sprint(testCase.name), func(t *testing.T) {
request := generateTestRequestByPodName(t, testCase.podName)
request.autopilotEnabled = testCase.autopilotEnabled
if len(testCase.annotation) != 0 {
createPodWithAnnotation(testCase.podName, testCase.annotation, *request.controllerContext)
}
createPodWithAnnotation(testCase.podName, testCase.annotation, testCase.wantError, *request.controllerContext)
response := validatePodAnnotation(request)
compareKubeletReadonlyCSRRequestResult(response, testCase.expectResult, t)
})
Expand All @@ -301,11 +343,11 @@ func TestValidateRbac(t *testing.T) {
testCases := []struct {
name string
allowed bool
wantError bool
expectResult kubeletReadonlyCSRResponse
}{
{
name: "not allow to create csr",
allowed: false,
name: "not allow to create csr",
expectResult: kubeletReadonlyCSRResponse{
result: false,
err: nil,
Expand All @@ -321,10 +363,20 @@ func TestValidateRbac(t *testing.T) {
message: "user test is allowed to create a CSR",
},
},
{
name: "error to create csr",
allowed: true,
wantError: true,
expectResult: kubeletReadonlyCSRResponse{
result: false,
err: fmt.Errorf("failed to get subject access review"),
message: "subject access review request failed: failed to get subject access review",
},
},
}
for _, testCase := range testCases {
t.Run(fmt.Sprint(testCase.name), func(t *testing.T) {
request := generateTestRequestBySubjectAccessReview(t, testCase.allowed)
request := generateTestRequestBySubjectAccessReview(t, testCase.allowed, testCase.wantError)
request.csr.Spec.Username = "test"
response := validateRbac(request)
compareKubeletReadonlyCSRRequestResult(response, testCase.expectResult, t)
Expand Down Expand Up @@ -356,7 +408,7 @@ func generateTestRequestByUsage(t *testing.T, usage []capi.KeyUsage) kubeletRead
return request
}

func createPodWithAnnotation(podName, annotation string, controllerContext controllerContext) {
func createPodWithAnnotation(podName, annotation string, wantError bool, controllerContext controllerContext) {
pod := &v1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Expand All @@ -380,21 +432,28 @@ func createPodWithAnnotation(podName, annotation string, controllerContext contr
},
}
client := controllerContext.client.(*fake.Clientset)
client.AddReactor("get", "pods", func(action testclient.Action) (handled bool, ret runtime.Object, err error) {
return true, pod, nil
})
if !wantError {
client.PrependReactor("get", "pods", func(action testclient.Action) (handled bool, ret runtime.Object, err error) {
return true, pod, nil
})
} else {
client.PrependReactor("get", "pods", func(action testclient.Action) (handled bool, ret runtime.Object, err error) {
return true, nil, fmt.Errorf("failed to get pod")
})
}
}

func generateTestRequest(t *testing.T, sarAllowed, podAnnotationAllowed, usageAllowed, autopilotEnabled bool) kubeletReadonlyCSRRequest {
request := generateTestRequestBySubjectAccessReview(t, sarAllowed)
func generateTestRequest(t *testing.T, sarAllowed, podAnnotationAllowed, usageAllowed, autopilotEnabled,
sarWantError, podAnnotationWantError bool) kubeletReadonlyCSRRequest {
request := generateTestRequestBySubjectAccessReview(t, sarAllowed, sarWantError)
request.csr.Spec.Username = "test"
request.csr.Spec.Extra = map[string]capi.ExtraValue{}
request.autopilotEnabled = autopilotEnabled
if podAnnotationAllowed {
request.csr.Spec.Extra = map[string]capi.ExtraValue{
podNameKey: []string{"test"},
}
createPodWithAnnotation("test", kubeletAPILimitedReaderAnnotationKey, *request.controllerContext)
createPodWithAnnotation("test", kubeletAPILimitedReaderAnnotationKey, podAnnotationWantError, *request.controllerContext)
}
if usageAllowed {
request.csr.Spec.Usages = []capi.KeyUsage{
Expand All @@ -413,21 +472,30 @@ func generateTestRequest(t *testing.T, sarAllowed, podAnnotationAllowed, usageAl
func generateTestRequestByPodName(t *testing.T, podName string) kubeletReadonlyCSRRequest {
request := generateTestKubeletReadonlyCSRRequest(t)
request.csr.Spec.Extra = map[string]capi.ExtraValue{
podNameKey: []string{podName},
"annotations": []string{"test"},
}
if len(podName) != 0 {
request.csr.Spec.Extra[podNameKey] = []string{podName}
}
return request
}

func generateTestRequestBySubjectAccessReview(t *testing.T, allowed bool) kubeletReadonlyCSRRequest {
func generateTestRequestBySubjectAccessReview(t *testing.T, allowed, wantError bool) kubeletReadonlyCSRRequest {
request := generateTestKubeletReadonlyCSRRequest(t)
client := request.controllerContext.client.(*fake.Clientset)
client.AddReactor("create", "subjectaccessreviews", func(action testclient.Action) (handled bool, ret runtime.Object, err error) {
return true, &authorization.SubjectAccessReview{
Status: authorization.SubjectAccessReviewStatus{
Allowed: allowed,
},
}, nil
})
if !wantError {
client.PrependReactor("create", "subjectaccessreviews", func(action testclient.Action) (handled bool, ret runtime.Object, err error) {
return true, &authorization.SubjectAccessReview{
Status: authorization.SubjectAccessReviewStatus{
Allowed: allowed,
},
}, nil
})
} else {
client.PrependReactor("create", "subjectaccessreviews", func(action testclient.Action) (handled bool, ret runtime.Object, err error) {
return true, nil, fmt.Errorf("failed to get subject access review")
})
}
return request
}

Expand Down