Skip to content

Commit ca380e9

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #316 from theobarberbany/tb/OCPBUGS-25483
OCPBUGS-25483: Adds CloudConfigTransformer for Azure
2 parents 3b2a160 + cf0220e commit ca380e9

4 files changed

Lines changed: 292 additions & 13 deletions

File tree

‎pkg/cloud/azure/azure.go‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,20 @@ package azure
22

33
import (
44
"embed"
5+
"encoding/json"
56
"fmt"
7+
"slices"
8+
"strings"
69

710
"github.com/asaskevich/govalidator"
11+
configv1 "github.com/openshift/api/config/v1"
812
appsv1 "k8s.io/api/apps/v1"
13+
"k8s.io/apimachinery/pkg/util/validation/field"
914
"sigs.k8s.io/controller-runtime/pkg/client"
1015

16+
azureconsts "sigs.k8s.io/cloud-provider-azure/pkg/consts"
17+
azure "sigs.k8s.io/cloud-provider-azure/pkg/provider"
18+
1119
"github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/common"
1220
"github.com/openshift/cluster-cloud-controller-manager-operator/pkg/config"
1321
)
@@ -23,6 +31,25 @@ var (
2331
}
2432
)
2533

34+
var (
35+
validAzureCloudNames = map[configv1.AzureCloudEnvironment]struct{}{
36+
configv1.AzurePublicCloud: struct{}{},
37+
configv1.AzureUSGovernmentCloud: struct{}{},
38+
configv1.AzureChinaCloud: struct{}{},
39+
configv1.AzureGermanCloud: struct{}{},
40+
configv1.AzureStackCloud: struct{}{},
41+
}
42+
43+
validAzureCloudNameValues = func() []string {
44+
v := make([]string, 0, len(validAzureCloudNames))
45+
for n := range validAzureCloudNames {
46+
v = append(v, string(n))
47+
}
48+
slices.Sort(v)
49+
return v
50+
}()
51+
)
52+
2653
type imagesReference struct {
2754
CloudControllerManager string `valid:"required"`
2855
CloudControllerManagerOperator string `valid:"required"`
@@ -85,3 +112,70 @@ func NewProviderAssets(config config.OperatorConfig) (common.CloudProviderAssets
85112
}
86113
return assets, nil
87114
}
115+
116+
// IsAzure ensures that the underlying platform is Azure. It will fail if the
117+
// CloudName is AzureStack as we handle it separately with it's own
118+
// CloudConfigTransformer.
119+
func IsAzure(infra *configv1.Infrastructure) bool {
120+
if infra.Status.PlatformStatus != nil {
121+
if infra.Status.PlatformStatus.Type == configv1.AzurePlatformType &&
122+
(infra.Status.PlatformStatus.Azure.CloudName != configv1.AzureStackCloud) {
123+
return true
124+
}
125+
}
126+
return false
127+
}
128+
129+
func CloudConfigTransformer(source string, infra *configv1.Infrastructure, network *configv1.Network) (string, error) {
130+
if !IsAzure(infra) {
131+
return "", fmt.Errorf("invalid platform, expected CloudName to be %s", configv1.AzurePublicCloud)
132+
}
133+
134+
var cfg azure.Config
135+
if err := json.Unmarshal([]byte(source), &cfg); err != nil {
136+
return "", fmt.Errorf("failed to unmarshal the cloud.conf: %w", err)
137+
}
138+
139+
// We are copying the behaviour from CCO's transformer we need to:
140+
// 1. Ensure that the Cloud is set in the cloud.conf
141+
// i. If it is set, verify that it is valid and does not conflict with the
142+
// infrastructure config. If it conflicts, we want to error
143+
// ii. If it is not set, default to public cloud (configv1.AzurePublicCloud)
144+
//
145+
// 2. Verify the cloud name set in the infra config is valid, if it is not
146+
// bail with an informative error
147+
148+
// Verify the cloud name set in the infra config is valid
149+
cloud := configv1.AzurePublicCloud
150+
if azurePlatform := infra.Status.PlatformStatus.Azure; azurePlatform != nil {
151+
if c := azurePlatform.CloudName; c != "" {
152+
if _, ok := validAzureCloudNames[c]; !ok {
153+
return "", field.NotSupported(field.NewPath("status", "platformStatus", "azure", "cloudName"), c, validAzureCloudNameValues)
154+
}
155+
cloud = c
156+
}
157+
}
158+
159+
// Ensure cloud set in cloud.conf matches infra
160+
if cfg.Cloud != "" {
161+
if !strings.EqualFold(string(cloud), cfg.Cloud) {
162+
return "",
163+
fmt.Errorf(`invalid user-provided cloud.conf: \"cloud\" field in user-provided
164+
cloud.conf conflicts with infrastructure object`)
165+
}
166+
}
167+
cfg.Cloud = string(cloud)
168+
169+
// If the virtual machine type is not set we need to make sure it uses the
170+
// "standard" instance type. See OCPBUGS-25483 and OCPBUGS-20213 for more
171+
// information
172+
if cfg.VMType == "" {
173+
cfg.VMType = azureconsts.VMTypeStandard
174+
}
175+
176+
cfgbytes, err := json.Marshal(cfg)
177+
if err != nil {
178+
return "", fmt.Errorf("failed to marshal the cloud.conf: %w", err)
179+
}
180+
return string(cfgbytes), nil
181+
}

‎pkg/cloud/azure/azure_test.go‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,26 @@
11
package azure
22

33
import (
4+
"encoding/json"
5+
"fmt"
46
"testing"
57

8+
. "github.com/onsi/gomega"
69
configv1 "github.com/openshift/api/config/v1"
710
"github.com/stretchr/testify/assert"
11+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
12+
azure "sigs.k8s.io/cloud-provider-azure/pkg/provider"
13+
14+
ratelimitconfig "sigs.k8s.io/cloud-provider-azure/pkg/provider/config"
815

916
"github.com/openshift/cluster-cloud-controller-manager-operator/pkg/config"
1017
)
1118

19+
const (
20+
infraCloudConfName = "test-config"
21+
infraCloudConfKey = "foo"
22+
)
23+
1224
func TestResourcesRenderingSmoke(t *testing.T) {
1325

1426
tc := []struct {
@@ -79,3 +91,155 @@ func TestResourcesRenderingSmoke(t *testing.T) {
7991
})
8092
}
8193
}
94+
95+
func makeInfrastructureResource(platform configv1.PlatformType, cloudName configv1.AzureCloudEnvironment) *configv1.Infrastructure {
96+
cfg := configv1.Infrastructure{
97+
ObjectMeta: metav1.ObjectMeta{
98+
Name: "cluster",
99+
},
100+
Status: configv1.InfrastructureStatus{
101+
PlatformStatus: &configv1.PlatformStatus{
102+
Type: platform,
103+
},
104+
},
105+
Spec: configv1.InfrastructureSpec{
106+
CloudConfig: configv1.ConfigMapFileReference{
107+
Name: infraCloudConfName,
108+
Key: infraCloudConfKey,
109+
},
110+
PlatformSpec: configv1.PlatformSpec{
111+
Type: platform,
112+
},
113+
},
114+
}
115+
116+
if platform == configv1.AzurePlatformType {
117+
cfg.Status.PlatformStatus.Azure = &configv1.AzurePlatformStatus{
118+
CloudName: cloudName,
119+
}
120+
}
121+
122+
return &cfg
123+
}
124+
125+
// This test is a little complicated with all the JSON marshalling and
126+
// unmarshalling, but it is necessary due to the nature of how this data
127+
// is stored in Kuberenetes. The ConfigMaps containing the cloud config
128+
// will have string encoded JSON objects in them, due to the non-deterministic
129+
// natue of map object in Go we will need to examine the data instead of
130+
// comparing strings.
131+
func TestCloudConfigTransformer(t *testing.T) {
132+
tc := []struct {
133+
name string
134+
source azure.Config
135+
expected azure.Config
136+
infra *configv1.Infrastructure
137+
errMsg string
138+
}{
139+
{
140+
name: "Non Azure returns an error",
141+
source: azure.Config{},
142+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureStackCloud),
143+
errMsg: fmt.Sprintf("invalid platform, expected CloudName to be %s", configv1.AzurePublicCloud),
144+
},
145+
{
146+
name: "Azure sets the vmType to standard and cloud to AzurePublicCloud when neither is set",
147+
source: azure.Config{},
148+
expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}},
149+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud),
150+
},
151+
{
152+
name: "Azure doesn't modify vmType if user set",
153+
source: azure.Config{VMType: "vmss"},
154+
expected: azure.Config{VMType: "vmss", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}},
155+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud),
156+
},
157+
{
158+
name: "Azure sets the cloud to AzurePublicCloud and keeps existing fields",
159+
source: azure.Config{
160+
ResourceGroup: "test-rg",
161+
},
162+
expected: azure.Config{VMType: "standard", ResourceGroup: "test-rg", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}},
163+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud),
164+
},
165+
{
166+
name: "Azure keeps the cloud set to AzurePublicCloud",
167+
source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}},
168+
expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}},
169+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud),
170+
},
171+
{
172+
name: "Azure keeps the cloud set to US Gov cloud",
173+
source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureUSGovernmentCloud)}},
174+
expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureUSGovernmentCloud)}},
175+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureUSGovernmentCloud),
176+
},
177+
{
178+
name: "Azure keeps the cloud set to China cloud",
179+
source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureChinaCloud)}},
180+
expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureChinaCloud)}},
181+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureChinaCloud),
182+
},
183+
{
184+
name: "Azure keeps the cloud set to German cloud",
185+
source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureGermanCloud)}},
186+
expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureGermanCloud)}},
187+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureGermanCloud),
188+
},
189+
{
190+
name: "Azure throws an error if the infra has an invalid cloud",
191+
source: azure.Config{},
192+
infra: makeInfrastructureResource(configv1.AzurePlatformType, "AzureAnotherCloud"),
193+
errMsg: "status.platformStatus.azure.cloudName: Unsupported value: \"AzureAnotherCloud\": supported values: \"AzureChinaCloud\", \"AzureGermanCloud\", \"AzurePublicCloud\", \"AzureStackCloud\", \"AzureUSGovernmentCloud\"",
194+
},
195+
{
196+
name: "Azure keeps the cloud set in the source when there is not one set in infrastructure",
197+
source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}},
198+
expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}},
199+
infra: makeInfrastructureResource(configv1.AzurePlatformType, ""),
200+
},
201+
{
202+
name: "Azure sets the cloud to match the infrastructure if an empty string is provided in source",
203+
source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: ""}},
204+
expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}},
205+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud),
206+
},
207+
{
208+
name: "Azure sets the cloud to match the infrastructure if an empty string is provided in source and the infrastructure is non standard",
209+
source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: ""}},
210+
expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzureUSGovernmentCloud)}},
211+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureUSGovernmentCloud),
212+
},
213+
{
214+
name: "Azure returns an error if the source config conflicts with the infrastructure",
215+
source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}},
216+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzureUSGovernmentCloud),
217+
errMsg: "invalid user-provided cloud.conf: \\\"cloud\\\" field in user-provided\n\t\t\t\tcloud.conf conflicts with infrastructure object",
218+
},
219+
{
220+
name: "Azure keeps the cloud set to AzurePublicCloud if the source is upper case",
221+
source: azure.Config{AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: "AZUREPUBLICCLOUD"}},
222+
expected: azure.Config{VMType: "standard", AzureAuthConfig: ratelimitconfig.AzureAuthConfig{Cloud: string(configv1.AzurePublicCloud)}},
223+
infra: makeInfrastructureResource(configv1.AzurePlatformType, configv1.AzurePublicCloud),
224+
},
225+
}
226+
227+
for _, tc := range tc {
228+
t.Run(tc.name, func(t *testing.T) {
229+
g := NewWithT(t)
230+
231+
src, err := json.Marshal(tc.source)
232+
g.Expect(err).NotTo(HaveOccurred(), "Marshal of source data should succeed")
233+
234+
actual, err := CloudConfigTransformer(string(src), tc.infra, nil)
235+
if tc.errMsg != "" {
236+
g.Expect(err).Should(MatchError(tc.errMsg))
237+
g.Expect(actual).Should(Equal(""))
238+
} else {
239+
var observed azure.Config
240+
g.Expect(json.Unmarshal([]byte(actual), &observed)).To(Succeed(), "Unmarshal of observed data should succeed")
241+
g.Expect(observed).Should(Equal(tc.expected))
242+
}
243+
})
244+
}
245+
}

‎pkg/cloud/cloud.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ func GetCloudConfigTransformer(platformStatus *configv1.PlatformStatus) (cloudCo
5353
if azurestack.IsAzureStackHub(platformStatus) {
5454
return azurestack.CloudConfigTransformer, true, nil
5555
}
56-
return nil, true, nil
56+
return azure.CloudConfigTransformer, true, nil
5757
case configv1.GCPPlatformType:
5858
return common.NoOpTransformer, false, nil
5959
case configv1.IBMCloudPlatformType:

0 commit comments

Comments
 (0)