fix: relax over-strict required params on five EC2 actions to match AWS (#474)

spinifex/gateway/ec2/natgw/CreateNatGateway.go

@@ -16,6 +16,11 @@ func ValidateCreateNatGatewayInput(input *ec2.CreateNatGatewayInput) error {
 	if input.SubnetId == nil || *input.SubnetId == "" {
 		return errors.New(awserrors.ErrorMissingParameter)
 	}
+	// Private NAT gateways (ConnectivityType=private) are not implemented.
+	if input.ConnectivityType != nil && *input.ConnectivityType == "private" {
+		return errors.New(awserrors.ErrorUnsupported)
+	}
+	// AllocationId is required for public NAT gateways, matching AWS.
 	if input.AllocationId == nil || *input.AllocationId == "" {
 		return errors.New(awserrors.ErrorMissingParameter)
 	}

spinifex/gateway/ec2/natgw/natgw_test.go

@@ -24,7 +24,10 @@ func TestValidateCreateNatGatewayInput(t *testing.T) {
 		{"empty SubnetId", &ec2.CreateNatGatewayInput{SubnetId: aws.String(""), AllocationId: aws.String("eipalloc-1")}, awserrors.ErrorMissingParameter},
 		{"missing AllocationId", &ec2.CreateNatGatewayInput{SubnetId: aws.String("subnet-1")}, awserrors.ErrorMissingParameter},
 		{"empty AllocationId", &ec2.CreateNatGatewayInput{SubnetId: aws.String("subnet-1"), AllocationId: aws.String("")}, awserrors.ErrorMissingParameter},
+		{"private connectivity unsupported", &ec2.CreateNatGatewayInput{SubnetId: aws.String("subnet-1"), ConnectivityType: aws.String("private")}, awserrors.ErrorUnsupported},
+		{"private connectivity unsupported with AllocationId", &ec2.CreateNatGatewayInput{SubnetId: aws.String("subnet-1"), AllocationId: aws.String("eipalloc-1"), ConnectivityType: aws.String("private")}, awserrors.ErrorUnsupported},
 		{"valid input", &ec2.CreateNatGatewayInput{SubnetId: aws.String("subnet-1"), AllocationId: aws.String("eipalloc-1")}, ""},
+		{"valid public connectivity", &ec2.CreateNatGatewayInput{SubnetId: aws.String("subnet-1"), AllocationId: aws.String("eipalloc-1"), ConnectivityType: aws.String("public")}, ""},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

spinifex/gateway/ec2/placementgroup/CreatePlacementGroup.go

@@ -16,9 +16,6 @@ func ValidateCreatePlacementGroupInput(input *ec2.CreatePlacementGroupInput) err
 	if input.GroupName == nil || *input.GroupName == "" {
 		return errors.New(awserrors.ErrorMissingParameter)
 	}
-	if input.Strategy == nil || *input.Strategy == "" {
-		return errors.New(awserrors.ErrorMissingParameter)
-	}
 	return nil
 }
 

spinifex/gateway/ec2/placementgroup/placementgroup_test.go

@@ -33,19 +33,19 @@ func TestCreatePlacementGroup_EmptyGroupName(t *testing.T) {
 	assert.EqualError(t, err, awserrors.ErrorMissingParameter)
 }
 
-func TestCreatePlacementGroup_NilStrategy(t *testing.T) {
-	_, err := CreatePlacementGroup(&ec2.CreatePlacementGroupInput{
+func TestCreatePlacementGroup_NilStrategy_PassesValidation(t *testing.T) {
+	err := ValidateCreatePlacementGroupInput(&ec2.CreatePlacementGroupInput{
 		GroupName: aws.String("my-group"),
-	}, nil, testAccountID)
-	assert.EqualError(t, err, awserrors.ErrorMissingParameter)
+	})
+	assert.NoError(t, err)
 }
 
-func TestCreatePlacementGroup_EmptyStrategy(t *testing.T) {
-	_, err := CreatePlacementGroup(&ec2.CreatePlacementGroupInput{
+func TestCreatePlacementGroup_EmptyStrategy_PassesValidation(t *testing.T) {
+	err := ValidateCreatePlacementGroupInput(&ec2.CreatePlacementGroupInput{
 		GroupName: aws.String("my-group"),
 		Strategy:  aws.String(""),
-	}, nil, testAccountID)
-	assert.EqualError(t, err, awserrors.ErrorMissingParameter)
+	})
+	assert.NoError(t, err)
 }
 
 func TestCreatePlacementGroup_NilNATS(t *testing.T) {

spinifex/gateway/ec2/routetable/AssociateRouteTable.go

@@ -16,9 +16,15 @@ func ValidateAssociateRouteTableInput(input *ec2.AssociateRouteTableInput) error
 	if input.RouteTableId == nil || *input.RouteTableId == "" {
 		return errors.New(awserrors.ErrorMissingParameter)
 	}
-	if input.SubnetId == nil || *input.SubnetId == "" {
+	hasSubnet := input.SubnetId != nil && *input.SubnetId != ""
+	hasGateway := input.GatewayId != nil && *input.GatewayId != ""
+	if !hasSubnet && !hasGateway {
 		return errors.New(awserrors.ErrorMissingParameter)
 	}
+	// Edge associations (GatewayId) are not implemented by the backend.
+	if hasGateway {
+		return errors.New(awserrors.ErrorUnsupported)
+	}
 	return nil
 }
 

spinifex/gateway/ec2/routetable/routetable_test.go

@@ -219,7 +219,9 @@ func TestValidateAssociateRouteTableInput(t *testing.T) {
 	}{
 		{"nil input", nil, awserrors.ErrorInvalidParameterValue},
 		{"missing RouteTableId", &ec2.AssociateRouteTableInput{SubnetId: aws.String("subnet-1")}, awserrors.ErrorMissingParameter},
-		{"missing SubnetId", &ec2.AssociateRouteTableInput{RouteTableId: aws.String("rtb-1")}, awserrors.ErrorMissingParameter},
+		{"missing SubnetId and GatewayId", &ec2.AssociateRouteTableInput{RouteTableId: aws.String("rtb-1")}, awserrors.ErrorMissingParameter},
+		{"empty SubnetId and GatewayId", &ec2.AssociateRouteTableInput{RouteTableId: aws.String("rtb-1"), SubnetId: aws.String(""), GatewayId: aws.String("")}, awserrors.ErrorMissingParameter},
+		{"GatewayId edge association unsupported", &ec2.AssociateRouteTableInput{RouteTableId: aws.String("rtb-1"), GatewayId: aws.String("igw-1")}, awserrors.ErrorUnsupported},
 		{"valid input", &ec2.AssociateRouteTableInput{RouteTableId: aws.String("rtb-1"), SubnetId: aws.String("subnet-1")}, ""},
 	}
 	for _, tt := range tests {

spinifex/gateway/ec2/volume/CreateVolume.go

@@ -14,7 +14,13 @@ func ValidateCreateVolumeInput(input *ec2.CreateVolumeInput) error {
 		return errors.New(awserrors.ErrorInvalidParameterValue)
 	}
 
-	if input.Size == nil || *input.Size < 1 || *input.Size > 16384 {
+	// Size may be omitted when restoring from a snapshot; the handler
+	// defaults to the snapshot size.
+	hasSnapshot := input.SnapshotId != nil && *input.SnapshotId != ""
+	if input.Size == nil && !hasSnapshot {
+		return errors.New(awserrors.ErrorMissingParameter)
+	}
+	if input.Size != nil && (*input.Size < 1 || *input.Size > 16384) {
 		return errors.New(awserrors.ErrorInvalidParameterValue)
 	}
 

spinifex/gateway/ec2/volume/CreateVolume_test.go

@@ -26,7 +26,7 @@ func TestValidateCreateVolumeInput(t *testing.T) {
 			name:    "EmptyInput",
 			input:   &ec2.CreateVolumeInput{},
 			wantErr: true,
-			errMsg:  awserrors.ErrorInvalidParameterValue,
+			errMsg:  awserrors.ErrorMissingParameter,
 		},
 		{
 			name: "ValidInput",
@@ -64,13 +64,40 @@ func TestValidateCreateVolumeInput(t *testing.T) {
 			errMsg:  awserrors.ErrorInvalidParameterValue,
 		},
 		{
-			name: "InvalidSize_Nil",
+			name: "InvalidSize_NilWithoutSnapshot",
 			input: &ec2.CreateVolumeInput{
 				AvailabilityZone: aws.String("ap-southeast-2a"),
 			},
 			wantErr: true,
+			errMsg:  awserrors.ErrorMissingParameter,
+		},
+		{
+			name: "ValidInput_NilSizeWithSnapshot",
+			input: &ec2.CreateVolumeInput{
+				AvailabilityZone: aws.String("ap-southeast-2a"),
+				SnapshotId:       aws.String("snap-12345678"),
+			},
+			wantErr: false,
+		},
+		{
+			name: "InvalidSize_ZeroWithSnapshot",
+			input: &ec2.CreateVolumeInput{
+				Size:             aws.Int64(0),
+				AvailabilityZone: aws.String("ap-southeast-2a"),
+				SnapshotId:       aws.String("snap-12345678"),
+			},
+			wantErr: true,
 			errMsg:  awserrors.ErrorInvalidParameterValue,
 		},
+		{
+			name: "InvalidSize_NilWithEmptySnapshot",
+			input: &ec2.CreateVolumeInput{
+				AvailabilityZone: aws.String("ap-southeast-2a"),
+				SnapshotId:       aws.String(""),
+			},
+			wantErr: true,
+			errMsg:  awserrors.ErrorMissingParameter,
+		},
 		{
 			name: "InvalidSize_TooLarge",
 			input: &ec2.CreateVolumeInput{

spinifex/gateway/ec2/volume/handler_test.go

@@ -16,7 +16,7 @@ func TestCreateVolume_ValidationErrors(t *testing.T) {
 	assert.EqualError(t, err, awserrors.ErrorInvalidParameterValue)
 
 	_, err = CreateVolume(&ec2.CreateVolumeInput{}, nil, "")
-	assert.EqualError(t, err, awserrors.ErrorInvalidParameterValue)
+	assert.EqualError(t, err, awserrors.ErrorMissingParameter)
 }
 
 func TestCreateVolume_NilNATS(t *testing.T) {

spinifex/gateway/ec2/vpc/ModifyNetworkInterfaceAttribute.go

@@ -16,9 +16,13 @@ func ValidateModifyNetworkInterfaceAttributeInput(input *ec2.ModifyNetworkInterf
 	if input.NetworkInterfaceId == nil || *input.NetworkInterfaceId == "" {
 		return errors.New(awserrors.ErrorMissingParameter)
 	}
-	if len(input.Groups) == 0 && input.Description == nil {
+	if len(input.Groups) == 0 && input.Description == nil && input.SourceDestCheck == nil {
 		return errors.New(awserrors.ErrorInvalidParameterValue)
 	}
+	// Disabling source/dest check is unsupported: OVN port security enforces it.
+	if input.SourceDestCheck != nil && input.SourceDestCheck.Value != nil && !*input.SourceDestCheck.Value {
+		return errors.New(awserrors.ErrorUnsupported)
+	}
 	return nil
 }
 

spinifex/gateway/ec2/vpc/ModifyNetworkInterfaceAttribute_test.go

@@ -33,6 +33,22 @@ func TestModifyNetworkInterfaceAttribute_NoAttributes(t *testing.T) {
 	assert.EqualError(t, err, awserrors.ErrorInvalidParameterValue)
 }
 
+func TestModifyNetworkInterfaceAttribute_SourceDestCheckTrue_PassesValidation(t *testing.T) {
+	err := ValidateModifyNetworkInterfaceAttributeInput(&ec2.ModifyNetworkInterfaceAttributeInput{
+		NetworkInterfaceId: aws.String("eni-abc123"),
+		SourceDestCheck:    &ec2.AttributeBooleanValue{Value: aws.Bool(true)},
+	})
+	assert.NoError(t, err)
+}
+
+func TestModifyNetworkInterfaceAttribute_SourceDestCheckFalse_Unsupported(t *testing.T) {
+	err := ValidateModifyNetworkInterfaceAttributeInput(&ec2.ModifyNetworkInterfaceAttributeInput{
+		NetworkInterfaceId: aws.String("eni-abc123"),
+		SourceDestCheck:    &ec2.AttributeBooleanValue{Value: aws.Bool(false)},
+	})
+	assert.EqualError(t, err, awserrors.ErrorUnsupported)
+}
+
 func TestModifyNetworkInterfaceAttribute_NilNATS(t *testing.T) {
 	_, err := ModifyNetworkInterfaceAttribute(&ec2.ModifyNetworkInterfaceAttributeInput{
 		NetworkInterfaceId: aws.String("eni-abc123"),

spinifex/handlers/ec2/instance/service_impl.go

@@ -1891,8 +1891,9 @@ func (s *InstanceServiceImpl) describeInstancesFromKV(input *ec2.DescribeInstanc
 	return &ec2.DescribeInstancesOutput{Reservations: reservations}, nil
 }
 
-// ModifyInstanceAttribute applies a single attribute change. SourceDestCheck
-// is a no-op on bare-metal VMs. InstanceType/UserData require a stopped instance.
+// ModifyInstanceAttribute applies a single attribute change. SourceDestCheck=true
+// is a no-op; false is unsupported because OVN port security enforces the check.
+// InstanceType/UserData require a stopped instance.
 func (s *InstanceServiceImpl) ModifyInstanceAttribute(input *ec2.ModifyInstanceAttributeInput, accountID string) (*ec2.ModifyInstanceAttributeOutput, error) {
 	if input.InstanceId == nil || *input.InstanceId == "" {
 		return nil, errors.New(awserrors.ErrorMissingParameter)
@@ -1900,7 +1901,10 @@ func (s *InstanceServiceImpl) ModifyInstanceAttribute(input *ec2.ModifyInstanceA
 	instanceID := *input.InstanceId
 
 	if input.SourceDestCheck != nil {
-		slog.Info("ModifyInstanceAttribute: accepting SourceDestCheck (no-op on bare metal)", "instanceId", instanceID)
+		if input.SourceDestCheck.Value != nil && !*input.SourceDestCheck.Value {
+			return nil, errors.New(awserrors.ErrorUnsupported)
+		}
+		slog.Info("ModifyInstanceAttribute: accepting SourceDestCheck=true (no-op)", "instanceId", instanceID)
 		return &ec2.ModifyInstanceAttributeOutput{}, nil
 	}
 

spinifex/handlers/ec2/instance/service_impl_test.go

@@ -1166,17 +1166,28 @@ func TestModifyInstanceAttribute_MissingInstanceID(t *testing.T) {
 	assert.Equal(t, awserrors.ErrorMissingParameter, err.Error())
 }
 
-func TestModifyInstanceAttribute_SourceDestCheckNoOp(t *testing.T) {
-	// SourceDestCheck succeeds without touching KV or requiring stopped state.
+func TestModifyInstanceAttribute_SourceDestCheckTrueNoOp(t *testing.T) {
+	// SourceDestCheck=true succeeds without touching KV or requiring stopped state.
 	svc := &InstanceServiceImpl{}
 	out, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{
 		InstanceId:      aws.String("i-sdc-001"),
-		SourceDestCheck: &ec2.AttributeBooleanValue{Value: aws.Bool(false)},
+		SourceDestCheck: &ec2.AttributeBooleanValue{Value: aws.Bool(true)},
 	}, "acc")
 	require.NoError(t, err)
 	require.NotNil(t, out)
 }
 
+func TestModifyInstanceAttribute_SourceDestCheckFalseUnsupported(t *testing.T) {
+	// Disabling is unsupported: OVN port security always enforces the check.
+	svc := &InstanceServiceImpl{}
+	_, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{
+		InstanceId:      aws.String("i-sdc-001"),
+		SourceDestCheck: &ec2.AttributeBooleanValue{Value: aws.Bool(false)},
+	}, "acc")
+	require.Error(t, err)
+	assert.Equal(t, awserrors.ErrorUnsupported, err.Error())
+}
+
 func TestModifyInstanceAttribute_NilStore(t *testing.T) {
 	svc := &InstanceServiceImpl{}
 	_, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{

spinifex/handlers/ec2/placementgroup/service_impl.go

@@ -78,9 +78,10 @@ func (s *PlacementGroupServiceImpl) CreatePlacementGroup(input *ec2.CreatePlacem
 		return nil, errors.New(awserrors.ErrorMissingParameter)
 	}
 
+	// AWS treats Strategy as optional, defaulting to cluster.
 	strategy := aws.StringValue(input.Strategy)
 	if strategy == "" {
-		return nil, errors.New(awserrors.ErrorMissingParameter)
+		strategy = ec2.PlacementStrategyCluster
 	}
 
 	// Only spread and cluster are supported; partition is rejected.

spinifex/handlers/ec2/placementgroup/service_impl_test.go

@@ -98,13 +98,23 @@ func TestCreatePlacementGroup_MissingName(t *testing.T) {
 	assert.Equal(t, awserrors.ErrorMissingParameter, err.Error())
 }
 
-func TestCreatePlacementGroup_MissingStrategy(t *testing.T) {
+func TestCreatePlacementGroup_MissingStrategyDefaultsToCluster(t *testing.T) {
 	svc := setupTestService(t)
-	_, err := svc.CreatePlacementGroup(&ec2.CreatePlacementGroupInput{
+	out, err := svc.CreatePlacementGroup(&ec2.CreatePlacementGroupInput{
 		GroupName: aws.String("no-strat"),
 	}, testAccountID)
-	require.Error(t, err)
-	assert.Equal(t, awserrors.ErrorMissingParameter, err.Error())
+	require.NoError(t, err)
+	assert.Equal(t, "cluster", *out.PlacementGroup.Strategy)
+}
+
+func TestCreatePlacementGroup_EmptyStrategyDefaultsToCluster(t *testing.T) {
+	svc := setupTestService(t)
+	out, err := svc.CreatePlacementGroup(&ec2.CreatePlacementGroupInput{
+		GroupName: aws.String("empty-strat"),
+		Strategy:  aws.String(""),
+	}, testAccountID)
+	require.NoError(t, err)
+	assert.Equal(t, "cluster", *out.PlacementGroup.Strategy)
 }
 
 func TestCreatePlacementGroup_SameNameDifferentAccounts(t *testing.T) {

spinifex/handlers/ec2/vpc/eni.go

@@ -268,14 +268,19 @@ func (s *VPCServiceImpl) deleteNetworkInterface(eniId, accountID string, force b
 	return &ec2.DeleteNetworkInterfaceOutput{}, nil
 }
 
-// ModifyNetworkInterfaceAttribute modifies ENI attributes (security groups, description).
+// ModifyNetworkInterfaceAttribute modifies ENI attributes (security groups,
+// description). SourceDestCheck=true is accepted as a no-op.
 func (s *VPCServiceImpl) ModifyNetworkInterfaceAttribute(input *ec2.ModifyNetworkInterfaceAttributeInput, accountID string) (*ec2.ModifyNetworkInterfaceAttributeOutput, error) {
 	if input.NetworkInterfaceId == nil || *input.NetworkInterfaceId == "" {
 		return nil, errors.New(awserrors.ErrorMissingParameter)
 	}
-	if len(input.Groups) == 0 && input.Description == nil {
+	if len(input.Groups) == 0 && input.Description == nil && input.SourceDestCheck == nil {
 		return nil, errors.New(awserrors.ErrorInvalidParameterValue)
 	}
+	// Disabling source/dest check is unsupported: OVN port security enforces it.
+	if input.SourceDestCheck != nil && input.SourceDestCheck.Value != nil && !*input.SourceDestCheck.Value {
+		return nil, errors.New(awserrors.ErrorUnsupported)
+	}
 
 	eniId := *input.NetworkInterfaceId
 	key := utils.AccountKey(accountID, eniId)

spinifex/handlers/ec2/vpc/eni_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/mulgadc/spinifex/spinifex/awserrors"
 	"github.com/mulgadc/spinifex/spinifex/testutil"
 	"github.com/nats-io/nats.go"
 	"github.com/stretchr/testify/assert"
@@ -564,6 +565,69 @@ func TestModifyNetworkInterfaceAttribute_Description(t *testing.T) {
 	assert.Equal(t, "updated description", *desc.NetworkInterfaces[0].Description)
 }
 
+func TestModifyNetworkInterfaceAttribute_SourceDestCheckOnly(t *testing.T) {
+	svc := setupTestVPCService(t)
+	vpcId := createTestVPC(t, svc, "10.0.0.0/16")
+	subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24")
+	eniId := createTestENI(t, svc, subnetId)
+
+	// New ENIs default to SourceDestCheck=true
+	desc, err := svc.DescribeNetworkInterfaces(&ec2.DescribeNetworkInterfacesInput{
+		NetworkInterfaceIds: []*string{aws.String(eniId)},
+	}, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, desc.NetworkInterfaces, 1)
+	assert.True(t, *desc.NetworkInterfaces[0].SourceDestCheck)
+
+	// SourceDestCheck=true as the sole attribute is an accepted no-op
+	_, err = svc.ModifyNetworkInterfaceAttribute(&ec2.ModifyNetworkInterfaceAttributeInput{
+		NetworkInterfaceId: aws.String(eniId),
+		SourceDestCheck:    &ec2.AttributeBooleanValue{Value: aws.Bool(true)},
+	}, testAccountID)
+	require.NoError(t, err)
+
+	// Disabling is unsupported: OVN port security always enforces the check
+	_, err = svc.ModifyNetworkInterfaceAttribute(&ec2.ModifyNetworkInterfaceAttributeInput{
+		NetworkInterfaceId: aws.String(eniId),
+		SourceDestCheck:    &ec2.AttributeBooleanValue{Value: aws.Bool(false)},
+	}, testAccountID)
+	require.Error(t, err)
+	assert.Equal(t, awserrors.ErrorUnsupported, err.Error())
+
+	desc, err = svc.DescribeNetworkInterfaces(&ec2.DescribeNetworkInterfacesInput{
+		NetworkInterfaceIds: []*string{aws.String(eniId)},
+	}, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, desc.NetworkInterfaces, 1)
+	assert.True(t, *desc.NetworkInterfaces[0].SourceDestCheck)
+}
+
+func TestModifyNetworkInterfaceAttribute_SourceDestCheckOnly_NoSGEvent(t *testing.T) {
+	svc, nc := setupTestVPCServiceWithNC(t)
+	vpcId := createTestVPC(t, svc, "10.0.0.0/16")
+	subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24")
+	eniId := createTestENI(t, svc, subnetId)
+
+	eventCh := make(chan *nats.Msg, 1)
+	sub, err := nc.Subscribe("vpc.update-port-sgs", func(msg *nats.Msg) {
+		eventCh <- msg
+	})
+	require.NoError(t, err)
+	defer func() { _ = sub.Unsubscribe() }()
+
+	_, err = svc.ModifyNetworkInterfaceAttribute(&ec2.ModifyNetworkInterfaceAttributeInput{
+		NetworkInterfaceId: aws.String(eniId),
+		SourceDestCheck:    &ec2.AttributeBooleanValue{Value: aws.Bool(true)},
+	}, testAccountID)
+	require.NoError(t, err)
+
+	select {
+	case <-eventCh:
+		t.Fatal("unexpected vpc.update-port-sgs event for source-dest-check-only modify")
+	case <-time.After(200 * time.Millisecond):
+	}
+}
+
 func TestModifyNetworkInterfaceAttribute_NoAttributes(t *testing.T) {
 	svc := setupTestVPCService(t)
 	vpcId := createTestVPC(t, svc, "10.0.0.0/16")