Merge #118681 #118829

118681: oidc: init in test after all settings are set r=maryliag a=dhartunian

We've had some intermittent failures with OIDC tests that look like the configuration change in the final setting has not been propagated through to the config. This test modifies the configuration to be enabled at the end of the cluster setting changes to reduce the chance of this happening.

Resolves: #118568
Resolves: #115908

Epic: None

Release note: None

118829: kvclient: simplify test r=arulajmani a=andrewbaptist

The test was previously using the index and getting the node id from that. This approach made it hard to avoid off-by-one errors. Using the node id directly makes the test easier to read.

Epic: none

Release note: None

Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Andrew Baptist <[email protected]>

Author: 3 people

pkg/ccl/oidcccl/authentication_oidc_test.go

@@ -129,8 +129,8 @@ func TestOIDCEnabled(t *testing.T) {
 	sqlDB.Exec(t, `SET CLUSTER SETTING server.oidc_authentication.redirect_url = "https://cockroachlabs.com/oidc/v1/callback"`)
 	sqlDB.Exec(t, `set cluster setting server.oidc_authentication.claim_json_key = "email"`)
 	sqlDB.Exec(t, `set cluster setting server.oidc_authentication.principal_regex = '^([^@]+)@[^@]+$'`)
-	sqlDB.Exec(t, `SET CLUSTER SETTING server.oidc_authentication.enabled = "true"`)
 	sqlDB.Exec(t, fmt.Sprintf(`SET CLUSTER SETTING server.http.base_path = "%s"`, basePath))
+	sqlDB.Exec(t, `SET CLUSTER SETTING server.oidc_authentication.enabled = "true"`)
 
 	testCertsContext := s.NewClientRPCContext(ctx, username.TestUserName())
 	client, err := testCertsContext.GetHTTPClient()
@@ -203,7 +203,7 @@ func TestOIDCEnabled(t *testing.T) {
 		t.Fatalf("expected 307 status code but got: %d", resp.StatusCode)
 	}
 	if resp.Header.Get("Location") != basePath {
-		t.Fatalf("expected to be redirected to root")
+		t.Fatalf("expected to be redirected to %s", basePath)
 	}
 	foundCookie := false
 	for _, c := range resp.Cookies() {

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go

@@ -361,7 +361,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 				Insecure: true,
 				Knobs: base.TestingKnobs{
 					KVClient: &kvcoord.ClientTestingKnobs{
-						TransportFactory: getInterceptingTransportFactory(roachpb.NodeID(1)),
+						TransportFactory: getInterceptingTransportFactory(1),
 					},
 					Store: &kvserver.StoreTestingKnobs{
 						EvalKnobs: kvserverbase.BatchEvalTestingKnobs{
@@ -517,7 +517,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 	execLeaseMover := func(t *testing.T, name string) error {
 		desc, err := tc.LookupRange(keyB)
 		assert.NoError(t, err)
-		t.Logf("Transferring r%d lease to n%d", desc.RangeID, tc.Target(0).NodeID)
+		t.Logf("Transferring r%d lease to n%d", desc.RangeID, 1)
 		assert.NoError(t, tc.TransferRangeLease(desc, tc.Target(0)))
 		return nil
 	}
@@ -585,7 +585,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 			// 3. txn1->n1: Put(a), EndTxn(parallel commit) -- Puts txn1 in STAGING.
 			// 4. txn1->n2: Put(b) -- Send the request, but pause before returning
 			// the response so we can inject network failure.
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				// Once we have seen the write on txn1 to n2 that we will fail, txn2
 				// can start.
 				close(txn2Ready)
@@ -607,7 +607,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 			// <transfer b's lease to n1>
 			// <inject a network failure and finally allow (4) txn1->n2: Put(b) to
 			// return with error>
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				// Hold the operation open until we are ready to retry on the new
 				// replica, after which we will return the injected failure.
 				req.pauseUntil(t, leaseMoveComplete, cp)
@@ -774,7 +774,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 			// 3. txn1->n1: Put(a), EndTxn(parallel commit) -- Puts txn1 in STAGING.
 			// 4. txn1->n2: Put(b) -- Send the request, but pause before returning
 			// the response so we can inject network failure.
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				// Once we have seen the write on txn1 to n2 that we will fail, txn2
 				// can start.
 				close(txn2Ready)
@@ -796,7 +796,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 			// <transfer b's lease to n1>
 			// <inject a network failure and finally allow (4) txn1->n2: Put(b) to
 			// return with error>
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				// Hold the operation open until we are ready to retry on the new
 				// replica, after which we will return the injected failure.
 				req.pauseUntil(t, leaseMoveComplete, cp)
@@ -937,7 +937,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 			// 3. txn1->n1: Put(a), EndTxn(parallel commit) -- Puts txn1 in STAGING.
 			// 4. txn1->n2: Put(b) -- Send the request, but pause before returning
 			// the response so we can inject network failure.
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				// Once we have seen the write on txn1 to n2 that we will fail, we can
 				// move the lease.
 				close(txn2Ready)
@@ -958,7 +958,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 			// <transfer b's lease to n1>
 			// <inject a network failure and finally allow (4) txn1->n2: Put(b) to
 			// return with error>
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				// Hold the operation open until we are ready to retry on the new
 				// replica, after which we will return the injected failure.
 				req.pauseUntil(t, leaseMoveComplete, cp)
@@ -1073,7 +1073,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 			// 3. txn1->n1: Put(a), EndTxn(parallel commit) -- Puts txn1 in STAGING.
 			// 4. txn1->n2: Put(b) -- Send the request, but pause before returning the
 			// response so we can inject network failure.
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				// Once we have seen the write on txn1 to n2 that we will fail, txn2/txn3 can start.
 				close(otherTxnsReady)
 			}
@@ -1095,7 +1095,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 			// <transfer b's lease to n1>
 			// <inject a network failure and finally allow (4) txn1->n2: Put(b) to
 			// return with error>
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				// Hold the operation open until we are ready to retry on the new
 				// replica, after which we will return the injected failure.
 				req.pauseUntil(t, leaseMoveComplete, cp)
@@ -1192,7 +1192,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 			// 4. txn1->n2: Put(b) -- Send the request, but pause before returning the
 			// response so we can inject network failure.
 			// <transfer b's lease to n1>
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				close(leaseMoveReady)
 				req.pauseUntil(t, leaseMoveComplete, cp)
 				close(txn2Ready)
@@ -1210,7 +1210,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 
 			// <inject a network failure and finally allow (4) txn1->n2: Put(b) to
 			// return with error>
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				// Hold the operation open until we are ready to retry on the new
 				// replica, after which we will return the injected failure.
 				req.pauseUntil(t, recoverComplete, cp)
@@ -1295,7 +1295,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 			// 4. txn1->n2: Put(b) -- Send the request, but pause before returning the
 			// response so we can inject network failure.
 			// <transfer b's lease to n1>
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				close(leaseMoveReady)
 				req.pauseUntil(t, leaseMoveComplete, cp)
 				close(txn2Ready)
@@ -1317,7 +1317,7 @@ func TestTransactionUnexpectedlyCommitted(t *testing.T) {
 
 			// <inject a network failure and finally allow (4) txn1->n2: Put(b) to
 			// return with error>
-			if req.txnName == "txn1" && hasPut && req.toNodeID == tc.Server(1).NodeID() && cp == AfterSending {
+			if req.txnName == "txn1" && hasPut && req.toNodeID == 2 && cp == AfterSending {
 				// Hold the operation open until we are ready to retry on the new
 				// replica, after which we will return the injected failure.
 				req.pauseUntil(t, txn2ETReady, cp)