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

feat: Pretty print fleet guard rail messages #605

Merged
merged 5 commits into from
Nov 15, 2023
Merged

feat: Pretty print fleet guard rail messages #605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c0e116a
Pretty print fleet guard rail messages
Arvindthiru Nov 10, 2023
54028e1
fix E2E
Arvindthiru Nov 10, 2023
73da391
add UT
Arvindthiru Nov 10, 2023
6309617
address comments
Arvindthiru Nov 11, 2023
c774674
fix E2E flake
Arvindthiru Nov 11, 2023
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
26 changes: 20 additions & 6 deletions pkg/utils/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@ import (
)

const (
kubePrefix = "kube-"
fleetPrefix = "fleet-"
FleetSystemNamespace = fleetPrefix + "system"
NamespaceNameFormat = fleetPrefix + "member-%s"
RoleNameFormat = fleetPrefix + "role-%s"
RoleBindingNameFormat = fleetPrefix + "rolebinding-%s"
kubePrefix = "kube-"
fleetPrefix = "fleet-"
FleetSystemNamespace = fleetPrefix + "system"
NamespaceNameFormat = fleetPrefix + "member-%s"
RoleNameFormat = fleetPrefix + "role-%s"
RoleBindingNameFormat = fleetPrefix + "rolebinding-%s"
lessGroupsStringFormat = "groups: %v"
moreGroupsStringFormat = "groups: [%s, %s, %s,......]"
)

const (
Expand Down Expand Up @@ -278,3 +280,15 @@ func ShouldPropagateNamespace(namespace string, skippedNamespaces map[string]boo
}
return true
}

// GenerateGroupString generates a string which prints groups in which a user belongs,
// it compresses the string to just display three groups if length of groups is more than 10.
func GenerateGroupString(groups []string) string {
var groupString string
if len(groups) > 10 {
groupString = fmt.Sprintf(moreGroupsStringFormat, groups[0], groups[1], groups[2])
} else {
groupString = fmt.Sprintf(lessGroupsStringFormat, groups)
}
return groupString
}
66 changes: 31 additions & 35 deletions pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook_test.go
View file
Open in desktop

Large diffs are not rendered by default.

27 changes: 14 additions & 13 deletions pkg/webhook/validation/uservalidation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
clusterv1beta1 "go.goms.io/fleet/apis/cluster/v1beta1"
placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1"
"go.goms.io/fleet/pkg/utils"
)

const (
Expand All @@ -31,12 +32,12 @@ const (
kubeControllerManagerUser = "system:kube-controller-manager"
serviceAccountFmt = "system:serviceaccount:fleet-system:%s"

resourceAllowedFormat = "user: %s in groups: %v is allowed to %s resource %+v/%s: %+v"
resourceDeniedFormat = "user: %s in groups: %v is not allowed to %s resource %+v/%s: %+v"
resourceAllowedGetMCFailed = "user: %s in groups: %v is allowed to %s resource %+v/%s: %+v because we failed to get MC"

allowedModifyResource = "user in groups is allowed to modify resource"
deniedModifyResource = "user in groups is not allowed to modify resource"

ResourceAllowedFormat = "user: '%s' in '%s' is allowed to %s resource %+v/%s: %+v"
ResourceDeniedFormat = "user: '%s' in '%s' is not allowed to %s resource %+v/%s: %+v"
ResourceAllowedGetMCFailed = "user: '%s' in '%s' is allowed to %s resource %+v/%s: %+v because we failed to get MC"
)

var (
Expand All @@ -58,10 +59,10 @@ func ValidateUserForFleetCRD(req admission.Request, whiteListedUsers []string, g
userInfo := req.UserInfo
if checkCRDGroup(group) && !isMasterGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) {
klog.V(2).InfoS(deniedModifyResource, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
return admission.Denied(fmt.Sprintf(resourceDeniedFormat, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName))
return admission.Denied(fmt.Sprintf(ResourceDeniedFormat, userInfo.Username, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName))
}
klog.V(3).InfoS(allowedModifyResource, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
return admission.Allowed(fmt.Sprintf(resourceAllowedFormat, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName))
return admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, userInfo.Username, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName))
}

// ValidateUserForResource checks to see if user is allowed to modify argued resource modified by request.
Expand All @@ -70,10 +71,10 @@ func ValidateUserForResource(req admission.Request, whiteListedUsers []string) a
userInfo := req.UserInfo
if isMasterGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) || isUserAuthenticatedServiceAccount(userInfo) || isUserKubeScheduler(userInfo) || isUserKubeControllerManager(userInfo) || isNodeGroupUser(userInfo) {
klog.V(3).InfoS(allowedModifyResource, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
return admission.Allowed(fmt.Sprintf(resourceAllowedFormat, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName))
return admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, userInfo.Username, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName))
}
klog.V(2).InfoS(deniedModifyResource, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
return admission.Denied(fmt.Sprintf(resourceDeniedFormat, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName))
return admission.Denied(fmt.Sprintf(ResourceDeniedFormat, userInfo.Username, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName))
}

