Skip to content

Commit

Permalink
ResourceExhausted error code mapping introduced for AWS (#129)
Browse files Browse the repository at this point in the history
* map aws error codes to mcm error codes

* temporary vendor changes

* add error code to replace PR #59

* map ResourceExhausted error code for CreateMachine call

* remove unused variables from mockclient.go

* revert mcm vendor changes

* resolved make check errors

* address review comments

* address review comments part-2
  • Loading branch information
rishabh-11 authored Sep 21, 2023
1 parent 745e0b7 commit 3b064bf
Show file tree
Hide file tree
Showing 18 changed files with 423 additions and 41 deletions.
6 changes: 6 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ MACHINE_CONTROLLER_MANAGER_DEPLOYMENT_NAME := machine-controller-manager
# Rules for running helper scripts
#########################################

include hack/tools.mk

.PHONY: rename-provider
rename-provider:
@./hack/rename-provider ${PROVIDER_NAME}
Expand Down Expand Up @@ -146,3 +148,7 @@ clean:
@rm -rf bin/
@rm -f *linux-amd64
@rm -f *darwin-amd64

.PHONY: add-license-headers
add-license-headers: $(GO_ADD_LICENSE)
@./hack/add_license_headers.sh
33 changes: 33 additions & 0 deletions hack/add_license_headers.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#!/usr/bin/env bash
# Copyright 2023 SAP SE or an SAP affiliate company
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.


set -e

echo "> Adding Apache License header to all go files where it is not present"

# Uses the tool https://github.com/google/addlicense
YEAR="$(date +%Y)"
addlicense \
-c "SAP SE or an SAP affiliate company" \
-y "${YEAR}" \
-l apache \
-ignore ".idea/**" \
-ignore ".vscode/**" \
-ignore "vendor/**" \
-ignore "**/*.md" \
-ignore "**/*.yaml" \
-ignore "**/Dockerfile" \
.
33 changes: 33 additions & 0 deletions hack/tools.mk
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Copyright (c) 2023 SAP SE or an SAP affiliate company. All rights reserved. This file is licensed under the Apache Software License, v. 2 except as noted otherwise in the LICENSE file
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

TOOLS_DIR := hack/tools
TOOLS_BIN_DIR := $(TOOLS_DIR)/bin
GO_ADD_LICENSE := $(TOOLS_BIN_DIR)/addlicense

# default tool versions
GO_ADD_LICENSE_VERSION ?= latest

export TOOLS_BIN_DIR := $(TOOLS_BIN_DIR)
export PATH := $(abspath $(TOOLS_BIN_DIR)):$(PATH)
$(info "TOOLS_BIN_DIR from tools.mk", $(TOOLS_BIN_DIR))
$(info "TOOLS_DIR from tools.mk", $(TOOLS_DIR))
$(info "PATH from tools.mk", $(PATH))

#########################################
# Tools #
#########################################

$(GO_ADD_LICENSE):
GOBIN=$(abspath $(TOOLS_BIN_DIR)) go install github.com/google/addlicense@$(GO_ADD_LICENSE_VERSION)
14 changes: 14 additions & 0 deletions pkg/aws/apis/validation/validation_suite_test.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
// Copyright 2023 SAP SE or an SAP affiliate company
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package validation

import (
Expand Down
14 changes: 14 additions & 0 deletions pkg/aws/apis/validation/validation_test.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
// Copyright 2023 SAP SE or an SAP affiliate company
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package validation

import (
Expand Down
19 changes: 10 additions & 9 deletions pkg/aws/core.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"github.com/gardener/machine-controller-manager/pkg/util/provider/machinecodes/status"
"k8s.io/klog/v2"

awserror "github.com/gardener/machine-controller-manager-provider-aws/pkg/aws/errors"
"github.com/gardener/machine-controller-manager-provider-aws/pkg/spi"
)

Expand Down Expand Up @@ -87,7 +88,7 @@ func (d *Driver) CreateMachine(ctx context.Context, req *driver.CreateMachineReq

svc, err := d.createSVC(secret, providerSpec.Region)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(awserror.GetMCMErrorCodeForCreateMachine(err), err.Error())
}

if userData, exists = secret.Data["userData"]; !exists {
Expand All @@ -104,14 +105,14 @@ func (d *Driver) CreateMachine(ctx context.Context, req *driver.CreateMachineReq
}
output, err := svc.DescribeImages(&describeImagesRequest)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(awserror.GetMCMErrorCodeForCreateMachine(err), err.Error())
} else if len(output.Images) < 1 {
return nil, status.Error(codes.Internal, fmt.Sprintf("Image %s not found", *imageID))
return nil, status.Error(codes.NotFound, fmt.Sprintf("Image %s not found", *imageID))
}

blkDeviceMappings, err := d.generateBlockDevices(providerSpec.BlockDevices, output.Images[0].RootDeviceName)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(codes.InvalidArgument, err.Error())
}

tagInstance, err := d.generateTags(providerSpec.Tags, resourceTypeInstance, req.Machine.Name)
Expand Down Expand Up @@ -215,7 +216,7 @@ func (d *Driver) CreateMachine(ctx context.Context, req *driver.CreateMachineReq

runResult, err := svc.RunInstances(&inputConfig)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(awserror.GetMCMErrorCodeForCreateMachine(err), err.Error())
}

response := &driver.CreateMachineResponse{
Expand All @@ -239,7 +240,7 @@ func (d *Driver) CreateMachine(ctx context.Context, req *driver.CreateMachineReq
if providerSpec.SrcAndDstChecksEnabled != nil && !*providerSpec.SrcAndDstChecksEnabled {
err := disableSrcAndDestCheck(svc, runResult.Instances[0].InstanceId)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(awserror.GetMCMErrorCodeForCreateMachine(err), err.Error())
}
}

Expand Down Expand Up @@ -299,14 +300,14 @@ func (d *Driver) DeleteMachine(ctx context.Context, req *driver.DeleteMachineReq

_, instanceID, err := decodeRegionAndInstanceID(req.Machine.Spec.ProviderID)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(codes.InvalidArgument, err.Error())
}

err = terminateInstance(req, svc, instanceID)
if err != nil {
return nil, err
}
klog.V(3).Infof("VM %q for Machine %q was terminated succesfully", req.Machine.Spec.ProviderID, req.Machine.Name)
klog.V(3).Infof("VM %q for Machine %q was terminated successfully", req.Machine.Spec.ProviderID, req.Machine.Name)

} else {
// ProviderID doesn't exist, hence check for any existing machine and then delete if exists
Expand All @@ -315,7 +316,7 @@ func (d *Driver) DeleteMachine(ctx context.Context, req *driver.DeleteMachineReq
if err != nil {
status, ok := status.FromError(err)
if ok && status.Code() == codes.NotFound {
klog.V(3).Infof("No matching VM found. Termination succesful for machine object %q", req.Machine.Name)
klog.V(3).Infof("No matching VM found. Termination successful for machine object %q", req.Machine.Name)
return &driver.DeleteMachineResponse{}, nil
}
return nil, err
Expand Down
48 changes: 31 additions & 17 deletions pkg/aws/core_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ package aws

import (
"context"
"fmt"
"github.com/gardener/machine-controller-manager/pkg/util/provider/machinecodes/codes"
"time"

"github.com/aws/aws-sdk-go/service/ec2"
Expand All @@ -29,12 +31,11 @@ import (
)

const (
awsAccessKeyIDIsMissing = "machine codes error: code = [Internal] message = [Error while validating ProviderSpec secretRef.AWSAccessKeyID: Required value: Mention atleast providerAccessKeyId or accessKeyID]"
awsSecretAccessKeyIsMissing = "machine codes error: code = [Internal] message = [Error while validating ProviderSpec secretRef.AWSSecretAccessKey: Required value: Mention atleast providerSecretAccessKey or secretAccessKey]"
awsSecretAccessKeyNUserDataAreMissing = "machine codes error: code = [Internal] message = [Error while validating ProviderSpec [secretRef.AWSSecretAccessKey: Required value: Mention atleast providerSecretAccessKey or secretAccessKey, secretRef.userData: Required value: Mention userData]]"
regionNAMIMissing = "machine codes error: code = [Internal] message = [Error while validating ProviderSpec [providerSpec.ami: Required value: AMI is required, providerSpec.region: Required value: Region is required]]"
userDataIsMissing = "machine codes error: code = [Internal] message = [Error while validating ProviderSpec secretRef.userData: Required value: Mention userData]"
cloudProviderReturnedError = "machine codes error: code = [Internal] message = [Cloud provider returned error]"
awsAccessKeyIDIsMissing = "machine codes error: code = [InvalidArgument] message = [Error while validating ProviderSpec secretRef.AWSAccessKeyID: Required value: Mention atleast providerAccessKeyId or accessKeyID]"
awsSecretAccessKeyIsMissing = "machine codes error: code = [InvalidArgument] message = [Error while validating ProviderSpec secretRef.AWSSecretAccessKey: Required value: Mention atleast providerSecretAccessKey or secretAccessKey]"
awsSecretAccessKeyNUserDataAreMissing = "machine codes error: code = [InvalidArgument] message = [Error while validating ProviderSpec [secretRef.AWSSecretAccessKey: Required value: Mention atleast providerSecretAccessKey or secretAccessKey, secretRef.userData: Required value: Mention userData]]"
regionNAMIMissing = "machine codes error: code = [InvalidArgument] message = [Error while validating ProviderSpec [providerSpec.ami: Required value: AMI is required, providerSpec.region: Required value: Region is required]]"
userDataIsMissing = "machine codes error: code = [InvalidArgument] message = [Error while validating ProviderSpec secretRef.userData: Required value: Mention userData]"
)

var _ = Describe("MachineServer", func() {
Expand Down Expand Up @@ -198,7 +199,7 @@ var _ = Describe("MachineServer", func() {
},
expect: expect{
errToHaveOccurred: true,
errMessage: "machine codes error: code = [Internal] message = [Error while validating ProviderSpec providerSpec.capacityReservation: Required value: CapacityReservationResourceGroupArn or CapacityReservationId are optional but only one should be used]",
errMessage: "machine codes error: code = [InvalidArgument] message = [Error while validating ProviderSpec providerSpec.capacityReservation: Required value: CapacityReservationResourceGroupArn or CapacityReservationId are optional but only one should be used]",
},
}),
Entry("Machine creation request for an AWS Capacity Reservation Group with capacityReservationPreference only", &data{
Expand Down Expand Up @@ -339,7 +340,7 @@ var _ = Describe("MachineServer", func() {
},
expect: expect{
errToHaveOccurred: true,
errMessage: "machine codes error: code = [Internal] message = [Region doesn't exist while trying to create session]",
errMessage: fmt.Sprintf("machine codes error: code = [Internal] message = [%s]", mockclient.AWSInvalidRegionError),
},
}),
Entry("Placement object with affinity, tenancy and availablityZone set", &data{
Expand All @@ -362,13 +363,13 @@ var _ = Describe("MachineServer", func() {
action: action{
machineRequest: &driver.CreateMachineRequest{
Machine: newMachine(-1, nil),
MachineClass: newMachineClass([]byte("{\"ami\":\"" + mockclient.FailQueryAtDescribeImages + "\",\"blockDevices\":[{\"ebs\":{\"volumeSize\":50,\"volumeType\":\"gp2\"}}],\"iam\":{\"name\":\"test-iam\"},\"keyName\":\"test-ssh-publickey\",\"machineType\":\"m4.large\",\"networkInterfaces\":[{\"securityGroupIDs\":[\"sg-00002132323\"],\"subnetID\":\"subnet-123456\"}],\"region\":\"eu-west-1\",\"tags\":{\"kubernetes.io/cluster/shoot--test\":\"1\",\"kubernetes.io/role/test\":\"1\"}}")),
MachineClass: newMachineClass([]byte(fmt.Sprintf("{\"ami\":\"%s\",\"blockDevices\":[{\"ebs\":{\"volumeSize\":50,\"volumeType\":\"gp2\"}}],\"iam\":{\"name\":\"test-iam\"},\"keyName\":\"test-ssh-publickey\",\"machineType\":\"m4.large\",\"networkInterfaces\":[{\"securityGroupIDs\":[\"sg-00002132323\"],\"subnetID\":\"subnet-123456\"}],\"region\":\"eu-west-1\",\"tags\":{\"kubernetes.io/cluster/shoot--test\":\"1\",\"kubernetes.io/role/test\":\"1\"}}", mockclient.FailQueryAtDescribeImages))),
Secret: providerSecret,
},
},
expect: expect{
errToHaveOccurred: true,
errMessage: "machine codes error: code = [Internal] message = [Couldn't find image with given ID]",
errMessage: fmt.Sprintf("machine codes error: code = [Internal] message = [%s]", mockclient.AWSImageNotFoundError),
},
}),
Entry("Name tag cannot be set on AWS instances", &data{
Expand All @@ -387,17 +388,30 @@ var _ = Describe("MachineServer", func() {
errToHaveOccurred: false,
},
}),
Entry("RunInstance call fails", &data{
Entry("RunInstance call fails with error code as Internal", &data{
action: action{
machineRequest: &driver.CreateMachineRequest{
Machine: newMachine(-1, nil),
MachineClass: newMachineClass([]byte("{\"ami\":\"" + mockclient.FailQueryAtRunInstances + "\",\"blockDevices\":[{\"ebs\":{\"volumeSize\":50,\"volumeType\":\"gp2\"}}],\"iam\":{\"name\":\"test-iam\"},\"keyName\":\"test-ssh-publickey\",\"machineType\":\"m4.large\",\"networkInterfaces\":[{\"securityGroupIDs\":[\"sg-00002132323\"],\"subnetID\":\"subnet-123456\"}],\"region\":\"eu-west-1\",\"tags\":{\"kubernetes.io/cluster/shoot--test\":\"1\",\"kubernetes.io/role/test\":\"1\"}}")),
MachineClass: newMachineClass([]byte(fmt.Sprintf("{\"ami\":\"%s\",\"blockDevices\":[{\"ebs\":{\"volumeSize\":50,\"volumeType\":\"gp2\"}}],\"iam\":{\"name\":\"test-iam\"},\"keyName\":\"test-ssh-publickey\",\"machineType\":\"m4.large\",\"networkInterfaces\":[{\"securityGroupIDs\":[\"sg-00002132323\"],\"subnetID\":\"subnet-123456\"}],\"region\":\"eu-west-1\",\"tags\":{\"kubernetes.io/cluster/shoot--test\":\"1\",\"kubernetes.io/role/test\":\"1\"}}", mockclient.FailQueryAtRunInstances))),
Secret: providerSecret,
},
},
expect: expect{
errToHaveOccurred: true,
errMessage: "machine codes error: code = [Internal] message = [Couldn't run instance with given ID]",
errMessage: fmt.Sprintf("machine codes error: code = [Internal] message = [%s]", mockclient.AWSInternalErrorForRunInstances),
},
}),
Entry("RunInstance call fails with error code as InsufficientCapacity", &data{
action: action{
machineRequest: &driver.CreateMachineRequest{
Machine: newMachine(-1, nil),
MachineClass: newMachineClass([]byte(fmt.Sprintf("{\"ami\":\"%s\",\"blockDevices\":[{\"ebs\":{\"volumeSize\":50,\"volumeType\":\"gp2\"}}],\"iam\":{\"name\":\"test-iam\"},\"keyName\":\"%s\",\"machineType\":\"m4.large\",\"networkInterfaces\":[{\"securityGroupIDs\":[\"sg-00002132323\"],\"subnetID\":\"subnet-123456\"}],\"region\":\"eu-west-1\",\"tags\":{\"kubernetes.io/cluster/shoot--test\":\"1\",\"kubernetes.io/role/test\":\"1\"}}", mockclient.FailQueryAtRunInstances, mockclient.InsufficientCapacity))),
Secret: providerSecret,
},
},
expect: expect{
errToHaveOccurred: true,
errMessage: fmt.Sprintf("machine codes error: code = [%s] message = [%s]", codes.ResourceExhausted, mockclient.AWSInsufficientCapacityError),
},
}),
Entry("Should Fail when APIs are not consistent for 10sec(in real situation its 5min)", &data{
Expand Down Expand Up @@ -786,7 +800,7 @@ var _ = Describe("MachineServer", func() {
expect: expect{
deleteMachineResponse: &driver.DeleteMachineResponse{},
errToHaveOccurred: true,
errMessage: cloudProviderReturnedError,
errMessage: fmt.Sprintf("machine codes error: code = [Internal] message = [%s]", mockclient.AWSInternalErrorForDescribeInstances),
},
}),
Entry("Termination of machine with any backing instance but no providerID", &data{
Expand Down Expand Up @@ -1180,7 +1194,7 @@ var _ = Describe("MachineServer", func() {
},
expect: expect{
errToHaveOccurred: true,
errMessage: "machine codes error: code = [Internal] message = [Region doesn't exist while trying to create session]",
errMessage: fmt.Sprintf("machine codes error: code = [Internal] message = [%s]", mockclient.AWSInvalidRegionError),
},
}),
Entry("Cluster details missing in machine class", &data{
Expand All @@ -1192,7 +1206,7 @@ var _ = Describe("MachineServer", func() {
},
expect: expect{
errToHaveOccurred: true,
errMessage: "machine codes error: code = [Internal] message = [Error while validating ProviderSpec providerSpec.tags[]: Required value: Tag required of the form kubernetes.io/cluster/****]",
errMessage: "machine codes error: code = [InvalidArgument] message = [Error while validating ProviderSpec providerSpec.tags[]: Required value: Tag required of the form kubernetes.io/cluster/****]",
},
}),
Entry("Cloud provider returned error while describing instance", &data{
Expand All @@ -1204,7 +1218,7 @@ var _ = Describe("MachineServer", func() {
},
expect: expect{
errToHaveOccurred: true,
errMessage: cloudProviderReturnedError,
errMessage: fmt.Sprintf("machine codes error: code = [Internal] message = [%s]", mockclient.AWSInternalErrorForDescribeInstances),
},
}),
Entry("List request without a create request", &data{
Expand Down
Loading

0 comments on commit 3b064bf

Please sign in to comment.