Skip to content

Commit f41d924

Browse files
mattreidmattreid
committed
PR Review feedback fixes
Rename creds env, central -debug, unified getToken fall-through, and clearer OAuth failure context without duplicating ADC discovery (removes logCredentialSearchLocations).
1 parent 145a214 commit f41d924

4 files changed

Lines changed: 96 additions & 111 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ option to list all command line options.
3636
OAuth client. The client configuration is provided in the conventional
3737
location (e.g., `${HOME}/.config/gcloud/application_default_credentials.json`
3838
or pointed to by the `GOOGLE_APPLICATION_CREDENTIALS` environment variable;
39-
if you set `COSTPULLER_CREDENTIALS` to a JSON file path, costpuller uses only
39+
if you set `COSTPULLER_GOOGLE_CREDENTIALS` to a JSON file path, costpuller uses only
4040
that file and ignores `GOOGLE_APPLICATION_CREDENTIALS` in this process;
4141
see the Google [ADC documentation](https://cloud.google.com/docs/authentication/set-up-adc-local-dev-environment))
4242
and can be downloaded from a project on https://console.developers.google.com,

‎aws.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,14 @@ type AwsPuller struct {
2525
debug bool
2626
}
2727

28-
// NewAwsPuller returns a new AWS client.
29-
func NewAwsPuller(profile string, debug bool) *AwsPuller {
28+
// NewAwsPuller returns a new AWS client. Debug output follows package costpullerDebug (-debug).
29+
func NewAwsPuller(profile string) *AwsPuller {
3030
awsP := new(AwsPuller)
3131
awsP.session = session.Must(session.NewSessionWithOptions(session.Options{
3232
Profile: profile,
3333
SharedConfigState: session.SharedConfigEnable,
3434
}))
35-
awsP.debug = debug
35+
awsP.debug = costpullerDebug
3636
return awsP
3737
}
3838

‎costpuller.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ type AccountsFile struct {
3636
type Configuration map[string]any
3737
type Team map[string][]AccountEntry
3838

39+
// costpullerDebug mirrors the -debug flag; set in main after flag.Parse.
40+
// Subsystems (OAuth, AWS, etc.) read it instead of threading a bool through helpers.
41+
var costpullerDebug bool
42+
3943
// AccountEntry describes an account with metadata.
4044
type AccountEntry struct {
4145
AccountID string `yaml:"accountid"`
@@ -65,6 +69,7 @@ func main() {
6569
taggedAccountsPtr: flag.Bool("taggedaccounts", false, "use the AWS tags as account list source"),
6670
}
6771
flag.Parse()
72+
costpullerDebug = *options.debugPtr
6873

6974
if *options.csvfilePtr == defaultCsvFile && *options.monthPtr != defaultMonth {
7075
newDefaultCsvFile := fmt.Sprintf("output-%s.csv", *options.monthPtr)
@@ -99,7 +104,7 @@ func main() {
99104
awsProfile,
100105
)
101106
}
102-
awsPuller := NewAwsPuller(awsProfile, *options.debugPtr)
107+
awsPuller := NewAwsPuller(awsProfile)
103108

104109
if *options.awsWriteTagsPtr {
105110
writeAwsTags(awsPuller, options)
@@ -163,7 +168,7 @@ func newOutputObject(options CommandLineOptions, accountsFile AccountsFile) *Out
163168
obj.csvFile = getCsvFile(options)
164169
} else if *options.outputTypePtr == "gsheet" {
165170
oauthConfig := getMapKeyValue(accountsFile.Configuration, "oauth", "configuration")
166-
obj.httpClient = getGoogleOAuthHttpClient(oauthConfig, *options.debugPtr)
171+
obj.httpClient = getGoogleOAuthHttpClient(oauthConfig)
167172
obj.gsheetConfig = getMapKeyValue(accountsFile.Configuration, "gsheet", "configuration")
168173
} else {
169174
log.Fatalf("[main] Unexpected value for output type, %q", *options.outputTypePtr)

‎gcloud_oauth.go‎

Lines changed: 85 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -29,166 +29,146 @@ const defaultTokenCachePath = "gcloud"
2929
// OAuth 2.0 access and refresh token values.
3030
const tokenFileName = "costpuller_token.json"
3131

32-
// costpullerCredentialsEnv is the environment variable that points to the
33-
// credentials JSON file for this program only. When set, Application Default
32+
// costpullerGoogleCredentialsEnv is the environment variable that points to the
33+
// Google credentials JSON file for this program only. When set, Application Default
3434
// Credentials do not use GOOGLE_APPLICATION_CREDENTIALS, so other tools on the
3535
// same machine can keep using that variable without affecting costpuller.
36-
const costpullerCredentialsEnv = "COSTPULLER_CREDENTIALS"
36+
const costpullerGoogleCredentialsEnv = "COSTPULLER_GOOGLE_CREDENTIALS"
3737

3838
const googleSheetsScope = "https://www.googleapis.com/auth/spreadsheets"
3939

40-
// oauthDebugf logs only when -debug is enabled (credential and token tracing).
41-
func oauthDebugf(debug bool, format string, args ...any) {
42-
if debug {
40+
// oauthDebugf logs only when costpullerDebug is true (-debug, set in main after flag.Parse).
41+
func oauthDebugf(format string, args ...any) {
42+
if costpullerDebug {
4343
log.Printf(format, args...)
4444
}
4545
}
4646

47-
// logCredentialSearchLocations logs where costpuller looks for credentials.
48-
func logCredentialSearchLocations(debug bool) {
49-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] Credential search (Google Sheets OAuth client JSON)")
50-
if p := strings.TrimSpace(os.Getenv(costpullerCredentialsEnv)); p != "" {
51-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] %s is set to %q (this path is used; GOOGLE_APPLICATION_CREDENTIALS is ignored)", costpullerCredentialsEnv, p)
52-
if _, err := os.Stat(p); err == nil {
53-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] File exists at %s path", costpullerCredentialsEnv)
54-
} else {
55-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] File does NOT exist at %s path: %v", costpullerCredentialsEnv, err)
56-
}
47+
// oauthDebugGoogleClientReady logs one line after credentials JSON is loaded and
48+
// oauth2.Config is built. For more detail later, consider: JSON log fields (e.g.
49+
// encoding/json on a struct), Go log/slog with attributes, or OpenTelemetry spans
50+
// instead of growing printf-style lines.
51+
func oauthDebugGoogleClientReady(credObj *google.Credentials, jsonType string, jsonTypeErr error, clientID string) {
52+
if !costpullerDebug {
5753
return
5854
}
59-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] %s is not set; using Application Default Credentials", costpullerCredentialsEnv)
60-
envCreds := os.Getenv("GOOGLE_APPLICATION_CREDENTIALS")
61-
if envCreds != "" {
62-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] GOOGLE_APPLICATION_CREDENTIALS is set to: %q", envCreds)
63-
if _, err := os.Stat(envCreds); err == nil {
64-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] File exists at GOOGLE_APPLICATION_CREDENTIALS path")
65-
} else {
66-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] File does NOT exist at GOOGLE_APPLICATION_CREDENTIALS path: %v", err)
67-
}
68-
} else {
69-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] GOOGLE_APPLICATION_CREDENTIALS environment variable is not set")
55+
t := jsonType
56+
if jsonTypeErr != nil {
57+
t = fmt.Sprintf("<?> (%v)", jsonTypeErr)
7058
}
71-
homeDir, err := os.UserHomeDir()
72-
if err == nil {
73-
defaultPath := filepath.Join(homeDir, ".config", "gcloud", "application_default_credentials.json")
74-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] Checking default ADC location: %q", defaultPath)
75-
if _, err := os.Stat(defaultPath); err == nil {
76-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] File exists at default ADC location")
77-
} else {
78-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] File does NOT exist at default ADC location: %v", err)
79-
}
59+
var src string
60+
if p := strings.TrimSpace(os.Getenv(costpullerGoogleCredentialsEnv)); p != "" {
61+
src = fmt.Sprintf("%s=%q", costpullerGoogleCredentialsEnv, p)
62+
} else {
63+
src = fmt.Sprintf("ADC(%s unset)", costpullerGoogleCredentialsEnv)
8064
}
65+
log.Printf("[getGoogleOAuthHttpClient] %s project_id=%q json_bytes=%d json_type=%s client_id=%q",
66+
src, credObj.ProjectID, len(credObj.JSON), t, clientID)
8167
}
8268

