mt_start_sql: enable enterprise features for multitenant sql servers

[jeffswenson]

Enterprise features are controlled by the enterprise.license setting.
Currently this setting applies only to the host tenant cluster. This
change enables enterprise features for all tenant clusters. Enabling
enterprise features for all tenants is reasonable, because multi
tenant deployments are an enterprise feature.

Release note: None

[cockroach-teamcity]

This change is 

[jeffswenson]

More context is available on slack: https://cockroachlabs.slack.com/archives/C01CDD4HRC5/p1628542467012900

[andy-kimball]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)

[knz]

Reviewed 3 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffswenson)

---

pkg/ccl/utilccl/license_check.go, line 118 at r2 (raw file):

	base.LicenseType = getLicenseType
	base.TimeToEnterpriseLicenseExpiry = TimeToEnterpriseLicenseExpiry
	server.EnableEnterpriseForTenant = EnableEnterpriseWithImpliedLicense

This does not sound right.
With this change, any person who downloads the source code from github, builds their own CCL binary and starts a crdb cluster with a tenant pod can use enterprise features without a license, and claim they are legally entitled to do so because the product did not prompt for anything.

IMHO this mode of operation should be opt-in; for example using a serverless-specific license key that we would set using an env var in the k8s pods in the CC infra.

[andy-kimball]

@knz, who owns these licensing decisions? We're willing to do whatever should be done, but who decides what should be done? Are there any written guidelines/criteria for these decisions and/or any process that should happen to make them? By blocking this PR, you're implicitly putting yourself in that role, so you need to specify what we need to do in order to unblock this PR and move forward.

[knz]

That's a product decision, from someone close to our licensing goals. I'm not sure who exactly is currently closest to licensing, but Nate would know.

That being said the engineering viewpoint (from my perspective) is to follow established semantics, where a valid license key should be supplied externally to the process to enable licensed features. It ensures that the decision to enable remains in the hands of the operator for the particular deployment of crdb, and does not automatically extend to all users who happen to have a similar execution script.

[kannanlakshmi]

@piyush-singh owns this from the Product side but he is out, so @mwang1026 is filling in for the next month till Piyush is back.

[andy-kimball]

Can we check the license in these situations:

1. When a new tenant is created (in KV processes).
2. When a KV process is started and at least one active tenant exists.

If we did this, then there'd be no need to recheck the license repeatedly in the SQL tenant processes.

[jeffswenson]

@knz are you okay with this change if the KV layer checks for the enterprise license on tenant creation and connection? Or do think there should be an explicit setting to enable enterprise features at the tenant level?

[knz]

At a basic level we're evolving crdb to offer multi-tenant servers for all deployments, so we'll want the ability to run mon-enterprise tenants.

My intuition is that this can be done using the logic you have implemented here but extend it with an env var that accepts a license and validates it upon process initialization (eg in an init function) and turns it into a boolean fed into the code you wrote already. Any obstacles to this?

[mwang1026]

My 2c: MT is right now for CockroachCloud only. If in the future we want to evolve the licensing story to allow non-enterprise MT deployments we can solve that problem then.

For now, I think (in Piyush's absence) it's fine to be more strict about those checks since the goal right now is to unblock CC Serverless users' access to enterprise features.

I'll leave it to eng to figure out best implementation re: not locking us in against future rolling back of said license check.

Based on ^ I think it's reasonable to check for license upon startup of a tenant.

[jeffswenson]

The big downside in my mind for creating an environment level setting for the tenant license is: an environment variable is a more complex solution for the case where we want all tenants to have enterprise features enabled. Both in the sense there is more code in the database and it is another setting the CC control plane needs to get right.

Even if we did want to support core and enterprise multi-tenant clusters, I still think the environment setting is suboptimal. Enabling enterprise support would require setting the license on the host cluster and setting an environment variable on each of the tenant servers. I think it would be better for the tenants to read the license configured at the host layer. That way updating or adding a license to a cluster only requires a single configuration change.

I think we should do the following:

1. Move forward with this PR as is and add license checks to tenant creation to maintain multi tenancy as an enterprise feature. Many of the tenant's dependencies are stored in the ccl directory (e.x. kvtennantccl). So the feature should not be supported by core clusters at this time.
2. In the future if we want to support multi tenant clusters in core, we should have the tenant read the license from the host cluster. This change would be a no-op when deployed to an enterprise multi tenant cluster because the license checks in tenant creation guarantee the host cluster has a valid license already.

[knz]

ok let us know when you have implemented step 1 as part of this PR

[jeffswenson]

I've changed course to align with knz@'s original suggestion. I added the COCKROACH_TENANT_LICENSE environment variable and check for its presence. I think we should replace the environment variable as soon as there is support in the tenant for reading settings from the host cluster.

[knz]

nice 💯
LGTM modulo a nit

I think we should replace the environment variable as soon as there is support in the tenant for reading settings from the host cluster.

Agreed

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffswenson)

---

pkg/util/envutil/env.go, line 367 at r3 (raw file):

// TestSetEnv sets an environment variable and the cleanup function
// resets it to the original value.
func TestSetEnv(name string, value string) func() {

if this is meant only for use in tests, you can accept a testutils.TB as argument here and use t.Fatal instead of panic below to get cleaner test errors.

[jeffswenson]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz)

---

pkg/util/envutil/env.go, line 367 at r3 (raw file):

Previously, knz (kena) wrote…

if this is meant only for use in tests, you can accept a testutils.TB as argument here and use t.Fatal instead of panic below to get cleaner test errors.

Done.

[jeffswenson]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[dt]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/util/envutil/env.go, line 367 at r3 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

Done.

@knz wait, really? we used to have a pretty strict rule about not importing "testing" in non _test.go files... did that change?

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/util/envutil/env.go, line 367 at r3 (raw file):

Previously, dt (David Taylor) wrote…

@knz wait, really? we used to have a pretty strict rule about not importing "testing" in non _test.go files... did that change?

I said testutils.TB. Jeff just made a mistake and I didn't notice during the review. Let's patch this.