server: fix over redacted log line in migration server

[aa-joshi]

Previosuly, few log lines were getting redacted assuming that it contains
sensitive information. Support team was facing challenges during dignostics. To
address this, this patch address the log line in migration server which was
overly redacted.

Epic: CRDB-37533
Part of: CRDB-44885
Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[arjunmahishi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi)

---

pkg/server/migration.go line 147 at r1 (raw file):

	}
	log.Infof(ctx, "active cluster version setting is now %s (up from %s)",
		redact.Safe(newCV.PrettyPrint()), redact.Safe(prevCV.PrettyPrint()))

I think we can directly pass newCV and prevCV without Safe or PrettyPrint. Because the underlying type implements SafeFormatter

It was getting redacted because PrettyPrint() returns string

[aa-joshi]

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

---

pkg/server/migration.go line 147 at r1 (raw file):

Previously, arjunmahishi (Arjun Mahishi) wrote…

I think we can directly pass newCV and prevCV without Safe or PrettyPrint. Because the underlying type implements SafeFormatter

It was getting redacted because PrettyPrint() returns string

I believe we need to wrap in PrettyPrint because it has formatting logic based on v.IsFence()
ref:

cockroach/pkg/roachpb/version.go

Line 115 in ac3c5f2

func (v Version) PrettyPrint() string {

[arjunmahishi]

pkg/server/migration.go line 147 at r1 (raw file):

Previously, aa-joshi (Akshay Joshi) wrote…

I believe we need to wrap in PrettyPrint because it has formatting logic based on v.IsFence()
ref:

cockroach/pkg/roachpb/version.go

Line 115 in ac3c5f2

func (v Version) PrettyPrint() string {

Then I think we can change PrettyPrint and make it return redact.RedactableString
I think it's only being used for logging anyway.

[aa-joshi]

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

---

pkg/server/migration.go line 147 at r1 (raw file):

Previously, arjunmahishi (Arjun Mahishi) wrote…

Then I think we can change PrettyPrint and make it return redact.RedactableString
I think it's only being used for logging anyway.

PrettyPrint is invoked at multiple flows. Changing return type would increase changes significantly. As part of the task, we want to keep changes minimal so that we can backport. Let's use SafeString here.

---

pkg/server/auto_upgrade.go line 52 at r1 (raw file):

			clusterVersion, err := s.clusterVersion(ctx)
			if err != nil {
				log.Errorf(ctx, "unable to retrieve cluster version: %v", redact.Safe(err))

I have reverted log redaction changes for errors as we have decided to build a standard framework for it.

[rafiss]

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

---

pkg/server/migration.go line 147 at r1 (raw file):

Previously, aa-joshi (Akshay Joshi) wrote…

PrettyPrint is invoked at multiple flows. Changing return type would increase changes significantly. As part of the task, we want to keep changes minimal so that we can backport. Let's use SafeString here.

That change does not significantly change any of the non-test call sites. I wrote up a quick PR here to show it: #138353

IMO we should change PrettyPrint to return a RedactableString rather than using SafeString here.

[rafiss]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arjunmahishi)

[aa-joshi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arjunmahishi and @rafiss)

---

pkg/server/migration.go line 147 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

That change does not significantly change any of the non-test call sites. I wrote up a quick PR here to show it: #138353

IMO we should change PrettyPrint to return a RedactableString rather than using SafeString here.

Thanks for the review! updated.

[arjunmahishi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi and @rafiss)

---

pkg/clusterversion/cockroach_versions_test.go line 150 at r3 (raw file):

	}
	for _, test := range tests {
		if actual := test.cv.PrettyPrint().StripMarkers(); actual != test.exp {

We should not be getting any redaction markers if the redaction is handled correctly. If we are just using StripMarkers to resolve the type to string, we can just convert the type of exp to redact.RedactableString on line 141.

Removing StripMarkers will ensure that the test asserts the output of PrettyPrint is not redacted.

[rafiss]

nice fix!

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

---

pkg/clusterversion/cockroach_versions_test.go line 150 at r3 (raw file):

Previously, arjunmahishi (Arjun Mahishi) wrote…

We should not be getting any redaction markers if the redaction is handled correctly. If we are just using StripMarkers to resolve the type to string, we can just convert the type of exp to redact.RedactableString on line 141.

Removing StripMarkers will ensure that the test asserts the output of PrettyPrint is not redacted.

in tests, there's no strong reason to use redacted strings. IMO, it's ok to use StripMarkers here. however, it's also true that changing the type of exp should fix it and is also a simple change. i'd be fine either way,

[aa-joshi]

TFTR
bors r+

[craig]

Build succeeded:

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

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 4570e95 to blathers/backport-release-23.2-137915: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}