Skip to content

Commit 72a7022

Browse files
author
b-lnimmala
committed
PR Review fixes
1 parent 470d90f commit 72a7022

7 files changed

Lines changed: 251 additions & 23 deletions

File tree

‎pkg/frontend/encryptionathost_validation.go‎

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -15,32 +15,55 @@ import (
1515
"github.com/Azure/ARO-RP/pkg/util/azureclient"
1616
)
1717

18-
func validateEncryptionAtHostFeature(
19-
ctx context.Context,
20-
azEnv *azureclient.AROEnvironment,
21-
environment env.Interface,
22-
subscriptionID, tenantID string,
23-
) error {
24-
fpCred, err := environment.FPNewClientCertificateCredential(tenantID, nil)
25-
if err != nil {
26-
return err
18+
type FeaturesClient interface {
19+
Get(ctx context.Context, resourceProviderNamespace string, featureName string, options *armfeatures.ClientGetOptions) (armfeatures.ClientGetResponse, error)
20+
}
21+
22+
type FeaturesValidator interface {
23+
ValidateSubscriptionFeatures(ctx context.Context, azEnv *azureclient.AROEnvironment, environment env.Interface, subscriptionID, tenantID string, oc *api.OpenShiftCluster) error
24+
}
25+
26+
type featuresValidator struct{}
27+
28+
func (f featuresValidator) ValidateSubscriptionFeatures(ctx context.Context, azEnv *azureclient.AROEnvironment, environment env.Interface, subscriptionID, tenantID string, oc *api.OpenShiftCluster) error {
29+
var fieldPath string
30+
if oc.Properties.MasterProfile.EncryptionAtHost == api.EncryptionAtHostEnabled {
31+
fieldPath = "properties.masterProfile.encryptionAtHost"
32+
} else if oc.Properties.WorkerProfiles[0].EncryptionAtHost == api.EncryptionAtHostEnabled {
33+
fieldPath = "properties.workerProfiles[0].encryptionAtHost"
2734
}
2835

29-
featuresClient, err := armfeatures.NewClient(subscriptionID, fpCred, azEnv.ArmClientOptions())
30-
if err != nil {
31-
return err
36+
if fieldPath != "" {
37+
fpCred, err := environment.FPNewClientCertificateCredential(tenantID, nil)
38+
if err != nil {
39+
return err
40+
}
41+
42+
featuresClient, err := armfeatures.NewClient(subscriptionID, fpCred, azEnv.ArmClientOptions())
43+
if err != nil {
44+
return err
45+
}
46+
47+
return validateEncryptionAtHostFeature(ctx, featuresClient, subscriptionID, fieldPath)
3248
}
49+
return nil
50+
}
3351

52+
func validateEncryptionAtHostFeature(
53+
ctx context.Context,
54+
featuresClient FeaturesClient,
55+
subscriptionID, fieldPath string,
56+
) error {
3457
resp, err := featuresClient.Get(ctx, "Microsoft.Compute", "EncryptionAtHost", nil)
3558
if err != nil {
3659
return err
3760
}
3861

3962
if resp.Properties == nil || resp.Properties.State == nil {
4063
return api.NewCloudError(
41-
http.StatusBadRequest,
42-
api.CloudErrorCodeInvalidParameter,
43-
"encryptionAtHost",
64+
http.StatusInternalServerError,
65+
api.CloudErrorCodeInternalServerError,
66+
"",
4467
fmt.Sprintf(
4568
"Microsoft.Compute/EncryptionAtHost"+
4669
" feature has no state for"+
@@ -52,7 +75,7 @@ func validateEncryptionAtHostFeature(
5275
return api.NewCloudError(
5376
http.StatusBadRequest,
5477
api.CloudErrorCodeInvalidParameter,
55-
"encryptionAtHost",
78+
fieldPath,
5679
fmt.Sprintf(
5780
"Microsoft.Compute/EncryptionAtHost feature is not registered on subscription %s. "+
5881
"Please register the feature on your subscription before creating the cluster.",

‎pkg/frontend/encryptionathost_validation_test.go‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
package frontend
2+
3+
// Copyright (c) Microsoft Corporation.
4+
// Licensed under the Apache License 2.0.
5+
6+
import (
7+
"context"
8+
"errors"
9+
"testing"
10+
11+
"go.uber.org/mock/gomock"
12+
13+
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armfeatures"
14+
15+
mock_frontend "github.com/Azure/ARO-RP/pkg/util/mocks/frontend"
16+
"github.com/Azure/ARO-RP/pkg/util/pointerutils"
17+
)
18+
19+
func TestValidateEncryptionAtHostFeature(t *testing.T) {
20+
ctx := context.Background()
21+
22+
for _, tt := range []struct {
23+
name string
24+
fieldPath string
25+
mockFeatureState *string
26+
mockPropertiesNil bool
27+
mockStateNil bool
28+
mockGetErr error
29+
wantErr string
30+
}{
31+
{
32+
name: "feature is registered - should return nil",
33+
fieldPath: "properties.masterProfile.encryptionAtHost",
34+
mockFeatureState: pointerutils.ToPtr("Registered"),
35+
wantErr: "",
36+
},
37+
{
38+
name: "feature is not registered - should return 400 BadRequest",
39+
fieldPath: "properties.masterProfile.encryptionAtHost",
40+
mockFeatureState: pointerutils.ToPtr("NotRegistered"),
41+
wantErr: "400: InvalidParameter: properties.masterProfile.encryptionAtHost: Microsoft.Compute/EncryptionAtHost feature is not registered on subscription test-subscription. Please register the feature on your subscription before creating the cluster.",
42+
},
43+
{
44+
name: "resp.Properties is nil - should return internal server error",
45+
fieldPath: "properties.workerProfiles[0].encryptionAtHost",
46+
mockPropertiesNil: true,
47+
wantErr: "500: InternalServerError: : Microsoft.Compute/EncryptionAtHost feature has no state for subscription test-subscription.",
48+
},
49+
{
50+
name: "resp.Properties.State is nil - should return internal server error",
51+
fieldPath: "properties.masterProfile.encryptionAtHost",
52+
mockStateNil: true,
53+
wantErr: "500: InternalServerError: : Microsoft.Compute/EncryptionAtHost feature has no state for subscription test-subscription.",
54+
},
55+
{
56+
name: "Get returns error",
57+
fieldPath: "properties.masterProfile.encryptionAtHost",
58+
mockGetErr: errors.New("random error"),
59+
wantErr: "random error",
60+
},
61+
} {
62+
t.Run(tt.name, func(t *testing.T) {
63+
controller := gomock.NewController(t)
64+
defer controller.Finish()
65+
66+
featuresClient := mock_frontend.NewMockFeaturesClient(controller)
67+
68+
var mockResponse armfeatures.ClientGetResponse
69+
if tt.mockPropertiesNil {
70+
mockResponse = armfeatures.ClientGetResponse{
71+
FeatureResult: armfeatures.FeatureResult{
72+
Properties: nil,
73+
},
74+
}
75+
} else if tt.mockStateNil {
76+
mockResponse = armfeatures.ClientGetResponse{
77+
FeatureResult: armfeatures.FeatureResult{
78+
Properties: &armfeatures.FeatureProperties{
79+
State: nil,
80+
},
81+
},
82+
}
83+
} else if tt.mockFeatureState != nil {
84+
mockResponse = armfeatures.ClientGetResponse{
85+
FeatureResult: armfeatures.FeatureResult{
86+
Properties: &armfeatures.FeatureProperties{
87+
State: tt.mockFeatureState,
88+
},
89+
},
90+
}
91+
}
92+
93+
featuresClient.EXPECT().
94+
Get(ctx, "Microsoft.Compute", "EncryptionAtHost", nil).
95+
Return(mockResponse, tt.mockGetErr)
96+
97+
err := validateEncryptionAtHostFeature(ctx, featuresClient, "test-subscription", tt.fieldPath)
98+
if err != nil && err.Error() != tt.wantErr ||
99+
err == nil && tt.wantErr != "" {
100+
t.Errorf("expected error %q, got %q", tt.wantErr, err)
101+
}
102+
})
103+
}
104+
}

‎pkg/frontend/frontend.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ type frontend struct {
101101
skuValidator SkuValidator
102102
quotaValidator QuotaValidator
103103
providersValidator ProvidersValidator
104+
featuresValidator FeaturesValidator
104105

105106
clusterEnricher clusterdata.BestEffortEnricher
106107

@@ -185,6 +186,7 @@ func NewFrontend(ctx context.Context,
185186
quotaValidator: quotaValidator{},
186187
skuValidator: skuValidator{},
187188
providersValidator: providersValidator{},
189+
featuresValidator: featuresValidator{},
188190

189191
clusterEnricher: enricher,
190192

‎pkg/frontend/generate.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,4 +7,5 @@ package frontend
77
//go:generate mockgen -source quota_validation.go -destination=../util/mocks/$GOPACKAGE/quota_validation.go github.com/Azure/ARO-RP/pkg/frontend QuotaValidator
88
//go:generate mockgen -source providers_validation.go -destination=../util/mocks/$GOPACKAGE/providers_validation.go github.com/Azure/ARO-RP/pkg/frontend ProvidersValidator
99
//go:generate mockgen -source sku_validation.go -destination=../util/mocks/$GOPACKAGE/sku_validation.go github.com/Azure/ARO-RP/pkg/frontend SkuValidator
10+
//go:generate mockgen -source encryptionathost_validation.go -destination=../util/mocks/$GOPACKAGE/encryptionathost_validation.go github.com/Azure/ARO-RP/pkg/frontend FeaturesClient,FeaturesValidator
1011
//go:generate mockgen -source adminreplies.go -destination=../util/mocks/$GOPACKAGE/adminreplies.go github.com/Azure/ARO-RP/pkg/frontend StreamResponder

‎pkg/frontend/openshiftcluster_putorpatch.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -455,6 +455,11 @@ func (f *frontend) ValidateNewCluster(ctx context.Context, subscription *api.Sub
455455
return err
456456
}
457457

458+
err = f.featuresValidator.ValidateSubscriptionFeatures(ctx, f.env.Environment(), f.env, subscription.ID, subscription.Subscription.Properties.TenantID, cluster)
459+
if err != nil {
460+
return err
461+
}
462+
458463
err = f.quotaValidator.ValidateQuota(ctx, f.env.Environment(), f.env, subscription.ID, subscription.Subscription.Properties.TenantID, cluster)
459464
if err != nil {
460465
return err

‎pkg/frontend/sku_validation.go‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,6 @@ func (s skuValidator) ValidateVMSku(ctx context.Context, azEnv *azureclient.AROE
3434
return err
3535
}
3636

37-
if oc.Properties.MasterProfile.EncryptionAtHost == api.EncryptionAtHostEnabled {
38-
err = validateEncryptionAtHostFeature(ctx, azEnv, environment, subscriptionID, tenantID)
39-
if err != nil {
40-
return err
41-
}
42-
}
43-
4437
return validateVMSku(ctx, oc, armResourceSKUsClient)
4538
}
4639

‎pkg/util/mocks/frontend/encryptionathost_validation.go‎

Lines changed: 100 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)