From d84263ecdca1150bc4044c4661c8e8a081eb2cdb Mon Sep 17 00:00:00 2001 From: rosstimothy <39066650+rosstimothy@users.noreply.github.com> Date: Mon, 25 Mar 2024 13:38:49 -0400 Subject: [PATCH] Improve performance of resource filtering (#39724) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * update benchmark tests * Avoid creating a logrus logger if RBAC logging is not enabled ```bash $ benchstat 03d741b33f153ff98d516b54a3e163bd443fdf63.txt new.txt goos: darwin goarch: arm64 pkg: github.com/gravitational/teleport/lib/auth │ 03d741b33f153ff98d516b54a3e163bd443fdf63.txt │ new.txt │ │ sec/op │ sec/op vs base │ ListUnifiedResources/labels-12 281.6m ± 10% 279.1m ± 6% ~ (p=0.631 n=10) ListUnifiedResources/predicate_path-12 503.1m ± 2% 509.2m ± 1% ~ (p=0.165 n=10) ListUnifiedResources/predicate_index-12 504.9m ± 4% 494.5m ± 4% ~ (p=0.190 n=10) ListUnifiedResources/search_lower-12 335.7m ± 1% 320.4m ± 7% -4.57% (p=0.002 n=10) ListUnifiedResources/search_upper-12 340.2m ± 13% 330.9m ± 4% ~ (p=0.105 n=10) geomean 382.3m 375.3m -1.83% │ 03d741b33f153ff98d516b54a3e163bd443fdf63.txt │ new.txt │ │ B/op │ B/op vs base │ ListUnifiedResources/labels-12 158.75Mi ± 0% 85.50Mi ± 0% -46.14% (p=0.000 n=10) ListUnifiedResources/predicate_path-12 396.9Mi ± 0% 323.7Mi ± 0% -18.45% (p=0.000 n=10) ListUnifiedResources/predicate_index-12 396.9Mi ± 0% 323.6Mi ± 0% -18.46% (p=0.000 n=10) ListUnifiedResources/search_lower-12 158.62Mi ± 0% 85.33Mi ± 0% -46.21% (p=0.000 n=10) ListUnifiedResources/search_upper-12 159.58Mi ± 0% 86.31Mi ± 0% -45.91% (p=0.000 n=10) geomean 229.2Mi 145.8Mi -36.38% │ 03d741b33f153ff98d516b54a3e163bd443fdf63.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ ListUnifiedResources/labels-12 2.412M ± 0% 1.662M ± 0% -31.10% (p=0.000 n=10) ListUnifiedResources/predicate_path-12 7.363M ± 0% 6.613M ± 0% -10.19% (p=0.000 n=10) ListUnifiedResources/predicate_index-12 7.212M ± 0% 6.462M ± 0% -10.40% (p=0.000 n=10) ListUnifiedResources/search_lower-12 3.162M ± 0% 2.412M ± 0% -23.73% (p=0.000 n=10) ListUnifiedResources/search_upper-12 3.312M ± 0% 2.562M ± 0% -22.65% (p=0.000 n=10) geomean 4.222M 3.376M -20.03% ``` * Simplify and reduce memory usage of MatchResourceByFilters ```bash $ benchstat 215637fd4bca413a43d3ae3db25e1aa382c4b032.txt new.txt goos: darwin goarch: arm64 pkg: github.com/gravitational/teleport/lib/auth │ 215637fd4bca413a43d3ae3db25e1aa382c4b032.txt │ new.txt │ │ sec/op │ sec/op vs base │ ListUnifiedResources/labels-12 279.1m ± 6% 210.3m ± 2% -24.66% (p=0.000 n=10) ListUnifiedResources/predicate_path-12 509.2m ± 1% 448.5m ± 12% -11.91% (p=0.009 n=10) ListUnifiedResources/predicate_index-12 494.5m ± 4% 478.1m ± 2% ~ (p=0.089 n=10) ListUnifiedResources/search_lower-12 320.4m ± 7% 271.6m ± 4% -15.24% (p=0.000 n=10) ListUnifiedResources/search_upper-12 330.9m ± 4% 272.8m ± 4% -17.57% (p=0.000 n=10) geomean 375.3m 319.7m -14.83% │ 215637fd4bca413a43d3ae3db25e1aa382c4b032.txt │ new.txt │ │ B/op │ B/op vs base │ ListUnifiedResources/labels-12 85.50Mi ± 0% 74.03Mi ± 0% -13.42% (p=0.000 n=10) ListUnifiedResources/predicate_path-12 323.7Mi ± 0% 312.2Mi ± 0% -3.56% (p=0.000 n=10) ListUnifiedResources/predicate_index-12 323.6Mi ± 0% 312.2Mi ± 0% -3.53% (p=0.000 n=10) ListUnifiedResources/search_lower-12 85.33Mi ± 0% 73.88Mi ± 0% -13.41% (p=0.000 n=10) ListUnifiedResources/search_upper-12 86.31Mi ± 0% 74.85Mi ± 0% -13.27% (p=0.000 n=10) geomean 145.8Mi 131.9Mi -9.57% │ 215637fd4bca413a43d3ae3db25e1aa382c4b032.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ ListUnifiedResources/labels-12 1.662M ± 0% 1.212M ± 0% -27.09% (p=0.000 n=10) ListUnifiedResources/predicate_path-12 6.613M ± 0% 6.162M ± 0% -6.81% (p=0.000 n=10) ListUnifiedResources/predicate_index-12 6.462M ± 0% 6.012M ± 0% -6.96% (p=0.000 n=10) ListUnifiedResources/search_lower-12 2.412M ± 0% 1.962M ± 0% -18.66% (p=0.000 n=10) ListUnifiedResources/search_upper-12 2.562M ± 0% 2.112M ± 0% -17.57% (p=0.000 n=10) geomean 3.376M 2.844M -15.77% ``` * Optimize memory usage of CombineLabels ```bash $ benchstat 6ef17028eb6f3d83cee141c4ecd9d30bfbf54460.txt new.txt goos: darwin goarch: arm64 pkg: github.com/gravitational/teleport/lib/auth │ 6ef17028eb6f3d83cee141c4ecd9d30bfbf54460.txt │ new.txt │ │ sec/op │ sec/op vs base │ ListUnifiedResourcesFilter/labels-12 210.3m ± 2% 186.1m ± 3% -11.48% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_path-12 448.5m ± 12% 418.5m ± 3% -6.68% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_index-12 478.1m ± 2% 422.8m ± 4% -11.56% (p=0.000 n=10) ListUnifiedResourcesFilter/search_lower-12 271.6m ± 4% 295.1m ± 7% +8.65% (p=0.001 n=10) ListUnifiedResourcesFilter/search_upper-12 272.8m ± 4% 270.4m ± 4% ~ (p=0.481 n=10) geomean 319.7m 304.7m -4.68% │ 6ef17028eb6f3d83cee141c4ecd9d30bfbf54460.txt │ new.txt │ │ B/op │ B/op vs base │ ListUnifiedResourcesFilter/labels-12 74.03Mi ± 0% 25.95Mi ± 0% -64.95% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_path-12 312.2Mi ± 0% 264.1Mi ± 0% -15.40% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_index-12 312.2Mi ± 0% 264.1Mi ± 0% -15.40% (p=0.000 n=10) ListUnifiedResourcesFilter/search_lower-12 73.88Mi ± 0% 73.94Mi ± 0% +0.07% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 74.85Mi ± 0% 74.84Mi ± 0% -0.02% (p=0.015 n=10) geomean 131.9Mi 100.0Mi -24.15% │ 6ef17028eb6f3d83cee141c4ecd9d30bfbf54460.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ ListUnifiedResourcesFilter/labels-12 1211.8k ± 0% 911.7k ± 0% -24.76% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_path-12 6.162M ± 0% 5.862M ± 0% -4.87% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_index-12 6.012M ± 0% 5.712M ± 0% -4.99% (p=0.000 n=10) ListUnifiedResourcesFilter/search_lower-12 1.962M ± 0% 1.962M ± 0% +0.00% (p=0.010 n=10) ListUnifiedResourcesFilter/search_upper-12 2.112M ± 0% 2.112M ± 0% ~ (p=0.423 n=10) geomean 2.844M 2.633M -7.42% ``` * Avoid combining labels in parser ```bash $ benchstat 8ddd25edecca2cb8cb16dc76e921b984defdefff.txt new.txt goos: darwin goarch: arm64 pkg: github.com/gravitational/teleport/lib/auth │ 8ddd25edecca2cb8cb16dc76e921b984defdefff.txt │ new.txt │ │ sec/op │ sec/op vs base │ ListUnifiedResourcesFilter/labels-12 186.1m ± 3% 161.1m ± 4% -13.45% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_path-12 418.5m ± 3% 383.6m ± 1% -8.34% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_index-12 422.8m ± 4% 396.6m ± 1% -6.21% (p=0.000 n=10) ListUnifiedResourcesFilter/search_lower-12 295.1m ± 7% 258.6m ± 2% -12.35% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 270.4m ± 4% 266.1m ± 8% ~ (p=0.796 n=10) geomean 304.7m 278.9m -8.48% │ 8ddd25edecca2cb8cb16dc76e921b984defdefff.txt │ new.txt │ │ B/op │ B/op vs base │ ListUnifiedResourcesFilter/labels-12 25.95Mi ± 0% 25.93Mi ± 0% -0.06% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_path-12 264.1Mi ± 0% 264.1Mi ± 0% ~ (p=0.393 n=10) ListUnifiedResourcesFilter/predicate_index-12 264.1Mi ± 0% 264.1Mi ± 0% ~ (p=0.971 n=10) ListUnifiedResourcesFilter/search_lower-12 73.94Mi ± 0% 73.94Mi ± 0% ~ (p=0.684 n=10) ListUnifiedResourcesFilter/search_upper-12 74.84Mi ± 0% 74.84Mi ± 0% ~ (p=0.971 n=10) geomean 100.0Mi 100.0Mi -0.01% │ 8ddd25edecca2cb8cb16dc76e921b984defdefff.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ ListUnifiedResourcesFilter/labels-12 911.7k ± 0% 911.7k ± 0% -0.01% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_path-12 5.862M ± 0% 5.862M ± 0% ~ (p=0.959 n=10) ListUnifiedResourcesFilter/predicate_index-12 5.712M ± 0% 5.712M ± 0% -0.00% (p=0.037 n=10) ListUnifiedResourcesFilter/search_lower-12 1.962M ± 0% 1.962M ± 0% ~ (p=0.085 n=10) ListUnifiedResourcesFilter/search_upper-12 2.112M ± 0% 2.112M ± 0% ~ (p=0.402 n=10) geomean 2.633M 2.633M -0.00% ``` * Reduce memory usage of MatchSearch ```bash $ benchstat f8aa6f9a7a26d34796a0ecb9d3eadf245c3b478d.txt new.txt goos: darwin goarch: arm64 pkg: github.com/gravitational/teleport/lib/auth │ f8aa6f9a7a26d34796a0ecb9d3eadf245c3b478d.txt │ new.txt │ │ sec/op │ sec/op vs base │ ListUnifiedResourcesFilter/labels-12 161.1m ± 4% 168.7m ± 5% +4.73% (p=0.043 n=10) ListUnifiedResourcesFilter/predicate_path-12 383.6m ± 1% 389.4m ± 1% +1.50% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_index-12 396.6m ± 1% 397.7m ± 3% ~ (p=0.190 n=10) ListUnifiedResourcesFilter/search_lower-12 258.6m ± 2% 266.6m ± 3% +3.07% (p=0.007 n=10) ListUnifiedResourcesFilter/search_upper-12 266.1m ± 8% 259.2m ± 2% ~ (p=0.052 n=10) geomean 278.9m 282.7m +1.36% │ f8aa6f9a7a26d34796a0ecb9d3eadf245c3b478d.txt │ new.txt │ │ B/op │ B/op vs base │ ListUnifiedResourcesFilter/labels-12 25.93Mi ± 0% 25.94Mi ± 0% ~ (p=0.089 n=10) ListUnifiedResourcesFilter/predicate_path-12 264.1Mi ± 0% 264.1Mi ± 0% ~ (p=0.853 n=10) ListUnifiedResourcesFilter/predicate_index-12 264.1Mi ± 0% 264.1Mi ± 0% ~ (p=0.927 n=10) ListUnifiedResourcesFilter/search_lower-12 73.94Mi ± 0% 55.63Mi ± 0% -24.77% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 74.84Mi ± 0% 56.53Mi ± 0% -24.46% (p=0.000 n=10) geomean 100.0Mi 89.33Mi -10.68% │ f8aa6f9a7a26d34796a0ecb9d3eadf245c3b478d.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ ListUnifiedResourcesFilter/labels-12 911.7k ± 0% 911.7k ± 0% ~ (p=0.469 n=10) ListUnifiedResourcesFilter/predicate_path-12 5.862M ± 0% 5.862M ± 0% -0.00% (p=0.019 n=10) ListUnifiedResourcesFilter/predicate_index-12 5.712M ± 0% 5.712M ± 0% ~ (p=0.305 n=10) ListUnifiedResourcesFilter/search_lower-12 1.962M ± 0% 1.662M ± 0% -15.29% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 2.112M ± 0% 1.812M ± 0% -14.20% (p=0.000 n=10) geomean 2.633M 2.470M -6.18% ``` * Switch to using containsFold instead of ToLower and Contains ```bash $ benchstat b4f5af277a8da79470673a9d4da95e1ae747deea.txt new.txt goos: darwin goarch: arm64 pkg: github.com/gravitational/teleport/lib/auth │ b4f5af277a8da79470673a9d4da95e1ae747deea.txt │ new.txt │ │ sec/op │ sec/op vs base │ ListUnifiedResourcesFilter/labels-12 168.7m ± 5% 160.5m ± 2% -4.84% (p=0.002 n=10) ListUnifiedResourcesFilter/predicate_path-12 389.4m ± 1% 387.2m ± 2% ~ (p=0.393 n=10) ListUnifiedResourcesFilter/predicate_index-12 397.7m ± 3% 396.4m ± 1% ~ (p=0.247 n=10) ListUnifiedResourcesFilter/search_lower-12 266.6m ± 3% 214.8m ± 1% -19.43% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 259.2m ± 2% 231.0m ± 1% -10.89% (p=0.000 n=10) geomean 282.7m 261.5m -7.50% │ b4f5af277a8da79470673a9d4da95e1ae747deea.txt │ new.txt │ │ B/op │ B/op vs base │ ListUnifiedResourcesFilter/labels-12 25.94Mi ± 0% 25.93Mi ± 0% -0.04% (p=0.004 n=10) ListUnifiedResourcesFilter/predicate_path-12 264.1Mi ± 0% 264.1Mi ± 0% ~ (p=0.481 n=10) ListUnifiedResourcesFilter/predicate_index-12 264.1Mi ± 0% 264.1Mi ± 0% ~ (p=0.684 n=10) ListUnifiedResourcesFilter/search_lower-12 55.63Mi ± 0% 53.42Mi ± 0% -3.97% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 56.53Mi ± 0% 53.42Mi ± 0% -5.50% (p=0.000 n=10) geomean 89.33Mi 87.61Mi -1.93% │ b4f5af277a8da79470673a9d4da95e1ae747deea.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ ListUnifiedResourcesFilter/labels-12 911.7k ± 0% 911.6k ± 0% -0.00% (p=0.011 n=10) ListUnifiedResourcesFilter/predicate_path-12 5.862M ± 0% 5.862M ± 0% ~ (p=0.812 n=10) ListUnifiedResourcesFilter/predicate_index-12 5.712M ± 0% 5.712M ± 0% ~ (p=0.812 n=10) ListUnifiedResourcesFilter/search_lower-12 1.662M ± 0% 1.212M ± 0% -27.09% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 1.812M ± 0% 1.212M ± 0% -33.12% (p=0.000 n=10) geomean 2.470M 2.139M -13.38% ``` * Perform RBAC only on matches ```bash $ benchstat 874a1dd0b8090dd0ae6f22754f0f989a57f08293.txt new.txt goos: darwin goarch: arm64 pkg: github.com/gravitational/teleport/lib/auth │ 874a1dd0b8090dd0ae6f22754f0f989a57f08293.txt │ new.txt │ │ sec/op │ sec/op vs base │ ListUnifiedResourcesFilter/labels-12 160.52m ± 2% 79.33m ± 3% -50.58% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_path-12 387.2m ± 2% 317.1m ± 8% -18.10% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_index-12 396.4m ± 1% 337.6m ± 2% -14.84% (p=0.000 n=10) ListUnifiedResourcesFilter/search_lower-12 214.8m ± 1% 154.4m ± 3% -28.11% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 231.0m ± 1% 171.9m ± 3% -25.57% (p=0.000 n=10) geomean 261.5m 186.5m -28.69% │ 874a1dd0b8090dd0ae6f22754f0f989a57f08293.txt │ new.txt │ │ B/op │ B/op vs base │ ListUnifiedResourcesFilter/labels-12 26551.2Ki ± 0% 732.7Ki ± 1% -97.24% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_path-12 264.1Mi ± 0% 238.9Mi ± 0% -9.54% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_index-12 264.1Mi ± 0% 238.9Mi ± 0% -9.54% (p=0.000 n=10) ListUnifiedResourcesFilter/search_lower-12 53.42Mi ± 0% 28.22Mi ± 0% -47.17% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 53.42Mi ± 0% 28.23Mi ± 0% -47.16% (p=0.000 n=10) geomean 87.61Mi 31.80Mi -63.70% │ 874a1dd0b8090dd0ae6f22754f0f989a57f08293.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ ListUnifiedResourcesFilter/labels-12 911.65k ± 0% 11.45k ± 0% -98.74% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_path-12 5.862M ± 0% 4.962M ± 0% -15.36% (p=0.000 n=10) ListUnifiedResourcesFilter/predicate_index-12 5.712M ± 0% 4.812M ± 0% -15.76% (p=0.000 n=10) ListUnifiedResourcesFilter/search_lower-12 1211.8k ± 0% 311.7k ± 0% -74.28% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 1211.8k ± 0% 311.7k ± 0% -74.28% (p=0.000 n=10) geomean 2.139M 484.0k -77.38% ``` * Reduce allocations in (ServerV2) MatchSearch ```bash $benchstat 4782fe42f72a5ed7628bdb4a94f16d5cb06b35bb.txt new.txt goos: darwin goarch: arm64 pkg: github.com/gravitational/teleport/lib/auth │ 4782fe42f72a5ed7628bdb4a94f16d5cb06b35bb.txt │ new.txt │ │ sec/op │ sec/op vs base │ ListUnifiedResourcesFilter/labels-12 79.33m ± 3% 77.79m ± 4% -1.94% (p=0.019 n=10) ListUnifiedResourcesFilter/predicate_path-12 317.1m ± 8% 307.0m ± 2% -3.18% (p=0.023 n=10) ListUnifiedResourcesFilter/predicate_index-12 337.6m ± 2% 310.2m ± 1% -8.12% (p=0.000 n=10) ListUnifiedResourcesFilter/search_lower-12 154.4m ± 3% 139.5m ± 2% -9.65% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 171.9m ± 3% 154.9m ± 4% -9.94% (p=0.000 n=10) geomean 186.5m 174.1m -6.63% │ 4782fe42f72a5ed7628bdb4a94f16d5cb06b35bb.txt │ new.txt │ │ B/op │ B/op vs base │ ListUnifiedResourcesFilter/labels-12 732.7Ki ± 1% 731.0Ki ± 0% ~ (p=0.481 n=10) ListUnifiedResourcesFilter/predicate_path-12 238.9Mi ± 0% 238.9Mi ± 0% ~ (p=0.123 n=10) ListUnifiedResourcesFilter/predicate_index-12 238.9Mi ± 0% 238.9Mi ± 0% -0.01% (p=0.001 n=10) ListUnifiedResourcesFilter/search_lower-12 28.22Mi ± 0% 16.76Mi ± 0% -40.60% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 28.23Mi ± 0% 16.77Mi ± 0% -40.58% (p=0.000 n=10) geomean 31.80Mi 25.81Mi -18.84% │ 4782fe42f72a5ed7628bdb4a94f16d5cb06b35bb.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ ListUnifiedResourcesFilter/labels-12 11.45k ± 0% 11.44k ± 0% ~ (p=0.077 n=10) ListUnifiedResourcesFilter/predicate_path-12 4.962M ± 0% 4.962M ± 0% ~ (p=0.060 n=10) ListUnifiedResourcesFilter/predicate_index-12 4.812M ± 0% 4.812M ± 0% -0.00% (p=0.015 n=10) ListUnifiedResourcesFilter/search_lower-12 311.7k ± 0% 161.6k ± 0% -48.15% (p=0.000 n=10) ListUnifiedResourcesFilter/search_upper-12 311.7k ± 0% 161.6k ± 0% -48.14% (p=0.000 n=10) geomean 484.0k 372.1k -23.11% ``` * Remove allocations from MatchLabels * fix: TestServerLabels * Add comment to containsFold Co-authored-by: Forrest <30576607+fspmarshall@users.noreply.github.com> * fix: log and skip match failures * fix: only create closure if required * fix: func (a *AppServerOrSAMLIdPServiceProviderV1) GetLabel * fix: validate provided predicate expressions --------- Co-authored-by: Forrest <30576607+fspmarshall@users.noreply.github.com> --- api/types/appserver_or_saml_idp_sp.go | 4 +- api/types/resource.go | 36 ++++--- api/types/server.go | 31 ++++-- lib/auth/auth_with_roles.go | 46 +++++++-- lib/auth/auth_with_roles_test.go | 137 +++++++++++++++++++++++++- lib/services/matchers.go | 30 +++--- lib/services/parser.go | 11 +-- lib/services/role.go | 11 ++- lib/services/suite/presence_test.go | 1 - 9 files changed, 243 insertions(+), 64 deletions(-) diff --git a/api/types/appserver_or_saml_idp_sp.go b/api/types/appserver_or_saml_idp_sp.go index 14465f9869179..9ca138630065c 100644 --- a/api/types/appserver_or_saml_idp_sp.go +++ b/api/types/appserver_or_saml_idp_sp.go @@ -193,7 +193,9 @@ func (a *AppServerOrSAMLIdPServiceProviderV1) GetLabel(key string) (value string v, ok := appServer.Spec.App.Metadata.Labels[key] return v, ok } else { - return "", true + sp := a.GetSAMLIdPServiceProvider() + v, ok := sp.Metadata.Labels[key] + return v, ok } } diff --git a/api/types/resource.go b/api/types/resource.go index 57293f4729046..22f11ac2b2b70 100644 --- a/api/types/resource.go +++ b/api/types/resource.go @@ -495,13 +495,8 @@ func (m *Metadata) CheckAndSetDefaults() error { // MatchLabels takes a map of labels and returns `true` if the resource has ALL // of them. func MatchLabels(resource ResourceWithLabels, labels map[string]string) bool { - if len(labels) == 0 { - return true - } - - resourceLabels := resource.GetAllLabels() - for name, value := range labels { - if resourceLabels[name] != value { + for key, value := range labels { + if v, ok := resource.GetLabel(key); !ok || v != value { return false } } @@ -535,15 +530,11 @@ func IsValidLabelKey(s string) bool { // Returns true if all search vals were matched (or if nil search vals). // Returns false if no or partial match (or nil field values). func MatchSearch(fieldVals []string, searchVals []string, customMatch func(val string) bool) bool { - // Case fold all values to avoid repeated case folding while matching. - caseFoldedSearchVals := utils.ToLowerStrings(searchVals) - caseFoldedFieldVals := utils.ToLowerStrings(fieldVals) - Outer: - for _, searchV := range caseFoldedSearchVals { + for _, searchV := range searchVals { // Iterate through field values to look for a match. - for _, fieldV := range caseFoldedFieldVals { - if strings.Contains(fieldV, searchV) { + for _, fieldV := range fieldVals { + if containsFold(fieldV, searchV) { continue Outer } } @@ -559,6 +550,23 @@ Outer: return true } +// containsFold is a case-insensitive alternative to strings.Contains, used to help avoid excess allocations during searches. +func containsFold(s, substr string) bool { + if len(s) < len(substr) { + return false + } + + n := len(s) - len(substr) + + for i := 0; i <= n; i++ { + if strings.EqualFold(s[i:i+len(substr)], substr) { + return true + } + } + + return false +} + func stringCompare(a string, b string, isDesc bool) bool { if isDesc { return a > b diff --git a/api/types/server.go b/api/types/server.go index 62e9d2368e531..b550b027c3582 100644 --- a/api/types/server.go +++ b/api/types/server.go @@ -350,7 +350,11 @@ func (s *ServerV2) GetAllLabels() map[string]string { // CombineLabels combines the passed in static and dynamic labels. func CombineLabels(static map[string]string, dynamic map[string]CommandLabelV2) map[string]string { - lmap := make(map[string]string) + if len(dynamic) == 0 { + return static + } + + lmap := make(map[string]string, len(static)+len(dynamic)) for key, value := range static { lmap[key] = value } @@ -492,20 +496,27 @@ func (s *ServerV2) CheckAndSetDefaults() error { // MatchSearch goes through select field values and tries to // match against the list of search values. func (s *ServerV2) MatchSearch(values []string) bool { - var fieldVals []string + if s.GetKind() != KindNode { + return false + } + var custom func(val string) bool + if s.GetUseTunnel() { + custom = func(val string) bool { + return strings.EqualFold(val, "tunnel") + } + } - if s.GetKind() == KindNode { - fieldVals = append(utils.MapToStrings(s.GetAllLabels()), s.GetName(), s.GetHostname(), s.GetAddr()) - fieldVals = append(fieldVals, s.GetPublicAddrs()...) + fieldVals := make([]string, 0, (len(s.Metadata.Labels)*2)+(len(s.Spec.CmdLabels)*2)+len(s.Spec.PublicAddrs)+3) - if s.GetUseTunnel() { - custom = func(val string) bool { - return strings.EqualFold(val, "tunnel") - } - } + labels := CombineLabels(s.Metadata.Labels, s.Spec.CmdLabels) + for key, value := range labels { + fieldVals = append(fieldVals, key, value) } + fieldVals = append(fieldVals, s.Metadata.Name, s.Spec.Hostname, s.Spec.Addr) + fieldVals = append(fieldVals, s.Spec.PublicAddrs...) + return MatchSearch(fieldVals, values, custom) } diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 726d68d76d97f..edd98add2a032 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -1329,18 +1329,34 @@ func (a *ServerWithRoles) checkUnifiedAccess(resource types.ResourceWithLabels, return false, trace.Wrap(canAccessErr) } - if resourceKind != types.KindSAMLIdPServiceProvider { - if err := checker.CanAccess(resource); err != nil { + // Filter first and only check RBAC if there is a match to improve perf. + match, err := services.MatchResourceByFilters(resource, filter, nil) + if err != nil { + log.WithFields(logrus.Fields{ + "resource_name": resource.GetName(), + "resource_kind": resourceKind, + "error": err, + }). + Warn("Unable to determine access to resource, matching with filter failed") + return false, nil + } - if trace.IsAccessDenied(err) { - return false, nil - } - return false, trace.Wrap(err) + if !match { + return false, nil + } + + if resourceKind == types.KindSAMLIdPServiceProvider { + return true, nil + } + + if err := checker.CanAccess(resource); err != nil { + if trace.IsAccessDenied(err) { + return false, nil } + return false, trace.Wrap(err) } - match, err := services.MatchResourceByFilters(resource, filter, nil) - return match, trace.Wrap(err) + return true, nil } // ListUnifiedResources returns a paginated list of unified resources filtered by user access. @@ -1358,6 +1374,20 @@ func (a *ServerWithRoles) ListUnifiedResources(ctx context.Context, req *proto.L Kinds: req.Kinds, } + // If a predicate expression was provided, evaluate it with an empty + // server to determine if the expression is valid before attempting + // to do any listing. + if filter.PredicateExpression != "" { + parser, err := services.NewResourceParser(&types.ServerV2{}) + if err != nil { + return nil, trace.Wrap(err) + } + + if _, err := parser.EvalBoolPredicate(filter.PredicateExpression); err != nil { + return nil, trace.BadParameter("failed to parse predicate expression: %s", err.Error()) + } + } + // Populate resourceAccessMap with any access errors the user has for each possible // resource kind. This allows the access check to occur a single time per resource // kind instead of once per matching resource. diff --git a/lib/auth/auth_with_roles_test.go b/lib/auth/auth_with_roles_test.go index 92c6f3dfb7bea..5b9431e2e7c59 100644 --- a/lib/auth/auth_with_roles_test.go +++ b/lib/auth/auth_with_roles_test.go @@ -4721,6 +4721,130 @@ func TestListUnifiedResources_WithPredicate(t *testing.T) { require.Error(t, err) } +func BenchmarkListUnifiedResourcesFilter(b *testing.B) { + const nodeCount = 150_000 + const roleCount = 32 + + logger := logrus.StandardLogger() + logger.ReplaceHooks(make(logrus.LevelHooks)) + logrus.SetFormatter(logutils.NewTestJSONFormatter()) + logger.SetLevel(logrus.PanicLevel) + logger.SetOutput(io.Discard) + + ctx := context.Background() + srv := newTestTLSServer(b) + + var ids []string + for i := 0; i < roleCount; i++ { + ids = append(ids, uuid.New().String()) + } + + ids[0] = "hidden" + + var hiddenNodes int + // Create test nodes. + for i := 0; i < nodeCount; i++ { + name := uuid.New().String() + id := ids[i%len(ids)] + if id == "hidden" { + hiddenNodes++ + } + + labels := map[string]string{ + "kEy": id, + "grouP": "useRs", + } + + if i == 10 { + labels["ip"] = "10.20.30.40" + labels["ADDRESS"] = "10.20.30.41" + labels["food"] = "POTATO" + } + + node, err := types.NewServerWithLabels( + name, + types.KindNode, + types.ServerSpecV2{}, + labels, + ) + require.NoError(b, err) + + _, err = srv.Auth().UpsertNode(ctx, node) + require.NoError(b, err) + } + + b.Run("labels", func(b *testing.B) { + benchmarkListUnifiedResources( + b, ctx, + 1, + srv, + ids, + func(role types.Role, id string) { + role.SetNodeLabels(types.Allow, types.Labels{types.Wildcard: []string{types.Wildcard}}) + }, + func(req *proto.ListUnifiedResourcesRequest) { + req.Labels = map[string]string{"ip": "10.20.30.40"} + }, + ) + }) + b.Run("predicate path", func(b *testing.B) { + benchmarkListUnifiedResources( + b, ctx, + 1, + srv, + ids, + func(role types.Role, id string) { + role.SetNodeLabels(types.Allow, types.Labels{types.Wildcard: []string{types.Wildcard}}) + }, + func(req *proto.ListUnifiedResourcesRequest) { + req.PredicateExpression = `labels.ip == "10.20.30.40"` + }, + ) + }) + b.Run("predicate index", func(b *testing.B) { + benchmarkListUnifiedResources( + b, ctx, + 1, + srv, + ids, + func(role types.Role, id string) { + role.SetNodeLabels(types.Allow, types.Labels{types.Wildcard: []string{types.Wildcard}}) + }, + func(req *proto.ListUnifiedResourcesRequest) { + req.PredicateExpression = `labels["ip"] == "10.20.30.40"` + }, + ) + }) + b.Run("search lower", func(b *testing.B) { + benchmarkListUnifiedResources( + b, ctx, + 1, + srv, + ids, + func(role types.Role, id string) { + role.SetNodeLabels(types.Allow, types.Labels{types.Wildcard: []string{types.Wildcard}}) + }, + func(req *proto.ListUnifiedResourcesRequest) { + req.SearchKeywords = []string{"10.20.30.40"} + }, + ) + }) + b.Run("search upper", func(b *testing.B) { + benchmarkListUnifiedResources( + b, ctx, + 1, + srv, + ids, + func(role types.Role, id string) { + role.SetNodeLabels(types.Allow, types.Labels{types.Wildcard: []string{types.Wildcard}}) + }, + func(req *proto.ListUnifiedResourcesRequest) { + req.SearchKeywords = []string{"POTATO"} + }, + ) + }) +} + // go test ./lib/auth -bench=BenchmarkListUnifiedResources -run=^$ -v -benchtime 1x // goos: darwin // goarch: arm64 @@ -4731,7 +4855,7 @@ func TestListUnifiedResources_WithPredicate(t *testing.T) { // PASS // ok github.com/gravitational/teleport/lib/auth 2.878s func BenchmarkListUnifiedResources(b *testing.B) { - const nodeCount = 50_000 + const nodeCount = 150_000 const roleCount = 32 logger := logrus.StandardLogger() @@ -4791,10 +4915,11 @@ func BenchmarkListUnifiedResources(b *testing.B) { b.Run(tc.desc, func(b *testing.B) { benchmarkListUnifiedResources( b, ctx, - nodeCount, hiddenNodes, + nodeCount-hiddenNodes, srv, ids, tc.editRole, + func(req *proto.ListUnifiedResourcesRequest) {}, ) }) } @@ -4802,10 +4927,11 @@ func BenchmarkListUnifiedResources(b *testing.B) { func benchmarkListUnifiedResources( b *testing.B, ctx context.Context, - nodeCount, hiddenNodes int, + expectedCount int, srv *TestTLSServer, ids []string, editRole func(r types.Role, id string), + editReq func(req *proto.ListUnifiedResourcesRequest), ) { var roles []types.Role for _, id := range ids { @@ -4839,6 +4965,9 @@ func benchmarkListUnifiedResources( SortBy: types.SortBy{IsDesc: false, Field: types.ResourceMetadataName}, Limit: 1_000, } + + editReq(req) + for { rsp, err := clt.ListUnifiedResources(ctx, req) require.NoError(b, err) @@ -4849,7 +4978,7 @@ func benchmarkListUnifiedResources( break } } - require.Len(b, resources, nodeCount-hiddenNodes) + require.Len(b, resources, expectedCount) } } diff --git a/lib/services/matchers.go b/lib/services/matchers.go index 31698cf51968a..cb1a78fcec1cd 100644 --- a/lib/services/matchers.go +++ b/lib/services/matchers.go @@ -19,7 +19,6 @@ package services import ( - "fmt" "slices" "github.com/gravitational/trace" @@ -129,7 +128,7 @@ func MatchResourceLabels(matchers []ResourceMatcher, labels map[string]string) b // ResourceSeenKey is used as a key for a map that keeps track // of unique resource names and address. Currently "addr" // only applies to resource Application. -type ResourceSeenKey struct{ name, addr string } +type ResourceSeenKey struct{ name, kind, addr string } // MatchResourceByFilters returns true if all filter values given matched against the resource. // @@ -144,20 +143,21 @@ type ResourceSeenKey struct{ name, addr string } // is not provided but is provided for kind `KubernetesCluster`. func MatchResourceByFilters(resource types.ResourceWithLabels, filter MatchResourceFilter, seenMap map[ResourceSeenKey]struct{}) (bool, error) { var specResource types.ResourceWithLabels - resourceKind := resource.GetKind() + kind := resource.GetKind() // We assume when filtering for services like KubeService, AppServer, and DatabaseServer // the user is wanting to filter the contained resource ie. KubeClusters, Application, and Database. - resourceKey := ResourceSeenKey{} - switch resourceKind { + key := ResourceSeenKey{ + kind: kind, + name: resource.GetName(), + } + switch kind { case types.KindNode, types.KindDatabaseService, types.KindKubernetesCluster, types.KindWindowsDesktop, types.KindWindowsDesktopService, types.KindUserGroup: specResource = resource - resourceKey.name = fmt.Sprintf("%s/%s", specResource.GetName(), resourceKind) - case types.KindKubeServer: if seenMap != nil { return false, trace.BadParameter("checking for duplicate matches for resource kind %q is not supported", filter.ResourceKind) @@ -170,18 +170,17 @@ func MatchResourceByFilters(resource types.ResourceWithLabels, filter MatchResou return false, trace.BadParameter("expected types.DatabaseServer, got %T", resource) } specResource = server.GetDatabase() - resourceKey.name = fmt.Sprintf("%s/%s/", specResource.GetName(), resourceKind) - + key.name = specResource.GetName() case types.KindAppServer, types.KindSAMLIdPServiceProvider, types.KindAppOrSAMLIdPServiceProvider: switch appOrSP := resource.(type) { case types.AppServer: app := appOrSP.GetApp() specResource = app - resourceKey.name = fmt.Sprintf("%s/%s/", specResource.GetName(), resourceKind) - resourceKey.addr = app.GetPublicAddr() + key.addr = app.GetPublicAddr() + key.name = app.GetName() case types.SAMLIdPServiceProvider: specResource = appOrSP - resourceKey.name = fmt.Sprintf("%s/%s/", specResource.GetName(), resourceKind) + key.name = specResource.GetName() default: return false, trace.BadParameter("expected types.SAMLIdPServiceProvider or types.AppServer, got %T", resource) } @@ -190,10 +189,9 @@ func MatchResourceByFilters(resource types.ResourceWithLabels, filter MatchResou // of cases we need to handle. If the resource type didn't match any arm before // and it is not a Kubernetes resource kind, we return an error. if !slices.Contains(types.KubernetesResourcesKinds, filter.ResourceKind) { - return false, trace.NotImplemented("filtering for resource kind %q not supported", resourceKind) + return false, trace.NotImplemented("filtering for resource kind %q not supported", kind) } specResource = resource - resourceKey.name = fmt.Sprintf("%s/%s/", specResource.GetName(), resourceKind) } var match bool @@ -212,10 +210,10 @@ func MatchResourceByFilters(resource types.ResourceWithLabels, filter MatchResou // Deduplicate matches. if match && seenMap != nil { - if _, exists := seenMap[resourceKey]; exists { + if _, exists := seenMap[key]; exists { return false, nil } - seenMap[resourceKey] = struct{}{} + seenMap[key] = struct{}{} } return match, nil diff --git a/lib/services/parser.go b/lib/services/parser.go index 797719478ae77..141c469749a87 100644 --- a/lib/services/parser.go +++ b/lib/services/parser.go @@ -800,18 +800,17 @@ func NewResourceParser(resource types.ResourceWithLabels) (BoolPredicateParser, GetIdentifier: func(fields []string) (interface{}, error) { switch fields[0] { case ResourceLabelsIdentifier: - combinedLabels := resource.GetAllLabels() switch { // Field length of 1 means the user is using // an index expression ie: labels["env"], which the // parser will expect a map for lookup in `GetProperty`. case len(fields) == 1: - return labels(combinedLabels), nil + return resource, nil case len(fields) > 2: return nil, trace.BadParameter("only two fields are supported with identifier %q, got %d: %v", ResourceLabelsIdentifier, len(fields), fields) default: key := fields[1] - val, ok := combinedLabels[key] + val, ok := resource.GetLabel(key) if ok { return label{key: key, value: val}, nil } @@ -838,7 +837,7 @@ func NewResourceParser(resource types.ResourceWithLabels) (BoolPredicateParser, } }, GetProperty: func(mapVal, keyVal interface{}) (interface{}, error) { - m, ok := mapVal.(labels) + r, ok := mapVal.(types.ResourceWithLabels) if !ok { return GetStringMapValue(mapVal, keyVal) } @@ -848,7 +847,7 @@ func NewResourceParser(resource types.ResourceWithLabels) (BoolPredicateParser, return nil, trace.BadParameter("only string keys are supported") } - val, ok := m[key] + val, ok := r.GetLabel(key) if ok { return label{key: key, value: val}, nil } @@ -865,5 +864,3 @@ func NewResourceParser(resource types.ResourceWithLabels) (BoolPredicateParser, type label struct { key, value string } - -type labels map[string]string diff --git a/lib/services/role.go b/lib/services/role.go index c4a29263d1664..e176c3d0e0882 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -2422,9 +2422,14 @@ type AccessCheckable interface { // It also returns a flag indicating whether debug logging is enabled, // allowing the RBAC system to generate more verbose errors in debug mode. func rbacDebugLogger() (debugEnabled bool, debugf func(format string, args ...interface{})) { - isDebugEnabled := log.IsLevelEnabled(log.TraceLevel) - log := log.WithField(trace.Component, teleport.ComponentRBAC) - return isDebugEnabled, log.Tracef + debugEnabled = log.IsLevelEnabled(log.TraceLevel) + debugf = func(format string, args ...interface{}) {} + + if debugEnabled { + debugf = log.WithField(trace.Component, teleport.ComponentRBAC).Tracef + } + + return } func (set RoleSet) checkAccess(r AccessCheckable, traits wrappers.Traits, state AccessState, matchers ...RoleMatcher) error { diff --git a/lib/services/suite/presence_test.go b/lib/services/suite/presence_test.go index 66c6fc69b807c..7cc0558635858 100644 --- a/lib/services/suite/presence_test.go +++ b/lib/services/suite/presence_test.go @@ -32,7 +32,6 @@ func TestServerLabels(t *testing.T) { emptyLabels := make(map[string]string) // empty server := &types.ServerV2{} - require.Empty(t, cmp.Diff(server.GetAllLabels(), emptyLabels)) require.Empty(t, server.GetAllLabels()) require.True(t, types.MatchLabels(server, emptyLabels)) require.False(t, types.MatchLabels(server, map[string]string{"a": "b"}))