83-
// loadGoogleCredentials loads credentials for the Sheets scope. If
84-
// COSTPULLER_CREDENTIALS is set, that file is read; otherwise standard ADC
85-
// resolution is used (including GOOGLE_APPLICATION_CREDENTIALS).
69+
// loadGoogleCredentials loads Google OAuth or service-account JSON for the
70+
// Sheets API scope (googleSheetsScope).
71+
//
72+
// If the environment variable named by costpullerGoogleCredentialsEnv is set to a
73+
// file path, that file is read and GOOGLE_APPLICATION_CREDENTIALS is not used
74+
// for this load. Otherwise Application Default Credentials apply (including
75+
// GOOGLE_APPLICATION_CREDENTIALS and well-known paths such as
76+
// ~/.config/gcloud/application_default_credentials.json).
8677
func loadGoogleCredentials(ctx context.Context) (*google.Credentials, error) {
87-
if p := strings.TrimSpace(os.Getenv(costpullerCredentialsEnv)); p != "" {
78+
if p := strings.TrimSpace(os.Getenv(costpullerGoogleCredentialsEnv)); p != "" {
8879
b, err := os.ReadFile(p)
8980
if err != nil {
90-
return nil, fmt.Errorf("read %s %q: %w", costpullerCredentialsEnv, p, err)
81+
return nil, fmt.Errorf("read %s %q: %w", costpullerGoogleCredentialsEnv, p, err)
9182
}
9283
return google.CredentialsFromJSON(ctx, b, googleSheetsScope)
9384
}
9485
return google.FindDefaultCredentials(ctx, googleSheetsScope)
9586
}
9687

88+
// googleCredentialsSourceSummary describes which non-secret credential source
89+
// loadGoogleCredentials used (path or ADC). Never includes file contents or tokens.
90+
func googleCredentialsSourceSummary() string {
91+
if p := strings.TrimSpace(os.Getenv(costpullerGoogleCredentialsEnv)); p != "" {
92+
return fmt.Sprintf("%s file %q", costpullerGoogleCredentialsEnv, p)
93+
}
94+
return fmt.Sprintf("Application Default Credentials (%s unset)", costpullerGoogleCredentialsEnv)
95+
}
96+
9797
// getGoogleOAuthHttpClient accepts a mapping of configuration value strings
9898
// and returns an HTTP client which can be used to make authorized Google API
9999
// requests. The token is obtained either using values cached in a local file
100100
// or by prompting the user to perform an authorization dialog; either way, the
101101
// new token is written to the cache file before returning.
102102
//
103-
// The Google OAuth 2.0 Client configuration is constructed from a local
104-
// credentials file (which can be downloaded from https://console.developers.google.com,
105-
// under "Credentials"). It is located using the default mechanisms (e.g., in
106-
// ${HOME}/.config/gcloud/application_default_credentials.json). Set
107-
// COSTPULLER_CREDENTIALS to a JSON path to use a dedicated file and ignore
108-
// GOOGLE_APPLICATION_CREDENTIALS for this process. (Currently, the scope of
109-
// the authorization is limited to the Google Sheets APIs.)
110-
func getGoogleOAuthHttpClient(oauthConfigMap Configuration, debug bool) *http.Client {
103+
// Client JSON is resolved by loadGoogleCredentials (see costpullerGoogleCredentialsEnv).
104+
// Credentials can be created in Google Cloud Console under "Credentials".
105+
func getGoogleOAuthHttpClient(oauthConfigMap Configuration) *http.Client {
111106
ctx := context.Background()
112107

113-
logCredentialSearchLocations(debug)
114-
115108
credObj, err := loadGoogleCredentials(ctx)
116109
if err != nil {
117-
log.Printf("[getGoogleOAuthHttpClient] Error loading credentials: %v", err)
118110
log.Fatalf("[getGoogleOAuthHttpClient] Unable to read OAuth client credentials file: %v", err)
119111
}
120112

121-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] Credentials found successfully")
122-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] Project ID: %q", credObj.ProjectID)
123-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] Credentials JSON length: %d bytes", len(credObj.JSON))
124-
125-
// Try to determine credential type from JSON
126113
var credType struct {
127114
Type string `json:"type"`
128115
}
129-
if err := json.Unmarshal(credObj.JSON, &credType); err == nil {
130-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] Credential type: %q", credType.Type)
131-
} else {
132-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] Unable to determine credential type from JSON: %v", err)
133-
}
116+
typeErr := json.Unmarshal(credObj.JSON, &credType)
134117

135118
config, err := google.ConfigFromJSON(credObj.JSON, googleSheetsScope)
136119
if err != nil {
137-
log.Printf("[getGoogleOAuthHttpClient] Error from ConfigFromJSON: %v", err)
138-
log.Printf("[getGoogleOAuthHttpClient] Credential type was: %q", credType.Type)
139-
log.Fatalf("[getGoogleOAuthHttpClient] Unable to construct a client configuration: %v", err)
120+
src := googleCredentialsSourceSummary()
121+
log.Fatalf(
122+
"[getGoogleOAuthHttpClient] Unable to construct oauth2.Config (source: %s; credential JSON type %q; json_bytes=%d): %v",
123+
src, credType.Type, len(credObj.JSON), err,
124+
)
140125
}
141-
142-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] OAuth2 config created successfully")
143-
oauthDebugf(debug, "[getGoogleOAuthHttpClient] Client ID: %q", config.ClientID)
144126

145-
token, tokenCachePath := getToken(oauthConfigMap, config, ctx, debug)
146-
cacheToken(token, tokenCachePath, debug)
127+
oauthDebugGoogleClientReady(credObj, credType.Type, typeErr, config.ClientID)
128+
129+
token, tokenCachePath := getToken(oauthConfigMap, config, ctx)
130+
cacheToken(token, tokenCachePath)
147131

148132
return config.Client(ctx, token)
149133
}
150134

151135
// getToken is a helper function which extracts configuration information from
152136
// the supplied mapping and returns either a cached token, if available, or a
153-
// new token.
137+
// new token. When the cache path cannot be resolved, the cache file is missing,
138+
// or the cached token cannot be refreshed, execution falls through to a single
139+
// getNewToken path at the end (same as the original control flow).
154140
func getToken(
155141
oauthConfigMap Configuration,
156142
config *oauth2.Config,
157143
ctx context.Context,
158-
debug bool,
159144
) (token *oauth2.Token, tokenCachePath string) {
160145
path := getMapKeyString(oauthConfigMap, "tokenCachePath", "")
161146
tokenCachePath, err := getCacheFileName(path)
162147
if err != nil {
163-
// Can't determine cache path, get a new token
164-
oauthDebugf(debug, "[getToken] Unable to determine cache path, getting new token")
165-
port := getMapKeyString(oauthConfigMap, "port", "")
166-
return getNewToken(config, port, ctx), tokenCachePath
167-
}
168-
169-
// Try to open the cache file
170-
tokenCacheFile, err := os.Open(tokenCachePath)
171-
if err == nil {
172-
// Try to use cached token, but if it fails due to invalid credentials,
173-
// fall through to getting a new token
174-
token, err = getCachedTokenSafe(config, tokenCacheFile, ctx, debug)
175-
closeFile(tokenCacheFile)
148+
oauthDebugf("[getToken] Unable to determine cache path; new token will not be cached on disk")
149+
} else {
150+
tokenCacheFile, err := os.Open(tokenCachePath)
176151
if err == nil {
177-
oauthDebugf(debug, "[getToken] Using cached token from %q", tokenCachePath)
178-
return token, tokenCachePath
179-
}
180-
// If we get here, the cached token was invalid - delete it and get a new one
181-
oauthDebugf(debug, "[getToken] Cached token invalid, will get a new token")
182-
if removeErr := os.Remove(tokenCachePath); removeErr != nil {
183-
log.Printf("[getToken] Warning: unable to delete invalid cached token: %v", removeErr)
152+
token, err = getCachedTokenSafe(config, tokenCacheFile, ctx)
153+
closeFile(tokenCacheFile)
154+
if err == nil {
155+
oauthDebugf("[getToken] Using cached token from %q", tokenCachePath)
156+
return token, tokenCachePath
157+
}
158+
oauthDebugf("[getToken] Cached token invalid, will get a new token")
159+
if removeErr := os.Remove(tokenCachePath); removeErr != nil {
160+
log.Printf("[getToken] Warning: unable to delete invalid cached token: %v", removeErr)
161+
}
162+
} else {
163+
if errors.Is(err, os.ErrNotExist) {
164+
oauthDebugf("[getToken] No token cache file at %q", tokenCachePath)
165+
} else {
166+
oauthDebugf("[getToken] Cannot open token cache %q: %v; using browser auth", tokenCachePath, err)
167+
}
184168
}
185-
} else if !errors.Is(err, os.ErrNotExist) {
186-
// Unexpected error opening cache file
187-
log.Fatalf("[getToken] Unexpected error accessing the token cache file, %q: %v", tokenCachePath, err)
188169
}
189-
190-
// Get a new token (either cache doesn't exist or was invalid)
191-
oauthDebugf(debug, "[getToken] Getting new OAuth token")
170+
171+
oauthDebugf("[getToken] Getting new OAuth token")
192172
port := getMapKeyString(oauthConfigMap, "port", "")
193173
token = getNewToken(config, port, ctx)
194174
return token, tokenCachePath
@@ -198,13 +178,13 @@ func getToken(
198178
// stores the token in the indicated file. The contents of the file are
199179
// replaced with the new value. If the path is blank, the function prints a
200180
// message and returns; other errors result in exiting the process.
201-
func cacheToken(token *oauth2.Token, tokenCachePath string, debug bool) {
181+
func cacheToken(token *oauth2.Token, tokenCachePath string) {
202182
if tokenCachePath == "" {
203-
log.Println(debug, "The token will not be cached.")
183+
log.Println("The token will not be cached.")
204184
} else {
205185
newTokenCacheFile, err := os.OpenFile(tokenCachePath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600)
206186
if err == nil {
207-
oauthDebugf(debug, "Caching oauth token in %q.", tokenCachePath)
187+
oauthDebugf("Caching oauth token in %q.", tokenCachePath)
208188
err = json.NewEncoder(newTokenCacheFile).Encode(token)
209189
closeFile(newTokenCacheFile)
210190
}
@@ -243,26 +223,26 @@ func getCacheFileName(tokenCachePath string) (string, error) {
243223
// provided file, refreshes it using the provided configuration and context,
244224
// and returns the resulting token. If the token is invalid (e.g., created with
245225
// different credentials), it returns an error instead of fatally exiting.
246-
func getCachedTokenSafe(config *oauth2.Config, cacheFile *os.File, ctx context.Context, debug bool) (*oauth2.Token, error) {
226+
func getCachedTokenSafe(config *oauth2.Config, cacheFile *os.File, ctx context.Context) (*oauth2.Token, error) {
247227
token := &oauth2.Token{}
248228
err := json.NewDecoder(cacheFile).Decode(token)
249229
if err != nil {
250230
return nil, fmt.Errorf("unable to parse cached OAuth tokens: %w", err)
251231
}
252232

253-
oauthDebugf(debug, "[getCachedTokenSafe] Attempting to refresh cached token from %q", cacheFile.Name())
233+
oauthDebugf("[getCachedTokenSafe] Attempting to refresh cached token from %q", cacheFile.Name())
254234
token, err = config.TokenSource(ctx, token).Token()
255235
if err != nil {
256236
// If the error is "unauthorized_client", it likely means the cached token
257237
// was created with different OAuth client credentials.
258238
if strings.Contains(err.Error(), "unauthorized_client") {
259-
oauthDebugf(debug, "[getCachedTokenSafe] Cached token is invalid (likely created with different credentials): %v", err)
239+
oauthDebugf("[getCachedTokenSafe] Cached token is invalid (likely created with different credentials): %v", err)
260240
return nil, fmt.Errorf("cached token invalid: %w", err)
261241
}
262242
return nil, fmt.Errorf("unable to refresh cached token: %w", err)
263243
}
264244

265-
oauthDebugf(debug, "[getCachedTokenSafe] Successfully refreshed cached token")
245+
oauthDebugf("[getCachedTokenSafe] Successfully refreshed cached token")
266246
return token, nil
267247
}
268248

0 commit comments

Comments
 (0)