Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: compatibility with BatchGetJobEntity API changes for jobRunAsUser #133

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

jusiskin
Copy link
Contributor

What was the problem/requirement? (What/Why)

The BatchGetJobEntity API response shape is changing. Specifically when the response contains "jobDetails" entities, the "jobRunAsUser" field will return a "runAs" field. For example:

{
  "entities": [
    {
      "jobDetails": {
        // ...
        "jobRunAsUser": {
          "posix": {
            "user": "myuser",
            "group": "myuser"
          },
          // New field below
          "runAs": "QUEUE_CONFIGURED_USER",
        }
        // ...
      }
    }
  ]
}

or if the job's queue has no queue configured user:

{
  "entities": [
    {
      "jobDetails": {
        // ...
        "jobRunAsUser": {
          // New field below
          "runAs": "WORKER_AGENT_USER",
        }
        // ...
      }
    }
  ]
}

This API change has not been deployed in the service yet. We need the worker agent to accomodate
the current API shape and the future API shape (forwards/backwards compatibility).

What was the solution? (How)

Updated the code to accomodate the future API change while still working with the current API
responses.

I've also updated some of the related developer scripts for testing this including the POSIX Docker
containers and the scripts to create service resources.

What is the impact of this change?

  • The Worker Agent is backwards/forwards compatible with the BatchGetJobEntity API changes that
    are about to land
  • Worker Agent developers can use the developer scripting for testing the Worker Agent's
    functional code that uses the queue jobRunAsUser feature.

How was this change tested?

"backwards" compatibility testing

Ran the Worker Agent integration tests, which assert that the jobRunAsUser worker agent
functionality works against the current service API responses.

"forwards" compatibility testing

  • Modified the scripts/create_service_resources.sh script to add the "runAs" when creating queues.
    This is now required in order to create a queue-fleet association, otherwise the API returns
    validation errors
  • Added a new POSIX testing container. This is similar to the posix_local_multiuser container
    except that the job user is not overridden

Used the src/deadline_worker_agent/boto/shim.py to mutate the API responses and return the new
API shape (includes the "runAs" field in the BatchGetJobEntity API responses).

I've also updated the unit tests to test both forwards and backwards compatibility.

Was this change documented?

No

Is this a breaking change?

No

@jusiskin jusiskin added the enhancement New feature or request label Jan 12, 2024
@jusiskin jusiskin requested a review from a team as a code owner January 12, 2024 18:12
@ddneilson ddneilson self-requested a review January 12, 2024 20:18
Comment on lines 98 to 99
user = job_run_as_user_posix["user"]
group = job_run_as_user_posix["group"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If posix isn't in the job_run_as_user_data, I think we get a KeyError here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye! I think that code path should not reachable because of earlier input validation:

https://github.com/casillas2/deadline-cloud-worker-agent/blob/40fcd7c7f5c3236a00c69e7535bfc662c9b76892/src/deadline_worker_agent/sessions/job_entities/job_details.py#L328-L331

Regardless, I have made this code more defensive in case someone accidentally changes/removes the validation code or my assumption is incorrect.

"runAs": "QUEUE_CONFIGURED_USER",
},
},
id="invalid new-style jobRunAsUser - empty user",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, I think we're trying to refrain from using invalid and to use non valid whenever possible. Applies elsewhere in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. I've replaced all instances of invalidnonvalid in the repo.

@jusiskin jusiskin force-pushed the jusiskin/job_run_as_user_api_changes branch from 40fcd7c to adb41ee Compare January 13, 2024 01:51
- provides forwards/backwards compatibility with the current API usage
  of the BatchGetJobEntity "jobDetails" entity responses
- Updates some developer test code and configuration to accomodate API
  changes to CreateQueue and to be able to end-to-end test these changes

Signed-off-by: Josh Usiskin <56369778+jusiskin@users.noreply.github.com>
@jusiskin jusiskin force-pushed the jusiskin/job_run_as_user_api_changes branch from adb41ee to c50caa9 Compare January 16, 2024 15:58
@jusiskin jusiskin merged commit 08d7ba6 into mainline Jan 16, 2024
9 checks passed
@jusiskin jusiskin deleted the jusiskin/job_run_as_user_api_changes branch January 16, 2024 16:04
gmchale79 pushed a commit that referenced this pull request Feb 12, 2024
…er (#133)

- provides forwards/backwards compatibility with the current API usage
  of the BatchGetJobEntity "jobDetails" entity responses
- Updates some developer test code and configuration to accomodate API
  changes to CreateQueue and to be able to end-to-end test these changes

Signed-off-by: Josh Usiskin <56369778+jusiskin@users.noreply.github.com>
gmchale79 pushed a commit that referenced this pull request Mar 11, 2024
…er (#133)

- provides forwards/backwards compatibility with the current API usage
  of the BatchGetJobEntity "jobDetails" entity responses
- Updates some developer test code and configuration to accomodate API
  changes to CreateQueue and to be able to end-to-end test these changes

Signed-off-by: Josh Usiskin <56369778+jusiskin@users.noreply.github.com>
Signed-off-by: Graeme McHale <gmchale@amazon.com>
jusiskin pushed a commit to jusiskin/deadline-cloud-worker-agent that referenced this pull request Sep 4, 2024
…tten (aws-deadline#133)

Problem:
 When starting a worker using PosixInstanceWorker it can sometimes be
the case that we query for the Worker's id before the worker.json file
has been written to disk. If this happens then the test will fail.

Solution:
 Repeatedly query for the worker.json file in a delaying loop, up to a
maximum of 10 queries after about a minute.

Signed-off-by: Daniel Neilson <53624638+ddneilson@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants