api: initial implementation of headless API (Bug 1941363) #194

cgsheeh · 2025-01-13T17:07:38Z

Build out the basic functionality of the headless API for Lando.
Using django-ninja we define two API endpoints, to POST automation
jobs and GET job statuses after submission. The API endpoints take
a set of actions defined in the request body which are stored in
the database for processing by a worker. Authentication is handled by
an API key associated with a user profile. A single action, add-commit
is implemented which can be used to test adding patches to the repo
as commits.

Build out the basic functionality of the headless API for Lando. Using django-ninja we define two API endpoints, to POST automation jobs and GET job statuses after submission. The API endpoints take a set of `actions` defined in the request body which are stored in the database for processing by a worker. Authentication is handled by an API key associated with a user profile. A single action, `add-commit` is implemented which can be used to test adding patches to the repo as commits.

zzzeid

Couple of very early comments:

the new api functionality would be better located in the lando.api app not the lando.main app. It could also go into a more specific headless_api app if we want to separate it from other functionality that was ported or will be ported from the old API.
I noticed some boilerplate functionality around generating / saving / getting API tokens, as well as some model fields to encrypt / decrypt / store those. Seems like potentially something that should be offloaded to the auth system (and the API framework), and not something that we should manually implement as libraries already exist that do this.
This PR should be broken up into multiple PRs (e.g., adding API functionality, adding "action" endpoints and worker functionality).

Again very early feedback based on a quick first pass.

cgsheeh · 2025-01-13T18:21:21Z

Thanks for looking it over, the PR is still a draft but I wanted to get something up to show progress.

* the new api functionality would be better located in the `lando.api` app not the `lando.main` app. It could also go into a more specific `headless_api` app if we want to separate it from other functionality that was ported or will be ported from the old API.

I had originally put the new code in lando.api but I ran into some issues with creating new models and referencing the existing models from them. In particular Django didn't like that I was trying to set a foreign key reference to the Repo model across apps. In the interest of staying focused on the headless API I moved everything into lando.main, but I will look into this further.

* I noticed some boilerplate functionality around generating / saving / getting API tokens, as well as some model fields to encrypt / decrypt / store those. Seems like potentially something that should be offloaded to the auth system (and the API framework), and not something that we should manually implement as libraries already exist that do this.

I just re-used the prior art around Phabricator API token storage/retrieval, but I haven't implemented anything for the API token generation. I was expecting to add the token management to the "settings page" where the Phab API tokens are managed.

* This PR should be broken up into multiple PRs (e.g., adding API functionality, adding "action" endpoints and worker functionality).

I will try and split out some of the changes around test fixtures, and perhaps the token generation/storage/etc could go in a separate PR. Moving the API definition out of the PR where the worker/job/etc is defined seems like it will make things harder to review since the API will do nothing but write to a DB, but I'll look into it.

shtrom

Looking good. Lots of half-baked musing in my comments. Feel free to consider or discard as you see fit (:

shtrom · 2025-01-21T05:35:08Z

src/lando/main/tests/test_automation_api.py

+    # TODO test a few more things? formatting?
+
+
+PATCH_NORMAL_1 = r"""


It might be worth grabbing this from the existing conftests (there's a normal_patch fixture that return the desired patch).

Also wondering if we should have a Git-formatted patch in the mix, too.

shtrom · 2025-01-21T05:38:17Z

src/lando/main/models/profile.py

+    def clear_lando_api_key(self):
+        """Set the Lando API key to an empty string and save."""
+        self.save_lando_api_key("")


Nit: I wonder if we'd be better making the key a random thing. I see there's a check to see if it's empty or not just above, but I feel it would be less error-prone to have a key that noone knows, rather than an empty string that one might forget to check in subsequent refactor.

Or maybe just make the key non decryptable (or the column nullable).

shtrom · 2025-01-21T05:40:12Z

src/lando/main/models/profile.py

@@ -77,6 +77,9 @@ class Meta:
    # Encrypted Phabricator API token.
    encrypted_phabricator_api_key = models.BinaryField(default=b"", blank=True)

+    # Encrypted API key.
+    encrypted_lando_api_key = models.BinaryField(default=b"", blank=True)


(Continuing my thought from the previous comment) I think we should make this column nullable, so that we can set it to None when clearing it, rather than encrypting an empty string, and having to check if the decrypted string is empty.

shtrom · 2025-01-21T05:41:06Z

src/lando/main/models/automation_job.py

+        AutomationJob, on_delete=models.CASCADE, related_name="actions"
+    )
+
+    action_type = models.CharField()


Would it be better as a models.ChoiceField?

shtrom · 2025-01-21T05:42:16Z

src/lando/main/models/automation_job.py

+class AutomationJob(BaseModel):
+    """An automation job.
+
+    TODO write better docstring


Suggested change

TODO write better docstring

TODO Write better docstring.

q:

shtrom · 2025-01-21T06:50:08Z

src/lando/api/legacy/workers/automation_worker.py

+        )
+
+    def run_automation_job(self, job: AutomationJob) -> bool:
+        """Run an automation job."""


Suggested change

"""Run an automation job."""

"""Run an automation job.

Returns True if the job is in a permanent failure state and should not be retried.

"""

shtrom · 2025-01-21T06:51:53Z

src/lando/api/legacy/workers/automation_worker.py

+        repo = job.target_repo
+        scm = repo.scm
+
+        # TODO should we check treestatus?


Wondering if TreeStatus should have a “CLOSING” state during which jobs like this are allowed to terminate and before the tree is fully closed.

This would mean no need to check here, but also clearer feedback in TreeStatus that the tree is still active (albeit draining).

shtrom · 2025-01-21T06:52:53Z

src/lando/api/legacy/workers/automation_worker.py

+                # TODO should we always update to the latest pull_path for a repo?
+                # or perhaps we need to specify some commit SHA?
+                scm.update_repo(repo.pull_path)


At the moment it cleans the repo back to a known state, and pulls the default branch.

shtrom · 2025-01-21T06:55:33Z

src/lando/api/legacy/workers/automation_worker.py

+    ) -> bool:
+        """Process an automation action."""
+        if action.action == "add-commit":
+            return self.add_commit_action(job, repo, scm, action)


Wondering the pro/cons of having the action on the ... err... Action object instead, so you could just do a action.run(job, repo, scm)

shtrom · 2025-01-21T06:57:44Z

src/lando/api/legacy/workers/automation_worker.py

+        patch_helper = HgPatchHelper(StringIO(action.content))
+
+        date = patch_helper.get_header("Date")
+        user = patch_helper.get_header("User")
+
+        try:


Wondering if there's a way to modularise the code from the landing worker, as this is essentially the same feature...

Maybe we could add the run() method as suggested in https://github.com/mozilla-conduit/lando/pull/194/files#r1923160292, and then just have the landing_worker also create an AddCommitAction and run it, so we have a single codepath for both.

Not request for change. Just comments.

zzzeid

A few more comments related to our discussion on tokens.

zzzeid · 2025-01-21T16:53:43Z

src/lando/main/api.py

+class HeadlessAPIAuthentication(HttpBearer):
+    """Authentication class to verify API token."""
+
+    def authenticate(self, request, token: str) -> str:


As we need a user name, I wonder if we'd be better off with Basic Auth, so we have a standard carrier for user+secret, rather than piggy-backing on the User-Agent (or other) header to pass the user identifier.

I agree with this. We can require a user-agent field, but we shouldn't use it as a username. At that point, we basically do have a username / password basic authentication.

However, if we want to continue with this username + token approach (which would have the added benefit of being able to manage the token more easily, than managing a user's password), I think it would make sense to create an api_token table to store these tokens in, and we can have a foreign key to a user profile. This is if we don't want to use any third party libraries.

zzzeid · 2025-01-21T16:55:02Z

src/lando/main/api.py

+        try:
+            user = User.objects.get(email=user_agent)
+        except User.DoesNotExist:
+            raise APIPermissionDenied(f"No user found for `User-Agent` {user_agent}")


I think it would be better to be more explicit about a username or email here, and not use the user-agent for it. The user agent should be something that can change (e.g., it may include a version number, for example).

zzzeid · 2025-01-21T16:59:15Z

src/lando/main/api.py

+        # some APIs may have authentication without user management. Our
+        # API tokens always correspond to a specific user, so set that on
+        # the request here.
+        request.user = user


Wondering if we should be using something like django.auth.login here to properly mark the user as authenticated. I.e., request.user.is_authenticated should be True.

zzzeid · 2025-01-21T17:01:09Z

src/lando/api/legacy/workers/automation_worker.py

+@contextmanager
+def job_processing(job: AutomationJob):
+    """Mutex-like context manager that manages job processing miscellany.
+
+    This context manager facilitates graceful worker shutdown, tracks the duration of
+    the current job, and commits changes to the DB at the very end.
+
+    Args:
+        job: the job currently being processed
+        db: active database session
+    """
+    start_time = datetime.now()
+    try:
+        yield
+    finally:
+        job.duration_seconds = (datetime.now() - start_time).seconds


This is copy-pasted, but should probably just be slightly modified and reused.

zzzeid · 2025-01-21T17:01:57Z

src/lando/api/legacy/workers/automation_worker.py

+            return
+
+        with job_processing(job):
+            job.status = LandingJobStatus.IN_PROGRESS


Is this a copy/paste error?

zzzeid · 2025-01-21T17:07:41Z

src/lando/main/api.py

+class HeadlessAPIAuthentication(HttpBearer):
+    """Authentication class to verify API token."""
+
+    def authenticate(self, request, token: str) -> str:


I think it would be more ideal if we could do proper authentication with only a token (i.e., fetch a user based on a valid token, which is typically what tokens are able to do.)

This could be done by means of adding a tokens table, and you could also possibly leverage Django's check_password and make_password functions for hashing.

zzzeid · 2025-01-21T17:09:21Z

src/lando/main/models/profile.py

@@ -77,6 +77,9 @@ class Meta:
    # Encrypted Phabricator API token.
    encrypted_phabricator_api_key = models.BinaryField(default=b"", blank=True)

+    # Encrypted API key.
+    encrypted_lando_api_key = models.BinaryField(default=b"", blank=True)


As mentioned elsewhere, this should probably go in a separate table. That way we can automatically track creation time, and more easily revoke keys (which should be added as a field token.is_valid or something like that.

Also, I think it's more secure to hash these tokens after showing them once to a user. Could leverage Django's django.contrib.auth.hashers for this too.

src/lando/main/api.py

zzzeid requested changes Jan 13, 2025

View reviewed changes

shtrom previously requested changes Jan 21, 2025

View reviewed changes

zzzeid requested changes Jan 21, 2025

View reviewed changes

shtrom reviewed Jan 22, 2025

View reviewed changes

src/lando/main/api.py Show resolved Hide resolved

shtrom mentioned this pull request Jan 23, 2025

landing_worker: add and use metadata from Revision rather than HgPatchHelper (bug 1936171) #200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: initial implementation of headless API (Bug 1941363) #194

api: initial implementation of headless API (Bug 1941363) #194

cgsheeh commented Jan 13, 2025

zzzeid left a comment

cgsheeh commented Jan 13, 2025

shtrom left a comment

shtrom Jan 21, 2025

shtrom Jan 21, 2025

shtrom Jan 21, 2025

shtrom Jan 21, 2025

shtrom Jan 21, 2025

shtrom Jan 21, 2025

shtrom Jan 21, 2025

shtrom Jan 21, 2025

shtrom Jan 21, 2025

shtrom Jan 21, 2025

shtrom Jan 21, 2025

shtrom Jan 23, 2025

zzzeid left a comment

zzzeid Jan 21, 2025

zzzeid Jan 21, 2025

zzzeid Jan 21, 2025

zzzeid Jan 21, 2025

zzzeid Jan 21, 2025

zzzeid Jan 21, 2025

zzzeid Jan 21, 2025

		# TODO test a few more things? formatting?


		PATCH_NORMAL_1 = r"""

api: initial implementation of headless API (Bug 1941363) #194

Are you sure you want to change the base?

api: initial implementation of headless API (Bug 1941363) #194

Conversation

cgsheeh commented Jan 13, 2025

zzzeid left a comment

Choose a reason for hiding this comment

cgsheeh commented Jan 13, 2025

shtrom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzzeid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment