From be880add20a295325acd88953881caef82c85bf8 Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Thu, 3 Oct 2024 04:37:19 -1000 Subject: [PATCH] fix: refine deny destination checks (#20045) * fix: refine server deny check Fixes #19804. The deny destination check can be made more intuitive by doing the following: * short-circuit any deny destination * first, for any deny server destination, _also_ check if the namespace matches * for any deny namespace destination, reject as before Signed-off-by: Blake Pettersson * fix: also assert that server matches on ns deny Signed-off-by: Blake Pettersson --------- Signed-off-by: Blake Pettersson --- .../application/v1alpha1/app_project_types.go | 9 +++++---- pkg/apis/application/v1alpha1/types_test.go | 20 +++++++++++++++++-- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/pkg/apis/application/v1alpha1/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index b888cd37391b3..d4fc39723358a 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -461,7 +461,6 @@ func (proj AppProject) IsDestinationPermitted(dst ApplicationDestination, projec func (proj AppProject) isDestinationMatched(dst ApplicationDestination) bool { anyDestinationMatched := false - noDenyDestinationsMatched := true for _, item := range proj.Spec.Destinations { dstNameMatched := dst.Name != "" && globMatch(item.Name, dst.Name, true) @@ -471,12 +470,14 @@ func (proj AppProject) isDestinationMatched(dst ApplicationDestination) bool { matched := (dstServerMatched || dstNameMatched) && dstNamespaceMatched if matched { anyDestinationMatched = true - } else if ((!dstNameMatched && isDenyPattern(item.Name)) || (!dstServerMatched && isDenyPattern(item.Server))) || (!dstNamespaceMatched && isDenyPattern(item.Namespace)) { - noDenyDestinationsMatched = false + } else if (!dstNameMatched && isDenyPattern(item.Name)) || (!dstServerMatched && isDenyPattern(item.Server)) && dstNamespaceMatched { + return false + } else if !dstNamespaceMatched && isDenyPattern(item.Namespace) && dstServerMatched { + return false } } - return anyDestinationMatched && noDenyDestinationsMatched + return anyDestinationMatched } func isDenyPattern(pattern string) bool { diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index 08b83c238a93d..6b5d6014041dd 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -320,7 +320,7 @@ func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { Server: "*", Namespace: "!kube-system", }}, appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "default"}, - isPermitted: false, + isPermitted: true, }, { projDest: []ApplicationDestination{{ Server: "*", Namespace: "*", @@ -339,6 +339,22 @@ func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { }}, appDest: ApplicationDestination{Name: "test", Namespace: "test"}, isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "test", + }, { + Server: "!https://test-server", Namespace: "other", + }}, + appDest: ApplicationDestination{Server: "https://test-server", Namespace: "test"}, + isPermitted: true, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "*", + }, { + Server: "https://test-server", Namespace: "!other", + }}, + appDest: ApplicationDestination{Server: "https://other-test-server", Namespace: "other"}, + isPermitted: true, }} for _, data := range testData { @@ -350,7 +366,7 @@ func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { permitted, _ := proj.IsDestinationPermitted(data.appDest, func(project string) ([]*Cluster, error) { return []*Cluster{}, nil }) - assert.Equal(t, data.isPermitted, permitted) + assert.Equalf(t, data.isPermitted, permitted, "appDest mismatch for %+v with project destinations %+v", data.appDest, data.projDest) } }