Make Terminate force=True work with Stop Protection

[ijrsvt]

Closes #7597

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: ijrsvt / name: Ian Rodney (9c71184, bc90d88)

[ajkerrigan]

@ijrsvt - could you please complete the EasyCLA when you get a chance? There's a link in the comment from the cla bot.

[ajkerrigan]

FYI - those lint failures aren't due to anything you added. Looks like they're subtly wrong bits of code that pycodestyle started flagging in its latest version. Opened #7600 to track that.

[ijrsvt]

@ajkerrigan CLA signed!

[ajkerrigan]

This looks good to me, thanks for the thorough investigation with new & old botocore versions too 👍 .

[ajkerrigan]

One lingering question before merging this (@thisisshi @kapilt @darrendao may have thoughts): this effectively doubles the modify attribute calls for any policy that uses force: True, even if only a small portion of the target instance pool has stop or termination protection enabled. I'm wondering if it would make more sense to try terminating first and only trigger attribute mods if we hit an OperationNotPermitted error 🤔

[kapilt]

ideally we parse the failure to identify the instance and remove from the set, as it does feel like it would only be a fraction of the set in common case.

[ijrsvt]

ideally we parse the failure

@kapilt Is there any existing logic for that in cloud-custodian? I'm unsure of the stability of AWS error message strings.

[ijrsvt]

@kapilt I also noticed that the call to terminate instances[1] does not actually batch. I could add a function like[2] to only call disable termination protection on the instances in the batch that fails with OperationNotPermitted?

[1]

cloud-custodian/c7n/resources/ec2.py

Line 1544 in d91b6c4

self.manager.retry(

[2]

def process_terminate_instances(self, client, instances):
   try:
      return self.manager.retry(client.terminate_instances, InstanceIds=[i['InstanceId'] for i in instances]))
   except OperationNotPermitted:
      if self.data.get("force"):
            self.log.info("Disabling termination protection on instances")
            self.disable_deletion_protection(
                client,
                [i for i in instances if i.get('InstanceLifecycle') != 'spot'])
         return self.manager.retry(client.terminate_instances, InstanceIds=[i['InstanceId'] for i in instances]))
     else:
         raise

[kapilt]

@kapilt I also noticed that the call to terminate instances[1] does not actually batch.

fwiw, the terminate batch behavior was addressed in a separate pr #7607

[kapilt]

I'm okay with the merge as is, since it matches extant behavior of the implementation, but it would be nice as a separate pr to convert this to doing the disable on exception to minimize api calls.

[kapilt]

lgtm, thanks