Skip to content

Commit

Permalink
remove AWS region from helm chart (#5567)
Browse files Browse the repository at this point in the history
# Description
This PR removes AWS region from helm charts and sets the right region
for AWS requests made by cloudcontrol and cloud formation clients based
on the region indicated by the request URL.

## Issue reference
Fixes: #5493

## Checklist

Please make sure you've completed the relevant tasks for this PR, out of
the following list:

* [ X] Code compiles correctly
* [ X] Adds necessary unit tests for change
* [ X] Adds necessary E2E tests for change
* [ X] Unit tests passing
* [ NA] Extended the documentation / Created issue for it
  • Loading branch information
nithyatsu authored May 22, 2023
1 parent 87e451d commit d8cce54
Show file tree
Hide file tree
Showing 28 changed files with 281 additions and 66 deletions.
2 changes: 1 addition & 1 deletion cmd/ucpd/ucp-self-hosted-dev.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ queueProvider:

profilerProvider:
enabled: true
port: 6060
port: 6061

#Default planes configuration with which ucp starts
# TODO: Remove azure and aws planes once rad provider commands are supported
Expand Down
4 changes: 0 additions & 4 deletions deploy/Chart/charts/ucp/templates/ucp.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,6 @@ spec:
value: '/var/tls/cert'
- name: PORT
value: '9443'
{{ if and .Values.provider .Values.provider.aws }}
- name: AWS_REGION
value: {{ .Values.provider.aws.region }}
{{ end }}
name: ucp
ports:
- containerPort: 9443
Expand Down
4 changes: 0 additions & 4 deletions deploy/Chart/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,6 @@ de:
ucp:
image: radius.azurecr.io/ucpd
tag: latest
# provider can be also set using rad env init --provider-aws
# provider:
# aws:
# region:"us-east"
rp:
image: radius.azurecr.io/appcore-rp
tag: latest
Expand Down
32 changes: 32 additions & 0 deletions pkg/ucp/frontend/controller/awsproxy/awsparsing.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import (
"net/http"
"strings"

v1 "github.com/project-radius/radius/pkg/armrpc/api/v1"
armrpc_rest "github.com/project-radius/radius/pkg/armrpc/rest"
awsoperations "github.com/project-radius/radius/pkg/aws/operations"
awsclient "github.com/project-radius/radius/pkg/ucp/aws"
"github.com/project-radius/radius/pkg/ucp/resources"
Expand Down Expand Up @@ -99,3 +101,33 @@ func computeResourceID(id resources.ID, resourceID string) string {
computedID := strings.Split(id.String(), "/:")[0] + resources.SegmentSeparator + resourceID
return computedID
}

// Extract Region from a URI like /apis/api.ucp.dev/v1alpha3/planes/aws/aws/accounts/817312594854/regions/us-west-2/providers/...
func readRegionFromRequest(path string, basePath string) (string, armrpc_rest.Response) {
path = strings.TrimPrefix(path, basePath)
resourceID, err := resources.Parse(path)
if err != nil {
errResponse := v1.ErrorResponse{
Error: v1.ErrorDetails{
Code: v1.CodeInvalid,
Message: "failed to read region from request path: invalid path",
},
}

response := armrpc_rest.NewBadRequestARMResponse(errResponse)
return "", response
}
region := resourceID.FindScope(resources.RegionsSegment)
if region == "" {
errResponse := v1.ErrorResponse{
Error: v1.ErrorDetails{
Code: v1.CodeInvalid,
Message: "failed to read region from request path: 'regions' not found",
},
}
response := armrpc_rest.NewBadRequestARMResponse(errResponse)
return "", response

}
return region, nil
}
15 changes: 15 additions & 0 deletions pkg/ucp/frontend/controller/awsproxy/awsparsing_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,18 @@ func TestGetPrimaryIdentifiersFromSchema_PrimaryIdentifierWrongDataType(t *testi
require.Nil(t, primaryIdentifiers)
require.EqualError(t, err, "primaryIdentifier is not an array")
}

func Test_ExtractRegionFromURLPath_Invalid(t *testing.T) {
URLPath := "/planes/deployments/local/resourcegroups/localrp/providers/Microsoft.Resources/deployments/rad-deploy-06221c5e-104d-4472-bb74-876b441c7663"
region, errResponse := readRegionFromRequest(URLPath, "/apis/api.ucp.dev/v1alpha3")
require.NotNil(t, errResponse)
require.Empty(t, region)

}

func Test_ExtractRegionFromURLPath_Valid(t *testing.T) {
URLPath := "/apis/api.ucp.dev/v1alpha3/planes/aws/aws/accounts/817312594854/regions/us-west-2/providers/AWS.S3/Bucket/:put?api-version=default"
region, errResponse := readRegionFromRequest(URLPath, "/apis/api.ucp.dev/v1alpha3")
require.Nil(t, errResponse)
require.Equal(t, "us-west-2", region)
}
32 changes: 32 additions & 0 deletions pkg/ucp/frontend/controller/awsproxy/awsproxytest.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,16 @@ func CreateKinesisStreamTestResource(resourceName string) *AWSTestResource {
return CreateAWSTestResource(resourceType, awsResourceType, resourceName, provider, arn, typeSchema)
}

func CreateKinesisStreamTestResourceWithInvalidRegion(resourceName string) *AWSTestResource {
resourceType := AWSKinesisStreamResourceType
awsResourceType := AWSKinesisStreamAWSResourceType
provider := "AWS.Kinesis"
arn := fmt.Sprintf("arn:aws:kinesis:us-west-2:123456789012:stream:%s", resourceName)
typeSchema := getMockKinesisStreamResourceTypeSchema()

return CreateAWSTestResourceWithInvalidRegion(resourceType, awsResourceType, resourceName, provider, arn, typeSchema)
}

func CreateMemoryDBClusterTestResource(resourceName string) *AWSTestResource {
resourceType := AWSMemoryDBClusterResourceType
awsResourceType := AWSMemoryDBClusterAWSResourceType
Expand Down Expand Up @@ -114,6 +124,28 @@ func CreateAWSTestResource(resourceType, awsResourceType, resourceName, provider
}
}

func CreateAWSTestResourceWithInvalidRegion(resourceType, awsResourceType, resourceName, provider, arn string, typeSchema map[string]any) *AWSTestResource {
serialized, err := json.Marshal(typeSchema)
if err != nil {
return nil
}
schema := string(serialized)

return &AWSTestResource{
ResourceType: resourceType,
AWSResourceType: awsResourceType,
ResourceName: resourceName,
ARN: arn,
CollectionPath: fmt.Sprintf("/planes/aws/aws/accounts/1234567/providers/%s", resourceType),
SingleResourcePath: fmt.Sprintf("/planes/aws/aws/accounts/1234567/providers/%s/%s", resourceType, resourceName),
OperationResultsPath: fmt.Sprintf("/planes/aws/aws/accounts/1234567/providers/%s/locations/us-west-2/operationResults/1234567", resourceType),
OperationStatusesPath: fmt.Sprintf("/planes/aws/aws/accounts/1234567/providers/%s/locations/us-west-2/operationStatuses/2345678", resourceType),
LocationHeader: fmt.Sprintf("http://localhost:5000/planes/aws/aws/accounts/1234567/providers/%s/locations/global/operationResults/79b9f0da-4882-4dc8-a367-6fd3bc122ded", provider),
AzureAsyncOpHeader: fmt.Sprintf("http://localhost:5000/planes/aws/aws/accounts/1234567/providers/%s/locations/global/operationStatuses/79b9f0da-4882-4dc8-a367-6fd3bc122ded", provider),
Schema: schema,
}
}

func getMockKinesisStreamResourceTypeSchema() map[string]any {
return map[string]any{
"primaryIdentifier": []any{
Expand Down
22 changes: 13 additions & 9 deletions pkg/ucp/frontend/controller/awsproxy/createorupdateawsresource.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ func (p *CreateOrUpdateAWSResource) Run(ctx context.Context, w http.ResponseWrit

decoder := json.NewDecoder(req.Body)
defer req.Body.Close()
region, errResponse := readRegionFromRequest(req.URL.Path, p.basePath)
if errResponse != nil {
return errResponse, nil
}

body := map[string]any{}
err := decoder.Decode(&body)
Expand All @@ -62,10 +66,7 @@ func (p *CreateOrUpdateAWSResource) Run(ctx context.Context, w http.ResponseWrit
}

response := armrpc_rest.NewBadRequestARMResponse(e)
err = response.Apply(ctx, w, req)
if err != nil {
return nil, err
}
return response, nil
}

properties := map[string]any{}
Expand All @@ -77,14 +78,16 @@ func (p *CreateOrUpdateAWSResource) Run(ctx context.Context, w http.ResponseWrit
}
}

cloudControlOpts := []func(*cloudcontrol.Options){CloudControlRegionOption(region)}
cloudFormationOpts := []func(*cloudformation.Options){CloudFormationWithRegionOption(region)}

// Create and update work differently for AWS - we need to know if the resource
// we're working on exists already.

existing := true
getResponse, err := p.awsOptions.AWSCloudControlClient.GetResource(ctx, &cloudcontrol.GetResourceInput{
TypeName: to.Ptr(serviceCtx.ResourceTypeInAWSFormat()),
Identifier: aws.String(serviceCtx.ResourceID.Name()),
})
}, cloudControlOpts...)
if awserror.IsAWSResourceNotFoundError(err) {
existing = false
} else if err != nil {
Expand Down Expand Up @@ -114,10 +117,11 @@ func (p *CreateOrUpdateAWSResource) Run(ctx context.Context, w http.ResponseWrit

if existing {
// Get resource type schema

describeTypeOutput, err := p.awsOptions.AWSCloudFormationClient.DescribeType(ctx, &cloudformation.DescribeTypeInput{
Type: types.RegistryTypeResource,
TypeName: to.Ptr(serviceCtx.ResourceTypeInAWSFormat()),
})
}, cloudFormationOpts...)
if err != nil {
return nil, err
}
Expand All @@ -141,7 +145,7 @@ func (p *CreateOrUpdateAWSResource) Run(ctx context.Context, w http.ResponseWrit
TypeName: to.Ptr(serviceCtx.ResourceTypeInAWSFormat()),
Identifier: aws.String(serviceCtx.ResourceID.Name()),
PatchDocument: aws.String(string(marshaled)),
})
}, cloudControlOpts...)
if err != nil {
return awserror.HandleAWSError(err)
}
Expand All @@ -168,7 +172,7 @@ func (p *CreateOrUpdateAWSResource) Run(ctx context.Context, w http.ResponseWrit
response, err := p.awsOptions.AWSCloudControlClient.CreateResource(ctx, &cloudcontrol.CreateResourceInput{
TypeName: to.Ptr(serviceCtx.ResourceTypeInAWSFormat()),
DesiredState: aws.String(string(desiredState)),
})
}, cloudControlOpts...)
if err != nil {
return awserror.HandleAWSError(err)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,66 @@ func Test_CreateAWSResource(t *testing.T) {
require.Equal(t, expectedResponseObject, actualResponseObject)
}

func Test_CreateAWSResourceInvalidRegion(t *testing.T) {
testResource := CreateKinesisStreamTestResourceWithInvalidRegion(uuid.NewString())
testOptions := setupTest(t)

requestBody := map[string]any{
"properties": map[string]any{
"RetentionPeriodHours": 178,
"ShardCount": 3,
},
}
requestBodyBytes, err := json.Marshal(requestBody)
require.NoError(t, err)

awsController, err := NewCreateOrUpdateAWSResource(ctrl.Options{
AWSOptions: ctrl.AWSOptions{
AWSCloudControlClient: testOptions.AWSCloudControlClient,
AWSCloudFormationClient: testOptions.AWSCloudFormationClient,
},
Options: armrpc_controller.Options{
StorageClient: testOptions.StorageClient,
},
})
require.NoError(t, err)

request, err := http.NewRequest(http.MethodPut, testResource.SingleResourcePath, bytes.NewBuffer(requestBodyBytes))
request.Host = testHost
request.URL.Host = testHost
request.URL.Scheme = testScheme

require.NoError(t, err)

ctx := testutil.ARMTestContextFromRequest(request)
actualResponse, err := awsController.Run(ctx, nil, request)
require.NoError(t, err)

w := httptest.NewRecorder()
err = actualResponse.Apply(ctx, w, request)
require.NoError(t, err)

res := w.Result()
require.Equal(t, http.StatusBadRequest, res.StatusCode)

defer res.Body.Close()
body, err := io.ReadAll(res.Body)
require.NoError(t, err)

expectedResponseObject := map[string]interface{}{
"error": map[string]interface{}{
"code": "BadRequest",
"message": "failed to read region from request path: 'regions' not found",
},
}

actualResponseObject := map[string]any{}
err = json.Unmarshal(body, &actualResponseObject)
require.NoError(t, err)

require.Equal(t, expectedResponseObject, actualResponseObject)
}

func Test_UpdateAWSResource(t *testing.T) {
testResource := CreateMemoryDBClusterTestResource(uuid.NewString())

Expand All @@ -130,7 +190,7 @@ func Test_UpdateAWSResource(t *testing.T) {

testOptions := setupTest(t)

testOptions.AWSCloudFormationClient.EXPECT().DescribeType(gomock.Any(), gomock.Any()).Return(&output, nil)
testOptions.AWSCloudFormationClient.EXPECT().DescribeType(gomock.Any(), gomock.Any(), gomock.Any()).Return(&output, nil)

testOptions.AWSCloudControlClient.EXPECT().GetResource(gomock.Any(), gomock.Any(), gomock.Any()).Return(
&cloudcontrol.GetResourceOutput{
Expand Down Expand Up @@ -224,7 +284,7 @@ func Test_UpdateNoChangesDoesNotCallUpdate(t *testing.T) {

testOptions := setupTest(t)

testOptions.AWSCloudFormationClient.EXPECT().DescribeType(gomock.Any(), gomock.Any()).Return(&output, nil)
testOptions.AWSCloudFormationClient.EXPECT().DescribeType(gomock.Any(), gomock.Any(), gomock.Any()).Return(&output, nil)

testOptions.AWSCloudControlClient.EXPECT().GetResource(gomock.Any(), gomock.Any(), gomock.Any()).Return(
&cloudcontrol.GetResourceOutput{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ type CreateOrUpdateAWSResourceWithPost struct {

// NewCreateOrUpdateAWSResourceWithPost creates a new CreateOrUpdateAWSResourceWithPost.
func NewCreateOrUpdateAWSResourceWithPost(opts ctrl.Options) (armrpc_controller.Controller, error) {

return &CreateOrUpdateAWSResourceWithPost{
Operation: armrpc_controller.NewOperation(opts.Options,
armrpc_controller.ResourceOptions[datamodel.AWSResource]{},
Expand All @@ -51,6 +52,10 @@ func NewCreateOrUpdateAWSResourceWithPost(opts ctrl.Options) (armrpc_controller.
func (p *CreateOrUpdateAWSResourceWithPost) Run(ctx context.Context, w http.ResponseWriter, req *http.Request) (armrpc_rest.Response, error) {
logger := ucplog.FromContextOrDiscard(ctx)
serviceCtx := servicecontext.AWSRequestContextFromContext(ctx)
region, errResponse := readRegionFromRequest(req.URL.Path, p.basePath)
if errResponse != nil {
return errResponse, nil
}

properties, err := readPropertiesFromBody(req)
if err != nil {
Expand All @@ -63,10 +68,13 @@ func (p *CreateOrUpdateAWSResourceWithPost) Run(ctx context.Context, w http.Resp
return armrpc_rest.NewBadRequestARMResponse(e), nil
}

cloudControlOpts := []func(*cloudcontrol.Options){CloudControlRegionOption(region)}
cloudFormationOpts := []func(*cloudformation.Options){CloudFormationWithRegionOption(region)}

describeTypeOutput, err := p.awsOptions.AWSCloudFormationClient.DescribeType(ctx, &cloudformation.DescribeTypeInput{
Type: types.RegistryTypeResource,
TypeName: to.Ptr(serviceCtx.ResourceTypeInAWSFormat()),
})
}, cloudFormationOpts...)
if err != nil {
return awserror.HandleAWSError(err)
}
Expand Down Expand Up @@ -96,7 +104,7 @@ func (p *CreateOrUpdateAWSResourceWithPost) Run(ctx context.Context, w http.Resp
getResponse, err = p.awsOptions.AWSCloudControlClient.GetResource(ctx, &cloudcontrol.GetResourceInput{
TypeName: to.Ptr(serviceCtx.ResourceTypeInAWSFormat()),
Identifier: aws.String(awsResourceIdentifier),
})
}, cloudControlOpts...)
if awserror.IsAWSResourceNotFoundError(err) {
existing = false
} else if err != nil {
Expand Down Expand Up @@ -136,7 +144,7 @@ func (p *CreateOrUpdateAWSResourceWithPost) Run(ctx context.Context, w http.Resp
TypeName: to.Ptr(serviceCtx.ResourceTypeInAWSFormat()),
Identifier: aws.String(awsResourceIdentifier),
PatchDocument: aws.String(string(marshaled)),
})
}, cloudControlOpts...)
if err != nil {
return awserror.HandleAWSError(err)
}
Expand Down Expand Up @@ -164,7 +172,7 @@ func (p *CreateOrUpdateAWSResourceWithPost) Run(ctx context.Context, w http.Resp
response, err := p.awsOptions.AWSCloudControlClient.CreateResource(ctx, &cloudcontrol.CreateResourceInput{
TypeName: to.Ptr(serviceCtx.ResourceTypeInAWSFormat()),
DesiredState: aws.String(string(desiredState)),
})
}, cloudControlOpts...)
if err != nil {
return awserror.HandleAWSError(err)
}
Expand Down
Loading

0 comments on commit d8cce54

Please sign in to comment.