logcrash, ccl: fail if setting incompatible license and reporting cfg

[angles-n-daemons]

With the rollout of the license requirements with CRDB, we are going to begin disallowing the disabling of diagnostics under specific license types. If the caller is to disable diagnostics, they must have a license which is not Free or Trial. Conversely, if the caller is to set a Free or Trial license, they must already be submitting diagnostics information for it to succeed.

Epic: CRDB-40209
Fixes: CRDB-41232

Release note (sql change): cluster settings enterprise.license and diagnostics.reporting.enabled have additional validation

[cockroach-teamcity]

This change is 

[angles-n-daemons]

looking for guidance, I very much don't like passing in empty values on register

[angles-n-daemons]

these are warnings because it's uncertain if on startup whether the value will be set by the time this point in code is hit

[dhartunian]

Reviewed 5 of 9 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @angles-n-daemons, @arjunmahishi, and @xinhaoz)

---

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

			}

			// if the license is limited and reporting is disabled, do not allow it to be set

nit: If you're going to have them, comments should be complete sentences with capitalization and periods.

---

pkg/settings/bool.go line 170 at r1 (raw file):

Previously, angles-n-daemons (Brian Dillmann) wrote…

looking for guidance, I very much don't like passing in empty values on register

I don't think we should be validating here. It's ok for the default value to sit outside the validation logic. Am I missing a key scenario that this fufills?

---

pkg/settings/bool.go line 142 at r2 (raw file):

		// As per the semantics of override, these values don't go through
		// validation.
		_ = b.set(ctx, sv, val.(bool))

should this use setOnValues instead?

---

pkg/util/log/logcrash/crash_reporting.go line 71 at r2 (raw file):

Previously, angles-n-daemons (Brian Dillmann) wrote…

these are warnings because it's uncertain if on startup whether the value will be set by the time this point in code is hit

To understand the motivation here: we allow change in diagnostics reporting setting when the license is corrupted or missing, so we don't want to just return these errors.

[angles-n-daemons]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @arjunmahishi, @dhartunian, and @xinhaoz)

---

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

Previously, dhartunian (David Hartunian) wrote…

nit: If you're going to have them, comments should be complete sentences with capitalization and periods.

updated

---

pkg/settings/bool.go line 170 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

I don't think we should be validating here. It's ok for the default value to sit outside the validation logic. Am I missing a key scenario that this fufills?

It's to match validation functionality with the other settings (link below). The challenge here is that boolean validation requires a values parameter, where the others don't. Do you feel this is an opportune spot to diverge?

https://github.com/cockroachdb/cockroach/blob/master/pkg/settings/int.go#L166

---

pkg/util/log/logcrash/crash_reporting.go line 71 at r2 (raw file):

Previously, dhartunian (David Hartunian) wrote…

To understand the motivation here: we allow change in diagnostics reporting setting when the license is corrupted or missing, so we don't want to just return these errors.

This is actually introduced to avoid the race condition on startup. To support validation on init, it's possible that the diagnostics setting can be registered prior to the license setting, which would cause the process to panic even if a valid license exists.

[dhartunian]

This implementation is making me wonder if we should instead use an atomic primitive to synchronize state between the two cluster settings. The dependencies between them seem to lead to unnecessarily complex validator logic.

What do you think?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @angles-n-daemons, @arjunmahishi, and @xinhaoz)

---

pkg/ccl/utilccl/license_check_test.go line 133 at r2 (raw file):

		{licenseccl.License_Evaluation, true, ""},
	} {
		lic, _ := (&licenseccl.License{

I wouldn't ignore errors, just grab the err and call require.NoError(t, err)

[angles-n-daemons]

I'm not sure how I feel about it, if anything I would prefer to make the license a singleton for reference, instead of an opaque value only to be used by diagnostics.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @arjunmahishi, @dhartunian, and @xinhaoz)

---

pkg/ccl/utilccl/license_check_test.go line 133 at r2 (raw file):

Previously, dhartunian (David Hartunian) wrote…

I wouldn't ignore errors, just grab the err and call require.NoError(t, err)

good point, added this check

[angles-n-daemons]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @arjunmahishi, @dhartunian, and @xinhaoz)

---

pkg/settings/bool.go line 142 at r2 (raw file):

Previously, dhartunian (David Hartunian) wrote…

should this use setOnValues instead?

removed the validation on init to match settings/string.go