utilccl,kvccl: improve performance when checking enterprise features

[erikgrinaker]

utilccl: cache license decoding

Previously, the utilccl package would decode the license from the the
base64-encoded Protobuf representation in settings every time it was
needed, which was sufficient for its uses. However, recently there's
been a need to check whether enterprise features are enabled in hot
paths (e.g. with follower reads as seen in #62447), making the decoding
cost too great.

This patch adds cluster.Settings.Cache as a shared cache, and uses it
to cache decoded licenses with a private key type.

utilccl,kvccl: add IsEnterpriseEnabled for faster license checks

utilccl.CheckEnterpriseEnabled() is used to check whether a valid
enterprise license exists for a given feature. If no valid license is
found, it returns an error with specific details.

However, kvccl used this function in follower read hot paths, and
instantiating an error when follower reads are unavailable could have
significant overhead -- see e.g. #62447.

This patch adds IsEnterpriseEnabled(), which has the same behavior as
CheckEnterpriseEnabled() but returns a boolean instead. This is
significantly faster since we can avoid instantiating a custom error
each time. kvccl is also updated to use this in hot paths.

Resolves #62489.

Release note: None

[cockroach-teamcity]

This change is 

[tbg]

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @nvanbenschoten, and @tbg)

---

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

// decodeCache is used to cache licenses for decodeCached().
var decodeCache = struct {

Maybe we can put a Cache sync.Map onto *Settings to avoid the singleton (the cache could grow quite a bit in the large packages). The map could be used for similar things in the future as they arise.

You'd use

type licenseCacheKey string

for the key, so different users wouldn't see each other.

[erikgrinaker]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)

---

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

Previously, tbg (Tobias Grieger) wrote…

Maybe we can put a Cache sync.Map onto *Settings to avoid the singleton (the cache could grow quite a bit in the large packages). The map could be used for similar things in the future as they arise.

You'd use

type licenseCacheKey string

for the key, so different users wouldn't see each other.

Wouldn't it be sort of odd to just "bolt on" this field that wasn't really used by the settings themselves? A cleaner approach might be to register encoders/decoders for settings values, but that seems somewhat excessive. Don't have a strong opinion on it though, can try out a field.

[tbg]

Reviewed 4 of 4 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @nvanbenschoten)

---

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

Previously, erikgrinaker (Erik Grinaker) wrote…

Wouldn't it be sort of odd to just "bolt on" this field that wasn't really used by the settings themselves? A cleaner approach might be to register encoders/decoders for settings values, but that seems somewhat excessive. Don't have a strong opinion on it though, can try out a field.

It's a little odd, I agree, but if we're trying to avoid singletons (as I think we should, given that we go through many ephemeral servers in a test package), you need one grab bag of things that gets passed ~everywhere and in our case that, de facto, is *Settings.

[erikgrinaker]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)

---

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

Previously, tbg (Tobias Grieger) wrote…

It's a little odd, I agree, but if we're trying to avoid singletons (as I think we should, given that we go through many ephemeral servers in a test package), you need one grab bag of things that gets passed ~everywhere and in our case that, de facto, is *Settings.

Makes sense, done.

[tbg]

Reviewed 3 of 3 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @nvanbenschoten)

---

pkg/ccl/utilccl/license_check.go, line 122 at r3 (raw file):

	if err != nil {
		return 0, err
	} else if license == nil {

isn't the linter yelling at you to write it like

if err != nil {
  return 0, err
}
if license == nil {
  return 0, nil
}

[tbg]

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

[erikgrinaker]

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

---

pkg/ccl/utilccl/license_check.go, line 122 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

isn't the linter yelling at you to write it like

if err != nil {
  return 0, err
}
if license == nil {
  return 0, nil
}

No, but I'll change it anyway. :)

[erikgrinaker]

bors=tbg

CI failure is flake, reproducible when stressing on master.

[erikgrinaker]

bors r=tbg

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed:

• GitHub CI (Cockroach)

[tbg]

bors r+

CI was broken, I think it's since been fixed

[craig]

Build succeeded:

• GitHub CI (Cockroach)