Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions charts/hub-agent/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ spec:
args:
- --leader-elect=true
- --enable-webhook={{ .Values.enableWebhook }}
- --whitelisted-users=system:serviceaccount:fleet-system:hub-agent-sa
- --webhook-client-connection-type={{.Values.webhookClientConnectionType}}
- --v={{ .Values.logVerbosity }}
- -add_dir_header
Expand Down
8 changes: 5 additions & 3 deletions cmd/hubagent/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package main
import (
"flag"
"os"
"strings"

"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
Expand Down Expand Up @@ -109,7 +110,8 @@ func main() {
}

if opts.EnableWebhook {
if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType)); err != nil {
whiteListedUsers := strings.Split(opts.WhiteListedUsers, ",")
if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), whiteListedUsers); err != nil {
klog.ErrorS(err, "unable to set up webhook")
exitWithErrorFunc()
}
Expand All @@ -130,7 +132,7 @@ func main() {
}

// SetupWebhook generates the webhook cert and then set up the webhook configurator.
func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType) error {
func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, whiteListedUsers []string) error {
// Generate self-signed key and crt files in FleetWebhookCertDir for the webhook server to start.
w, err := webhook.NewWebhookConfig(mgr, FleetWebhookPort, &webhookClientConnectionType, FleetWebhookCertDir)
if err != nil {
Expand All @@ -141,7 +143,7 @@ func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.Webho
klog.ErrorS(err, "unable to add WebhookConfig")
return err
}
if err = webhook.AddToManager(mgr); err != nil {
if err = webhook.AddToManager(mgr, whiteListedUsers); err != nil {
klog.ErrorS(err, "unable to register webhooks to the manager")
return err
}
Expand Down
3 changes: 3 additions & 0 deletions cmd/hubagent/options/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ type Options struct {
MetricsBindAddress string
// EnableWebhook indicates if we will run a webhook
EnableWebhook bool
// WhiteListedUsers indicates the list of user who are allowed to modify fleet resources
WhiteListedUsers string
// Sets the connection type for the webhook.
WebhookClientConnectionType string
// NetworkingAgentsEnabled indicates if we enable network agents
Expand Down Expand Up @@ -91,6 +93,7 @@ func (o *Options) AddFlags(flags *flag.FlagSet) {
flags.DurationVar(&o.LeaderElection.LeaseDuration.Duration, "leader-lease-duration", 15*time.Second, "This is effectively the maximum duration that a leader can be stopped before someone else will replace it.")
flag.StringVar(&o.LeaderElection.ResourceNamespace, "leader-election-namespace", utils.FleetSystemNamespace, "The namespace in which the leader election resource will be created.")
flag.BoolVar(&o.EnableWebhook, "enable-webhook", true, "If set, the fleet webhook is enabled.")
flag.StringVar(&o.WhiteListedUsers, "whitelisted-users", "", "If set, white listed users can modify fleet related resources.")
flag.StringVar(&o.WebhookClientConnectionType, "webhook-client-connection-type", "url", "Sets the connection type used by the webhook client. Only URL or Service is valid.")
flag.BoolVar(&o.NetworkingAgentsEnabled, "networking-agents-enabled", false, "Whether the networking agents are enabled or not.")
flags.DurationVar(&o.ClusterUnhealthyThreshold.Duration, "cluster-unhealthy-threshold", 60*time.Second, "The duration for a member cluster to be in a degraded state before considered unhealthy.")
Expand Down
15 changes: 0 additions & 15 deletions pkg/webhook/add_clusterresourceplacement.go

This file was deleted.

8 changes: 0 additions & 8 deletions pkg/webhook/add_fleetresourcehandler.go

This file was deleted.

16 changes: 16 additions & 0 deletions pkg/webhook/add_handler.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package webhook

import (
"go.goms.io/fleet/pkg/webhook/clusterresourceplacement"
"go.goms.io/fleet/pkg/webhook/fleetresourcehandler"
"go.goms.io/fleet/pkg/webhook/pod"
"go.goms.io/fleet/pkg/webhook/replicaset"
)

func init() {
// AddToManagerFuncs is a list of functions to create webhook and add them to a manager.
AddToManagerFuncs = append(AddToManagerFuncs, fleetresourcehandler.Add)
AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacement.Add)
AddToManagerFuncs = append(AddToManagerFuncs, pod.Add)
AddToManagerFuncs = append(AddToManagerFuncs, replicaset.Add)
}
14 changes: 0 additions & 14 deletions pkg/webhook/add_pod.go

This file was deleted.

14 changes: 0 additions & 14 deletions pkg/webhook/add_replicaset.go

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ const (
ValidationPath = "/validate-fleet-azure-com-v1alpha1-clusterresourceplacement"
)

func Add(mgr manager.Manager) error {
func Add(mgr manager.Manager, _ []string) error {
return (&fleetv1alpha1.ClusterResourcePlacement{}).SetupWebhookWithManager(mgr)
}
77 changes: 56 additions & 21 deletions pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,41 +9,48 @@ import (
admissionv1 "k8s.io/api/admission/v1"
v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/klog/v2"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/webhook"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1"
"go.goms.io/fleet/pkg/webhook/validation"
)

const (
// ValidationPath is the webhook service path which admission requests are routed to for validating custom resource definition resources.
ValidationPath = "/validate-v1-fleetresourcehandler"
groupMatch = `^[^.]*\.(.*)`
crdKind = "CustomResourceDefinition"
ValidationPath = "/validate-v1-fleetresourcehandler"
groupMatch = `^[^.]*\.(.*)`
crdKind = "CustomResourceDefinition"
memberClusterKind = "MemberCluster"
)

// Add registers the webhook for K8s bulit-in object types.
func Add(mgr manager.Manager) error {
func Add(mgr manager.Manager, whiteListedUsers []string) error {
hookServer := mgr.GetWebhookServer()
hookServer.Register(ValidationPath, &webhook.Admission{Handler: &fleetResourceValidator{Client: mgr.GetClient()}})
hookServer.Register(ValidationPath, &webhook.Admission{Handler: &fleetResourceValidator{client: mgr.GetClient(), whiteListedUsers: whiteListedUsers}})
return nil
}

type fleetResourceValidator struct {
Client client.Client
decoder *admission.Decoder
client client.Client
whiteListedUsers []string
decoder *admission.Decoder
}

func (v *fleetResourceValidator) Handle(_ context.Context, req admission.Request) admission.Response {
func (v *fleetResourceValidator) Handle(ctx context.Context, req admission.Request) admission.Response {
var response admission.Response
if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete {
switch req.Kind {
case createCRDGVK():
klog.V(2).InfoS("handling CRD resource", "GVK", createCRDGVK())
response = v.handleCRD(req)
case createMemberClusterGVK():
klog.V(2).InfoS("handling Member cluster resource", "GVK", createMemberClusterGVK())
response = v.handleMemberCluster(ctx, req)
default:
klog.V(2).InfoS("resource is not monitored by fleet resource validator webhook", "GVK", req.Kind.String())
response = admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify resource with GVK: %s", req.UserInfo.Username, req.UserInfo.Groups, req.Kind.String()))
Expand All @@ -54,25 +61,50 @@ func (v *fleetResourceValidator) Handle(_ context.Context, req admission.Request

func (v *fleetResourceValidator) handleCRD(req admission.Request) admission.Response {
var crd v1.CustomResourceDefinition
if err := v.decodeRequestObject(req, &crd); err != nil {
return admission.Errored(http.StatusBadRequest, err)
}

// This regex works because every CRD name in kubernetes follows this pattern <plural>.<group>.
group := regexp.MustCompile(groupMatch).FindStringSubmatch(crd.Name)[1]
if validation.CheckCRDGroup(group) && !validation.ValidateUserForCRD(v.whiteListedUsers, req.UserInfo) {
return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify fleet CRD: %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name))
}
return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify CRD: %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name))
}

func (v *fleetResourceValidator) handleMemberCluster(ctx context.Context, req admission.Request) admission.Response {
var mc fleetv1alpha1.MemberCluster
if err := v.decodeRequestObject(req, &mc); err != nil {
return admission.Errored(http.StatusBadRequest, err)
}

if !validation.ValidateUserForFleetCR(ctx, v.client, v.whiteListedUsers, req.UserInfo) {
return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify member cluster CR: %s", req.UserInfo.Username, req.UserInfo.Groups, mc.Name))
}
klog.V(2).InfoS("user in groups is allowed to modify member cluster CR", "user", req.UserInfo.Username, "groups", req.UserInfo.Groups)
return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify member cluster: %s", req.UserInfo.Username, req.UserInfo.Groups, mc.Name))
}

func (v *fleetResourceValidator) decodeRequestObject(req admission.Request, obj runtime.Object) error {
if req.Operation == admissionv1.Delete {
// req.Object is not populated for delete: https://github.com/kubernetes-sigs/controller-runtime/issues/1762.
if err := v.decoder.DecodeRaw(req.OldObject, &crd); err != nil {
if err := v.decoder.DecodeRaw(req.OldObject, obj); err != nil {
klog.ErrorS(err, "failed to decode old request object for delete operation", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups)
return admission.Errored(http.StatusBadRequest, err)
return err
}
} else {
if err := v.decoder.Decode(req, &crd); err != nil {
if err := v.decoder.Decode(req, obj); err != nil {
klog.ErrorS(err, "failed to decode request object for create/update operation", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups)
return admission.Errored(http.StatusBadRequest, err)
return err
}
}
return nil
}

// This regex works because every CRD name in kubernetes follows this pattern <plural>.<group>.
group := regexp.MustCompile(groupMatch).FindStringSubmatch(crd.Name)[1]
if validation.CheckCRDGroup(group) && !validation.ValidateUserForCRD(req.UserInfo) {
return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify fleet CRD: %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name))
}
return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify CRD: %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name))
func (v *fleetResourceValidator) InjectDecoder(d *admission.Decoder) error {
v.decoder = d
return nil
}

func createCRDGVK() metav1.GroupVersionKind {
Expand All @@ -83,7 +115,10 @@ func createCRDGVK() metav1.GroupVersionKind {
}
}

func (v *fleetResourceValidator) InjectDecoder(d *admission.Decoder) error {
v.decoder = d
return nil
func createMemberClusterGVK() metav1.GroupVersionKind {
return metav1.GroupVersionKind{
Group: fleetv1alpha1.GroupVersion.Group,
Version: fleetv1alpha1.GroupVersion.Version,
Kind: memberClusterKind,
}
}
2 changes: 1 addition & 1 deletion pkg/webhook/pod/pod_validating_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const (
)

// Add registers the webhook for K8s bulit-in object types.
func Add(mgr manager.Manager) error {
func Add(mgr manager.Manager, _ []string) error {
hookServer := mgr.GetWebhookServer()
hookServer.Register(ValidationPath, &webhook.Admission{Handler: &podValidator{Client: mgr.GetClient()}})
return nil
Expand Down
2 changes: 1 addition & 1 deletion pkg/webhook/replicaset/replicaset_validating_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type replicaSetValidator struct {
}

// Add registers the webhook for K8s bulit-in object types.
func Add(mgr manager.Manager) error {
func Add(mgr manager.Manager, _ []string) error {
hookServer := mgr.GetWebhookServer()
hookServer.Register(ValidationPath, &webhook.Admission{Handler: &replicaSetValidator{Client: mgr.GetClient()}})
return nil
Expand Down
34 changes: 30 additions & 4 deletions pkg/webhook/validation/uservalidation.go
Original file line number Diff line number Diff line change
@@ -1,17 +1,43 @@
package validation

import (
"context"

authenticationv1 "k8s.io/api/authentication/v1"
"k8s.io/klog/v2"
"k8s.io/utils/strings/slices"
"sigs.k8s.io/controller-runtime/pkg/client"

fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1"
)

const (
mastersGroup = "system:masters"
)

// TODO:(Arvindthiru) Get valid usernames as flag and allow those usernames.

// ValidateUserForCRD checks to see if user is authenticated to make a request to modify fleet CRDs.
func ValidateUserForCRD(userInfo authenticationv1.UserInfo) bool {
return slices.Contains(userInfo.Groups, mastersGroup)
func ValidateUserForCRD(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool {
return isMasterGroupUserOrWhiteListedUser(whiteListedUsers, userInfo)
}

// ValidateUserForFleetCR checks to see if user is authenticated to make a request to modify Fleet CRs.
func ValidateUserForFleetCR(ctx context.Context, client client.Client, whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this used anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

is the whiteList only useful fro FleetCR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using whiteList for CRD check as well

if isMasterGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) {
return true
}
var memberClusterList fleetv1alpha1.MemberClusterList
if err := client.List(ctx, &memberClusterList); err != nil {
klog.V(2).ErrorS(err, "failed to list member clusters")
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

is this list from the cache?

Copy link
Contributor Author

@Arvindthiru Arvindthiru Jun 21, 2023

Choose a reason for hiding this comment

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

This client is from the manager that's constructed in hub agent's main.go where we don't pass a NewClient func, since we are not passing it looks like we get new delegating client which uses the cache on reads https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/manager/manager.go#L141

Seems like we are using a different version of controller runtime but this seems to be true for both versions
image

I can see why it would be concern if we read stale data from the cache and reject certain requests from the member agents, but even if the webhook rejects the request once the member agent will retry in the next reconcile and in the meantime I'm assuming the cache will be populated with new data. If that's not the case we may need to use a new client here which doesn't use the cache

Copy link
Contributor

Choose a reason for hiding this comment

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

we probably should upgrade our controller-runtime now.
I looked at the code, I think we are reading it from cache.

}
identities := make([]string, len(memberClusterList.Items))
for i := range memberClusterList.Items {
identities = append(identities, memberClusterList.Items[i].Spec.Identity.Name)
}
// this ensures will allow all member agents are validated.
return slices.Contains(identities, userInfo.Username)
}

func isMasterGroupUserOrWhiteListedUser(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool {
return slices.Contains(whiteListedUsers, userInfo.Username) || slices.Contains(userInfo.Groups, mastersGroup)
}
Loading