Skip to content

Commit

Permalink
Merge pull request #376 from blackzlq/myfeature
Browse files Browse the repository at this point in the history 
Fix error message in readonly approver
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Oct 17, 2022
2 parents f8910f4 + a97cf60 commit 064029e
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 52 deletions.
4 changes: 2 additions & 2 deletions cmd/gcp-controller-manager/kubelet_readonly_csr_approver.go
View file
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
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)
}
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

0 comments on commit 064029e

Please sign in to comment.