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

azure: allow installer to consume AZURE_AUTH_LOCATION env for credentials file #1785

Merged

Conversation

abhinavdahiya
Copy link
Contributor

Installer uses NewAuthorizerFromFileWithResource here, which uses GetSettingsFromFile here to locate and load the file with auth credentials.
GetSettingsFromFile here uses the AZURE_AUTH_LOCATION env here to locate the file with no way to override or specify explicitly.

Currently the installer uses the hard-coded location ~/.azure/osServicePrincipal.json to load the credentials. But for CI, it would be important to override this
location to another location like we do for AWS here. So this change allows users to set AZURE_AUTH_LOCATION env to provider installer custom location to auth file.

/cc @openshift/installer

@openshift-ci-robot openshift-ci-robot requested a review from a team May 23, 2019 16:32
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 23, 2019
@abhinavdahiya
Copy link
Contributor Author

:P the installer github team has incomplete list.

/cc @jstuever @wking

@trown
Copy link

trown commented May 23, 2019

/test e2e-openstack

1 similar comment
@trown
Copy link

trown commented May 23, 2019

/test e2e-openstack

@abhinavdahiya
Copy link
Contributor Author

Need this PR to get CI rolling,
https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/openshift_release/3877/rehearse-3877-pull-ci-openshift-installer-master-e2e-azure/4#operator-run-template-e2e-azure---e2e-azure-container-setup

level=fatal msg="failed to fetch Terraform Variables: failed to fetch dependency of \"Terraform Variables\": failed to fetch dependency of \"Bootstrap Ignition Config\": failed to fetch dependency of \"Master Machines\": failed to generate asset \"Platform Credentials Check\": creating Azure session: failed to retrieve credentials from user: EOF"
�[0G�[2K�[1;92m? �[1;99mazure subscription id [? for help] �[?25l�7�[999;999f�[6n�[6n

ping @jstuever @wking

@abhinavdahiya
Copy link
Contributor Author

/retest

@@ -16,15 +16,15 @@ import (

const azureAuthEnv = "AZURE_AUTH_LOCATION"

var authFilePath = os.Getenv("HOME") + "/.azure/osServicePrincipal.json"
var defaultAuthFilePath = os.Getenv("HOME") + "/.azure/osServicePrincipal.json"
Copy link
Member

Choose a reason for hiding this comment

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

I prefer filePath.Join so we aren't so far off when we get around to compiling for Windows. But whatever, the + concat approach is what we had before this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed with 4bff9ab...0e9453e

os.Setenv(azureAuthEnv, authFilePath)
return newSessionFromFile()
authFile := defaultAuthFilePath
if f, ok := os.LookupEnv(azureAuthEnv); ok && len(f) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

If you're going to gate on length anyway, you might as well just use os.Getenv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed with 4bff9ab...0e9453e

Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

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

Two minor nits, I'm ok landing this with either or none of them addressed, but wanted to give you time before a /lgtm in case you want to address them. If you're fine as things stand, just pull the hold:

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels May 24, 2019
…ntials file

Installer uses `NewAuthorizerFromFileWithResource` [1], which uses `GetSettingsFromFile` [2] to locate and load the file with auth credentials.
`GetSettingsFromFile` [2] uses the `AZURE_AUTH_LOCATION` env [3] to locate the file with no way to override or specify explicitly.

Currently the installer uses the hard-coded location `~/.azure/osServicePrincipal.json` to load the credentials. But for CI, it would be important to override this
location to another location like we do for AWS [4]. So this change allows users to set `AZURE_AUTH_LOCATION` env to provider installer custom location to auth file.

[1]: https://github.com/Azure/go-autorest/blob/v12.0.0/autorest/azure/auth/auth.go#L243
[2]: https://github.com/Azure/go-autorest/blob/v12.0.0/autorest/azure/auth/auth.go#L287
[3]: https://github.com/Azure/go-autorest/blob/v12.0.0/autorest/azure/auth/auth.go#L289
[4]: https://github.com/openshift/release/blob/6c0b409639d6dcd074238e5396cddcc5c4da1510/ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml#L373-L374
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 24, 2019
@abhinavdahiya
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 24, 2019
@wking
Copy link
Member

wking commented May 25, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 25, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 5f631ff into openshift:master May 25, 2019
@openshift-ci-robot
Copy link
Contributor

@abhinavdahiya: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 0e9453e link /test e2e-aws-scaleup-rhel7

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. platform/azure size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants