Skip to content

Commit 5f5c615

Browse files
committed
fix: prevent panic on nil APIResource in permission validator
Add a nil check for the APIResource parameter in the permission validator callback to prevent panic when API server connectivity is interrupted during sync operations. Extracted the function for testability. Added Unit Tests. Fixes #26608 Signed-off-by: Andy Lo-A-Foe <andy.loafoe@gmail.com>
1 parent ff83056 commit 5f5c615

2 files changed

Lines changed: 147 additions & 16 deletions

File tree

controller/sync.go

Lines changed: 33 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -308,22 +308,9 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, project *v1alp
308308
sync.WithLogr(logutils.NewLogrusLogger(logEntry)),
309309
sync.WithHealthOverride(lua.ResourceHealthOverrides(resourceOverrides)),
310310
sync.WithPermissionValidator(func(un *unstructured.Unstructured, res *metav1.APIResource) error {
311-
if !project.IsGroupKindNamePermitted(un.GroupVersionKind().GroupKind(), un.GetName(), res.Namespaced) {
312-
return fmt.Errorf("resource %s:%s is not permitted in project %s", un.GroupVersionKind().Group, un.GroupVersionKind().Kind, project.Name)
313-
}
314-
if res.Namespaced {
315-
permitted, err := project.IsDestinationPermitted(destCluster, un.GetNamespace(), func(project string) ([]*v1alpha1.Cluster, error) {
316-
return m.db.GetProjectClusters(context.TODO(), project)
317-
})
318-
if err != nil {
319-
return err
320-
}
321-
322-
if !permitted {
323-
return fmt.Errorf("namespace %v is not permitted in project '%s'", un.GetNamespace(), project.Name)
324-
}
325-
}
326-
return nil
311+
return validateSyncPermissions(project, destCluster, func(proj string) ([]*v1alpha1.Cluster, error) {
312+
return m.db.GetProjectClusters(context.TODO(), proj)
313+
}, un, res)
327314
}),
328315
sync.WithOperationSettings(syncOp.DryRun, syncOp.Prune, syncOp.SyncStrategy.Force(), syncOp.IsApplyStrategy() || len(syncOp.Resources) > 0),
329316
sync.WithInitialState(state.Phase, state.Message, initialResourcesRes, state.StartedAt),
@@ -604,3 +591,33 @@ func deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application
604591
// if there is no match found in the AppProject.Spec.DestinationServiceAccounts, use the default service account of the destination namespace.
605592
return "", fmt.Errorf("no matching service account found for destination server %s and namespace %s", application.Spec.Destination.Server, serviceAccountNamespace)
606593
}
594+
595+
// validateSyncPermissions checks whether the given resource is permitted by the project's
596+
// allow/deny lists and destination rules. It returns an error if the API resource info is nil
597+
// (preventing a nil-pointer panic), if the resource's group/kind is not permitted, or if
598+
// the resource's namespace is not an allowed destination.
599+
func validateSyncPermissions(
600+
project *v1alpha1.AppProject,
601+
destCluster *v1alpha1.Cluster,
602+
getProjectClusters func(string) ([]*v1alpha1.Cluster, error),
603+
un *unstructured.Unstructured,
604+
res *metav1.APIResource,
605+
) error {
606+
if res == nil {
607+
return fmt.Errorf("failed to get API resource info for %s/%s: unable to verify permissions", un.GroupVersionKind().Group, un.GroupVersionKind().Kind)
608+
}
609+
if !project.IsGroupKindNamePermitted(un.GroupVersionKind().GroupKind(), un.GetName(), res.Namespaced) {
610+
return fmt.Errorf("resource %s:%s is not permitted in project %s", un.GroupVersionKind().Group, un.GroupVersionKind().Kind, project.Name)
611+
}
612+
if res.Namespaced {
613+
permitted, err := project.IsDestinationPermitted(destCluster, un.GetNamespace(), getProjectClusters)
614+
if err != nil {
615+
return err
616+
}
617+
618+
if !permitted {
619+
return fmt.Errorf("namespace %v is not permitted in project '%s'", un.GetNamespace(), project.Name)
620+
}
621+
}
622+
return nil
623+
}

controller/sync_test.go

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1414
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
1515
"k8s.io/apimachinery/pkg/runtime"
16+
"k8s.io/apimachinery/pkg/runtime/schema"
1617

