Merge pull request #13318 from hpidcock/fix-1942948

#13318

Follow up to #13316 to correct testing of security groups and also handle revoke/delete of ec2 security groups.

## QA steps

- Bootstrap to aws with non-default VPC id
- Deploy charm
- Expose app
- Check sg has opened the ports
- Delete app check sg is deleted

## Documentation changes

N/A

## Bug reference

https://bugs.launchpad.net/juju/+bug/1942948

provider/ec2/environ.go

@@ -1859,7 +1859,6 @@ func (e *environ) closePortsInGroup(ctx context.ProviderCallContext, name string
 	}
 	_, err = e.ec2Client.RevokeSecurityGroupIngress(ctx, &ec2.RevokeSecurityGroupIngressInput{
 		GroupId:       g.GroupId,
-		GroupName:     g.GroupName,
 		IpPermissions: rulesToIPPerms(rules),
 	})
 	if err != nil {
@@ -2139,8 +2138,7 @@ var deleteSecurityGroupInsistently = func(client SecurityGroupCleaner, ctx conte
 		Clock:       clock,
 		Func: func() error {
 			_, err := client.DeleteSecurityGroup(ctx, &ec2.DeleteSecurityGroupInput{
-				GroupId:   g.GroupId,
-				GroupName: g.GroupName,
+				GroupId: g.GroupId,
 			})
 			if err == nil || isNotFoundError(err) {
 				logger.Debugf("deleting security group %q", aws.ToString(g.GroupName))

provider/ec2/internal/testing/securty_groups.go

@@ -48,6 +48,9 @@ func (srv *Server) CreateSecurityGroup(ctx context.Context, in *ec2.CreateSecuri
 func (srv *Server) DeleteSecurityGroup(ctx context.Context, in *ec2.DeleteSecurityGroupInput, opts ...func(*ec2.Options)) (*ec2.DeleteSecurityGroupOutput, error) {
 	srv.mu.Lock()
 	defer srv.mu.Unlock()
+	if in.GroupName != nil {
+		return nil, apiError("InvalidParameterValue", "group should only be accessed by id")
+	}
 	g := srv.group(types.GroupIdentifier{
 		GroupId:   in.GroupId,
 		GroupName: in.GroupName,
@@ -94,6 +97,9 @@ func (srv *Server) group(group types.GroupIdentifier) *securityGroup {
 func (srv *Server) AuthorizeSecurityGroupIngress(ctx context.Context, in *ec2.AuthorizeSecurityGroupIngressInput, opts ...func(*ec2.Options)) (*ec2.AuthorizeSecurityGroupIngressOutput, error) {
 	srv.mu.Lock()
 	defer srv.mu.Unlock()
+	if in.GroupName != nil {
+		return nil, apiError("InvalidParameterValue", "group should only be accessed by id")
+	}
 	g := srv.group(types.GroupIdentifier{
 		GroupId:   in.GroupId,
 		GroupName: in.GroupName,
@@ -121,6 +127,9 @@ func (srv *Server) AuthorizeSecurityGroupIngress(ctx context.Context, in *ec2.Au
 func (srv *Server) RevokeSecurityGroupIngress(ctx context.Context, in *ec2.RevokeSecurityGroupIngressInput, opts ...func(*ec2.Options)) (*ec2.RevokeSecurityGroupIngressOutput, error) {
 	srv.mu.Lock()
 	defer srv.mu.Unlock()
+	if in.GroupName != nil {
+		return nil, apiError("InvalidParameterValue", "group should only be accessed by id")
+	}
 	g := srv.group(types.GroupIdentifier{
 		GroupId:   in.GroupId,
 		GroupName: in.GroupName,

provider/ec2/local_test.go

@@ -74,17 +74,6 @@ func registerLocalTests() {
 	gc.Suite(&localNonUSEastSuite{})
 }
 
-var deleteSecurityGroupForTestFunc = func(client ec2.SecurityGroupCleaner, ctx context.ProviderCallContext, group types.GroupIdentifier, _ clock.Clock) error {
-	// With an exponential retry for deleting security groups,
-	// we never return from local live tests.
-	// No need to re-try in tests anyway - just call delete.
-	_, err := client.DeleteSecurityGroup(ctx, &awsec2.DeleteSecurityGroupInput{
-		GroupId:   group.GroupId,
-		GroupName: group.GroupName,
-	})
-	return err
-}
-
 // localLiveSuite runs tests from LiveTests using a fake
 // EC2 server that runs within the test process itself.
 type localLiveSuite struct {
@@ -109,7 +98,6 @@ func (t *localLiveSuite) SetUpSuite(c *gc.C) {
 	imagetesting.PatchOfficialDataSources(&t.BaseSuite.CleanupSuite, "test:")
 	t.BaseSuite.PatchValue(&imagemetadata.SimplestreamsImagesPublicKey, sstesting.SignedMetadataPublicKey)
 	t.BaseSuite.PatchValue(&keys.JujuPublicKey, sstesting.SignedMetadataPublicKey)
-	t.BaseSuite.PatchValue(ec2.DeleteSecurityGroupInsistently, deleteSecurityGroupForTestFunc)
 	t.srv.createRootDisks = true
 	t.srv.startServer(c)
 }
@@ -270,7 +258,6 @@ func (t *localServerSuite) SetUpSuite(c *gc.C) {
 	t.BaseSuite.PatchValue(&jujuversion.Current, coretesting.FakeVersionNumber)
 	t.BaseSuite.PatchValue(&arch.HostArch, func() string { return arch.AMD64 })
 	t.BaseSuite.PatchValue(&series.HostSeries, func() (string, error) { return jujuversion.DefaultSupportedLTS(), nil })
-	t.BaseSuite.PatchValue(ec2.DeleteSecurityGroupInsistently, deleteSecurityGroupForTestFunc)
 	t.srv.createRootDisks = true
 	t.srv.startServer(c)
 	// TODO(jam) I don't understand why we shouldn't do this.
@@ -564,7 +551,6 @@ func (t *localServerSuite) TestTerminateInstancesIgnoresNotFound(c *gc.C) {
 		})
 	c.Assert(err, jc.ErrorIsNil)
 
-	t.BaseSuite.PatchValue(ec2.DeleteSecurityGroupInsistently, deleteSecurityGroupForTestFunc)
 	insts, err := env.AllRunningInstances(t.callCtx)
 	c.Assert(err, jc.ErrorIsNil)
 	idsToStop := make([]instance.Id, len(insts)+1)
@@ -2063,7 +2049,6 @@ func (t *localNonUSEastSuite) SetUpSuite(c *gc.C) {
 
 	t.PatchValue(&imagemetadata.SimplestreamsImagesPublicKey, sstesting.SignedMetadataPublicKey)
 	t.PatchValue(&keys.JujuPublicKey, sstesting.SignedMetadataPublicKey)
-	t.BaseSuite.PatchValue(ec2.DeleteSecurityGroupInsistently, deleteSecurityGroupForTestFunc)
 }
 
 func (t *localNonUSEastSuite) TearDownSuite(c *gc.C) {