// ValidateMemberClusterUpdate checks to see if user had updated the member cluster resource and allows/denies the request.
Expand All @@ -90,7 +91,7 @@ func ValidateMemberClusterUpdate(currentObj, oldObj client.Object, req admission
if (isLabelUpdated || isAnnotationUpdated) && !isObjUpdated {
// we allow any user to modify MemberCluster/Namespace labels/annotations.
klog.V(3).InfoS("user in groups is allowed to modify member cluster labels/annotations", "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
response = admission.Allowed(fmt.Sprintf(resourceAllowedFormat, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName))
response = admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, userInfo.Username, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName))
}
if isObjUpdated {
response = ValidateUserForResource(req, whiteListedUsers)
Expand Down Expand Up @@ -186,7 +187,7 @@ func ValidateMCIdentity(ctx context.Context, client client.Client, req admission
// fail open, if the webhook cannot get member cluster resources we don't block the request.
klog.ErrorS(err, fmt.Sprintf("failed to get v1alpha1 member cluster resource for request to modify %+v/%s, allowing request to be handled by api server", req.RequestKind, req.SubResource),
"user", userInfo.Username, "groups", userInfo.Groups, "namespacedName", namespacedName)
return admission.Allowed(fmt.Sprintf(resourceAllowedGetMCFailed, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName))
return admission.Allowed(fmt.Sprintf(ResourceAllowedGetMCFailed, userInfo.Username, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName))
}
identity = mc.Spec.Identity.Name
} else if *req.RequestKind == IMCGVK || *req.RequestKind == WorkGVK {
Expand All @@ -195,16 +196,16 @@ func ValidateMCIdentity(ctx context.Context, client client.Client, req admission
// fail open, if the webhook cannot get member cluster resources we don't block the request.
klog.ErrorS(err, fmt.Sprintf("failed to get member cluster resource for request to modify %+v/%s, allowing request to be handled by api server", req.RequestKind, req.SubResource),
"user", userInfo.Username, "groups", userInfo.Groups, "namespacedName", namespacedName)
return admission.Allowed(fmt.Sprintf(resourceAllowedGetMCFailed, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName))
return admission.Allowed(fmt.Sprintf(ResourceAllowedGetMCFailed, userInfo.Username, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName))
}
identity = mc.Spec.Identity.Name
}

// For the upstream E2E we use hub agent service account's token which allows member agent to modify Work status, hence we use serviceAccountFmt to make the check.
if identity == userInfo.Username || fmt.Sprintf(serviceAccountFmt, identity) == userInfo.Username {
klog.V(3).InfoS(allowedModifyResource, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
return admission.Allowed(fmt.Sprintf(resourceAllowedFormat, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName))
return admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, userInfo.Username, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName))
}
klog.V(2).InfoS(deniedModifyResource, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
return admission.Denied(fmt.Sprintf(resourceDeniedFormat, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName))
return admission.Denied(fmt.Sprintf(ResourceDeniedFormat, userInfo.Username, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName))
}
33 changes: 26 additions & 7 deletions pkg/webhook/validation/uservalidation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package validation

