Skip to content

Conversation

@mplsgrant
Copy link
Collaborator

The users need access to various resources to launch scenarios and view scenario logs.

I also included the "fixup namespace directory constants" here because it's needed.

Copy link
Collaborator

@josibake josibake left a comment

Choose a reason for hiding this comment

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

nice! left a few comments that I think would make this less verbose and some ideas about how to structure this long term.

from a permissions standpoint, I think its nice to have values.yaml not give anything by default, have namespace-defaults.yaml give the needed permissions for running a user in a namespace, and then namespaces.yaml can be used to apply overrides as needed.

Comment on lines +17 to +28
- apiGroups: [""]
resources: ["pods/log", "pods/exec", "pods/attach", "pods/portforward"]
verbs: ["get"]
- apiGroups: [""]
resources: ["configmaps", "secrets"]
verbs: ["get"]
- apiGroups: [""]
resources: ["persistentvolumeclaims"]
verbs: ["get", "list"]
- apiGroups: [""]
resources: ["events"]
verbs: ["get"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like these could all be set on the namespace-defaults.yaml file? that way we only specify rules in the namespaces.yaml when we want to override defaults for a particular user/namespace.

Comment on lines +17 to +28
verbs: ["get"]
- apiGroups: [""]
resources: ["persistentvolumeclaims"]
verbs: ["get", "list"]
- apiGroups: [""]
resources: ["events"]
verbs: ["get"]
- name: pod-manager
rules:
- apiGroups: [""]
resources: ["pods"]
verbs: ["get", "list", "watch", "create", "update", "delete"]
verbs: ["get", "list", "watch", "create", "delete", "update"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

longterm, we might want to have the values.yaml provide no permissions by default (or the most minimal possible), and then have sensible defaults in namespace-defaults.yaml , specific to the "war-games" use case. then namespaces.yaml is simply for overriding defaults as needed.

(what you have here is fine, more just thinking out loud of how we might want to structure this long term)

@josibake
Copy link
Collaborator

josibake commented Sep 7, 2024

#544 merged, can be rebased on main

Users need access to a number of resources so that they can run scenarios.
@mplsgrant mplsgrant force-pushed the 2024-09-users-configmaps branch from 6371898 to dbeadd6 Compare September 7, 2024 15:26
@mplsgrant
Copy link
Collaborator Author

Thanks for the feeback @josibake

I rebased on main but will leave the rest as-is for now. I like your ideas about trimming the files down.

@josibake
Copy link
Collaborator

josibake commented Sep 8, 2024

I rebased on main but will leave the rest as-is for now

Looking at this again, I do think the namespaces.yaml file needs to be de-duplicated before merging. Per https://github.com/bitcoin-dev-project/warnet/blob/main/docs/config.md, values.yaml is meant to be overridden by namespace-defaults.yaml and namespace-defaults.yaml is meant to be overridden by namespaces.yaml. What you have currently is applying the same defaults multiple times in namespaces.yaml, whereas what is currently in namespace-defaults.yaml is incorrect/outdated.

The reason this is important is a user is expected to to be able to create a custom-namespaces.yaml config and specify just the namespaces and users they want without requiring any additional config, because the appropriate permissions are defined in namespace-defaults.yaml. In the event a user wants to change defaults for all namespaces in their custom deployment, they would edit the namespace-defaults.yaml file. In the event they want to make a change for a specific user in a specific namespace, they would apply those changes in the custom-namespaces.yaml file.

@josibake
Copy link
Collaborator

Merging this as is for now (since its just changing defaults for configs) and will clean up in a follow up by me.

@josibake josibake merged commit 278a15b into bitcoin-dev-project:main Sep 11, 2024
@mplsgrant mplsgrant deleted the 2024-09-users-configmaps branch September 17, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants