Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HPR-1260] Support Project Level Service Principals Auth with HCP Packer #12520

Merged
merged 2 commits into from
Jul 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 96 additions & 46 deletions internal/hcp/api/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ package api
import (
"fmt"
"log"
"net/http"
"os"
"time"

"github.com/hashicorp/hcp-sdk-go/clients/cloud-packer-service/stable/2021-04-30/client/packer_service"
packerSvc "github.com/hashicorp/hcp-sdk-go/clients/cloud-packer-service/stable/2021-04-30/client/packer_service"
organizationSvc "github.com/hashicorp/hcp-sdk-go/clients/cloud-resource-manager/preview/2019-12-10/client/organization_service"
projectSvc "github.com/hashicorp/hcp-sdk-go/clients/cloud-resource-manager/preview/2019-12-10/client/project_service"
Expand Down Expand Up @@ -44,39 +46,69 @@ func NewClient() (*Client, error) {
}
}

cl, err := httpclient.New(httpclient.Config{
hcpClientCfg := httpclient.Config{
SourceChannel: fmt.Sprintf("packer/%s", version.PackerVersion.FormattedVersion()),
})
if err != nil {
}
if err := hcpClientCfg.Canonicalize(); err != nil {
return nil, &ClientError{
StatusCode: InvalidClientConfig,
Err: err,
}
}

cl, err := httpclient.New(hcpClientCfg)
if err != nil {
return nil, &ClientError{
StatusCode: InvalidClientConfig,
Err: err,
}
}
client := &Client{
Packer: packerSvc.New(cl, nil),
Organization: organizationSvc.New(cl, nil),
Project: projectSvc.New(cl, nil),
}
// A client.Config.hcpConfig is set when calling Canonicalize on basic HCP httpclient, as on line 52.
// If a user sets HCP_* env. variables they will be loaded into the client via the SDK and used for any client calls.
// For HCP_ORGANIZATION_ID and HCP_PROJECT_ID if they are both set via env. variables the call to hcpClientCfg.Connicalize()
// will automatically loaded them using the FromEnv configOption.
//
// If both values are set we should have all that we need to continue so we can returned the configured client.
if hcpClientCfg.Profile().OrganizationID != "" && hcpClientCfg.Profile().ProjectID != "" {
client.OrganizationID = hcpClientCfg.Profile().OrganizationID
client.ProjectID = hcpClientCfg.Profile().ProjectID

return client, nil
}

if err := client.loadOrganizationID(); err != nil {
return nil, &ClientError{
StatusCode: InvalidClientConfig,
Err: err,
if client.OrganizationID == "" {
err := client.loadOrganizationID()
if err != nil {
return nil, &ClientError{
StatusCode: InvalidClientConfig,
Err: err,
}
}
}
if err := client.loadProjectID(); err != nil {
return nil, &ClientError{
StatusCode: InvalidClientConfig,
Err: err,

if client.ProjectID == "" {
err := client.loadProjectID()
if err != nil {
return nil, &ClientError{
StatusCode: InvalidClientConfig,
Err: err,
}
}
}

return client, nil
}

func (c *Client) loadOrganizationID() error {
if env.HasOrganizationID() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding this check, doesn't that conflict with the comment left on line 71?

if both HCP_ORGANIZATION_ID and HCP_PROJECT_ID are set via env variables the hcpConfig may have all we need already.

If the client's Profile has them set already via the environment variable, is there a reason why we check for this here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the initialization code done in Canonicalize, I don't see where the org/project ID are loaded from the environment, is it possible that the Profile.OrganizationID or Profile.ProjectID are never set beforehand? If this is so, the check on line 72 seems to be always false, is my assumption correct?

Copy link
Contributor Author

@nywilken nywilken Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey great question! This took me for a little ride at first, as well, but if you take a deep dive into the Canonicalize call there is logic that builds the default UserProfile (Profile()) using the FromEnv configuration options. FromEnv will check the the local environment for a number of environment variables and set each one if the criteria for loading the env. variable is met. For some variables the rule is "If set use it" for others like HCP_ORGANIZATION_ID and HCP_PROJECT_ID they both must be set in order for the SDK to load them from the env. variable.

The SDK does not validate the values like we are doing for the Project ID but if the SDK logic changes in the future to do some validation and our Packer user sets both values we would benefit from the SDK doing the lifting. This is why I added the logic to check if set use. I'm checking the loading of the SDK because it may help us in the future when it comes to handling these vars consistently between products.

If the client's Profile has them set already via the environment variable, is there a reason why we check for this here?

As for this question the check is a guard clause if they are both set then we return and we don't check if they are set again. If only one is set then we have to check which is set and validate both accordingly.

Packer does not need the Organization ID set via a env. variable at this time. But I don't see any harm in adding it. In fact when I first wrote this code Packer loaded both but it was removed in favor of ListProjects and ListOrganization. Given the recent changes in service principles and how the platform is evolving my thinking is we should support the four main env variables until we can fully rely on the SDK for them.

Please let me know if that clears things up for you.

I will update the comment in the code to call out that they are only set by the SDK if both Project ID and Organization ID are set.

c.OrganizationID = os.Getenv(env.HCPOrganizationID)
return nil
}
// Get the organization ID.
listOrgParams := organizationSvc.NewOrganizationServiceListParams()
listOrgResp, err := c.Organization.OrganizationServiceList(listOrgParams, nil)
Expand All @@ -92,63 +124,81 @@ func (c *Client) loadOrganizationID() error {
}

func (c *Client) loadProjectID() error {
if env.HasProjectID() {
c.ProjectID = os.Getenv(env.HCPProjectID)
lbajolet-hashicorp marked this conversation as resolved.
Show resolved Hide resolved
err := c.ValidateRegistryForProject()
if err != nil {
return fmt.Errorf("project validation for id %q responded in error: %v", c.ProjectID, err)
}
return nil
}
// Get the project using the organization ID.
listProjParams := projectSvc.NewProjectServiceListParams()
listProjParams.ScopeID = &c.OrganizationID
scopeType := string(rmmodels.HashicorpCloudResourcemanagerResourceIDResourceTypeORGANIZATION)
listProjParams.ScopeType = &scopeType
listProjResp, err := c.Project.ProjectServiceList(listProjParams, nil)
if err != nil {
return fmt.Errorf("unable to fetch project id: %v", err)
}

if env.HasProjectID() {
proj, err := findProjectByID(os.Getenv(env.HCPProjectID), listProjResp.Payload.Projects)
if err != nil {
return err
}

c.ProjectID = proj.ID
} else {
if len(listProjResp.Payload.Projects) > 1 {
log.Printf("[WARNING] Multiple HCP projects found, will pick the oldest one by default\n" +
"To specify which project to use, set the HCP_PROJECT_ID environment variable to the one you want to use.")
if err != nil {
//For permission errors, our service principal may not have the ability
// to see all projects for an Org; this is the case for project-level service principals.
serviceErr, ok := err.(*projectSvc.ProjectServiceListDefault)
if !ok {
return fmt.Errorf("unable to fetch project list: %v", err)
}

proj, err := findOldestProject(listProjResp.Payload.Projects)
if err != nil {
return err
if serviceErr.Code() == http.StatusForbidden {
return fmt.Errorf("unable to fetch project\n\n"+
"If the provided credentials are tied to a specific project try setting the %s environment variable to one you want to use.", env.HCPProjectID)
}
}

c.ProjectID = proj.ID
if len(listProjResp.Payload.Projects) > 1 {
log.Printf("[WARNING] Multiple HCP projects found, will pick the oldest one by default\n"+
"To specify which project to use, set the %s environment variable to the one you want to use.", env.HCPProjectID)
}

proj, err := getOldestProject(listProjResp.Payload.Projects)
if err != nil {
return err
}
c.ProjectID = proj.ID
return nil
}

func findOldestProject(projs []*models.HashicorpCloudResourcemanagerProject) (*models.HashicorpCloudResourcemanagerProject, error) {
if len(projs) == 0 {
// getOldestProject retrieves the oldest project from a list based on its created_at time.
func getOldestProject(projects []*models.HashicorpCloudResourcemanagerProject) (*models.HashicorpCloudResourcemanagerProject, error) {
if len(projects) == 0 {
return nil, fmt.Errorf("no project found")
}

proj := projs[0]
for i := 1; i < len(projs); i++ {
nxtProj := projs[i]

if time.Time(nxtProj.CreatedAt).Before(time.Time(proj.CreatedAt)) {
proj = nxtProj
oldestTime := time.Now()
var oldestProj *models.HashicorpCloudResourcemanagerProject
for _, proj := range projects {
projTime := time.Time(proj.CreatedAt)
if projTime.Before(oldestTime) {
oldestProj = proj
oldestTime = projTime
}
}

return proj, nil
return oldestProj, nil
}

func findProjectByID(projID string, projs []*models.HashicorpCloudResourcemanagerProject) (*models.HashicorpCloudResourcemanagerProject, error) {
for _, proj := range projs {
if proj.ID == projID {
return proj, nil
}
// ValidateRegistryForProject validates that there is an active registry associated to the configured organization and project ids.
// A successful validation will result in a nil response. All other response represent an invalid registry error request or a registry not found error.
func (client *Client) ValidateRegistryForProject() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting and good idea!

params := packer_service.NewPackerServiceGetRegistryParams()
params.LocationOrganizationID = client.OrganizationID
params.LocationProjectID = client.ProjectID

resp, err := client.Packer.PackerServiceGetRegistry(params, nil)
if err != nil {
return err
}

if resp.GetPayload().Registry == nil {
return fmt.Errorf("No active HCP Packer registry was found for the organization %q and project %q", client.OrganizationID, client.ProjectID)
}

return nil, fmt.Errorf("No project %q found", projID)
return nil

}
87 changes: 2 additions & 85 deletions internal/hcp/api/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,90 +11,7 @@ import (
"github.com/hashicorp/hcp-sdk-go/clients/cloud-resource-manager/preview/2019-12-10/models"
)

func TestFindProjectID(t *testing.T) {
testcases := []struct {
Name string
ProjectID string
ProjectList []*models.HashicorpCloudResourcemanagerProject
ExpectProjectID string
ExpectErr bool
}{
{
"Only one project, project exists, success",
"test-project-exists",
[]*models.HashicorpCloudResourcemanagerProject{
{
ID: "test-project-exists",
},
},
"test-project-exists",
false,
},
{
"Multiple projects, project exists, success",
"test-project-exists",
[]*models.HashicorpCloudResourcemanagerProject{
{
ID: "other-project-exists",
},
{
ID: "test-project-exists",
},
},
"test-project-exists",
false,
},
{
"One project, no id match, fail",
"test-project-exists",
[]*models.HashicorpCloudResourcemanagerProject{
{
ID: "other-project-exists",
},
},
"",
true,
},
{
"Multiple projects, no id match, fail",
"test-project-exists",
[]*models.HashicorpCloudResourcemanagerProject{
{
ID: "other-project-exists",
},
{
ID: "yet-another-project-exists",
},
},
"",
true,
},
{
"No projects, no id match, fail",
"test-project-exists",
[]*models.HashicorpCloudResourcemanagerProject{},
"",
true,
},
}

for _, tt := range testcases {
t.Run(tt.Name, func(t *testing.T) {
proj, err := findProjectByID(tt.ProjectID, tt.ProjectList)
if (err != nil) != tt.ExpectErr {
t.Errorf("test findProjectByID, expected %t, got %t",
tt.ExpectErr,
err != nil)
}

if proj != nil && proj.ID != tt.ExpectProjectID {
t.Errorf("expected to select project %q, got %q", tt.ExpectProjectID, proj.ID)
}
})
}
}

func TestFindOldestProject(t *testing.T) {
func TestGetOldestProject(t *testing.T) {
testcases := []struct {
Name string
ProjectList []*models.HashicorpCloudResourcemanagerProject
Expand Down Expand Up @@ -151,7 +68,7 @@ func TestFindOldestProject(t *testing.T) {

for _, tt := range testcases {
t.Run(tt.Name, func(t *testing.T) {
proj, err := findOldestProject(tt.ProjectList)
proj, err := getOldestProject(tt.ProjectList)
if (err != nil) != tt.ExpectErr {
t.Errorf("test findProjectByID, expected %t, got %t",
tt.ExpectErr,
Expand Down
4 changes: 4 additions & 0 deletions internal/hcp/env/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ func HasProjectID() bool {
return hasEnvVar(HCPProjectID)
}

func HasOrganizationID() bool {
return hasEnvVar(HCPOrganizationID)
}

func HasClientID() bool {
return hasEnvVar(HCPClientID)
}
Expand Down
1 change: 1 addition & 0 deletions internal/hcp/env/variables.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const (
HCPClientID = "HCP_CLIENT_ID"
HCPClientSecret = "HCP_CLIENT_SECRET"
HCPProjectID = "HCP_PROJECT_ID"
HCPOrganizationID = "HCP_ORGANIZATION_ID"
HCPPackerRegistry = "HCP_PACKER_REGISTRY"
HCPPackerBucket = "HCP_PACKER_BUCKET_NAME"
HCPPackerBuildFingerprint = "HCP_PACKER_BUILD_FINGERPRINT"
Expand Down