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 HostInfo output to Node response #1938

Closed
wants to merge 10 commits into from

Conversation

hopleus
Copy link
Contributor

@hopleus hopleus commented May 14, 2024

  • have read the CONTRIBUTING.md file
  • 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

Usecase

As an administrator, I want to see the information that is available only in HostInfo, for example, the version of the client's TailScale, the OS name, its version, etc.

@hopleus hopleus requested review from juanfont and kradalby as code owners May 14, 2024 11:24
@hopleus hopleus force-pushed the hostinfo-to-node branch from 9d4ced3 to c6ef22d Compare May 14, 2024 12:23
@hopleus
Copy link
Contributor Author

hopleus commented May 14, 2024

@juanfont, I saw your comment #1205 (comment) but I think that adding the base fields from HostInfo will not affect the HeadScale in any way, because it is unlikely that TailScale will change the fields used in any way

@kradalby
Copy link
Collaborator

Quoting the issue referred above:

After discussing it with @kradalby, we reckon it would be safer/saner for Headscale if you just marshal the Machine.HostInfo as a string into gRPC, rather than transforming it into a proto object in Headscale. This way, if something changes (and it might from Tailscale side) Headscale would not be affected.

We cannot maintain all these field hand typed into the API, however just marshalling it as JSON and then unmarshalling it on the other side using tailcfg.Hostinfo would be fine.

If this is implemented, it should also be part of the JSON output of headscale nodes list --json and it should have integration tests to verify the output.

As mentioned in our contribution guidelines, all contributions needs tests and there are a few more tickboxes that has not been addressed.

@hopleus hopleus requested a review from ohdearaugustin as a code owner May 14, 2024 18:38
@hopleus
Copy link
Contributor Author

hopleus commented May 14, 2024

Quoting the issue referred above:

After discussing it with @kradalby, we reckon it would be safer/saner for Headscale if you just marshal the Machine.HostInfo as a string into gRPC, rather than transforming it into a proto object in Headscale. This way, if something changes (and it might from Tailscale side) Headscale would not be affected.

We cannot maintain all these field hand typed into the API, however just marshalling it as JSON and then unmarshalling it on the other side using tailcfg.Hostinfo would be fine.

If this is implemented, it should also be part of the JSON output of headscale nodes list --json and it should have integration tests to verify the output.

As mentioned in our contribution guidelines, all contributions needs tests and there are a few more tickboxes that has not been addressed.

Replaced with marshaling in JSON (protobuf.Struct).
Now, in the api, as well as through the cli, what is stored in the database will be output, without any processing

hscontrol/types/node.go Outdated Show resolved Hide resolved
integration/cli_test.go Outdated Show resolved Hide resolved
proto/headscale/v1/node.proto Outdated Show resolved Hide resolved
@hopleus hopleus requested a review from kradalby May 16, 2024 19:07
hscontrol/types/node.go Outdated Show resolved Hide resolved
hscontrol/types/node.go Outdated Show resolved Hide resolved
integration/cli_test.go Outdated Show resolved Hide resolved
@hopleus hopleus requested a review from kradalby May 18, 2024 07:12
@tale
Copy link

tale commented Jul 23, 2024

@kradalby as someone who works on a web interface for Headscale I'm interested in seeing this PR merged. I'd be willing to also take it on and move it over the finish line if the original author is no longer available 😄

@kradalby
Copy link
Collaborator

kradalby commented Sep 5, 2024

@tale happy for you to give it a go.

I am not really sure what I want done atm, I really dont like having it as a string and then having the user having to unmarshal it after, it makes it all ver unergonomic.

I dont have a good solution, but I am more conservative to adding it than adding it for the sake of it. If you want to have a look and come up with a proposal I am willing to hear it.

@hopleus hopleus closed this Oct 5, 2024
@hopleus hopleus deleted the hostinfo-to-node branch October 5, 2024 15:38
@ArcticLampyrid
Copy link
Contributor

I dont have a good solution, but I am more conservative to adding it than adding it for the sake of it. If you want to have a look and come up with a proposal I am willing to hear it.

I respectfully disagree with this perspective. Regardless of its limitations, a unified mechanism to check the client version is essential as it helps mitigate security vulnerabilities. While avoiding action may reduce the risk of introducing errors, it unfortunately does not address the underlying issues.

We cannot maintain all these field hand typed into the API

May I ask why this is a concern? These fields are also utilized by Tailscale’s console, which is almost certainly stable. Following this approach would not introduce more compatibility issues than those present in Tailscale’s official instance.

@kradalby
Copy link
Collaborator

I respectfully disagree with this perspective. Regardless of its limitations, a unified mechanism to check the client version is essential as it helps mitigate security vulnerabilities. While avoiding action may reduce the risk of introducing errors, it unfortunately does not address the underlying issues.

I disagree with this perspective. If the version field specifically is needed, then I propose that being added.

May I ask why this is a concern? These fields are also utilized by Tailscale’s console, which is almost certainly stable. Following this approach would not introduce more compatibility issues than those present in Tailscale’s official instance.

The alternative is this: https://github.com/juanfont/headscale/pull/942/files#diff-ac94c91f8ea84762d55b6932ef1466a2165984591d3e646360c5bb31b2130900R248

The fields do change, manually managing drift is not an option.

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