Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: Support Non-Azure Stack Custom Clouds with custom endpoints/root certs/sources.list #3063

Merged
merged 10 commits into from
Apr 16, 2020

Conversation

ericsuhong
Copy link
Contributor

@ericsuhong ericsuhong commented Apr 12, 2020

Reason for Change:
Currently, when CustomCloudProfile is specified, aks-engine treats it as Azure Stack and executes many Azure Stack-specific logic (i.e. loading azure stack specific image, setting lower arm api versions, disabling instance metadata, etc).

This makes impossible for users to specify custom cloud endpoints without triggering Azure Stack specific logic.

This PR decouples Azure Stack logic from Custom Cloud logic by introducing an internally computed property "IsCustomCloudProfile".

The main idea is to use “IsAzureStackCloud” property for doing any Azure Stack specific works, and use “IsCustomCloudProfile” property for generating actual azure.json and azurestackcloud.json files in VMs in the end.

I also added following two additional fields to support specifying custom root certificates and sources.list:

  1. "customCloudRootCertificates": comma-separated list of base64-encoded pem certificates, which will be installed to /usr/local/shared/ca-certificates folder
  2. "customCloudSourcesList": base64-encoded custom sources.list file. It will overwrite /etc/apt/sources.list (while taking its backup)

Sample customCloudProfile section:

"customCloudProfile": {
      "environment": {
        "name": "MyCustomCloud",
        "resourceManagerEndpoint": "https://management.customcloud/",
        "activeDirectoryEndpoint": "https://login.customcloud/",
        ...
        }
      },
      "customCloudRootCertificates": "LS0tLS1CRUdJTiBDRV...",
      "customCloudSourcesList": "ZGViIGh0dHBzOi8vcmVwb2RlcG90LmF6dXJlLmVhZ2..."
    },

Produces azure.json:

{
    "cloud":"AzureStackCloud",
    "tenantId": "...",
    "subscriptionId": "...",
    ...
}

and produces azurestackcloud.json:

{
    "name": "MyCustomCloud",
    "resourceManagerEndpoint": "https://management.customcloud/",
    "activeDirectoryEndpoint": "https://login.customcloud/",
    ...
}

Requirements:

Notes:

@ericsuhong
Copy link
Contributor Author

@devigned @jadarsie @honcao I closed old PR and created a new one. Would really appreciate if you can review this PR again.

pkg/api/types.go Outdated
@@ -2071,13 +2094,14 @@ func (p *Properties) GetKubernetesHyperkubeSpec() string {

// IsAzureStackCloud return true if the cloud is AzureStack
func (p *Properties) IsAzureStackCloud() bool {
return p.CustomCloudProfile != nil
// For backward compatibility, treat nil Environment and empty Environment name as AzureStackCloud as well
return p.CustomCloudProfile != nil && (p.CustomCloudProfile.Environment == nil || p.CustomCloudProfile.Environment.Name == "" || strings.EqualFold(p.CustomCloudProfile.Environment.Name, "AzureStackCloud"))
Copy link
Member

Choose a reason for hiding this comment

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

nit:

return p.IsCustomCloudProfile() instead of copying the p.CustomCloudProfile != nil implementation from the IsCustomCloudProfile method (i.e., let's just re-use the method)

@jadarsie do we want the p.CustomCloudProfile.Environment == nil back-compat foo for AzureStackCloud? It would be nice to have IsAzureStackCloud actually require that the Environment.Name string property be equal to "AzureStackCloud".

// IsAzureStackCloud return true if the cloud is AzureStack
func (p *Properties) IsAzureStackCloud() bool {
return p.CustomCloudProfile != nil
// For backward compatibility, treat nil Environment and empty Environment name as AzureStackCloud as well
return p.CustomCloudProfile != nil && (p.CustomCloudProfile.Environment == nil || p.CustomCloudProfile.Environment.Name == "" || strings.EqualFold(p.CustomCloudProfile.Environment.Name, "AzureStackCloud"))
Copy link
Member

Choose a reason for hiding this comment

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

same comments/questions in api/types.go as well

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov
Copy link

codecov bot commented Apr 13, 2020

Codecov Report

Merging #3063 into master will increase coverage by 0.26%.
The diff coverage is 79.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3063      +/-   ##
==========================================
+ Coverage   70.58%   70.85%   +0.26%     
==========================================
  Files         145      145              
  Lines       25219    25274      +55     
==========================================
+ Hits        17802    17907     +105     
+ Misses       6312     6251      -61     
- Partials     1105     1116      +11     
Impacted Files Coverage Δ
cmd/addpool.go 18.91% <0.00%> (ø)
cmd/scale.go 12.39% <0.00%> (ø)
cmd/upgrade.go 45.25% <0.00%> (ø)
pkg/engine/templates_generated.go 33.60% <ø> (+2.61%) ⬆️
pkg/api/vlabs/types.go 73.30% <27.27%> (-2.72%) ⬇️
cmd/deploy.go 62.61% <50.00%> (ø)
pkg/engine/engine.go 86.04% <50.00%> (-0.27%) ⬇️
pkg/api/types.go 93.98% <75.00%> (-0.16%) ⬇️
pkg/api/defaults-custom-cloud-profile.go 85.71% <87.83%> (-3.58%) ⬇️
pkg/api/addons.go 97.74% <100.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1612db...1d74e95. Read the comment docs.

@jackfrancis jackfrancis force-pushed the sukhong_customcloudsupportv3 branch from 553609e to 992835c Compare April 14, 2020 16:27
@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

jackfrancis
jackfrancis previously approved these changes Apr 14, 2020
Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

lgtm

@jadarsie for approval as well

Copy link
Member

@jadarsie jadarsie left a comment

Choose a reason for hiding this comment

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

So far so good, let me run e2e against Azure Stack

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jackfrancis jackfrancis force-pushed the sukhong_customcloudsupportv3 branch from e689d48 to 1d74e95 Compare April 16, 2020 00:16
@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jadarsie
Copy link
Member

/lgtm

@jackfrancis jackfrancis merged commit 4163ea6 into Azure:master Apr 16, 2020
alexeldeib pushed a commit to alexeldeib/aks-engine that referenced this pull request Apr 21, 2020
haofan-ms pushed a commit to haofan-ms/aks-engine that referenced this pull request Jul 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants