Pull Request Overview

This PR updates the TestServerControllerLoginLogout test to accommodate secondary tenant behavior by modifying server startup parameters, adjusting the HTTP method for logout, and tailoring cookie expectations based on the deployment mode.

• Removed the DefaultTestTenant parameter when starting the server.
• Changed the logout request from POST to GET to reflect updated endpoint behavior.
• Altered expected cookie names and values based on the external deployment mode.

srv := serverutils.StartServerOnly(t, base.TestServerArgs{})

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @copilot-pull-request-reviewer[bot], @cthumuluru-crdb, and @shubhamdhama)

pkg/ccl/serverccl/server_controller_test.go line 587 at r1 (raw file):

		// TODO: Not sure why 'Secure' isn't set for logout in external mode,
		// even though it's set in other cases. This isn't just a test issue —
		// it happens outside of tests too.

I think this is because there's different code for logging out in single and multi-tenant setups:

multi-tenant logout: https://github.com/cockroachdb/cockroach/blob/master/pkg/server/server_controller_http.go#L334-L336

single-tenant logout: https://github.com/cockroachdb/cockroach/blob/master/pkg/server/authserver/authentication.go#L416-L419

I wrote these changes and if I want to remember why I did this, I think it was likely because when I added the forHTTPSOnly flag I didn't want to break existing cookie callsites so I defaulted to false. It seemed OK not to force the logout cookie to be secure. Also, it seems like during logout we don't have the context of whether our original cookie was secure, although I'm sure we can retrieve that somehow. My thinking was that forcing logout in more scenarios than not (by not making the cookie secure) was a good tradeoff.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @copilot-pull-request-reviewer[bot], @dhartunian, and @shubhamdhama)

pkg/ccl/serverccl/server_controller_test.go line 587 at r1 (raw file):

Thanks, @dhartunian. @shubhamdhama, can you please summarize this context and replace the comment.