Auto-configure Org/Project ID for SPs #46
Conversation
If authenticating the CLI with a service principal and an empty profile, auto-detect the organization and project ID where possible. This enables use cases such as `HCP_CLIENT_ID=XXX HCP_CLIENT_SECRET=YYY hcp <command>` to work even if no profile exists yet. This can be particularly useful when authenticating in oneshot environments such as a non-interactive run of the Docker container.
There was a problem hiding this comment.
Looks good to me. Just one suggestions to reduce repetition.
I am using Feedback Ladder annotations to make the importance/urgency of comments easier to understand.
| Emoji | Name | Importance/Urgency |
|---|---|---|
| 🏔 | Mountain | This change should not go live. Back to the drawing board. |
| 🧗🏻♂️ | Boulder | Has to be addressed before this can be merged. |
| ⚪️ | Pebble | Needs to be addressed. Can be in a future PR. |
| ⏳ | Sand | Needs to be considered. Can be in a future PR. |
| ✏️ | Pen | Typo or grammatical error. Can be addressed in a future PR. |
| 💨 | Dust | "Take it or Leave it" |
| if p.OrganizationID != "" || p.ProjectID != "" || err != nil || !tkn.Expiry.After(time.Now()) { | ||
| return p, nil | ||
| } | ||
|
|
||
| // Get the caller identity. If it is a service principal, we can set the | ||
| // organization and potentially project automatically. This is particularly | ||
| // useful when authenticating the CLI with a service principal and running | ||
| // one off commands, where the profile have not been set interactively. | ||
| callerIdentityParams := iam_service.NewIamServiceGetCallerIdentityParamsWithContext(ctx) | ||
| ident, err := iam.IamServiceGetCallerIdentity(callerIdentityParams, nil) | ||
| if err != nil { | ||
| return p, nil | ||
| } | ||
|
|
||
| // Skip if the caller isn't a service principal | ||
| if ident.Payload == nil || ident.Payload.Principal == nil || ident.Payload.Principal.Service == nil { | ||
| return p, nil | ||
| } | ||
|
|
||
| // Set the organization and project. Project may be empty. | ||
| p.OrganizationID = ident.Payload.Principal.Service.OrganizationID | ||
| p.ProjectID = ident.Payload.Principal.Service.ProjectID | ||
|
|
||
| // Save the profile. | ||
| if err := p.Write(); err != nil { | ||
| return nil, fmt.Errorf("failed to save default profile: %w", err) | ||
| } |
There was a problem hiding this comment.
⌛ This seems to be fairly similar to the above. Would it be an option to extract this into a function SetHierarchyBasedOnSP(...) (ok bool, err error) or such?
There was a problem hiding this comment.
Yeah they are close but it isn't that much code and they are different enough I thought duplication was better.
The main difference is login knows it has a valid token, and has to reconfigure the HTTP client, and the cmd.Main doesn't know if there is a token source yet and on error will just ignore vs bubbling up for the login case. The thought there is it lets you run logout / other commands to fix the problem.
Changes proposed in this PR:
If authenticating the CLI with a service principal and an empty profile, auto-detect the organization and project ID where possible. This enables use cases such as
HCP_CLIENT_ID=XXX HCP_CLIENT_SECRET=YYY hcp <command>to work even if no profile exists yet. This can be particularly useful when authenticating in oneshot environments such as a non-interactive run of the Docker container.How I've tested this PR:
Create a credential file:
Test one-shot authentication with docker:
Prior to this change, this would result in:
How I expect reviewers to test this PR:
Code review would be sufficient, but if you want to run the commands, check out the branch and follow the commands I showed in the description and similarly during
hcp auth loginChecklist: