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

docs(proposals): add proposal for the ACL API #743

Closed
wants to merge 4 commits into from

Conversation

restanrm
Copy link
Contributor

@restanrm restanrm commented Aug 15, 2022

This Pull Request is a proposal for the ACL API that we could add to Headscale in order to improve usability of Headscale.

This is related to some issues:

Also the PR #581 is implementing part of the GET endpoint.

juanfont
juanfont previously approved these changes Sep 4, 2022
Copy link
Owner

@juanfont juanfont left a comment

Choose a reason for hiding this comment

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

This is very good. I like that it follows Tailscale API.

still maintain this behavior with a switch in the configuration stating that we
want to load the ACL's from a configured file if it exists. Also it's still
possible to apply configuration as code like terraform is doing with static
configuration loaded to destination with the API.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am trying to think if we could have something like "if nothing in the DB, then load from file", but that might be confusing to users as they might think they are updating it by updating on disk and restarting.

If we were to do a behaviour which is based on "keeping" the old behaviour, then I think it should use the API under the hood so we dont have to maintain too much duplicate code.

I would not be too oppose to removing the file behaviour, we just need to tell people a couple of versions in advance and write specific docs on how to load it now. That said, as an advocate for "config as code" and immutability, I do like the current setup where you do it in files.

Copy link
Owner

Choose a reason for hiding this comment

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

Can we keep the file as the actual storage for the ACLs, rather than in the DB?

Even if consumed by the API, users would still have a good old file to do changes too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this part I meant that the switch in the configuration is meant to overwrite the ACL's each time we reboot or reload the configuration. This is meant to keep the current behavior as using the file as a source of truth. This flag could even forbid changes to the rules stored in the database. I think it's interesting because not everyone wants to be use an API to manage the ACL's (I don't think you are the only one @kradalby 😄).

@juanfont I don't think that we can keep the actual storage for the ACL's. As an example for Headscale instances running in Kubernetes, it would require to much logic specific to Kubernetes to update the ConfigMap associated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we must add an option to chose between the config file and the database as, even if there's not an official support yet, a lot of peoples want to use Headscale with a GUI, and having the ACLs stored in a database will be more suitable for this purpose, but the "old techies" would like to have file, maybe we can add a condition like if there is a file we use the file (and having an API route to know if ACLs are managed in a file), else storing the config in the database.

And maybe having an error on editing the ACLs with the API as writing a file doesn't seam a good practice to me while dealing with an API.

What do you think ?


```console
headscale acl get --help
Get will retrieve the ACLs currently loaded in Headscale. Use the `-o json`
Copy link
Collaborator

Choose a reason for hiding this comment

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

And YAML

Copy link
Owner

Choose a reason for hiding this comment

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

♥️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course 😄

@loprima-l
Copy link
Contributor

@juanfont Can you merge the PL please ? Or close it because I think the PL is not active anymore

@kradalby kradalby closed this May 10, 2023
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.

4 participants