1718
"github.com/argoproj/argo-cd/v3/common"
1819
"github.com/argoproj/argo-cd/v3/controller/testdata"
@@ -1653,3 +1654,116 @@ func dig(obj any, path ...any) any {
16531654

16541655
return i
16551656
}
1657+
1658+
func TestValidateSyncPermissions(t *testing.T) {
1659+
t.Parallel()
1660+
1661+
newResource := func(group, kind, name, namespace string) *unstructured.Unstructured {
1662+
obj := &unstructured.Unstructured{}
1663+
obj.SetGroupVersionKind(schema.GroupVersionKind{Group: group, Version: "v1", Kind: kind})
1664+
obj.SetName(name)
1665+
obj.SetNamespace(namespace)
1666+
return obj
1667+
}
1668+
1669+
project := &v1alpha1.AppProject{
1670+
ObjectMeta: metav1.ObjectMeta{
1671+
Name: "test-project",
1672+
Namespace: "argocd",
1673+
},
1674+
Spec: v1alpha1.AppProjectSpec{
1675+
Destinations: []v1alpha1.ApplicationDestination{
1676+
{Namespace: "default", Server: "*"},
1677+
},
1678+
},
1679+
}
1680+
1681+
destCluster := &v1alpha1.Cluster{
1682+
Server: "https://kubernetes.default.svc",
1683+
}
1684+
1685+
noopGetClusters := func(_ string) ([]*v1alpha1.Cluster, error) {
1686+
return nil, nil
1687+
}
1688+
1689+
t.Run("nil APIResource returns error", func(t *testing.T) {
1690+
t.Parallel()
1691+
un := newResource("apps", "Deployment", "my-deploy", "default")
1692+
1693+
err := validateSyncPermissions(project, destCluster, noopGetClusters, un, nil)
1694+
1695+
require.Error(t, err)
1696+
assert.Contains(t, err.Error(), "failed to get API resource info for apps/Deployment")
1697+
assert.Contains(t, err.Error(), "unable to verify permissions")
1698+
})
1699+
1700+
t.Run("permitted namespaced resource returns no error", func(t *testing.T) {
1701+
t.Parallel()
1702+
un := newResource("", "ConfigMap", "my-cm", "default")
1703+
res := &metav1.APIResource{Name: "configmaps", Namespaced: true}
1704+
1705+
err := validateSyncPermissions(project, destCluster, noopGetClusters, un, res)
1706+
1707+
assert.NoError(t, err)
1708+
})
1709+
1710+
t.Run("group kind not permitted returns error", func(t *testing.T) {
1711+
t.Parallel()
1712+
projectWithDenyList := &v1alpha1.AppProject{
1713+
ObjectMeta: metav1.ObjectMeta{
1714+
Name: "restricted-project",
1715+
Namespace: "argocd",
1716+
},
1717+
Spec: v1alpha1.AppProjectSpec{
1718+
Destinations: []v1alpha1.ApplicationDestination{
1719+
{Namespace: "*", Server: "*"},
1720+
},
1721+
ClusterResourceBlacklist: []v1alpha1.ClusterResourceRestrictionItem{
1722+
{Group: "rbac.authorization.k8s.io", Kind: "ClusterRole"},
1723+
},
1724+
},
1725+
}
1726+
un := newResource("rbac.authorization.k8s.io", "ClusterRole", "my-role", "")
1727+
res := &metav1.APIResource{Name: "clusterroles", Namespaced: false}
1728+
1729+
err := validateSyncPermissions(projectWithDenyList, destCluster, noopGetClusters, un, res)
1730+
1731+
require.Error(t, err)
1732+
assert.Contains(t, err.Error(), "is not permitted in project")
1733+
})
1734+
1735+
t.Run("namespace not permitted returns error", func(t *testing.T) {
1736+
t.Parallel()
1737+
un := newResource("", "ConfigMap", "my-cm", "kube-system")
1738+
res := &metav1.APIResource{Name: "configmaps", Namespaced: true}
1739+
1740+
err := validateSyncPermissions(project, destCluster, noopGetClusters, un, res)
1741+
1742+
require.Error(t, err)
1743+
assert.Contains(t, err.Error(), "namespace kube-system is not permitted in project")
1744+
})
1745+
1746+
t.Run("cluster-scoped resource skips namespace check", func(t *testing.T) {
1747+
t.Parallel()
1748+
projectWithClusterResources := &v1alpha1.AppProject{
1749+
ObjectMeta: metav1.ObjectMeta{
1750+
Name: "test-project",
1751+
Namespace: "argocd",
1752+
},
1753+
Spec: v1alpha1.AppProjectSpec{
1754+
Destinations: []v1alpha1.ApplicationDestination{
1755+
{Namespace: "default", Server: "*"},
1756+
},
1757+
ClusterResourceWhitelist: []v1alpha1.ClusterResourceRestrictionItem{
1758+
{Group: "*", Kind: "*"},
1759+
},
1760+
},
1761+
}
1762+
un := newResource("", "Namespace", "my-ns", "")
1763+
res := &metav1.APIResource{Name: "namespaces", Namespaced: false}
1764+
1765+
err := validateSyncPermissions(projectWithClusterResources, destCluster, noopGetClusters, un, res)
1766+
1767+
assert.NoError(t, err)
1768+
})
1769+
}

0 commit comments

Comments
 (0)