Skip to content

fix: tighten the VAP rule for all managed resources #1212

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

Merged
nwnt merged 4 commits into from
Oct 23, 2025
Merged

fix: tighten the VAP rule for all managed resources #1212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
840e808
Tighten the VAP rule for all managed resources
tsujiri Oct 18, 2025
d31bf8e
Remove isHub parameter
tsujiri Oct 21, 2025
9066d2d
Adjust the allow list for managed resource vap
tsujiri Oct 21, 2025
a828d88
Remove system:serviceaccounts:openshift-kube-controller-manager
tsujiri Oct 22, 2025
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
2 changes: 2 additions & 0 deletions charts/member-agent-arc/templates/serviceaccount.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
apiVersion: v1
kind: ServiceAccount
metadata:
# the name is also used in the managed resource validating admission webhook.
# please make the change accordingly when you change the name.
name: fleet-member-agent-sa
namespace: fleet-system
labels:
Expand Down
2 changes: 1 addition & 1 deletion cmd/hubagent/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ func main() {
exitWithErrorFunc()
}

if err := managedresource.EnsureVAP(ctx, mgr.GetClient(), true); err != nil {
if err := managedresource.EnsureVAP(ctx, mgr.GetClient()); err != nil {
klog.Errorf("unable to create managed resource validating admission policy: %s", err)
exitWithErrorFunc()
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/controllers/internalmembercluster/v1beta1/member_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu

switch imc.Spec.State {
case clusterv1beta1.ClusterStateJoin:
if err := managedresource.EnsureVAP(ctx, r.memberClient, false); err != nil {
if err := managedresource.EnsureVAP(ctx, r.memberClient); err != nil {
return ctrl.Result{}, err
}
klog.V(2).InfoS("Successfully installed managed resource validating admission policy")
Expand Down Expand Up @@ -247,7 +247,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{RequeueAfter: time.Millisecond *
(time.Duration(hbinterval) + time.Duration(utilrand.Int63nRange(0, jitterRange)-jitterRange/2))}, nil
case clusterv1beta1.ClusterStateLeave:
if err := managedresource.EnsureNoVAP(ctx, r.memberClient, false); err != nil {
if err := managedresource.EnsureNoVAP(ctx, r.memberClient); err != nil {
return ctrl.Result{}, err
}
klog.V(2).InfoS("Successfully uninstalled managed resource validating admission policy")
Expand Down
14 changes: 7 additions & 7 deletions pkg/webhook/managedresource/createordelete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import (
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
)

func EnsureNoVAP(ctx context.Context, c client.Client, isHub bool) error {
objs := []client.Object{getValidatingAdmissionPolicy(isHub), getValidatingAdmissionPolicyBinding()}
func EnsureNoVAP(ctx context.Context, c client.Client) error {
objs := []client.Object{getValidatingAdmissionPolicy(), getValidatingAdmissionPolicyBinding()}
for _, obj := range objs {
err := c.Delete(ctx, obj)
switch {
Expand All @@ -34,13 +34,13 @@ func EnsureNoVAP(ctx context.Context, c client.Client, isHub bool) error {
return nil
}

func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error {
func EnsureVAP(ctx context.Context, c client.Client) error {
type vapObjectAndMutator struct {
obj client.Object
mutate func() error
}
// TODO: this can be simplified by dealing with the specific type rather than using client.Object
vap, mutateVAP := getVAPWithMutator(isHub)
vap, mutateVAP := getVAPWithMutator()
vapb, mutateVAPB := getVAPBindingWithMutator()
objsAndMutators := []vapObjectAndMutator{
{
Expand Down Expand Up @@ -69,10 +69,10 @@ func EnsureVAP(ctx context.Context, c client.Client, isHub bool) error {
return nil
}

func getVAPWithMutator(isHub bool) (*v1.ValidatingAdmissionPolicy, func() error) {
vap := getValidatingAdmissionPolicy(isHub)
func getVAPWithMutator() (*v1.ValidatingAdmissionPolicy, func() error) {
vap := getValidatingAdmissionPolicy()
return vap, func() error {
mutateValidatingAdmissionPolicy(vap, isHub)
mutateValidatingAdmissionPolicy(vap)
return nil
}
}
Expand Down
100 changes: 31 additions & 69 deletions pkg/webhook/managedresource/createordelete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,44 +32,30 @@ func TestEnsureVAP(t *testing.T) {
t.Fatalf("Failed to add admissionregistration scheme: %v", err)
}

vapHub := getValidatingAdmissionPolicy(true)
vapMember := getValidatingAdmissionPolicy(false)
vap := getValidatingAdmissionPolicy()
binding := getValidatingAdmissionPolicyBinding()

tests := []struct {
name string
isHub bool
existingObjs []client.Object
createOrUpdateErrors map[client.ObjectKey]error
wantErr bool
wantErrMessage string
wantObjects []client.Object
}{
{
name: "hub cluster - create new objects",
isHub: true,
name: "create new objects",
existingObjs: []client.Object{},
wantErr: false,
wantObjects: []client.Object{
vapHub.DeepCopy(),
vap.DeepCopy(),
binding.DeepCopy(),
},
},
{
name: "member cluster - create new objects",
isHub: false,
existingObjs: []client.Object{},
wantErr: false,
wantObjects: []client.Object{
vapMember.DeepCopy(),
binding.DeepCopy(),
},
},
{
name: "hub cluster - update existing objects",
isHub: true,
name: "update existing objects",
existingObjs: func() []client.Object {
existingVAP := vapHub.DeepCopy()
existingVAP := vap.DeepCopy()
existingBinding := binding.DeepCopy()

existingVAP.Spec.Validations = nil
Expand All @@ -79,32 +65,29 @@ func TestEnsureVAP(t *testing.T) {
}(),
wantErr: false,
wantObjects: []client.Object{
vapHub.DeepCopy(),
vap.DeepCopy(),
binding.DeepCopy(),
},
},
{
name: "hub cluster - skip no match error",
isHub: true,
name: "skip no match error (cluster version < v1.30)",
existingObjs: []client.Object{},
createOrUpdateErrors: map[client.ObjectKey]error{
client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}},
client.ObjectKeyFromObject(vap): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}},
},
wantErr: false,
},
{
name: "hub cluster - create error propagated",
isHub: true,
name: "vap create error",
existingObjs: []client.Object{},
createOrUpdateErrors: map[client.ObjectKey]error{
client.ObjectKeyFromObject(vapHub): apierrors.NewInternalError(errors.New("internal server error")),
client.ObjectKeyFromObject(vap): apierrors.NewInternalError(errors.New("internal server error")),
},
wantErr: true,
wantErrMessage: "internal server error",
},
{
name: "member cluster - binding creation error",
isHub: false,
name: "binding create error",
existingObjs: []client.Object{},
createOrUpdateErrors: map[client.ObjectKey]error{
client.ObjectKeyFromObject(binding): apierrors.NewForbidden(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name, errors.New("forbidden")),
Expand Down Expand Up @@ -173,7 +156,7 @@ func TestEnsureVAP(t *testing.T) {
WithInterceptorFuncs(interceptorFuncs).
Build()

err := EnsureVAP(context.Background(), fakeClient, tt.isHub)
err := EnsureVAP(context.Background(), fakeClient)

if tt.wantErr {
if err == nil {
Expand Down Expand Up @@ -219,74 +202,57 @@ func TestEnsureNoVAP(t *testing.T) {
t.Fatalf("Failed to add admissionregistration scheme: %v", err)
}

vapHub := getValidatingAdmissionPolicy(true)
vapMember := getValidatingAdmissionPolicy(false)
vap := getValidatingAdmissionPolicy()
binding := getValidatingAdmissionPolicyBinding()

tests := []struct {
name string
isHub bool
existingObjs []client.Object
deleteErrors map[client.ObjectKey]error
wantErr bool
wantErrMessage string
}{
{
name: "hub cluster - no existing objects",
isHub: true,
name: "no existing objects",
existingObjs: []client.Object{},
wantErr: false,
},
{
name: "hub cluster - existing objects deleted successfully",
isHub: true,
name: "existing objects deleted successfully",
existingObjs: []client.Object{
vapHub.DeepCopy(),
vap.DeepCopy(),
binding.DeepCopy(),
},
wantErr: false,
},
{
name: "member cluster - existing objects deleted successfully",
isHub: false,
existingObjs: []client.Object{
vapMember.DeepCopy(),
binding.DeepCopy(),
},
wantErr: false,
},
{
name: "hub cluster - not found errors ignored",
isHub: true,
name: "not found errors ignored",
existingObjs: []client.Object{},
deleteErrors: map[client.ObjectKey]error{
client.ObjectKeyFromObject(vapHub): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicies"}, vapHub.Name),
client.ObjectKeyFromObject(vap): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicies"}, vap.Name),
client.ObjectKeyFromObject(binding): apierrors.NewNotFound(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name),
},
wantErr: false,
},
{
name: "hub cluster - no match error handled gracefully",
isHub: true,
name: "no match error handled gracefully",
existingObjs: []client.Object{},
deleteErrors: map[client.ObjectKey]error{
client.ObjectKeyFromObject(vapHub): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}},
client.ObjectKeyFromObject(vap): &meta.NoKindMatchError{GroupKind: schema.GroupKind{Group: "admissionregistration.k8s.io", Kind: "ValidatingAdmissionPolicy"}},
},
wantErr: false,
},
{
name: "hub cluster - delete error",
isHub: true,
name: "delete error on vap",
existingObjs: []client.Object{},
deleteErrors: map[client.ObjectKey]error{
client.ObjectKeyFromObject(vapHub): apierrors.NewInternalError(errors.New("internal server error")),
client.ObjectKeyFromObject(vap): apierrors.NewInternalError(errors.New("internal server error")),
},
wantErr: true,
wantErrMessage: "internal server error",
},
{
name: "member cluster - delete error on binding propagated",
isHub: false,
name: "delete error on vap binding",
existingObjs: []client.Object{},
deleteErrors: map[client.ObjectKey]error{
client.ObjectKeyFromObject(binding): apierrors.NewForbidden(schema.GroupResource{Group: "admissionregistration.k8s.io", Resource: "validatingadmissionpolicybindings"}, binding.Name, errors.New("forbidden")),
Expand Down Expand Up @@ -316,7 +282,7 @@ func TestEnsureNoVAP(t *testing.T) {
WithInterceptorFuncs(interceptorFuncs).
Build()

err := EnsureNoVAP(context.Background(), fakeClient, tt.isHub)
err := EnsureNoVAP(context.Background(), fakeClient)

if tt.wantErr {
if err == nil {
Expand All @@ -334,7 +300,7 @@ func TestEnsureNoVAP(t *testing.T) {
}

// Verify objects are deleted (or don't exist)
expectedObjs := []client.Object{getValidatingAdmissionPolicy(tt.isHub), getValidatingAdmissionPolicyBinding()}
expectedObjs := []client.Object{getValidatingAdmissionPolicy(), getValidatingAdmissionPolicyBinding()}
for _, obj := range expectedObjs {
err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)
if !apierrors.IsNotFound(err) {
Expand All @@ -349,36 +315,32 @@ func TestGetVAPWithMutator(t *testing.T) {
t.Parallel()

tests := []struct {
name string
isHub bool
name string
}{
{
name: "hub cluster VAP",
isHub: true,
},
{
name: "member cluster VAP",
isHub: false,
name: "VAP with mutator",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

vap, mutateFunc := getVAPWithMutator(tt.isHub)
vap, mutateFunc := getVAPWithMutator()

// Verify initial state
if vap == nil {
t.Fatal("getVAPWithMutator() returned nil VAP")
return

@jim-minter jim-minter Oct 20, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Surprised this is needed - I thought t.Fatal() will not execute further.

@nwnt nwnt Oct 20, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I got a lint error/warning from not having this.

@nwnt nwnt Oct 22, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I couldn't remember what it was that gave this warning/error, but I have gone ahead and remove this from it. Let's see if I'm going to get a lint error from the checks.

}
if mutateFunc == nil {
t.Fatal("getVAPWithMutator() returned nil mutate function")
return
}

// Verify mutate function works
gotVAP := vap.DeepCopy()
wantVAP := getValidatingAdmissionPolicy(tt.isHub)
wantVAP := getValidatingAdmissionPolicy()
ignoreOpts := cmp.Options{
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "ManagedFields", "Generation"),
}
Expand Down
Loading
Loading