Add command to force tags on nodes and list tags #558

restanrm · 2022-04-21T18:37:32Z

read the CONTRIBUTING guidelines
raised a GitHub issue or discussed it on the projects chat beforehand
added unit tests
added integration tests
updated documentation if needed
updated CHANGELOG.md

This PR will fix #356 and fix #525

Work to be done before finalisation of this PR

Handle validation errors or normalisation of the name of the tags (tags must start with the tag: prefix)
this means that we should define a way to return errors in the protobuf. Should we define status and message ?
replace add/del in CLI by set
make a generic contains function
change updateDBMachine to UpdateTags
add integration tests
add documentation

Also change ID in proto for ForcedTags since the previous ID's should be reserved for commented fields

kradalby

I think this looks quite reasonable, some questions, some design.

gen/go/headscale/v1/headscale.pb.go

grpcv1.go

machine.go

cmd/headscale/cli/tags.go

acls_test.go

kradalby · 2022-04-22T20:24:45Z

machine.go

+	validTagMap := make(map[string]bool)
+	invalidTagMap := make(map[string]bool)
+	for _, tag := range machine.HostInfo.RequestTags {
+		owners, err := expandTagOwners(aclPolicy, tag, stripEmailDomain)


This whole part of the invalidTagMap really confuses me, and I cant really test what happens from the tests, could you try to elaborate?

What is bothering you ? The fact that I use a map or the concept of invalidTags ? I'm using a map to remove duplicated tags at insertion. It is possible for a machine to have multiple times the same tag (declared in tailscale up).

It doesnt necessary bother me, I just dont think I understand, what is an invalid and what is a valid?

Right, so an invalid tag is a tag added by a user that does'nt have permission to add tags. If joe (a simple user) can setup his computer to be a server (just because he is root on his laptop and added --advertise-tags=tag:server), then he can talk to the database directly even if he should not. So the validation of the tags are made with the TagOwners in the ACL file. The documentation of this feature has not been made yet. It seems necessary.

I feel that we need to explain this somewhere. Do you have any good location for this ?

hmm, maybe in the signature of the function 🤔

kradalby · 2022-04-22T20:27:31Z

And we will want some integration tests

…hines

Execute command doesn't fail, the result is passed in json content.

kradalby

I think this looks reasonable. Happy to get this in when things passes :)

utils.go

kradalby · 2022-05-15T11:05:25Z

machine.go

