Thanks for doing this! Looks good modulo couple minor comments/suggestions.

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru)

pkg/roachpb/span_config.go, line 89 at r1 (raw file):

		return errors.AssertionFailedf("LeasePreferences set on system span config")
	}
	if s.RangefeedEnabled == true {

The linter is complaining about simplifying this to just if s.RangefeedEnabled {

pkg/roachpb/span_config.go, line 91 at r1 (raw file):

	if s.RangefeedEnabled == true {
		return errors.AssertionFailedf("RangefeedEnabled set on system span config")
	}

Do we want to check ExcludeFromBackup here as well?

pkg/roachpb/span_config.proto, line 189 at r1 (raw file):

  // Next ID: 12
  //
  // When adding a field, also add a check a to `ValidateSystemTargetSpanConfig`

nit: you could add a test that codifies this, something similar in essence to what we do in TestValidateCoversAllDescriptorFields.

pkg/spanconfig/spanconfig.go, line 277 at r1 (raw file):

// Record ties a target to its corresponding config.
//
// Use `MakeRecord` to construct a new Record instead of directly reaching for

Instead of this comment, how about enforcing this in code by making Target and Config private fields and adding getters?

pkg/spanconfig/spanconfig.go, line 279 at r1 (raw file):

// Use `MakeRecord` to construct a new Record instead of directly reaching for
// this struct to ensure proper validation.
type Record struct {

super nit: should we pull this struct, its getters, and constructors into its own file?

pkg/spanconfig/spanconfig.go, line 289 at r1 (raw file):

// MakeRecord returns a Record with the specified Target and SpanConfig. If the
// Record targets a SystemTarget, we also validate the SpanConfig.
func MakeRecord(target Target, cfg roachpb.SpanConfig) (Record, error) {

Let's add a basic test that ensures we return validation errors correctly?

pkg/spanconfig/spanconfig.go, line 293 at r1 (raw file):

		if err := cfg.ValidateSystemTargetSpanConfig(); err != nil {
			return Record{},
				errors.AssertionFailedf("failed to validate SystemTarget SpanConfig: %+v", err.Error())

nit: we could panic here, like we do in MakeTargetFromSpan and a few other places in target.go where we know for sure it is a programming mistake if this ever happens.

pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 180 at r1 (raw file):

			}
		}
		clusterSystemRecord, err := spanconfig.MakeRecord(

nit: missing error check

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @arulajmani)

pkg/roachpb/span_config.go, line 89 at r1 (raw file):

Done.

pkg/roachpb/span_config.go, line 91 at r1 (raw file):

Done.

pkg/spanconfig/spanconfig.go, line 277 at r1 (raw file):

Done.

pkg/spanconfig/spanconfig.go, line 279 at r1 (raw file):

Done.

pkg/spanconfig/spanconfig.go, line 293 at r1 (raw file):

meh I'm generally panic averse. Most of the places already had proper error handling so it wasn't too painful. In light of issues like #76734 and having being bitten by panics in bulk code I prefer to return an error.

pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 180 at r1 (raw file):

oops, done.

modulo a couple minor comments.

Reviewed 18 of 20 files at r2, 1 of 1 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru)

pkg/spanconfig/spanconfig.go, line 289 at r1 (raw file):

The line this comment was referencing has moved, but were hoping to do this?

pkg/spanconfig/spanconfig.go, line 293 at r1 (raw file):

Okay, no strong preference.

pkg/spanconfig/spanconfig.go, line 369 at r4 (raw file):

	record, err := MakeRecord(target, roachpb.SpanConfig{}) // delete
	if err != nil {
		log.Fatalf(context.Background(), "%+v", err)

I don't think context.Background() is correct here is it? We should either thread in the context or panic here, it's pretty much the same effect. Alternatively, you could bubble up the error.

We should be a consistent in how we handle these MakeRecord errors though -- if we're okay crashing the node here, then let's just do it in the constructor to MakeRecord as well. If we're not, then let's bubble up the error here as well.

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

pkg/spanconfig/spanconfig.go, line 289 at r1 (raw file):

Done.

pkg/spanconfig/spanconfig.go, line 369 at r4 (raw file):

agreed, switched to bubble up the error.

Reviewed 9 of 10 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru)