Merge #141454 #153630 #153672

141454: roachprod: refactor IAP authentication r=DarrylWong a=golgeek

Previously, the authentication mechanism for testeng Identity-Aware Proxy protected endpoints was relying on a shared service account key accessed via an encrypted GCS bucket. This was causing the need for secret rotation and was limiting auditability as everyone was using the same JSON key that was cached in the `~/.roachprod` directory.

This patch introduces a new mechanism based on short-lived OAuth tokens and service account impersonation through the default local credentials.

The identity of the caller is determined with the following precedence:
1. `GOOGLE_EPHEMERAL_CREDENTIALS` environment variable
2. Application Default Credentials (ADC):
  a) `GOOGLE_APPLICATION_CREDENTIALS` environment variable
  b) Default service account (application_default_credentials.json) file
  c) App Engine standard environment
  d) GCE metadata server
4. `gcloud config config-helper` output

The caller needs to have the `roles/iam.serviceAccountTokenCreator` role on the service account to be able to impersonate the service account and generate short lived OAuth AccessTokens.

Both `promhelperclient` and `grafana annotations` switch to this new method, via the new `IAPTokenSourceIface` interface that handles the service account impersonation, the AccessToken caching and renewal and that provides a pre-authenticated `http.Client`.


`@cockroachdb/dev-inf,` the added dependency (`github.com/binxio/gcloudconfig`) is a helper that runs `gcloud config config-helper` to get credentials from the account logged in in gcloud without the need for application default credentials file (when someone ran `gcloud auth login`, but not `gcloud auth application-default login`).

Epic: none
Release note: None

153630: backupdest: remove IncludeManifest arg in FindAllIncrementalPaths r=kev-cao a=msbutler

Epic: none

Release note: none

153672: server: always use dropDRPCHeaderListener with drpc server r=rafiss a=shubhamdhama

Previously, the DRPC listener was conditionally created based on the DRPC setting. When DRPC was disabled, we used a noopListener that would block on Accept() until closed, effectively rejecting all DRPC connections.

In #153503 we changed the DRPC server to always run regardless of the setting, but missed updating the corresponding listener logic. This commit completes that change by always using the dropDRPCHeaderListener to properly handle incoming DRPC connections.

Fixes: #153612

Epic: none
Release note: none

Co-authored-by: Ludovic Leroux <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Shubham Dhama <[email protected]>

Author: 4 people

DEPS.bzl

@@ -1127,6 +1127,16 @@ def go_deps():
             "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/bgentry/speakeasy/com_github_bgentry_speakeasy-v0.1.0.zip",
         ],
     )
+    go_repository(
+        name = "com_github_binxio_gcloudconfig",
+        build_file_proto_mode = "disable_global",
+        importpath = "github.com/binxio/gcloudconfig",
+        sha256 = "82797ef5d9fa4cba09d64ca885a3b6b8867d046c8f144ed15dc102085b0c6ceb",
+        strip_prefix = "github.com/binxio/[email protected]",
+        urls = [
+            "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/binxio/gcloudconfig/com_github_binxio_gcloudconfig-v0.1.5.zip",
+        ],
+    )
     go_repository(
         name = "com_github_biogo_store",
         build_file_proto_mode = "disable_global",

build/bazelutil/distdir_files.bzl

@@ -288,6 +288,7 @@ DISTDIR_FILES = {
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/beorn7/perks/com_github_beorn7_perks-v1.0.1.zip": "25bd9e2d94aca770e6dbc1f53725f84f6af4432f631d35dd2c46f96ef0512f1a",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/bgentry/go-netrc/com_github_bgentry_go_netrc-v0.0.0-20140422174119-9fd32a8b3d3d.zip": "59fbb1e8e307ccd7052f77186990d744284b186e8b1c5ebdfb12405ae8d7f935",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/bgentry/speakeasy/com_github_bgentry_speakeasy-v0.1.0.zip": "d4bfd48b9bf68c87f92c94478ac910bcdab272e15eb909d58f1fb939233f75f0",
+    "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/binxio/gcloudconfig/com_github_binxio_gcloudconfig-v0.1.5.zip": "82797ef5d9fa4cba09d64ca885a3b6b8867d046c8f144ed15dc102085b0c6ceb",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/biogo/store/com_github_biogo_store-v0.0.0-20160505134755-913427a1d5e8.zip": "26551f8829c5ada84a68ef240732375be6747252aba423cf5c88bc03002c3600",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/bitly/go-hostpool/com_github_bitly_go_hostpool-v0.0.0-20171023180738-a3a6125de932.zip": "9a55584d7fa2c1639d0ea11cd5b437786c2eadc2401d825e699ad6445fc8e476",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/bitly/go-simplejson/com_github_bitly_go_simplejson-v0.5.0.zip": "53930281dc7fba8947c1b1f07c82952a38dcaefae23bd3c8e71d70a6daa6cb40",

go.mod

@@ -124,6 +124,7 @@ require (
 	github.com/aws/smithy-go v1.22.3
 	github.com/axiomhq/hyperloglog v0.2.5
 	github.com/bazelbuild/rules_go v0.26.0
+	github.com/binxio/gcloudconfig v0.1.5
 	github.com/biogo/store v0.0.0-20160505134755-913427a1d5e8
 	github.com/blevesearch/snowballstem v0.9.0
 	github.com/buchgr/bazel-remote v1.3.3

go.sum

@@ -450,6 +450,8 @@ github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6r
 github.com/bgentry/go-netrc v0.0.0-20140422174119-9fd32a8b3d3d h1:xDfNPAt8lFiC1UJrqV3uuy861HCTo708pDMbjHHdCas=
 github.com/bgentry/go-netrc v0.0.0-20140422174119-9fd32a8b3d3d/go.mod h1:6QX/PXZ00z/TKoufEY6K/a0k6AhaJrQKdFe6OfVXsa4=
 github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kBD4zp0CCIs=
+github.com/binxio/gcloudconfig v0.1.5 h1:nbvWtpqn7yJs4qPuXxTu9D3DYrSyc0FHkXraseMMCV4=
+github.com/binxio/gcloudconfig v0.1.5/go.mod h1:IpQXzgqmv2JS1i+hbhqhHqzeYWg5zWkdN4sZJznJDUM=
 github.com/biogo/store v0.0.0-20160505134755-913427a1d5e8 h1:tYoz1OeRpx3dJZlh9T4dQt4kAndcmpl+VNdzbSgFC/0=
 github.com/biogo/store v0.0.0-20160505134755-913427a1d5e8/go.mod h1:Iev9Q3MErcn+w3UOJD/DkEzllvugfdx7bGcMOFhvr/4=
 github.com/bitly/go-hostpool v0.0.0-20171023180738-a3a6125de932/go.mod h1:NOuUCSz6Q9T7+igc/hlvDOUdtWKryOrtFyIVABv/p7k=

pkg/backup/backupdest/backup_destination.go

@@ -228,7 +228,7 @@ func ResolveDest(
 	defer rootStore.Close()
 	priors, err := FindAllIncrementalPaths(
 		ctx, execCfg, incrementalStore, rootStore,
-		chosenSuffix, OmitManifest, len(dest.IncrementalStorage) > 0,
+		chosenSuffix, len(dest.IncrementalStorage) > 0,
 	)
 	if err != nil {
 		return ResolvedDestination{}, errors.Wrap(err, "adjusting backup destination to append new layer to existing backup")

pkg/backup/backupdest/incrementals.go

@@ -86,15 +86,14 @@ func FindAllIncrementalPaths(
 	incStore cloud.ExternalStorage,
 	rootStore cloud.ExternalStorage,
 	subdir string,
-	includeManifest bool,
 	customIncLocation bool,
 ) ([]string, error) {
 	ctx, sp := tracing.ChildSpan(ctx, "backupdest.FindAllIncrementalPaths")
 	defer sp.Finish()
 
 	// Backup indexes do not support custom incremental locations.
 	if customIncLocation || !ReadBackupIndexEnabled.Get(&execCfg.Settings.SV) {
-		return LegacyFindPriorBackups(ctx, incStore, includeManifest)
+		return LegacyFindPriorBackups(ctx, incStore, OmitManifest)
 	}
 
 	indexes, err := ListIndexes(ctx, rootStore, subdir)
@@ -105,25 +104,15 @@ func FindAllIncrementalPaths(
 	// backup chain, we can assume the index is complete. So either an index has
 	// been written for every backup in the chain, or there are no indexes at all.
 	if len(indexes) == 0 {
-		return LegacyFindPriorBackups(ctx, incStore, includeManifest)
+		return LegacyFindPriorBackups(ctx, incStore, OmitManifest)
 	}
 
-	paths, err := util.MapE(
+	return util.MapE(
 		indexes[1:], // We skip the full backup
 		func(indexFilename string) (string, error) {
 			return parseBackupFilePathFromIndexFileName(subdir, indexFilename)
 		},
 	)
-	if err != nil {
-		return nil, err
-	}
-
-	if includeManifest {
-		paths = util.Map(paths, func(p string) string {
-			return path.Join(p, backupbase.DeprecatedBackupManifestName)
-		})
-	}
-	return paths, nil
 }
 
 // LegacyFindPriorBackups finds "appended" incremental backups via the legacy
@@ -194,7 +183,7 @@ func backupsFromLocation(
 	defer incStore.Close()
 
 	return FindAllIncrementalPaths(
-		ctx, execCfg, incStore, rootStore, subdir, false /* includeManifest */, customIncLocation,
+		ctx, execCfg, incStore, rootStore, subdir, customIncLocation,
 	)
 }
 

pkg/backup/backupdest/incrementals_test.go

@@ -243,7 +243,7 @@ func TestFindAllIncrementalPaths(t *testing.T) {
 
 			incs, err := backupdest.FindAllIncrementalPaths(
 				ctx, &execCfg, incStore, store,
-				targetSubdir, false /* includeManifest */, false, /* customIncLocation */
+				targetSubdir, false, /* customIncLocation */
 			)
 			require.NoError(t, err)
 			require.Len(t, incs, len(targetChain)-1)
@@ -324,7 +324,7 @@ func TestFindAllIncrementalPathsFallbackLogic(t *testing.T) {
 
 		incs, err := backupdest.FindAllIncrementalPaths(
 			ctx, &execCfg, stores.inc, stores.root,
-			subdir, false /* includeManifest */, false, /* customIncLocation */
+			subdir, false, /* customIncLocation */
 		)
 		require.NoError(t, err)
 		require.Len(t, incs, numBackups-1)
@@ -348,7 +348,7 @@ func TestFindAllIncrementalPathsFallbackLogic(t *testing.T) {
 
 		incs, err := backupdest.FindAllIncrementalPaths(
 			ctx, &execCfg, stores.customInc, stores.root,
-			subdir, false /* includeManifest */, true, /* customIncLocation */
+			subdir, true, /* customIncLocation */
 		)
 		require.NoError(t, err)
 		require.Len(t, incs, numBackups-1)
@@ -390,14 +390,14 @@ func TestFindAllIncrementalPathsFallbackLogic(t *testing.T) {
 		// List from both default and custom incremental locations.
 		incs, err := backupdest.FindAllIncrementalPaths(
 			ctx, &execCfg, stores.inc, stores.root,
-			subdir, false /* includeManifest */, false, /* customIncLocation */
+			subdir, false, /* customIncLocation */
 		)
 		require.NoError(t, err)
 		require.Len(t, incs, numDefaultIncs)
 
 		incs, err = backupdest.FindAllIncrementalPaths(
 			ctx, &execCfg, stores.customInc, stores.root,
-			subdir, false /* includeManifest */, true, /* customIncLocation */
+			subdir, true, /* customIncLocation */
 		)
 		require.NoError(t, err)
 		require.Len(t, incs, numCustomIncs)

pkg/cmd/roachprod/cli/commands.go

@@ -1723,9 +1723,9 @@ func (cr *commandRegistry) buildGrafanaAnnotationCmd() *cobra.Command {
 		Long: fmt.Sprintf(`Adds an annotation to the specified grafana instance
 
 By default, we assume the grafana instance needs an authentication token to connect
-to. A service account json and audience will be read in from the environment
-variables %s and %s to attempt authentication through google IDP. Use the --insecure
-option when a token is not necessary.
+to. Unless the %s environment variable exists, the default Google Application Credentials
+will be used to derive an Access Token to authenticate against Google Identity-Aware Proxy.
+Use the --insecure option when a token is not necessary.
 
 --tags specifies the tags the annotation should have.
 
@@ -1740,7 +1740,7 @@ creates an annotation over time range.
 Example:
 # Create an annotation over time range 1-100 on the centralized grafana instance, which needs authentication.
 roachprod grafana-annotation grafana.testeng.crdb.io example-annotation-event --tags my-cluster --tags test-run-1 --dashboard-uid overview --time-range 1,100
-`, roachprodutil.ServiceAccountJson, roachprodutil.ServiceAccountAudience),
+`, roachprodutil.CredentialsEnvironmentVariable),
 		Args: cobra.ExactArgs(2),
 		Run: Wrap(func(cmd *cobra.Command, args []string) error {
 			req := grafana.AddAnnotationRequest{

pkg/cmd/roachprod/grafana/BUILD.bazel

@@ -22,12 +22,12 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/cmd/roachprod/grafana",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/roachprod/promhelperclient",
         "//pkg/roachprod/roachprodutil",
         "//pkg/util/httputil",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_go_openapi_strfmt//:strfmt",
         "@com_github_grafana_grafana_openapi_client_go//client",
         "@com_github_grafana_grafana_openapi_client_go//models",
-        "@org_golang_google_api//idtoken",
     ],
 )

pkg/cmd/roachprod/grafana/annotations.go

@@ -7,16 +7,15 @@ package grafana
 
 import (
 	"context"
-	"fmt"
 	"strings"
 
+	"github.com/cockroachdb/cockroach/pkg/roachprod/promhelperclient"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/roachprodutil"
 	"github.com/cockroachdb/cockroach/pkg/util/httputil"
 	"github.com/cockroachdb/errors"
 	"github.com/go-openapi/strfmt"
 	grafana "github.com/grafana/grafana-openapi-client-go/client"
 	"github.com/grafana/grafana-openapi-client-go/models"
-	"google.golang.org/api/idtoken"
 )
 
 // newGrafanaClient is a helper function that creates an HTTP client to
@@ -26,30 +25,37 @@ import (
 func newGrafanaClient(
 	ctx context.Context, host string, secure bool,
 ) (*grafana.GrafanaHTTPAPI, error) {
-	headers := map[string]string{}
 	scheme := "http"
 
+	// Use the default HTTP client for unsecure Grafana calls.
+	grafanaHttpClient := httputil.DefaultClient.Client
+
 	if secure {
 		scheme = "https"
 
-		// Read in the service account key and audience, so we can retrieve the identity token.
-		if _, err := roachprodutil.SetServiceAccountCredsEnv(ctx, false); err != nil {
-			return nil, err
-		}
-
-		token, err := roachprodutil.GetServiceAccountToken(ctx, idtoken.NewTokenSource)
+		// Grafana annotations currently use the same service account
+		// and OAuth client ID  as the prometheus helper service.
+		iapTokenSource, err := roachprodutil.NewIAPTokenSource(roachprodutil.IAPTokenSourceOptions{
+			OAuthClientID:       promhelperclient.OAuthClientID,
+			ServiceAccountEmail: promhelperclient.ServiceAccountEmail,
+		})
 		if err != nil {
 			return nil, err
 		}
-		headers["Authorization"] = fmt.Sprintf("Bearer %s", token)
+
+		// Override the default HTTP client with the one
+		// that has the IAP token source.
+		grafanaHttpClient = iapTokenSource.GetHTTPClient()
 	}
 
-	headers[httputil.ContentTypeHeader] = httputil.JSONContentType
 	cfg := &grafana.TransportConfig{
-		Host:        host,
-		BasePath:    "/api",
-		Schemes:     []string{scheme},
-		HTTPHeaders: headers,
+		Host:     host,
+		BasePath: "/api",
+		Schemes:  []string{scheme},
+		HTTPHeaders: map[string]string{
+			httputil.ContentTypeHeader: httputil.JSONContentType,
+		},
+		Client: grafanaHttpClient,
 	}
 
 	return grafana.NewHTTPClientWithConfig(strfmt.Default, cfg), nil