Merge ~mitchdz/ubuntu/+source/ec2-hibinit-agent:add-IMDSv2-mantic into ubuntu/+source/ec2-hibinit-agent:ubuntu/devel

[Mitchell Dzurick]

https://launchpad.net/~mitchdz/+archive/ubuntu/ec2-hibinit-agent-add-imdsv2-support

[Athos Ribeiro (athos-ribeiro)]

Hi, Thanks for submitting this one!

From the upstream commit in [1], I see that this is actually a backport (you had to edit the patch so it applies, right?). In this case, you can state that in the patch DEP3 header "Origin" field by replacing "upstream" with "backport".

Also, could you provide a PPA with a build of this package?

Finally, is there any specific reasons not to upgrade this to the latest upstream version (1.0.5) instead of backporting this patch here?

[1] https://github.com/aws/amazon-ec2-hibinit-agent/commit/9d9bca5c61fa9256289e68c88bd3747af2f62e28

review: Needs Information

[Mitchell Dzurick (mitchdz)]

Thank you Athos! I updated the DEP3 header. My PPA is in the MP message

- https://launchpad.net/~mitchdz/+archive/ubuntu/ec2-hibinit-agent-add-imdsv2-support

A discussion up upgrading to meet upstream did happen, and we decided to just backport this change for a few reasons: This package is not updated frequently, there's only a request to support the new metadata format, and our package has diverged from upstream, so it makes sense to make minimal changes here.

[Mitchell Dzurick (mitchdz)]

FYI in the main bug [1] I do have a Test Case in the Bug Description

[1] https://bugs.launchpad.net/ubuntu/+source/ec2-hibinit-agent/+bug/1941785

[Mitchell Dzurick (mitchdz)]

For a little extra information,

After creating an EC2 instance configured correctly you can add my PPA to test, and if you see `Running: swapoff /swap-hibinit` from the agent service and upgrade then that likely means everything is good.

$ sudo add-apt-repository ppa:mitchdz/ec2-hibinit-agent-add-imdsv2-support
$ sudo apt update -y && sudo apt upgrade -y
$ systemctl status hibinit-agent
○ hibinit-agent.service - EC2 instance hibernation setup agent
     Loaded: loaded (/lib/systemd/system/hibinit-agent.service; enabled; preset: enabled)
     Active: inactive (dead) since Wed 2023-05-17 17:53:54 UTC; 20s ago
   Duration: 1.209s
       Docs: file:/usr/share/doc/ec2-hibinit-agent/README
    Process: 2511 ExecStart=/usr/bin/hibinit-agent -c /etc/hibinit-config.cfg (code=exited, status=0/SUCCESS)
        CPU: 1.000s

May 17 17:53:54 ip-172-31-3-141 hibinit-agent[2513]: GRUB configuration is updated
May 17 17:53:54 ip-172-31-3-141 hibinit-agent[2513]: Setting swap device to 66305 with offset 749568
May 17 17:53:54 ip-172-31-3-141 hibinit-agent[2513]: GRUB configuration is updated
May 17 17:53:54 ip-172-31-3-141 hibinit-agent[2513]: Setting swap device to 66305 with offset 749568
May 17 17:53:54 ip-172-31-3-141 hibinit-agent[2513]: Done updating the swap offset. Turning swapoff
May 17 17:53:54 ip-172-31-3-141 hibinit-agent[2513]: Done updating the swap offset. Turning swapoff
May 17 17:53:54 ip-172-31-3-141 hibinit-agent[2513]: Running: swapoff /swap-hibinit
May 17 17:53:54 ip-172-31-3-141 hibinit-agent[2513]: Running: swapoff /swap-hibinit
May 17 17:53:54 ip-172-31-3-141 systemd[1]: hibinit-agent.service: Deactivated successfully.
May 17 17:53:54 ip-172-31-3-141 systemd[1]: hibinit-agent.service: Consumed 1.000s CPU time.

From this point, start a daemon process and then initiate hibernation, start the machine, and the daemon process should still be running. I do provide an example daemon process to use in the parent bug report.

[Athos Ribeiro (athos-ribeiro)]

Thanks for clarifying things here Mitchell!

IMHO, we should consider either moving to the new upstream version or re-evaluating this package.

For instance, the agent/hibinit-agent file being patched still has calls to os.errno which was removed from python since 3.7 [1] (focal has python 3.8). There are also several lines in the codebase where tabs are used instead of spaces (not by design, but by mistake) which can hinder the intended code paths.

I added a few more comments inline below. The patch LGTM given those are addressed. I still need to test this before we upload it though.

[1] https://docs.python.org/3.7/whatsnew/3.7.html#changes-in-the-python-api

[Athos Ribeiro (athos-ribeiro)]

Hi Mitchell, thanks for addressing my comments.

Here are my test results for mantic:

- I launched 2 EC2 instances with the same configuration

- I installed the fixed package from your PPA in one of them

- Then I started hibernation for both of them

- The instances eventually stopped, with both the affected and the fixed package. However, the instance with fixed package stops way earlier. The affected one hangs in the "stopping" state for a long time before it is stopped. While it hangs there, I can still ssh into it.

- After the affected instance finally halted, I re-started it and verified that the state was not preserved.

- After the fixed instance halted, I re-started it and verified that the state was indeed preserved.

review: Approve

[Athos Ribeiro (athos-ribeiro)]

Uploaded:

Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading ec2-hibinit-agent_1.0.0-0ubuntu14.dsc: done.
  Uploading ec2-hibinit-agent_1.0.0-0ubuntu14.debian.tar.xz: done.
  Uploading ec2-hibinit-agent_1.0.0-0ubuntu14_source.buildinfo: done.
  Uploading ec2-hibinit-agent_1.0.0-0ubuntu14_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index 87ea519..e3aaae8 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+ec2-hibinit-agent (1.0.0-0ubuntu14) mantic; urgency=medium
