Enable NWC to mutate workflows created by service accounts #16

iameskild · 2023-05-31T16:57:56Z

Reference Issues or PRs

This PR is a related to the work being done to integrate Jupyter-Scheduler (#1832).

What does this implement/fix?

Put a x in the boxes that apply

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds a feature)
Breaking change (fix or feature that would cause existing features not to work as expected)
Documentation Update
Code style update (formatting, renaming)
Refactoring (no functional changes, no API changes)
Build related changes
Other (please describe):

Testing

Did you test the pull request locally?
Did you add new tests?

Documentation

Access-centered content checklist

Text styling

The content is written with plain language (where relevant).
If there are headers, they use the proper header tags (with only one level-one header: H1 or # in markdown).
All links describe where they link to (for example, check the Nebari website).
This content adheres to the Nebari style guides.

Non-text content

All content is represented as text (for example, images need alt text, and videos need captions or descriptive transcripts).
If there are emojis, there are not more than three in a row.
Don't use flashing GIFs or videos.
If the content were to be read as plain text, it still makes sense, and no information is missing.

Any other comments?

Adam-D-Lewis · 2023-06-27T17:42:13Z

nebari_workflow_controller/utils.py

@@ -45,6 +48,28 @@ def sent_by_argo(workflow: dict):
    return None


+def valid_argo_roles():
+    # TODO: determine a more extensible way of generating a list of valid roles
+    return ["argo-admin", "argo-developer"]


I think this should be a config option somehow. Most other things in Nebari use traitlets, but I'd feel okay using an VALID_ARGO_ROLES ENV var for now.

Agreed, this is not a good solution. I added a configmap to the nebari-workflow-controller deployment that provides a list of valid Argo roles (here). How does that solution work for you?

Adam-D-Lewis · 2023-06-27T17:49:32Z

nebari_workflow_controller/utils.py

+            preferred_username = workflow["metadata"]["labels"][
+                "workflows.argoproj.io/creator-preferred-username"
+            ]
+            if validate_service_account(keycloak_uid):
+                for user in kcadm.get_users():
+                    if user["username"] == preferred_username:
+                        return user["id"], preferred_username


This won't work in some cases. You can't always go back to the actual username b/c "Labels only contain [-_.0-9a-zA-Z]", so any other characters will be turned into "-") so not a 1 to 1 mapping.

That's a good catch! I realize though that this is an issue elsewhere here. I added both the sanitize_label and desanitize_label functions to handle grabbing usernames from k8s labels. This is how JupyterHub converts usernames with disallowed characters (although I can't find the source code for how they accomplish this).

The two places where this is most important are:

when we list_namespaced_pods based on keycloak username (here)

when we grab the username from the label (here)

nebari_workflow_controller/utils.py

Adam-D-Lewis

see comments

…cess

Adam-D-Lewis · 2023-07-18T10:54:45Z

Have you run the tests? Could we add a test for the new functionality?

costrouc

LGTM. But like @Adam-D-Lewis mentioned it would be helpful to have tests for this worklfow.

iameskild · 2023-07-20T01:35:34Z

@costrouc @Adam-D-Lewis I added new tests that cover the special case where the Workflow was created via a service-account.

costrouc

@iameskild this are some detailed tests and I like the fixtures that you created in conftest.py. Thanks! LGTM.

costrouc · 2023-07-20T19:39:48Z

Since @Adam-D-Lewis is on PTO and we are trying to get a release out for nebari. I am comfortable with merging. @iameskild feel free to merge when it works best.

iameskild added 2 commits May 31, 2023 09:27

Allow Argo Service Accounts to create workflows

a93a14c

Simplify

a47e74f

iameskild mentioned this pull request May 31, 2023

Look into Jupyter Scheduler for possible integration into Nebari nebari-dev/nebari#1755

Closed

iameskild added 2 commits June 1, 2023 12:03

Extend lists in mutate_template

22cb60f

Modify mutate_template to also modify script templates

e6ef4b8

iameskild changed the title ~~[WIP] Enable NWC to mutate workflows created by service accounts~~ Enable NWC to mutate workflows created by service accounts Jun 13, 2023

iameskild requested a review from Adam-D-Lewis June 13, 2023 13:04

iameskild added area: user experience 👩🏻‍💻 needs: review 👀 This PR is complete and ready for reviewing type: enhancement 💅🏼 New feature or request labels Jun 13, 2023

iameskild mentioned this pull request Jun 13, 2023

Changes required for Jupyter-Scheduler integration nebari-dev/nebari#1832

Merged

10 tasks

Adam-D-Lewis reviewed Jun 27, 2023

View reviewed changes

nebari_workflow_controller/utils.py Outdated Show resolved Hide resolved

Adam-D-Lewis requested changes Jun 27, 2023

View reviewed changes

iameskild added 4 commits July 11, 2023 18:19

Add sanitize_label function

6d498f5

Ensure username-specific labels are properly sanitized

86f0fbe

Lint

1ceed5e

Use valid_argo_roles configmap to get valid roles

c921973

iameskild requested a review from Adam-D-Lewis July 12, 2023 20:01

iameskild added 3 commits July 12, 2023 13:55

Update valid-argo-roles name

6d2d447

Update valid_argo_roles fn, raise exception when user doesn't have ac…

051db78

…cess

lint

4ea1cd9

This comment was marked as outdated.

Sign in to view

costrouc requested changes Jul 18, 2023

View reviewed changes

Minor updates, add tests for special case

a3e51b0

iameskild requested a review from costrouc July 20, 2023 01:35

Add tests workflow

d2ce564

iameskild added 2 commits July 19, 2023 18:41

Update import ref

880f981

Add __init__, lint

98ff5ec

costrouc approved these changes Jul 20, 2023

View reviewed changes

iameskild merged commit aa6b897 into main Jul 20, 2023

iameskild deleted the 20230531eae branch July 20, 2023 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable NWC to mutate workflows created by service accounts #16

Enable NWC to mutate workflows created by service accounts #16

iameskild commented May 31, 2023 •

edited

Loading

Adam-D-Lewis Jun 27, 2023

iameskild Jul 12, 2023

Adam-D-Lewis Jun 27, 2023

iameskild Jul 12, 2023 •

edited

Loading

Adam-D-Lewis left a comment

Adam-D-Lewis commented Jul 18, 2023

This comment was marked as outdated.

costrouc left a comment

iameskild commented Jul 20, 2023

costrouc left a comment

costrouc commented Jul 20, 2023

Enable NWC to mutate workflows created by service accounts #16

Enable NWC to mutate workflows created by service accounts #16

Conversation

iameskild commented May 31, 2023 • edited Loading

Reference Issues or PRs

What does this implement/fix?

Testing

Documentation

Access-centered content checklist

Text styling

Non-text content

Any other comments?

Adam-D-Lewis Jun 27, 2023

Choose a reason for hiding this comment

iameskild Jul 12, 2023

Choose a reason for hiding this comment

Adam-D-Lewis Jun 27, 2023

Choose a reason for hiding this comment

iameskild Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Adam-D-Lewis left a comment

Choose a reason for hiding this comment

Adam-D-Lewis commented Jul 18, 2023

This comment was marked as outdated.

costrouc left a comment

Choose a reason for hiding this comment

iameskild commented Jul 20, 2023

costrouc left a comment

Choose a reason for hiding this comment

costrouc commented Jul 20, 2023

iameskild commented May 31, 2023 •

edited

Loading

iameskild Jul 12, 2023 •

edited

Loading