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

VAULT-11830: Expand NodeStatusReporter with new fields #18302

Merged
merged 11 commits into from
Jan 7, 2023

Conversation

ccapurso
Copy link
Contributor

@ccapurso ccapurso commented Dec 9, 2022

The associated PR #18203 introduces the following fields to LinkedClusterNodeStatusResponse:

  • Hostname
  • ListenerAddresses
  • OperatingSystem
  • OperatingSystemVersion
  • LogLevel
  • ActiveTime
  • RaftStatus with underlying IsVoter field

This PR modifies the GetNodeStatus handler of NodeStatusReporter to includes these fields in the response.

Hostname, OperatingSystem, and OperatingSystemVersion are all obtained via host.InfoWithContext which is also used by the sys/host-info endpoint.

A few new Core methods have been added for the purpose of de-coupling via WrappedCoreNodeStatus:

  • ListenerAddresses surfaces the configured listener addresses
  • LogLevel surfaces the underlying Core.logLevel to expose the configured log level.
  • IsRaftVoter uses Core.raftInfo to determine if the node is a raft voter

TODO:

@ccapurso ccapurso marked this pull request as draft December 9, 2022 21:30
@ccapurso ccapurso added this to the 1.13.0-rc1 milestone Dec 16, 2022
@ccapurso ccapurso changed the title expand NodeStatusReporter with new fields VAULT-11830: Expand NodeStatusReporter with new fields Jan 4, 2023
Copy link
Contributor

@hghaf099 hghaf099 left a comment

Choose a reason for hiding this comment

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

Looks good

vault/core.go Outdated
}

listeners := conf.(*server.Config).Listeners
if lns == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am seeing a millions of GH warnings about an undefined lns :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@ccapurso I think this still needs to be addressed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh shoot! I fixed that but apparently I didn't commit and push. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it looks like I did and the build works locally. I'll dig in and try to figure that out.

@ccapurso ccapurso marked this pull request as ready for review January 5, 2023 18:41
vault/core.go Show resolved Hide resolved
@hghaf099
Copy link
Contributor

hghaf099 commented Jan 6, 2023

Looks good! Just a couple of last minor comments.

@ccapurso ccapurso merged commit 40e87ae into main Jan 7, 2023
@ccapurso ccapurso deleted the vault-11830-node-status-reporter branch January 7, 2023 01:53
AnPucel pushed a commit that referenced this pull request Jan 14, 2023
* expand NodeStatusReporter with new fields

* only call IsRaftVoter if using raft storage

* add changelog entry

* fix listeners

* return LogLevel as enum

* update github.com/hashicorp/vault/vault/hcp_link/proto

* add changelog entry

* bump github.com/hashicorp/vault/vault/hcp_link/proto

* go mod tidy
AnPucel pushed a commit that referenced this pull request Feb 3, 2023
* expand NodeStatusReporter with new fields

* only call IsRaftVoter if using raft storage

* add changelog entry

* fix listeners

* return LogLevel as enum

* update github.com/hashicorp/vault/vault/hcp_link/proto

* add changelog entry

* bump github.com/hashicorp/vault/vault/hcp_link/proto

* go mod tidy
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.

3 participants