+	validTagMap := make(map[string]bool)
+	invalidTagMap := make(map[string]bool)
+	for _, tag := range machine.HostInfo.RequestTags {
+		owners, err := expandTagOwners(aclPolicy, tag, stripEmailDomain)


hmm, maybe in the signature of the function 🤔

…scale into feat-list-tags-of-machines

When running in CI, I obtained the following error: ``` Running [/home/runner/golangci-lint-1.41.0-linux-amd64/golangci-lint run --out-format=github-actions --new-from-patch=/tmp/tmp-1795-28vaWZek2jfM/pull.patch --new=false --new-from-rev=] in [] ... level=error msg="Running error: unknown linters: 'ireturn,maintidx', run 'golangci-lint linters' to see the list of supported linters" ```

kradalby · 2022-05-16T15:35:46Z

Let's merge this, but can you open a pr or issue with the golang ci snippets that's removed to we can debug that separately ?

I don't want to remove it, but we can remove and then add it back in.

restanrm · 2022-05-16T18:00:25Z

Let's merge this, but can you open a pr or issue with the golang ci snippets that's removed to we can debug that separately ?

I don't want to remove it, but we can remove and then add it back in.

You mean the version pinning in the GithubAction and the ignored linters ?

kradalby · 2022-05-16T18:03:24Z

You mean the version pinning in the GithubAction and the ignored linters ?

9f08212

This one

restanrm · 2022-05-16T18:10:04Z

oh, ok it didn't work anyway…

anuragbhatia · 2023-04-03T17:42:58Z

Can someone please share exact command for listing nodes?

headscale nodes list

Gives me nodes list but not the tags. Tried few others like headscale nodes tag list but did not work either.

blargg · 2023-05-13T23:16:59Z

headscale node list --tags will list the nodes with tag information. It's not shown in the default node list (without --tags).

restanrm and others added 13 commits April 15, 2022 16:00

feat: add forcedTags field and update proto

02f68eb

feat(cli): add tag subcommand to add and remove tags

9de9bc2

feat(acls): add support for forced tags

cd1d107

chore: apply format and lint

98f54c9

feat: add valid and invalid fields

587bdc7

Also change ID in proto for ForcedTags since the previous ID's should be reserved for commented fields

feat: add invalid and valid tags to grpc response

db1528b

feat: add unit tests and fmt

89a1a56

chore: fmt for grpc file

4fcc5e2

feat: print tags in nodes list

4651c44

Merge branch 'main' into feat-list-tags-of-machines

ea7bcff

fix: move tag command to subcommand of nodes

f53bb63

chore: fix lint

17d6624

fix: linting

b9fee36

kradalby reviewed Apr 22, 2022

View reviewed changes

kradalby and others added 9 commits April 22, 2022 22:27

Merge branch 'main' into feat-list-tags-of-machines

3a90079

feat: rewrite proto to only update tags of machine

31debf7

feat: update generated files

25f1dcf

feat: updating cli to match the set command

cc9eeda

feat: update functions to use set command

ea9aaa6

refact: use generics for contains functions

8061abe

fix: order error in the tests

3d30244

chore: apply linting

2c448d4

fix: fix linting issue on my computer

fec8cda

cryptagoras mentioned this pull request Apr 28, 2022

How to setup an unstrusted private Derper #564

Closed

restanrm added 5 commits May 3, 2022 20:35

Merge remote-tracking branch 'origin/main' into feat-list-tags-of-mac…

a2fb5b2

…hines

fix(go): add missing updated files

68417cc

fix: handle empty aclPolicy for integration tests

dc8c20e

fix: remove debug code

ad4401a

feat: return error if validation is failed

fdbc965

Merge remote-tracking branch 'origin/main' into feat-list-tags-of-mac…

72c1eda

…hines

restanrm marked this pull request as ready for review May 13, 2022 09:58

restanrm requested a review from juanfont as a code owner May 13, 2022 09:58

restanrm added 4 commits May 13, 2022 12:14

fix: loop over result machines instead of startup machines

49ec994

chore: update vendorSha after update of go.mod and go.sum

09836cd

fix: update tag in db if acl is enabled

fcdc292

fix: integration tests result

b511295

Execute command doesn't fail, the result is passed in json content.

kradalby reviewed May 15, 2022

View reviewed changes

kradalby and others added 11 commits May 15, 2022 12:11

Merge branch 'main' into feat-list-tags-of-machines

a28eebf

docs: add small documentation on getTags func

ca71830

Merge branch 'feat-list-tags-of-machines' of github.com:restanrm/head…

bc1909f

…scale into feat-list-tags-of-machines

fix: pin version of golangci-lint in GA

0445f40

fix: revert previous commit and add exclusion of linter

844ad15

feat: add golangci-lint in nix develop

852dc0f

fix: pin version of golangci-lint to match dev config

02ae7a0

chore: apply lint recommendations

4435a4f

fix: ignore exhaust linter

c4e69fe

fix: flake.nex update sha256

1158210

restanrm force-pushed the feat-list-tags-of-machines branch from 3000742 to 1158210 Compare May 16, 2022 13:26

fix: remove version pinning for golangci-lint it does not work

9f08212

Merge branch 'main' into feat-list-tags-of-machines

c9efd5c

kradalby approved these changes May 16, 2022

View reviewed changes

kradalby merged commit 747d64c into juanfont:main May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command to force tags on nodes and list tags #558

Add command to force tags on nodes and list tags #558

restanrm commented Apr 21, 2022 •

edited

Loading

kradalby left a comment

kradalby Apr 22, 2022

restanrm Apr 25, 2022

kradalby Apr 25, 2022

restanrm Apr 25, 2022 •

edited

Loading

restanrm May 13, 2022

kradalby May 15, 2022

kradalby commented Apr 22, 2022

kradalby left a comment

kradalby May 15, 2022

kradalby commented May 16, 2022

restanrm commented May 16, 2022

kradalby commented May 16, 2022

restanrm commented May 16, 2022

anuragbhatia commented Apr 3, 2023

blargg commented May 13, 2023

Add command to force tags on nodes and list tags #558

Add command to force tags on nodes and list tags #558

Conversation

restanrm commented Apr 21, 2022 • edited Loading

kradalby left a comment

Choose a reason for hiding this comment

kradalby Apr 22, 2022

Choose a reason for hiding this comment

restanrm Apr 25, 2022

Choose a reason for hiding this comment

kradalby Apr 25, 2022

Choose a reason for hiding this comment

restanrm Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

restanrm May 13, 2022

Choose a reason for hiding this comment

kradalby May 15, 2022

Choose a reason for hiding this comment

kradalby commented Apr 22, 2022

kradalby left a comment

Choose a reason for hiding this comment

kradalby May 15, 2022

Choose a reason for hiding this comment

kradalby commented May 16, 2022

restanrm commented May 16, 2022

kradalby commented May 16, 2022

restanrm commented May 16, 2022

anuragbhatia commented Apr 3, 2023

blargg commented May 13, 2023

restanrm commented Apr 21, 2022 •

edited

Loading

restanrm Apr 25, 2022 •

edited

Loading