-
Notifications
You must be signed in to change notification settings - Fork 430
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
✨ automatically generate azure.json #802
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,11 +17,12 @@ limitations under the License. | |
package scope | ||
|
||
import ( | ||
"os" | ||
"fmt" | ||
"strings" | ||
|
||
"github.com/Azure/go-autorest/autorest" | ||
"github.com/Azure/go-autorest/autorest/azure" | ||
"github.com/Azure/go-autorest/autorest/azure/auth" | ||
"github.com/pkg/errors" | ||
) | ||
|
||
const ( | ||
|
@@ -37,40 +38,73 @@ const ( | |
|
||
// AzureClients contains all the Azure clients used by the scopes. | ||
type AzureClients struct { | ||
SubscriptionID string | ||
Authorizer autorest.Authorizer | ||
environment string | ||
ResourceManagerEndpoint string | ||
ResourceManagerVMDNSSuffix string | ||
Authorizer autorest.Authorizer | ||
subscriptionID string | ||
tenantID string | ||
clientID string | ||
clientSecret string | ||
} | ||
|
||
// CloudEnvironment returns the Azure environment the controller runs in. | ||
func (c *AzureClients) CloudEnvironment() string { | ||
return c.environment | ||
} | ||
|
||
// SubscriptionID returns the Azure subscription id from the controller environment | ||
func (c *AzureClients) SubscriptionID() string { | ||
return c.subscriptionID | ||
} | ||
|
||
// TenantID returns the Azure tenant id the controller runs in. | ||
func (c *AzureClients) TenantID() string { | ||
return c.tenantID | ||
} | ||
|
||
// ClientID returns the Azure client id from the controller environment | ||
func (c *AzureClients) ClientID() string { | ||
return c.clientID | ||
} | ||
|
||
// ClientSecret returns the Azure client secret from the controller environment | ||
func (c *AzureClients) ClientSecret() string { | ||
return c.clientSecret | ||
} | ||
|
||
func (c *AzureClients) setCredentials(subscriptionID string) error { | ||
subID, err := getSubscriptionID(subscriptionID) | ||
if err != nil { | ||
return err | ||
} | ||
c.SubscriptionID = subID | ||
settings, err := auth.GetSettingsFromEnvironment() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if subscriptionID == "" { | ||
subscriptionID = settings.GetSubscriptionID() | ||
if subscriptionID == "" { | ||
return fmt.Errorf("error creating azure services. subscriptionID is not set in cluster or AZURE_SUBSCRIPTION_ID env var") | ||
} | ||
} | ||
|
||
c.subscriptionID = subscriptionID | ||
c.tenantID = strings.TrimSuffix(settings.Values[auth.TenantID], "\n") | ||
c.clientID = strings.TrimSuffix(settings.Values[auth.ClientID], "\n") | ||
c.clientSecret = strings.TrimSuffix(settings.Values[auth.ClientSecret], "\n") | ||
|
||
c.environment = settings.Values[auth.EnvironmentName] | ||
if c.environment == "" { | ||
c.environment = azure.PublicCloud.Name | ||
} | ||
|
||
c.ResourceManagerEndpoint = settings.Environment.ResourceManagerEndpoint | ||
c.ResourceManagerVMDNSSuffix = GetAzureDNSZoneForEnvironment(settings.Environment.Name) | ||
settings.Values[auth.SubscriptionID] = subscriptionID | ||
settings.Values[auth.TenantID] = c.tenantID | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this set here? Doesn't the value of c.tenantID come from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you need to re-apply it into settings so we should split all the env settings and credentials code up. With cluster describer we'll mostly split out subscription/credentials, should make sure we have a nice place for tenant ID (even if it's just the env for now) |
||
|
||
c.Authorizer, err = settings.GetAuthorizer() | ||
return err | ||
} | ||
|
||
func getSubscriptionID(subscriptionID string) (string, error) { | ||
if subscriptionID != "" { | ||
return subscriptionID, nil | ||
} | ||
subscriptionID = os.Getenv("AZURE_SUBSCRIPTION_ID") | ||
if subscriptionID == "" { | ||
return "", errors.New("error creating azure services. Environment variable AZURE_SUBSCRIPTION_ID is not set") | ||
} | ||
return subscriptionID, nil | ||
} | ||
|
||
// GetAzureDNSZoneForEnvironment returnes the DNSZone to be used with the | ||
// cloud environment, the default is the public cloud | ||
func GetAzureDNSZoneForEnvironment(environmentName string) string { | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could use a rename and maybe some refactoring, the setCredentials func is a bit funky to me. out of scope for this pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah also SubscriptionID should follow the same pattern as
tenantID
etc., ie.and remove the SubscriptionID() func in cluster.go on scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the flip actually, sub isn't necessary for auth. remove it here and retrieve it from the cluster/control plane (clusterdescriber) spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(still not in this PR though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nvm, I see what you mean