server: enable _status/connectivity for secondary tenants (shared-process)

[shubhamdhama]

This works well for shared-process mode where inter-node network
connectivity for a secondary tenant is the same as for the system tenant.
However, in external-process mode, this endpoint won't provide a complete
picture of network connectivity since the SQL server may run entirely
outside the KV node. We may need to extend this endpoint or create a new
one for SQL-SQL servers and SQL server to KV nodes. This work is left for
the future, and currently, this endpoint only shows KV-KV nodes network
connectivity. As a result, this endpoint isn't ready for external-process
mode and should only be enabled for shared-mode tenants. On the backend,
there is nothing enforcing this, which shouldn't be a problem.

Fixes: #110024

Epic: #CRDB-38968

Release note: None

[cockroach-teamcity]

This change is 

[stevendanna]

This seems reasonable to me.

Would it be difficult to add a test that shows that if you don't have the permission then this endpoint returns an error?

[dhartunian]

Reviewed 18 of 18 files at r3, 2 of 17 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cthumuluru-crdb, @kyle-a-wong, @shubhamdhama, and @stevendanna)

---

-- commits line 5 at r4:
nit: The endpoint is _status/connectivity if you're going to quote the route.

---

-- commits line 16 at r4:
nit: This is helpful background but can you just add a note above it describing that this change is just enabling tenant access to the feature via the can_debug_process capability? It's not super clear what's being done.

---

pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go line 393 at r3 (raw file):

}

func (a *Authorizer) HasTSDBAllMetricsCapability(

Any reason why this check cannot also be refactored?

[shubhamdhama]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cthumuluru-crdb, @dhartunian, @kyle-a-wong, and @stevendanna)

---

-- commits line 16 at r4:

Previously, dhartunian (David Hartunian) wrote…

nit: This is helpful background but can you just add a note above it describing that this change is just enabling tenant access to the feature via the can_debug_process capability? It's not super clear what's being done.

Done

---

pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go line 393 at r3 (raw file):

Previously, dhartunian (David Hartunian) wrote…

Any reason why this check cannot also be refactored?

Not really, the authorizerModeV222 case had the errCaonnotQueryTSDB error, which is different from the err...AllMetrics at the end. But I think that was just a typo, so I refactored this one too.

[shubhamdhama]

I wonder if we should do a cleanup and remove this setting option in a separate PR

[cthumuluru-crdb]

should only be enabled for secondary tenants.

should only be enabled for shared secondary tenants. ?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @kyle-a-wong, @shubhamdhama, and @stevendanna)

[shubhamdhama]

Already fixed in the code comment. Now fixed in the description too.

[shubhamdhama]

Thanks all for the review!

bors r=stevendanna,dhartunian,cthumuluru-crdb

[craig]

Build succeeded:

• windows_build
• examples_orms
• local_roachtest_fips
• linux_amd64_build
• lint
• linux_arm64_build
• unit_tests
• docker_image_amd64
• local_roachtest
• linux_amd64_fips_build
• check_generated_code
• macos_arm64_build
• macos_amd64_build
• acceptance