diff --git a/Makefile b/Makefile index db65078bc2..21a3c630bc 100644 --- a/Makefile +++ b/Makefile @@ -108,6 +108,18 @@ test-e2e-external: GINKGO_SKIP="\[Disruptive\]|\[Serial\]" \ ./hack/e2e/run.sh +.PHONY: test-e2e-external-eks +test-e2e-external-eks: + CLUSTER_TYPE=eksctl \ + K8S_VERSION="1.20" \ + HELM_VALUES_FILE="./hack/values_eksctl.yaml" \ + AWS_REGION=us-west-2 \ + AWS_AVAILABILITY_ZONES=us-west-2a,us-west-2b \ + TEST_PATH=./tests/e2e-kubernetes/... \ + GINKGO_FOCUS="External.Storage" \ + GINKGO_SKIP="\[Disruptive\]|\[Serial\]" \ + ./hack/e2e/run.sh + .PHONY: image-release image-release: docker build -t $(IMAGE):$(VERSION) . --target debian-base diff --git a/charts/aws-ebs-csi-driver/templates/node.yaml b/charts/aws-ebs-csi-driver/templates/node.yaml index 8e2378106a..556710a39a 100644 --- a/charts/aws-ebs-csi-driver/templates/node.yaml +++ b/charts/aws-ebs-csi-driver/templates/node.yaml @@ -37,7 +37,6 @@ spec: {{- with .Values.node.nodeSelector }} {{- toYaml . | nindent 8 }} {{- end }} - hostNetwork: true serviceAccountName: {{ .Values.serviceAccount.node.name }} priorityClassName: {{ .Values.node.priorityClassName | default "system-node-critical" }} tolerations: diff --git a/deploy/kubernetes/base/node.yaml b/deploy/kubernetes/base/node.yaml index e59bd43f38..9894ca7f2a 100644 --- a/deploy/kubernetes/base/node.yaml +++ b/deploy/kubernetes/base/node.yaml @@ -29,7 +29,6 @@ spec: - fargate nodeSelector: kubernetes.io/os: linux - hostNetwork: true serviceAccountName: ebs-csi-node-sa priorityClassName: system-node-critical tolerations: diff --git a/hack/e2e/eksctl.sh b/hack/e2e/eksctl.sh index 2d96860405..e069ec9cd8 100644 --- a/hack/e2e/eksctl.sh +++ b/hack/e2e/eksctl.sh @@ -20,6 +20,7 @@ function eksctl_create_cluster() { K8S_VERSION=${6} CLUSTER_FILE=${7} KUBECONFIG=${8} + EKSCTL_PATCH_FILE=${9} generate_ssh_key "${SSH_KEY_PATH}" @@ -38,10 +39,13 @@ function eksctl_create_cluster() { --nodes=3 \ --instance-types="${INSTANCE_TYPE}" \ --version="${K8S_VERSION}" \ + --disable-pod-imds \ --dry-run \ "${CLUSTER_NAME}" > "${CLUSTER_FILE}" - # TODO implement patching + if test -f "$EKSCTL_PATCH_FILE"; then + eksctl_patch_cluster_file "$CLUSTER_FILE" "$EKSCTL_PATCH_FILE" + fi loudecho "Creating cluster $CLUSTER_NAME with $CLUSTER_FILE" ${BIN} create cluster -f "${CLUSTER_FILE}" --kubeconfig "${KUBECONFIG}" @@ -73,3 +77,23 @@ function eksctl_delete_cluster() { loudecho "Deleting cluster ${CLUSTER_NAME}" ${BIN} delete cluster "${CLUSTER_NAME}" } + +function eksctl_patch_cluster_file() { + CLUSTER_FILE=${1} # input must be yaml + EKSCTL_PATCH_FILE=${2} # input must be yaml + + loudecho "Patching cluster $CLUSTER_NAME with $EKSCTL_PATCH_FILE" + + # Temporary intermediate files for patching + CLUSTER_FILE_0=$CLUSTER_FILE.0 + CLUSTER_FILE_1=$CLUSTER_FILE.1 + + cp "$CLUSTER_FILE" "$CLUSTER_FILE_0" + + # Patch only the Cluster + kubectl patch -f "$CLUSTER_FILE_0" --local --type merge --patch "$(cat "$EKSCTL_PATCH_FILE")" -o yaml > "$CLUSTER_FILE_1" + mv "$CLUSTER_FILE_1" "$CLUSTER_FILE_0" + + # Done patching, overwrite original CLUSTER_FILE + mv "$CLUSTER_FILE_0" "$CLUSTER_FILE" # output is yaml +} diff --git a/hack/e2e/kops.sh b/hack/e2e/kops.sh index 40bca20d4f..f7fcb487a4 100644 --- a/hack/e2e/kops.sh +++ b/hack/e2e/kops.sh @@ -46,7 +46,9 @@ function kops_create_cluster() { -o json \ "${CLUSTER_NAME}" > "${CLUSTER_FILE}" - kops_patch_cluster_file "$CLUSTER_FILE" "$KOPS_PATCH_FILE" + if test -f "$KOPS_PATCH_FILE"; then + kops_patch_cluster_file "$CLUSTER_FILE" "$KOPS_PATCH_FILE" + fi loudecho "Creating cluster $CLUSTER_NAME with $CLUSTER_FILE" ${BIN} create --state "${KOPS_STATE_FILE}" -f "${CLUSTER_FILE}" @@ -86,10 +88,11 @@ function kops_delete_cluster() { ${BIN} delete cluster --name "${CLUSTER_NAME}" --state "${KOPS_STATE_FILE}" --yes } -# TODO switch this to python, all this hacking with jq stinks! +# TODO switch this to python or work exclusively with yaml, all this +# hacking with jq stinks! function kops_patch_cluster_file() { - CLUSTER_FILE=${1} - KOPS_PATCH_FILE=${2} + CLUSTER_FILE=${1} # input must be json + KOPS_PATCH_FILE=${2} # input must be yaml loudecho "Patching cluster $CLUSTER_NAME with $KOPS_PATCH_FILE" @@ -116,5 +119,5 @@ function kops_patch_cluster_file() { mv "$CLUSTER_FILE_1" "$CLUSTER_FILE_0" # Done patching, overwrite original CLUSTER_FILE - mv "$CLUSTER_FILE_0" "$CLUSTER_FILE" + mv "$CLUSTER_FILE_0" "$CLUSTER_FILE" # output is yaml } diff --git a/hack/e2e/run.sh b/hack/e2e/run.sh index 3eeac41142..fd1c7fef2f 100755 --- a/hack/e2e/run.sh +++ b/hack/e2e/run.sh @@ -36,7 +36,7 @@ TEST_DIR=${BASE_DIR}/csi-test-artifacts BIN_DIR=${TEST_DIR}/bin SSH_KEY_PATH=${TEST_DIR}/id_rsa CLUSTER_FILE=${TEST_DIR}/${CLUSTER_NAME}.${CLUSTER_TYPE}.json -KUBECONFIG=${KUBECONFIG:-"${TEST_DIR}/${CLUSTER_NAME}.kubeconfig"} +KUBECONFIG=${KUBECONFIG:-"${TEST_DIR}/${CLUSTER_NAME}.${CLUSTER_TYPE}.kubeconfig"} REGION=${AWS_REGION:-us-west-2} ZONES=${AWS_AVAILABILITY_ZONES:-us-west-2a,us-west-2b,us-west-2c} @@ -55,6 +55,8 @@ KOPS_VERSION=${KOPS_VERSION:-1.20.0} KOPS_STATE_FILE=${KOPS_STATE_FILE:-s3://k8s-kops-csi-e2e} KOPS_PATCH_FILE=${KOPS_PATCH_FILE:-./hack/kops-patch.yaml} +EKSCTL_PATCH_FILE=${EKSCTL_PATCH_FILE:-./hack/eksctl-patch.yaml} + HELM_VALUES_FILE=${HELM_VALUES_FILE:-./hack/values.yaml} TEST_PATH=${TEST_PATH:-"./tests/e2e/..."} @@ -127,7 +129,8 @@ elif [[ "${CLUSTER_TYPE}" == "eksctl" ]]; then "$INSTANCE_TYPE" \ "$K8S_VERSION" \ "$CLUSTER_FILE" \ - "$KUBECONFIG" + "$KUBECONFIG" \ + "$EKSCTL_PATCH_FILE" if [[ $? -ne 0 ]]; then exit 1 fi @@ -135,14 +138,20 @@ fi loudecho "Deploying driver" startSec=$(date +'%s') -"${HELM_BIN}" upgrade --install "${DRIVER_NAME}" \ - --namespace kube-system \ - --set image.repository="${IMAGE_NAME}" \ - --set image.tag="${IMAGE_TAG}" \ - -f "${HELM_VALUES_FILE}" \ - --wait \ - --kubeconfig "${KUBECONFIG}" \ - ./charts/"${DRIVER_NAME}" + +HELM_ARGS=(upgrade --install "${DRIVER_NAME}" + --namespace kube-system + --set image.repository="${IMAGE_NAME}" + --set image.tag="${IMAGE_TAG}" + --wait + --kubeconfig "${KUBECONFIG}" + ./charts/"${DRIVER_NAME}") +if test -f "$HELM_VALUES_FILE"; then + HELM_ARGS+=(-f "${HELM_VALUES_FILE}") +fi +set -x +"${HELM_BIN}" "${HELM_ARGS[@]}" +set +x if [[ -r "${EBS_SNAPSHOT_CRD}" ]]; then loudecho "Deploying snapshot CRD" diff --git a/hack/eksctl-patch.yaml b/hack/eksctl-patch.yaml new file mode 100644 index 0000000000..a8d3e4aa15 --- /dev/null +++ b/hack/eksctl-patch.yaml @@ -0,0 +1,9 @@ +iam: + vpcResourceControllerPolicy: true + withOIDC: true + serviceAccounts: + - metadata: + name: ebs-csi-controller-sa + namespace: kube-system + wellKnownPolicies: + ebsCSIController: true diff --git a/hack/values.yaml b/hack/values.yaml index 6d331b8acf..c84cf6e220 100644 --- a/hack/values.yaml +++ b/hack/values.yaml @@ -3,4 +3,3 @@ controller: logLevel: 5 node: logLevel: 5 - diff --git a/hack/values_eksctl.yaml b/hack/values_eksctl.yaml new file mode 100644 index 0000000000..f84eeefad0 --- /dev/null +++ b/hack/values_eksctl.yaml @@ -0,0 +1,8 @@ +enableVolumeSnapshot: true +controller: + logLevel: 5 +node: + logLevel: 5 +serviceAccount: + controller: + create: false # let eksctl create it diff --git a/pkg/cloud/metadata.go b/pkg/cloud/metadata.go index e2c7ec29ec..995b043a59 100644 --- a/pkg/cloud/metadata.go +++ b/pkg/cloud/metadata.go @@ -34,6 +34,7 @@ import ( "k8s.io/klog" ) +// Metadata is info about the ec2 instance on which the driver is running type Metadata struct { InstanceID string InstanceType string @@ -72,84 +73,58 @@ func (m *Metadata) GetOutpostArn() arn.ARN { return m.OutpostArn } -func NewMetadata() (MetadataService, error) { +type EC2MetadataClient func() (EC2Metadata, error) + +var DefaultEC2MetadataClient = func() (EC2Metadata, error) { sess := session.Must(session.NewSession(&aws.Config{})) svc := ec2metadata.New(sess) - var clientset *kubernetes.Clientset - if !svc.Available() { - // creates the in-cluster config - config, err := rest.InClusterConfig() - if err != nil { - return nil, err - } - // creates the clientset - clientset, err = kubernetes.NewForConfig(config) - if err != nil { - return nil, err - } + return svc, nil +} + +type KubernetesAPIClient func() (kubernetes.Interface, error) + +var DefaultKubernetesAPIClient = func() (kubernetes.Interface, error) { + // creates the in-cluster config + config, err := rest.InClusterConfig() + if err != nil { + return nil, err } - metadataService, err := NewMetadataService(svc, clientset) + // creates the clientset + clientset, err := kubernetes.NewForConfig(config) if err != nil { - return nil, fmt.Errorf("error getting information from metadata service or node object: %w", err) + return nil, err } - return metadataService, err + return clientset, nil } -// NewMetadataService returns a new MetadataServiceImplementation. -func NewMetadataService(svc EC2Metadata, clientset kubernetes.Interface) (MetadataService, error) { +func NewMetadataService(ec2MetadataClient EC2MetadataClient, k8sAPIClient KubernetesAPIClient) (MetadataService, error) { + klog.Infof("retrieving instance data from ec2 metadata") + svc, err := ec2MetadataClient() if !svc.Available() { - klog.Warningf("EC2 instance metadata is not available") - nodeName := os.Getenv("CSI_NODE_NAME") - if nodeName == "" { - return nil, fmt.Errorf("instance metadata is unavailable and CSI_NODE_NAME env var not set") - } - - // get node with k8s API - node, err := clientset.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{}) - if err != nil { - return nil, err - } - - providerID := node.Spec.ProviderID - if providerID == "" { - return nil, fmt.Errorf("node providerID empty, cannot parse") - } - - awsRegionRegex := "([a-z]{2}(-gov)?)-(central|(north|south)?(east|west)?)-[0-9]" - awsAvailabilityZoneRegex := "([a-z]{2}(-gov)?)-(central|(north|south)?(east|west)?)-[0-9][a-z]" - awsInstanceIDRegex := "i-[a-z0-9]+$" - - re := regexp.MustCompile(awsRegionRegex) - region := re.FindString(providerID) - if region == "" { - return nil, fmt.Errorf("did not find aws region in node providerID string") - } - - re = regexp.MustCompile(awsAvailabilityZoneRegex) - availabilityZone := re.FindString(providerID) - if availabilityZone == "" { - return nil, fmt.Errorf("did not find aws availability zone in node providerID string") - } - - re = regexp.MustCompile(awsInstanceIDRegex) - instanceID := re.FindString(providerID) - if instanceID == "" { - return nil, fmt.Errorf("did not find aws instance ID in node providerID string") - } - - metadata := Metadata{ - InstanceID: instanceID, - InstanceType: "", // we have no way to find this, so we leave it empty - Region: region, - AvailabilityZone: availabilityZone, - } + klog.Warning("ec2 metadata is not available") + } else if err != nil { + klog.Warningf("error creating ec2 metadata client: %v", err) + } else { + klog.Infof("ec2 metadata is available") + return EC2MetadataInstanceInfo(svc) + } - return &metadata, nil + klog.Infof("retrieving instance data from kubernetes api") + clientset, err := k8sAPIClient() + if err != nil { + klog.Warningf("error creating kubernetes api client: %v", err) + } else { + klog.Infof("kubernetes api is available") + return KubernetesAPIInstanceInfo(clientset) } + return nil, fmt.Errorf("error getting instance data from ec2 metadata or kubernetes api") +} + +func EC2MetadataInstanceInfo(svc EC2Metadata) (*Metadata, error) { doc, err := svc.GetInstanceIdentityDocument() if err != nil { - return nil, fmt.Errorf("could not get EC2 instance identity metadata") + return nil, fmt.Errorf("could not get EC2 instance identity metadata: %v", err) } if len(doc.InstanceID) == 0 { @@ -168,6 +143,13 @@ func NewMetadataService(svc EC2Metadata, clientset kubernetes.Interface) (Metada return nil, fmt.Errorf("could not get valid EC2 availability zone") } + instanceInfo := Metadata{ + InstanceID: doc.InstanceID, + InstanceType: doc.InstanceType, + Region: doc.Region, + AvailabilityZone: doc.AvailabilityZone, + } + outpostArn, err := svc.GetMetadata(OutpostArnEndpoint) // "outpust-arn" returns 404 for non-outpost instances. note that the request is made to a link-local address. // it's guaranteed to be in the form `arn::outposts:::outpost/` @@ -176,23 +158,64 @@ func NewMetadataService(svc EC2Metadata, clientset kubernetes.Interface) (Metada return nil, fmt.Errorf("something went wrong while getting EC2 outpost arn: %s", err.Error()) } else if err == nil { klog.Infof("Running in an outpost environment with arn: %s", outpostArn) + outpostArn = strings.ReplaceAll(outpostArn, "outpost/", "") + parsedArn, err := arn.Parse(outpostArn) + if err != nil { + klog.Warningf("Failed to parse the outpost arn: %s", outpostArn) + } else { + klog.Infof("Using outpost arn: %v", parsedArn) + instanceInfo.OutpostArn = parsedArn + } } - metadata := Metadata{ - InstanceID: doc.InstanceID, - InstanceType: doc.InstanceType, - Region: doc.Region, - AvailabilityZone: doc.AvailabilityZone, + return &instanceInfo, nil +} + +func KubernetesAPIInstanceInfo(clientset kubernetes.Interface) (*Metadata, error) { + nodeName := os.Getenv("CSI_NODE_NAME") + if nodeName == "" { + return nil, fmt.Errorf("CSI_NODE_NAME env var not set") } - outpostArn = strings.ReplaceAll(outpostArn, "outpost/", "") - parsedArn, err := arn.Parse(outpostArn) + // get node with k8s API + node, err := clientset.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{}) if err != nil { - klog.Warningf("Failed to parse the outpost arn: %s", outpostArn) - } else { - klog.Infof("Using outpost arn: %v", parsedArn) - metadata.OutpostArn = parsedArn + return nil, fmt.Errorf("error getting Node %v: %v", nodeName, err) + } + + providerID := node.Spec.ProviderID + if providerID == "" { + return nil, fmt.Errorf("node providerID empty, cannot parse") + } + + awsRegionRegex := "([a-z]{2}(-gov)?)-(central|(north|south)?(east|west)?)-[0-9]" + awsAvailabilityZoneRegex := "([a-z]{2}(-gov)?)-(central|(north|south)?(east|west)?)-[0-9][a-z]" + awsInstanceIDRegex := "i-[a-z0-9]+$" + + re := regexp.MustCompile(awsRegionRegex) + region := re.FindString(providerID) + if region == "" { + return nil, fmt.Errorf("did not find aws region in node providerID string") + } + + re = regexp.MustCompile(awsAvailabilityZoneRegex) + availabilityZone := re.FindString(providerID) + if availabilityZone == "" { + return nil, fmt.Errorf("did not find aws availability zone in node providerID string") + } + + re = regexp.MustCompile(awsInstanceIDRegex) + instanceID := re.FindString(providerID) + if instanceID == "" { + return nil, fmt.Errorf("did not find aws instance ID in node providerID string") + } + + instanceInfo := Metadata{ + InstanceID: instanceID, + InstanceType: "", // we have no way to find this, so we leave it empty + Region: region, + AvailabilityZone: availabilityZone, } - return &metadata, nil + return &instanceInfo, nil } diff --git a/pkg/cloud/metadata_test.go b/pkg/cloud/metadata_test.go index 11afa7b512..07ebd34bae 100644 --- a/pkg/cloud/metadata_test.go +++ b/pkg/cloud/metadata_test.go @@ -29,20 +29,17 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" k8s_testing "k8s.io/client-go/testing" ) -var ( - stdInstanceID = "instance-1" - stdInstanceType = "t2.medium" - stdRegion = "instance-1" - stdAvailabilityZone = "az-1" -) - const ( - nodeName = "ip-123-45-67-890.us-west-2.compute.internal" - nodeObjectInstanceID = "i-abcdefgh123456789" + nodeName = "ip-123-45-67-890.us-west-2.compute.internal" + stdInstanceID = "i-abcdefgh123456789" + stdInstanceType = "t2.medium" + stdRegion = "us-west-2" + stdAvailabilityZone = "us-west-2b" ) func TestNewMetadataService(t *testing.T) { @@ -51,75 +48,66 @@ func TestNewMetadataService(t *testing.T) { validOutpostArn, _ := arn.Parse(strings.ReplaceAll(validRawOutpostArn, "outpost/", "")) testCases := []struct { - name string - isAvailable bool - isPartial bool - identityDocument ec2metadata.EC2InstanceIdentityDocument - rawOutpostArn string - outpostArn arn.ARN - getInstanceDocErr error - getOutpostArnErr error // We should keep this specific to outpost-arn until we need to use more endpoints - getNodeErr error - node v1.Node - nodeNameEnvVar string + name string + ec2metadataAvailable bool + clientsetReactors func(*fake.Clientset) + getInstanceIdentityDocumentValue ec2metadata.EC2InstanceIdentityDocument + getInstanceIdentityDocumentError error + invalidInstanceIdentityDocument bool + getMetadataValue string + getMetadataError error + expectedOutpostArn arn.ARN + expectedErr error + node v1.Node + nodeNameEnvVar string }{ { - name: "success: normal", - isAvailable: true, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ + name: "success: normal", + ec2metadataAvailable: true, + getInstanceIdentityDocumentValue: ec2metadata.EC2InstanceIdentityDocument{ InstanceID: stdInstanceID, InstanceType: stdInstanceType, Region: stdRegion, AvailabilityZone: stdAvailabilityZone, }, - getInstanceDocErr: nil, }, { - name: "success: outpost-arn is available", - isAvailable: true, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ + name: "success: outpost-arn is available", + ec2metadataAvailable: true, + getInstanceIdentityDocumentValue: ec2metadata.EC2InstanceIdentityDocument{ InstanceID: stdInstanceID, InstanceType: stdInstanceType, Region: stdRegion, AvailabilityZone: stdAvailabilityZone, }, - rawOutpostArn: validRawOutpostArn, - outpostArn: validOutpostArn, - getInstanceDocErr: nil, + getMetadataValue: validRawOutpostArn, + expectedOutpostArn: validOutpostArn, }, { - name: "success: outpost-arn is invalid", - isAvailable: true, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ + name: "success: outpost-arn is invalid", + ec2metadataAvailable: true, + getInstanceIdentityDocumentValue: ec2metadata.EC2InstanceIdentityDocument{ InstanceID: stdInstanceID, InstanceType: stdInstanceType, Region: stdRegion, AvailabilityZone: stdAvailabilityZone, }, - getInstanceDocErr: nil, + getMetadataValue: "foo", }, { - name: "success: outpost-arn is not found", - isAvailable: true, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ + name: "success: outpost-arn is not found", + ec2metadataAvailable: true, + getInstanceIdentityDocumentValue: ec2metadata.EC2InstanceIdentityDocument{ InstanceID: stdInstanceID, InstanceType: stdInstanceType, Region: stdRegion, AvailabilityZone: stdAvailabilityZone, }, - getInstanceDocErr: nil, - getOutpostArnErr: fmt.Errorf("404"), + getMetadataError: fmt.Errorf("404"), }, { - name: "success: metadata not available, used k8s api", - isAvailable: false, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ - InstanceID: stdInstanceID, - InstanceType: stdInstanceType, - Region: stdRegion, - AvailabilityZone: stdAvailabilityZone, - }, - getInstanceDocErr: nil, + name: "success: metadata not available, used k8s api", + ec2metadataAvailable: false, node: v1.Node{ TypeMeta: metav1.TypeMeta{ Kind: "Node", @@ -129,50 +117,34 @@ func TestNewMetadataService(t *testing.T) { Name: nodeName, }, Spec: v1.NodeSpec{ - ProviderID: "aws:///us-west-2b/i-abcdefgh123456789", + ProviderID: "aws:///" + stdAvailabilityZone + "/" + stdInstanceID, }, Status: v1.NodeStatus{}, }, nodeNameEnvVar: nodeName, }, { - name: "failure: metadata not available, k8s client error", - isAvailable: false, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ - InstanceID: stdInstanceID, - InstanceType: stdInstanceType, - Region: stdRegion, - AvailabilityZone: stdAvailabilityZone, + name: "failure: metadata not available, k8s client error", + ec2metadataAvailable: false, + clientsetReactors: func(clientset *fake.Clientset) { + clientset.PrependReactor("get", "*", func(action k8s_testing.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, fmt.Errorf("client failure") + }) }, - getInstanceDocErr: nil, - getNodeErr: fmt.Errorf("client failure"), - nodeNameEnvVar: nodeName, + expectedErr: fmt.Errorf("error getting Node %s: client failure", nodeName), + nodeNameEnvVar: nodeName, }, { - name: "failure: metadata not available, node name env var not set", - isAvailable: false, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ - InstanceID: stdInstanceID, - InstanceType: stdInstanceType, - Region: stdRegion, - AvailabilityZone: stdAvailabilityZone, - }, - getInstanceDocErr: nil, - getNodeErr: fmt.Errorf("instance metadata is unavailable and CSI_NODE_NAME env var not set"), - nodeNameEnvVar: "", + name: "failure: metadata not available, node name env var not set", + ec2metadataAvailable: false, + expectedErr: fmt.Errorf("CSI_NODE_NAME env var not set"), + nodeNameEnvVar: "", }, { - name: "failure: metadata not available, no provider ID", - isAvailable: false, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ - InstanceID: stdInstanceID, - InstanceType: stdInstanceType, - Region: stdRegion, - AvailabilityZone: stdAvailabilityZone, - }, - getInstanceDocErr: nil, - getNodeErr: fmt.Errorf("node providerID empty, cannot parse"), + name: "failure: metadata not available, no provider ID", + ec2metadataAvailable: false, + expectedErr: fmt.Errorf("node providerID empty, cannot parse"), node: v1.Node{ TypeMeta: metav1.TypeMeta{ Kind: "Node", @@ -189,16 +161,9 @@ func TestNewMetadataService(t *testing.T) { nodeNameEnvVar: nodeName, }, { - name: "failure: metadata not available, invalid region", - isAvailable: false, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ - InstanceID: stdInstanceID, - InstanceType: stdInstanceType, - Region: stdRegion, - AvailabilityZone: stdAvailabilityZone, - }, - getInstanceDocErr: nil, - getNodeErr: fmt.Errorf("did not find aws region in node providerID string"), + name: "failure: metadata not available, invalid region", + ec2metadataAvailable: false, + expectedErr: fmt.Errorf("did not find aws region in node providerID string"), node: v1.Node{ TypeMeta: metav1.TypeMeta{ Kind: "Node", @@ -215,16 +180,9 @@ func TestNewMetadataService(t *testing.T) { nodeNameEnvVar: nodeName, }, { - name: "failure: metadata not available, invalid az", - isAvailable: false, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ - InstanceID: stdInstanceID, - InstanceType: stdInstanceType, - Region: stdRegion, - AvailabilityZone: stdAvailabilityZone, - }, - getInstanceDocErr: nil, - getNodeErr: fmt.Errorf("did not find aws availability zone in node providerID string"), + name: "failure: metadata not available, invalid az", + ec2metadataAvailable: false, + expectedErr: fmt.Errorf("did not find aws availability zone in node providerID string"), node: v1.Node{ TypeMeta: metav1.TypeMeta{ Kind: "Node", @@ -241,16 +199,9 @@ func TestNewMetadataService(t *testing.T) { nodeNameEnvVar: nodeName, }, { - name: "failure: metadata not available, invalid instance id", - isAvailable: false, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ - InstanceID: stdInstanceID, - InstanceType: stdInstanceType, - Region: stdRegion, - AvailabilityZone: stdAvailabilityZone, - }, - getInstanceDocErr: nil, - getNodeErr: fmt.Errorf("did not find aws instance ID in node providerID string"), + name: "failure: metadata not available, invalid instance id", + ec2metadataAvailable: false, + expectedErr: fmt.Errorf("did not find aws instance ID in node providerID string"), node: v1.Node{ TypeMeta: metav1.TypeMeta{ Kind: "Node", @@ -267,143 +218,123 @@ func TestNewMetadataService(t *testing.T) { nodeNameEnvVar: nodeName, }, { - name: "fail: GetInstanceIdentityDocument returned error", - isAvailable: true, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ - InstanceID: stdInstanceID, - InstanceType: stdInstanceType, - Region: stdRegion, - AvailabilityZone: stdAvailabilityZone, - }, - getInstanceDocErr: fmt.Errorf(""), + name: "fail: GetInstanceIdentityDocument returned error", + ec2metadataAvailable: true, + getInstanceIdentityDocumentError: fmt.Errorf("foo"), + expectedErr: fmt.Errorf("could not get EC2 instance identity metadata: foo"), }, { - name: "fail: GetInstanceIdentityDocument returned empty instance", - isAvailable: true, - isPartial: true, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ + name: "fail: GetInstanceIdentityDocument returned empty instance", + ec2metadataAvailable: true, + getInstanceIdentityDocumentValue: ec2metadata.EC2InstanceIdentityDocument{ InstanceID: "", InstanceType: stdInstanceType, Region: stdRegion, AvailabilityZone: stdAvailabilityZone, }, - getInstanceDocErr: nil, + invalidInstanceIdentityDocument: true, + expectedErr: fmt.Errorf("could not get valid EC2 instance ID"), }, { - name: "fail: GetInstanceIdentityDocument returned empty region", - isAvailable: true, - isPartial: true, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ + name: "fail: GetInstanceIdentityDocument returned empty region", + ec2metadataAvailable: true, + getInstanceIdentityDocumentValue: ec2metadata.EC2InstanceIdentityDocument{ InstanceID: stdInstanceID, InstanceType: stdInstanceType, Region: "", AvailabilityZone: stdAvailabilityZone, }, - getInstanceDocErr: nil, + invalidInstanceIdentityDocument: true, + expectedErr: fmt.Errorf("could not get valid EC2 region"), }, { - name: "fail: GetInstanceIdentityDocument returned empty az", - isAvailable: true, - isPartial: true, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ + name: "fail: GetInstanceIdentityDocument returned empty az", + ec2metadataAvailable: true, + getInstanceIdentityDocumentValue: ec2metadata.EC2InstanceIdentityDocument{ InstanceID: stdInstanceID, InstanceType: stdInstanceType, Region: stdRegion, AvailabilityZone: "", }, - getInstanceDocErr: nil, + invalidInstanceIdentityDocument: true, + expectedErr: fmt.Errorf("could not get valid EC2 availability zone"), }, { - name: "fail: outpost-arn failed", - isAvailable: true, - identityDocument: ec2metadata.EC2InstanceIdentityDocument{ + name: "fail: outpost-arn failed", + ec2metadataAvailable: true, + getInstanceIdentityDocumentValue: ec2metadata.EC2InstanceIdentityDocument{ InstanceID: stdInstanceID, InstanceType: stdInstanceType, Region: stdRegion, AvailabilityZone: stdAvailabilityZone, }, - getInstanceDocErr: nil, - getOutpostArnErr: fmt.Errorf("405"), + getMetadataError: fmt.Errorf("405"), + expectedErr: fmt.Errorf("something went wrong while getting EC2 outpost arn: 405"), }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { clientset := fake.NewSimpleClientset(&tc.node) - if tc.name == "failure: metadata not available, k8s client error" { - clientset.PrependReactor("get", "*", func(action k8s_testing.Action) (handled bool, ret runtime.Object, err error) { - return true, nil, fmt.Errorf("client failure") - }) + clientsetInitialized := false + if tc.clientsetReactors != nil { + tc.clientsetReactors(clientset) } + mockCtrl := gomock.NewController(t) mockEC2Metadata := mocks.NewMockEC2Metadata(mockCtrl) - mockEC2Metadata.EXPECT().Available().Return(tc.isAvailable) - os.Setenv("CSI_NODE_NAME", tc.nodeNameEnvVar) - if tc.isAvailable { - mockEC2Metadata.EXPECT().GetInstanceIdentityDocument().Return(tc.identityDocument, tc.getInstanceDocErr) - } + ec2MetadataClient := func() (EC2Metadata, error) { return mockEC2Metadata, nil } + k8sAPIClient := func() (kubernetes.Interface, error) { clientsetInitialized = true; return clientset, nil } - if tc.isAvailable && tc.getInstanceDocErr == nil && !tc.isPartial { - mockEC2Metadata.EXPECT().GetMetadata(OutpostArnEndpoint).Return(tc.rawOutpostArn, tc.getOutpostArnErr) - } + mockEC2Metadata.EXPECT().Available().Return(tc.ec2metadataAvailable) + if tc.ec2metadataAvailable { + mockEC2Metadata.EXPECT().GetInstanceIdentityDocument().Return(tc.getInstanceIdentityDocumentValue, tc.getInstanceIdentityDocumentError) - m, err := NewMetadataService(mockEC2Metadata, clientset) - if tc.isAvailable && tc.getInstanceDocErr == nil && tc.getOutpostArnErr == nil && !tc.isPartial { - if err != nil { - t.Fatalf("NewMetadataService() failed: expected no error, got %v", err) + // GetMetadata is to get the outpost ARN. It should be skipped if + // GetInstanceIdentityDocument returns an error or (somehow?) partial + // output + if tc.getInstanceIdentityDocumentError == nil && !tc.invalidInstanceIdentityDocument { + if tc.getMetadataValue != "" || tc.getMetadataError != nil { + mockEC2Metadata.EXPECT().GetMetadata(OutpostArnEndpoint).Return(tc.getMetadataValue, tc.getMetadataError) + } else { + mockEC2Metadata.EXPECT().GetMetadata(OutpostArnEndpoint).Return("", fmt.Errorf("404")) + } } - - if m.GetInstanceID() != tc.identityDocument.InstanceID { - t.Fatalf("GetInstanceID() failed: expected %v, got %v", tc.identityDocument.InstanceID, m.GetInstanceID()) + if clientsetInitialized == true { + t.Errorf("kubernetes client was unexpectedly initialized when metadata is available!") + if len(clientset.Actions()) > 0 { + t.Errorf("kubernetes client was unexpectedly called! %v", clientset.Actions()) + } } + } - if m.GetInstanceType() != tc.identityDocument.InstanceType { - t.Fatalf("GetInstanceType() failed: expected %v, got %v", tc.identityDocument.InstanceType, m.GetInstanceType()) - } + os.Setenv("CSI_NODE_NAME", tc.nodeNameEnvVar) - if m.GetRegion() != tc.identityDocument.Region { - t.Fatalf("GetRegion() failed: expected %v, got %v", tc.identityDocument.Region, m.GetRegion()) + m, err := NewMetadataService(ec2MetadataClient, k8sAPIClient) + if err != nil { + if tc.expectedErr == nil { + t.Errorf("got error %q, expected no error", err) + } else if err.Error() != tc.expectedErr.Error() { + t.Errorf("got error %q, expected %q", err, tc.expectedErr) } - - if m.GetAvailabilityZone() != tc.identityDocument.AvailabilityZone { - t.Fatalf("GetAvailabilityZone() failed: expected %v, got %v", tc.identityDocument.AvailabilityZone, m.GetAvailabilityZone()) + } else { + if m == nil { + t.Fatalf("metadataService is unexpectedly nil!") } - - if m.GetOutpostArn() != tc.outpostArn { - t.Fatalf("GetOutpostArn() failed: expected %v, got %v", tc.outpostArn, m.GetOutpostArn()) + if m.GetInstanceID() != stdInstanceID { + t.Errorf("NewMetadataService() failed: got wrong instance ID %v, expected %v", m.GetInstanceID(), stdInstanceID) } - } else if !tc.isAvailable { - if tc.name == "success: metadata not available, used k8s api" { - if err != nil { - t.Fatalf("NewMetadataService() failed: expected no error, got %v", err) - } - if m.GetInstanceID() != nodeObjectInstanceID { - t.Fatalf("NewMetadataService() failed: got wrong instance ID %v, expected %v", m.GetInstanceID(), nodeObjectInstanceID) - } - if m.GetRegion() != "us-west-2" { - t.Fatalf("NewMetadataService() failed: got wrong region %v, expected %v", m.GetRegion(), "us-west-2") - } - if m.GetAvailabilityZone() != "us-west-2b" { - t.Fatalf("NewMetadataService() failed: got wrong AZ %v, expected %v", m.GetRegion(), "us-west-2b") - } - if m.GetOutpostArn() != tc.outpostArn { - t.Fatalf("GetOutpostArn() failed: got %v, expected %v", m.GetOutpostArn(), tc.outpostArn) - } - } else { - if err == nil { - t.Fatalf("NewMetadataService() failed: expected error but got nothing") - } - if err.Error() != tc.getNodeErr.Error() { - t.Fatalf("NewMetadataService() returned an unexpected error. Expected %v, got %v", tc.getNodeErr, err) - } + if m.GetRegion() != stdRegion { + t.Errorf("NewMetadataService() failed: got wrong region %v, expected %v", m.GetRegion(), stdRegion) } - } else { - if err == nil && tc.getOutpostArnErr == nil { - t.Fatal("NewMetadataService() failed: expected error when GetInstanceIdentityDocument returns partial data, got nothing") + if m.GetAvailabilityZone() != stdAvailabilityZone { + t.Errorf("NewMetadataService() failed: got wrong AZ %v, expected %v", m.GetAvailabilityZone(), stdAvailabilityZone) + } + if m.GetOutpostArn() != tc.expectedOutpostArn { + t.Errorf("GetOutpostArn() failed: got %v, expected %v", m.GetOutpostArn(), tc.expectedOutpostArn) } } - mockCtrl.Finish() }) } diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 21f37eb97f..367612b7e4 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -66,7 +66,7 @@ type controllerService struct { var ( // NewMetadataFunc is a variable for the cloud.NewMetadata function that can // be overwritten in unit tests. - NewMetadataFunc = cloud.NewMetadata + NewMetadataFunc = cloud.NewMetadataService // NewCloudFunc is a variable for the cloud.NewCloud function that can // be overwritten in unit tests. NewCloudFunc = cloud.NewCloud @@ -78,7 +78,7 @@ func newControllerService(driverOptions *DriverOptions) controllerService { region := os.Getenv("AWS_REGION") if region == "" { klog.V(5).Infof("[Debug] Retrieving region from metadata service") - metadata, err := NewMetadataFunc() + metadata, err := NewMetadataFunc(cloud.DefaultEC2MetadataClient, cloud.DefaultKubernetesAPIClient) if err != nil { panic(err) } diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index ca2f96115a..1af505b325 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -109,7 +109,7 @@ func TestNewControllerService(t *testing.T) { oldNewMetadataFunc := NewMetadataFunc defer func() { NewMetadataFunc = oldNewMetadataFunc }() - NewMetadataFunc = func() (cloud.MetadataService, error) { + NewMetadataFunc = func(cloud.EC2MetadataClient, cloud.KubernetesAPIClient) (cloud.MetadataService, error) { if tc.newMetadataFuncErrors { return nil, testErr } diff --git a/pkg/driver/node.go b/pkg/driver/node.go index 7b67a51271..c844cc4416 100644 --- a/pkg/driver/node.go +++ b/pkg/driver/node.go @@ -83,7 +83,7 @@ type nodeService struct { // it panics if failed to create the service func newNodeService(driverOptions *DriverOptions) nodeService { klog.V(5).Infof("[Debug] Retrieving node info from metadata service") - metadata, err := cloud.NewMetadata() + metadata, err := cloud.NewMetadataService(cloud.DefaultEC2MetadataClient, cloud.DefaultKubernetesAPIClient) if err != nil { panic(err) } diff --git a/tests/integration/setup_test.go b/tests/integration/setup_test.go index 10cec7b371..d1d3140ca3 100644 --- a/tests/integration/setup_test.go +++ b/tests/integration/setup_test.go @@ -32,6 +32,7 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "google.golang.org/grpc" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" ) @@ -110,7 +111,7 @@ func newMetadata() (cloud.MetadataService, error) { return nil, err } - return cloud.NewMetadataService(ec2metadata.New(s), fake.NewSimpleClientset()) + return cloud.NewMetadataService(func() (cloud.EC2Metadata, error) { return ec2metadata.New(s), nil }, func() (kubernetes.Interface, error) { return fake.NewSimpleClientset(), nil }) } func newEC2Client() (*ec2.EC2, error) {