Skip to content

ACL testing (#1803)#3005

Open
Janhouse wants to merge 1 commit intojuanfont:mainfrom
Janhouse:acl-testing
Open

ACL testing (#1803)#3005
Janhouse wants to merge 1 commit intojuanfont:mainfrom
Janhouse:acl-testing

Conversation

@Janhouse
Copy link
Copy Markdown

@Janhouse Janhouse commented Jan 12, 2026

Fixing #1803

@Janhouse
Copy link
Copy Markdown
Author

@kradalby said that maybe someone could give it a shot in #1803, so I did that. Golang is not my primary language so I did use Claude Code and tested with tests and on my running instance.
There are no integration tests though was not sure if they are needed here since it is mostly internal.

@Janhouse Janhouse force-pushed the acl-testing branch 2 times, most recently from 2672a2f to 1be0b25 Compare January 12, 2026 10:51
@kradalby
Copy link
Copy Markdown
Collaborator

@kradalby said that maybe someone could give it a shot in #1803, so I did that. Golang is not my primary language so I did use Claude Code and tested with tests and on my running instance.

Nice feature to get in, its quite a large change, and we need to be careful about it as we do not want to provide users with a false sense of security or confidence, but definitely something to work towards.

There are no integration tests though was not sure if they are needed here since it is mostly internal.

I agree with this assessment, it should live more or less in the policy package and not need integration.

That said, this will be an important one which we need test exhaustively.

Another important question is, should we implement this exhaustively? Should we support everything from "day one", and should implementing new things in the policy be blocked on supporting acl tests?

It's quite a large change, and I do have a lot to do, but will try to look at it in the upcoming weeks, I already have quite a backlog of other large PRs queued up.

@Janhouse
Copy link
Copy Markdown
Author

Another important question is, should we implement this exhaustively? Should we support everything from "day one", and should implementing new things in the policy be blocked on supporting acl tests?

I think it does not matter, this should not block adding new things in the policy. It uses the same filter rule resolution logic.

So if new autogroups or something like that are added to types.go with proper Resolve() implementation, it should still work fine. Of course if a new dimension is added to FilterRule then we would have to update it somewhat. But I don't expect that happening anytime soon. Not even sure if Tailscale itself has something other than source, destination and protocol.

Of course more testing is needed.

I also added the UI part in headplane, see the screenshots in tale/headplane#425

@kradalby
Copy link
Copy Markdown
Collaborator

Next release will focus on grants and be policy oriented, so I think it is reasonable to work on this pr and that in tandem, so will get to it at that point.

@kradalby
Copy link
Copy Markdown
Collaborator

kradalby commented Feb 7, 2026

There is some good potential here, as this release will be focused on policy stuff (mainly Grants), I think this would be good work to pursue, can you rebase it of the current main?

@Janhouse
Copy link
Copy Markdown
Author

Janhouse commented Feb 7, 2026

can you rebase it of the current main?

Rebased

@kradalby
Copy link
Copy Markdown
Collaborator

I just had a refresher on this as I hope to get to it within a couple of weeks.

In general the test engine looks reasonable, but I do not think we should add the CLI/API part.
The main goal of the tests is to keep them running every time to prevent and understand undesired behaviour and not one-offs, so we should not encourage that with the CLI.

The other part is that the CLI/API looks quite fragile and large to maintain, and it just does not seem worth it.

The cycle for checking a test should be to add it to the ACL and try to load the acl, we can add a check mode so the whole thing is evaluate (we might have that but can make it more safe for tests).

Initial thoughts, I'll start poking at it at some point.

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