-
Notifications
You must be signed in to change notification settings - Fork 442
Azure Stack Support #5532
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
base: main
Are you sure you want to change the base?
Azure Stack Support #5532
Changes from all commits
59750a5
11f3e63
bc9c9ea
69a52d6
db3664e
271a656
b64d4e6
36ca4a4
bdbe2bc
4605c9e
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 |
---|---|---|
|
@@ -48,6 +48,7 @@ type AzureClusterClassSpec struct { | |
// - GermanCloud: "AzureGermanCloud" | ||
// - PublicCloud: "AzurePublicCloud" | ||
// - USGovernmentCloud: "AzureUSGovernmentCloud" | ||
// - StackCloud: "HybridEnvironment" | ||
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. On further testing, it seems "AzureStackCloud" may indeed be required by some components. I will iron this out. 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. Should this be renamed to AzureStackCloud to keep it consistent with the other clouds, or is it required to be called "HybridCloud"? 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. "HybridCloud" is what is returned by the azure autorest package, but I believe it should be possible to allow users to set "AzureStackCloud" in the cluster spec, but we accept "HybridCloud" internally within the code. |
||
// | ||
// Note that values other than the default must also be accompanied by corresponding changes to the | ||
// aso-controller-settings Secret to configure ASO to refer to the non-Public cloud. ASO currently does | ||
|
@@ -77,6 +78,12 @@ type AzureClusterClassSpec struct { | |
// See: https://learn.microsoft.com/azure/reliability/availability-zones-overview | ||
// +optional | ||
FailureDomains clusterv1.FailureDomains `json:"failureDomains,omitempty"` | ||
|
||
// ARMEndpoint specifies a URL for the ARM Resource Manager endpoint. | ||
// It may only be specified when the AzureEnvironment is set to AzureStackCloud, | ||
// in which case it is required. | ||
// +optional | ||
ARMEndpoint string `json:"armEndpoint,omitempty"` | ||
} | ||
|
||
// AzureManagedControlPlaneClassSpec defines the AzureManagedControlPlane properties that may be shared across several azure managed control planes. | ||
|
@@ -186,6 +193,7 @@ type AzureManagedControlPlaneClassSpec struct { | |
// - PublicCloud: "AzurePublicCloud" | ||
// - USGovernmentCloud: "AzureUSGovernmentCloud" | ||
// | ||
// | ||
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. Is it intended to add AzureStack to the comment here as well? |
||
// Note that values other than the default must also be accompanied by corresponding changes to the | ||
// aso-controller-settings Secret to configure ASO to refer to the non-Public cloud. ASO currently does | ||
// not support referring to multiple different clouds in a single installation. The following fields must | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -27,6 +27,7 @@ | |||||
"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" | ||||||
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" | ||||||
"github.com/Azure/azure-sdk-for-go/sdk/tracing/azotel" | ||||||
"github.com/Azure/go-autorest/autorest/azure" | ||||||
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.
Suggested change
|
||||||
"go.opentelemetry.io/otel" | ||||||
|
||||||
"sigs.k8s.io/cluster-api-provider-azure/util/tele" | ||||||
|
@@ -44,6 +45,8 @@ | |||||
ChinaCloudName = "AzureChinaCloud" | ||||||
// USGovernmentCloudName is the name of the Azure US Government cloud. | ||||||
USGovernmentCloudName = "AzureUSGovernmentCloud" | ||||||
// StackCloudName is the name for Azure Stack hybrid cloud environments. | ||||||
StackCloudName = "HybridEnvironment" | ||||||
) | ||||||
|
||||||
const ( | ||||||
|
@@ -109,6 +112,16 @@ | |||||
CustomHeaderPrefix = "infrastructure.cluster.x-k8s.io/custom-header-" | ||||||
) | ||||||
|
||||||
const ( | ||||||
// StackAPIVersion is the API version profile to set for ARM clients. See: | ||||||
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.
Suggested change
|
||||||
// https://learn.microsoft.com/en-us/azure-stack/user/azure-stack-profiles-azure-resource-manager-versions?view=azs-2408#overview-of-the-2020-09-01-hybrid-profile | ||||||
StackAPIVersionProfile = "2020-06-01" | ||||||
|
||||||
// StackDiskAPIVersionProfile is the API Version to set for the disk client. | ||||||
// API Version Profile "2020-06-01" is not supported for disks. | ||||||
StackDiskAPIVersionProfile = "2018-06-01" | ||||||
) | ||||||
|
||||||
var ( | ||||||
// LinuxBootstrapExtensionCommand is the command the VM bootstrap extension will execute to verify Linux nodes bootstrap completes successfully. | ||||||
LinuxBootstrapExtensionCommand = fmt.Sprintf("for i in $(seq 1 %d); do test -f %s && break; if [ $i -eq %d ]; then exit 1; else sleep %d; fi; done", bootstrapExtensionRetries, bootstrapSentinelFile, bootstrapExtensionRetries, bootstrapExtensionSleep) | ||||||
|
@@ -357,7 +370,7 @@ | |||||
} | ||||||
|
||||||
// ARMClientOptions returns default ARM client options for CAPZ SDK v2 requests. | ||||||
func ARMClientOptions(azureEnvironment string, extraPolicies ...policy.Policy) (*arm.ClientOptions, error) { | ||||||
func ARMClientOptions(azureEnvironment, armEndpoint string, extraPolicies ...policy.Policy) (*arm.ClientOptions, error) { | ||||||
opts := &arm.ClientOptions{} | ||||||
|
||||||
switch azureEnvironment { | ||||||
|
@@ -367,6 +380,21 @@ | |||||
opts.Cloud = cloud.AzureChina | ||||||
case USGovernmentCloudName: | ||||||
opts.Cloud = cloud.AzureGovernment | ||||||
case StackCloudName: | ||||||
cloudEnv, err := azure.EnvironmentFromURL(armEndpoint) | ||||||
if err != nil { | ||||||
return nil, fmt.Errorf("unable to get Azure Stack cloud environment: %w", err) | ||||||
} | ||||||
opts.APIVersion = StackAPIVersionProfile | ||||||
opts.Cloud = cloud.Configuration{ | ||||||
ActiveDirectoryAuthorityHost: cloudEnv.ActiveDirectoryEndpoint, | ||||||
Services: map[cloud.ServiceName]cloud.ServiceConfiguration{ | ||||||
cloud.ResourceManager: { | ||||||
Audience: cloudEnv.TokenAudience, | ||||||
Endpoint: cloudEnv.ResourceManagerEndpoint, | ||||||
}, | ||||||
}, | ||||||
} | ||||||
Comment on lines
+383
to
+397
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. Can you add a unit test case for this in |
||||||
case "": | ||||||
// No cloud name provided, so leave at defaults. | ||||||
default: | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,12 @@ | |
return errors.As(err, &rerr) && rerr.StatusCode == http.StatusNotFound | ||
} | ||
|
||
// BadRequest parses an error to check if it its status code is Bad Request (400). | ||
func BadRequest(err error) bool { | ||
var rerr *azcore.ResponseError | ||
return errors.As(err, &rerr) && rerr.StatusCode == http.StatusBadRequest | ||
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. Can you add a simple unit test for this function in |
||
} | ||
|
||
// VMDeletedError is returned when a virtual machine is deleted outside of capz. | ||
type VMDeletedError struct { | ||
ProviderID string | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -81,12 +81,12 @@ | |||||
return base64.URLEncoding.EncodeToString(hasher.Sum(nil)) | ||||||
} | ||||||
|
||||||
func (c *AzureClients) setCredentialsWithProvider(ctx context.Context, subscriptionID, environmentName string, credentialsProvider CredentialsProvider) error { | ||||||
func (c *AzureClients) setCredentialsWithProvider(ctx context.Context, subscriptionID, environmentName, armEndpoint string, credentialsProvider CredentialsProvider) error { | ||||||
if credentialsProvider == nil { | ||||||
return fmt.Errorf("credentials provider cannot have an empty value") | ||||||
} | ||||||
|
||||||
settings, err := c.getSettingsFromEnvironment(environmentName) | ||||||
settings, err := c.getSettingsFromEnvironment(environmentName, armEndpoint) | ||||||
if err != nil { | ||||||
return err | ||||||
} | ||||||
|
@@ -121,7 +121,7 @@ | |||||
return err | ||||||
} | ||||||
|
||||||
func (c *AzureClients) getSettingsFromEnvironment(environmentName string) (s auth.EnvironmentSettings, err error) { | ||||||
func (c *AzureClients) getSettingsFromEnvironment(environmentName, armEndpoint string) (s auth.EnvironmentSettings, err error) { | ||||||
s = auth.EnvironmentSettings{ | ||||||
Values: map[string]string{}, | ||||||
} | ||||||
|
@@ -138,6 +138,8 @@ | |||||
setValue(s, "AZURE_AD_RESOURCE") | ||||||
if v := s.Values["AZURE_ENVIRONMENT"]; v == "" { | ||||||
s.Environment = azureautorest.PublicCloud | ||||||
} else if len(armEndpoint) > 0 { | ||||||
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.
Suggested change
|
||||||
s.Environment, err = azureautorest.EnvironmentFromURL(armEndpoint) | ||||||
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. Instead of adding an extra if check, is it possible to use |
||||||
} else { | ||||||
s.Environment, err = azureautorest.EnvironmentFromName(v) | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -150,7 +150,8 @@ | |||||
} | ||||||
|
||||||
m.cache.availabilitySetSKU, err = skuCache.Get(ctx, string(armcompute.AvailabilitySetSKUTypesAligned), resourceskus.AvailabilitySets) | ||||||
if err != nil { | ||||||
// Resource SKU API for availability sets may not be available in Azure Stack environments. | ||||||
if err != nil && !strings.EqualFold(m.CloudEnvironment(), "HybridEnvironment") { | ||||||
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.
Suggested change
|
||||||
return errors.Wrapf(err, "failed to get availability set SKU %s in compute api", string(armcompute.AvailabilitySetSKUTypesAligned)) | ||||||
} | ||||||
} | ||||||
|
@@ -494,12 +495,13 @@ | |||||
} | ||||||
|
||||||
spec := &availabilitysets.AvailabilitySetSpec{ | ||||||
Name: availabilitySetName, | ||||||
ResourceGroup: m.NodeResourceGroup(), | ||||||
ClusterName: m.ClusterName(), | ||||||
Location: m.Location(), | ||||||
SKU: nil, | ||||||
AdditionalTags: m.AdditionalTags(), | ||||||
Name: availabilitySetName, | ||||||
ResourceGroup: m.NodeResourceGroup(), | ||||||
ClusterName: m.ClusterName(), | ||||||
Location: m.Location(), | ||||||
CloudEnvironment: m.CloudEnvironment(), | ||||||
SKU: nil, | ||||||
AdditionalTags: m.AdditionalTags(), | ||||||
} | ||||||
|
||||||
if m.cache != nil { | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -19,24 +19,27 @@ | |||||
import ( | ||||||
"context" | ||||||
"strconv" | ||||||
"strings" | ||||||
|
||||||
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" | ||||||
"github.com/pkg/errors" | ||||||
"k8s.io/utils/ptr" | ||||||
|
||||||
infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" | ||||||
"sigs.k8s.io/cluster-api-provider-azure/azure" | ||||||
"sigs.k8s.io/cluster-api-provider-azure/azure/converters" | ||||||
"sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus" | ||||||
) | ||||||
|
||||||
// AvailabilitySetSpec defines the specification for an availability set. | ||||||
type AvailabilitySetSpec struct { | ||||||
Name string | ||||||
ResourceGroup string | ||||||
ClusterName string | ||||||
Location string | ||||||
SKU *resourceskus.SKU | ||||||
AdditionalTags infrav1.Tags | ||||||
Name string | ||||||
ResourceGroup string | ||||||
ClusterName string | ||||||
Location string | ||||||
CloudEnvironment string | ||||||
SKU *resourceskus.SKU | ||||||
AdditionalTags infrav1.Tags | ||||||
} | ||||||
|
||||||
// ResourceName returns the name of the availability set. | ||||||
|
@@ -64,20 +67,10 @@ | |||||
return nil, nil | ||||||
} | ||||||
|
||||||
if s.SKU == nil { | ||||||
return nil, errors.New("unable to get required availability set SKU from machine cache") | ||||||
} | ||||||
|
||||||
var faultDomainCount *int32 | ||||||
faultDomainCountStr, ok := s.SKU.GetCapability(resourceskus.MaximumPlatformFaultDomainCount) | ||||||
if !ok { | ||||||
return nil, errors.Errorf("unable to get required availability set SKU capability %s", resourceskus.MaximumPlatformFaultDomainCount) | ||||||
} | ||||||
count, err := strconv.ParseInt(faultDomainCountStr, 10, 32) | ||||||
faultDomainCount, err := getFaultDomainCount(s.SKU, s.CloudEnvironment) | ||||||
if err != nil { | ||||||
return nil, errors.Wrapf(err, "unable to parse availability set fault domain count") | ||||||
return nil, err | ||||||
} | ||||||
faultDomainCount = ptr.To[int32](int32(count)) | ||||||
|
||||||
asParams := armcompute.AvailabilitySet{ | ||||||
SKU: &armcompute.SKU{ | ||||||
|
@@ -98,3 +91,27 @@ | |||||
|
||||||
return asParams, nil | ||||||
} | ||||||
|
||||||
func getFaultDomainCount(SKU *resourceskus.SKU, cloudEnvironment string) (*int32, error) { | ||||||
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.
Suggested change
|
||||||
// Azure Stack environments may not implement the resource SKU API | ||||||
// for availability sets. Use a default value instead. | ||||||
if strings.EqualFold(cloudEnvironment, azure.StackCloudName) { | ||||||
return ptr.To(int32(2)), nil | ||||||
} | ||||||
Comment on lines
+98
to
+100
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. Can you add a unit test case for when cloud environment is Azure Stack to |
||||||
|
||||||
if SKU == nil { | ||||||
return nil, errors.New("unable to get required availability set SKU from machine cache") | ||||||
} | ||||||
|
||||||
var faultDomainCount *int32 | ||||||
faultDomainCountStr, ok := SKU.GetCapability(resourceskus.MaximumPlatformFaultDomainCount) | ||||||
if !ok { | ||||||
return nil, errors.Errorf("unable to get required availability set SKU capability %s", resourceskus.MaximumPlatformFaultDomainCount) | ||||||
} | ||||||
count, err := strconv.ParseInt(faultDomainCountStr, 10, 32) | ||||||
if err != nil { | ||||||
return nil, errors.Wrapf(err, "unable to parse availability set fault domain count") | ||||||
} | ||||||
faultDomainCount = ptr.To[int32](int32(count)) | ||||||
return faultDomainCount, nil | ||||||
} |
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.
Is Azure Stack officially supported? Autorest is a deprecated library. I don't see Azure Stack listed in the new Cloud library - https://github.com/Azure/azure-sdk-for-go/blob/d165f8083de58e12135bce2102205081eaade930/sdk/azcore/cloud/cloud.go#L10.
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.
For example, the GermanCloud isn't supported anymore.
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 know there is an issue too to try and get rid of the autorest library implementations in CAPZ - #2974
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.
Right, I discussed this a little in the issue #5201. AFAIK there is no available alternative.
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.
No idea whether it is still supported, although I'm pretty sure it is still sold.