+
+   * d/p/lp1941785-Add-support-for-IMDSv2.patch: add IMDSv2 support
+     (LP: #1941785)
+
+ -- Mitchell Dzurick <[e-mail]>  Tue, 16 May 2023 06:58:08 -0700
+
 ec2-hibinit-agent (1.0.0-0ubuntu13) lunar; urgency=medium
 
   * d/p/fix-setuptools-package-discovery.patch: fix FTBFS with recent
diff --git a/debian/patches/lp1941785-Add-support-for-IMDSv2.patch b/debian/patches/lp1941785-Add-support-for-IMDSv2.patch
new file mode 100644
index 0000000..6855eb2
--- /dev/null
+++ b/debian/patches/lp1941785-Add-support-for-IMDSv2.patch
@@ -0,0 +1,161 @@
+Description: Add support for IMDSv2
+Author: Frederick Lefebvre <[e-mail]>
+Origin: backport, https://github.com/aws/amazon-ec2-hibinit-agent/commit/9d9bca5c61fa9256289e68c88bd3747af2f62e28
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/ec2-hibinit-agent/+bug/1941785
+Last-Update: 2023-05-16
+---
+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
+--- a/agent/hibinit-agent
++++ b/agent/hibinit-agent
+@@ -18,6 +18,7 @@
+ import sys
+ import syslog
+ import math
++import requests
+ from subprocess import check_call, check_output, STDOUT
+ from threading import Thread
+ from math import ceil
+@@ -25,11 +26,6 @@
+ 
+ 
+ try:
+-    from urllib.request import urlopen, Request
+-except ImportError:
+-    from urllib2 import urlopen, Request, HTTPError
+-
+-try:
+     from ConfigParser import ConfigParser, NoSectionError, NoOptionError
+ except:
+     from configparser import ConfigParser, NoSectionError, NoOptionError
+@@ -41,7 +37,12 @@
+ log_to_syslog = True
+ log_to_stderr = True
+ SWAP_FILE = '/swap-hibinit'
+-URL = "http://169.254.169.254/latest/meta-data/hibernation/configured"
++
++DEFAULT_STATE_DIR = '/var/lib/hibinit-agent'
++HIB_ENABLED_FILE = "hibernation-enabled"
++IMDS_BASEURL = 'http://169.254.169.254'
++IMDS_API_TOKEN_PATH = 'latest/api/token'
++IMDS_SPOT_ACTION_PATH = 'latest/meta-data/hibernation/configured'
+ 
+ def log(message):
+     if log_to_syslog:
+@@ -314,6 +315,9 @@
+             get_int('swap', 'percentage-of-ram'), args.swap_ram_percentage, 100)
+         self.swap_mb = self.merge(
+             get_int('swap', 'target-size-mb'), args.swap_target_size_mb, 4000)
++        self.state_dir = get('core', 'state-dir')
++        if self.state_dir is None:
++            self.state_dir = DEFAULT_STATE_DIR
+ 
+ 
+     def merge(self, cf_value, arg_value, def_val):
+@@ -337,31 +341,55 @@
+     def __str__(self):
+         return str(self.__dict__)
+ 
+-def hibernationEnabled():
+-     """Returns a boolean indicating whether hibernation is enabled or not."""
+-     response = None
+-     try:
+-         response = urlopen(URL)
+-         data = response.read()
+-         if data.lower() in ('false', b'false'):
+-             return False
+-     except:
+-         return False
+-     finally:
+-         if response:
+-             response.close()
+-     return True
++def get_imds_token(seconds=21600):
++    """ Get a token to access instance metadata. """
++    log("Requesting new IMDSv2 token.")
++    request_header = {'X-aws-ec2-metadata-token-ttl-seconds': str(seconds)}
++    token_url = '{}/{}'.format(IMDS_BASEURL, IMDS_API_TOKEN_PATH)
++    response = requests.put(token_url, headers=request_header)
++    response.close()
++    if response.status_code != 200:
++        return None
++
++    return response.text
++
++def create_state_dir(state_dir):
++    """ Create agent run dir if it doesn't exists."""
++    if not os.path.isdir(state_dir):
++        os.makedirs(state_dir)
++
++def hibernation_enabled(state_dir):
++    """Returns a boolean indicating whether hibernation is enabled or not.
++    Hibernation can't be enabled/disabled the instance launch. If we find
++    hibernation to be enabled, we create a semephore file so that we don't
++    have to probe IMDS again. That is useful when a instance is rebooted
++    after/if the IMDS http endpoint has been disabled.
++    """
++    hib_sem_file = os.path.join(state_dir, HIB_ENABLED_FILE)
++    if os.path.isfile(hib_sem_file):
++        log("Found {!r}, configuring hibernation".format(hib_sem_file))
++        return True
++
++    imds_token = get_imds_token()
++    if imds_token is None:
++        # IMDS http endpoint is disabled
++        return False
++
++    request_header = {'X-aws-ec2-metadata-token': imds_token}
++    response = requests.get("{}/{}".format(IMDS_BASEURL, IMDS_SPOT_ACTION_PATH),
++                 headers=request_header)
++    response.close()
++    if response.status_code != 200 or response.text.lower() == "false":
++        return False
++
++    log("Hibernation Configured Flag found")
++    os.mknod(hib_sem_file)
++
++    return True
++
+ 
+ def main():
+     
+-    if not hibernationEnabled():
+-        log("Instance Launch has not enabled Hibernation Configured Flag. hibinit-agent exiting!!")
+-        exit(0)
+-    # Validate if disk space>total RAM 
+-    ram_bytes = os.sysconf('SC_PAGE_SIZE') * os.sysconf('SC_PHYS_PAGES')
+-    if get_rootfs_size()<=(math.ceil(float(ram_bytes)/(1024*1024*1024))):
+-        log("Insufficient disk space. Cannot create setup for hibernation. Please allocate a larger root device")
+-        exit(1)
+     # Parse arguments
+     parser = argparse.ArgumentParser(description="An EC2 background process that creates a setup for instance hibernation "
+                                                  "at instance launch and also registers ACPI sleep event/actions")
+@@ -388,6 +416,17 @@
+     log_to_syslog = config.log_to_syslog
+ 
+     log("Effective config: %s" % config)
++    create_state_dir(config.state_dir)
++
++    # Let's first check if we even need to run
++    if not hibernation_enabled(config.state_dir):
++        log("Instance Launch has not enabled Hibernation Configured Flag. hibinit-agent exiting!!")
++        exit(0)
++    # Validate if disk space>total RAM
++    ram_bytes = os.sysconf('SC_PAGE_SIZE') * os.sysconf('SC_PHYS_PAGES')
++    if get_rootfs_size()<=(math.ceil(float(ram_bytes)/(1024*1024*1024))):
++        log("Insufficient disk space. Cannot create setup for hibernation. Please allocate a larger root device")
++        exit(1)
+ 
+     target_swap_size = config.swap_mb * 1024 * 1024
+     swap_percentage_size = ram_bytes * config.swap_percentage // 100
+--- a/etc/hibinit-config.cfg
++++ b/etc/hibinit-config.cfg
+@@ -11,6 +11,9 @@
+ # filesystems.
+ touch-swap = False
+ 
++# Location where to create any state files
++state-dir = "/var/lib/hibinit-agent"
++
+ [swap]
+ # If there's no swap then we create it to be equal to the specified
+ # percentage of RAM or to the target size, whichever is greater
diff --git a/debian/patches/series b/debian/patches/series
index f419db3..043841d 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -9,3 +9,4 @@ detect-hibernate-cmd-by-default.patch
 0010-Update-grub-configuration-when-it-needs-an-update.patch
 lp1968805-Swapon-with-maximum-priority-before-hibernation.patch
 fix-setuptools-package-discovery.patch
+lp1941785-Add-support-for-IMDSv2.patch