import (
"fmt"
"sort"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -21,6 +22,8 @@ var (
)

func TestValidateUserForResource(t *testing.T) {
manyGroups := []string{mastersGroup, "random0", "random1", "random2", "random3", "random4", "random5", "random6", "random7", "random8", "random9"}
sort.Strings(manyGroups)
testCases := map[string]struct {
req admission.Request
whiteListedUsers []string
Expand All @@ -39,7 +42,23 @@ func TestValidateUserForResource(t *testing.T) {
Operation: admissionv1.Create,
},
},
wantResponse: admission.Allowed(fmt.Sprintf(resourceAllowedFormat, "test-user", []string{mastersGroup}, admissionv1.Create, &roleGVK, "", types.NamespacedName{Name: "test-role", Namespace: "test-namespace"})),
wantResponse: admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, "test-user", utils.GenerateGroupString([]string{mastersGroup}), admissionv1.Create, &roleGVK, "", types.NamespacedName{Name: "test-role", Namespace: "test-namespace"})),
},
// UT to test GenerateGroupString in pkg/utils/common.gp
"allow user in system:masters group along with 10 other groups": {
req: admission.Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Name: "test-role",
Namespace: "test-namespace",
RequestKind: &roleGVK,
UserInfo: authenticationv1.UserInfo{
Username: "test-user",
Groups: manyGroups,
},
Operation: admissionv1.Create,
},
},
wantResponse: admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, "test-user", "'groups: [random0, random1, random2,......]'", admissionv1.Create, &roleGVK, "", types.NamespacedName{Name: "test-role", Namespace: "test-namespace"})),
},
"allow white listed user not in system:masters group": {
req: admission.Request{
Expand All @@ -55,7 +74,7 @@ func TestValidateUserForResource(t *testing.T) {
},
},
whiteListedUsers: []string{"test-user"},
wantResponse: admission.Allowed(fmt.Sprintf(resourceAllowedFormat, "test-user", []string{"test-group"}, admissionv1.Update, &roleBindingGVK, "", types.NamespacedName{Name: "test-role-binding", Namespace: "test-namespace"})),
wantResponse: admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, "test-user", utils.GenerateGroupString([]string{"test-group"}), admissionv1.Update, &roleBindingGVK, "", types.NamespacedName{Name: "test-role-binding", Namespace: "test-namespace"})),
},
"allow valid service account": {
req: admission.Request{
Expand All @@ -70,7 +89,7 @@ func TestValidateUserForResource(t *testing.T) {
Operation: admissionv1.Delete,
},
},
wantResponse: admission.Allowed(fmt.Sprintf(resourceAllowedFormat, "test-user", []string{serviceAccountsGroup}, admissionv1.Delete, &roleBindingGVK, "", types.NamespacedName{Name: "test-role-binding", Namespace: "test-namespace"})),
wantResponse: admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, "test-user", utils.GenerateGroupString([]string{serviceAccountsGroup}), admissionv1.Delete, &roleBindingGVK, "", types.NamespacedName{Name: "test-role-binding", Namespace: "test-namespace"})),
},
"allow user in system:node group": {
req: admission.Request{
Expand All @@ -85,7 +104,7 @@ func TestValidateUserForResource(t *testing.T) {
Operation: admissionv1.Create,
},
},
wantResponse: admission.Allowed(fmt.Sprintf(resourceAllowedFormat, "test-user", []string{nodeGroup}, admissionv1.Create, &podGVK, "", types.NamespacedName{Name: "test-pod", Namespace: "test-namespace"})),
wantResponse: admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, "test-user", utils.GenerateGroupString([]string{nodeGroup}), admissionv1.Create, &podGVK, "", types.NamespacedName{Name: "test-pod", Namespace: "test-namespace"})),
},
"allow system:kube-scheduler user": {
req: admission.Request{
Expand All @@ -100,7 +119,7 @@ func TestValidateUserForResource(t *testing.T) {
Operation: admissionv1.Update,
},
},
wantResponse: admission.Allowed(fmt.Sprintf(resourceAllowedFormat, "system:kube-scheduler", []string{"system:authenticated"}, admissionv1.Update, &podGVK, "", types.NamespacedName{Name: "test-pod", Namespace: "test-namespace"})),
wantResponse: admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, "system:kube-scheduler", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Update, &podGVK, "", types.NamespacedName{Name: "test-pod", Namespace: "test-namespace"})),
},
"allow system:kube-controller-manager user": {
req: admission.Request{
Expand All @@ -115,7 +134,7 @@ func TestValidateUserForResource(t *testing.T) {
Operation: admissionv1.Delete,
},
},
wantResponse: admission.Allowed(fmt.Sprintf(resourceAllowedFormat, "system:kube-controller-manager", []string{"system:authenticated"}, admissionv1.Delete, &podGVK, "", types.NamespacedName{Name: "test-pod", Namespace: "test-namespace"})),
wantResponse: admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, "system:kube-controller-manager", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Delete, &podGVK, "", types.NamespacedName{Name: "test-pod", Namespace: "test-namespace"})),
},
"fail to validate user with invalid username, groups": {
req: admission.Request{
Expand All @@ -130,7 +149,7 @@ func TestValidateUserForResource(t *testing.T) {
Operation: admissionv1.Delete,
},
},
wantResponse: admission.Denied(fmt.Sprintf(resourceDeniedFormat, "test-user", []string{"test-group"}, admissionv1.Delete, &roleGVK, "", types.NamespacedName{Name: "test-role", Namespace: "test-namespace"})),
wantResponse: admission.Denied(fmt.Sprintf(ResourceDeniedFormat, "test-user", utils.GenerateGroupString([]string{"test-group"}), admissionv1.Delete, &roleGVK, "", types.NamespacedName{Name: "test-role", Namespace: "test-namespace"})),
},
}

Expand Down
Loading
Loading