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

added cli options to output ACLs #581

Closed
wants to merge 16 commits into from

Conversation

samson4649
Copy link

@samson4649 samson4649 commented May 12, 2022

  • 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

Drafted to Fix #492

Verified

This commit was signed with the committer’s verified signature.
chrisduerr Christian Duerr
@samson4649
Copy link
Author

Requires integration testing with separate instances of headscale per test or by dynamically loading ACLs into a single running instance (issue #582 )


return
}
policy := h.GetACLPolicy()
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't do, you need to go through the protobuf API for this. Add message type and response in proto folder, generate code via make generate then edit grpcv1.go to add server-side part

@samson4649
Copy link
Author

@restanrm - as discussed, changes have been made to implement this over the grpc server.

@samson4649 samson4649 marked this pull request as ready for review June 17, 2022 08:42
@restanrm
Copy link
Contributor

It behaves as expected what do you think of this @kradalby ?

@samson4649
Copy link
Author

This still needs some things fixed up - still working on it

@samson4649
Copy link
Author

g2g now - ready for review

@kradalby
Copy link
Collaborator

Hi, I am happy to give this a go, apologise for the delay, can you do a pass through the linters (proto, golangci and prettier) and then we can merge?

And update the hash in flake.nix (see the fail build)

@juanfont
Copy link
Owner

Hey @samson4649, apologies for the delay. Can you check the conflicts?

@juanfont
Copy link
Owner

juanfont commented Aug 9, 2022

@samson4649 can you check the (minor) conflicts? I think we are pretty ready to merge.

@juanfont
Copy link
Owner

@samson4649 we have the linters working again, and found a couple of minor things here.

There is also an issue with a dependency. Can you have a look? Thanks!!

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.

In addition to those comments, if you make changes to go.mod and go.sum, you need to update its checksum in flake.nix.

@@ -30,15 +30,15 @@ require (
golang.org/x/crypto v0.0.0-20220722155217-630584e8d5aa
golang.org/x/oauth2 v0.0.0-20220808172628-8227340efae7
golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4
google.golang.org/genproto v0.0.0-20220808204814-fd01256a5276
Copy link
Owner

Choose a reason for hiding this comment

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

Do not downgrade, please

google.golang.org/grpc v1.48.0
google.golang.org/protobuf v1.28.1
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c
gopkg.in/yaml.v2 v2.4.0
gopkg.in/yaml.v3 v3.0.1
gorm.io/driver/postgres v1.3.8
gorm.io/gorm v1.23.8
inet.af/netaddr v0.0.0-20220617031823-097006376321
Copy link
Owner

Choose a reason for hiding this comment

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

Downgrade :(

@@ -70,7 +70,7 @@ require (
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect
github.com/google/uuid v1.3.0 // indirect
github.com/gookit/color v1.5.0 // indirect
github.com/hashicorp/go-version v1.4.0 // indirect
Copy link
Owner

Choose a reason for hiding this comment

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

Same

return h.aclPolicy, nil
}

func ACLProtoToStruct(v *v1.ACLPolicy) (*ACLPolicy, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Here the linter is complaining

parameter name 'v' is too short for the scope of its usage (varnamelen)

Replace vfor policy or something like this, for instance.

protoHosts := make(map[string]string, len(*h))
for k, v := range *h {
protoHosts[k] = v.String()
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
}
}

protoHosts[k] = v.String()
}
return protoHosts
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
}
}

output,
)

return
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return

Copy link
Owner

Choose a reason for hiding this comment

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

not required

@juanfont juanfont added this to the v0.17.0 milestone Aug 15, 2022
@kradalby
Copy link
Collaborator

Closing this for now, it will require a large rebase after our code reorg work goes in. If picked up again, please open when ready.

@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.

Print out loaded ACLs
4 participants