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

agent: support multiple http address in addresses.http #11582

Merged
merged 14 commits into from
Jan 3, 2022

Conversation

kevinschoonover
Copy link
Contributor

@kevinschoonover kevinschoonover commented Nov 29, 2021

naive partial support for #9257 (HTTP addresses only) by creating a http.Server for every space-seperated address defined in addresses.http. Example:

addresses {
  http="127.0.0.1 {{ GetPrivateIP }} ..."
}

Hoping to get some feedback if this is the approach that should be take or something like consul is doing by abstracting the servers into apiserver.go. There are also one or two TODOs I am not sure the best approach to take.

@vercel vercel bot temporarily deployed to Preview – nomad November 29, 2021 01:23 Inactive
@kevinschoonover kevinschoonover changed the title core: support multiple http address in 'http.address' agent: support multiple http address in 'http.address' Nov 29, 2021
@kevinschoonover kevinschoonover changed the title agent: support multiple http address in 'http.address' agent: support multiple http address in addresses.http Nov 29, 2021
@vercel vercel bot temporarily deployed to Preview – nomad December 4, 2021 02:00 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 4, 2021 04:02 Inactive
@tgross tgross self-requested a review December 8, 2021 21:22
@tgross tgross self-assigned this Dec 8, 2021
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @kevinschoonover! Thanks for this PR -- this is looking really good! I've left a few comments in command/agent/http.go where I think it'd be good to see what's safe to reuse across listeners.

Hoping to get some feedback if this is the approach that should be take or something like consul is doing by abstracting the servers into apiserver.go.

Nomad's has a bit of a stronger difference in the RPC server between servers and clients than Consul does, so I think trying to abstract it the way they've done would probably be more trouble than it's worth (especially at this point in time). The approach you've got here works fine for HTTP. I think RPC is going to prove to be more complicated if we try to do that.

command/agent/config_test.go Outdated Show resolved Hide resolved
command/agent/agent.go Outdated Show resolved Hide resolved
command/agent/http.go Outdated Show resolved Hide resolved
command/agent/http.go Outdated Show resolved Hide resolved
command/agent/http.go Outdated Show resolved Hide resolved
command/agent/http.go Outdated Show resolved Hide resolved
command/agent/http.go Outdated Show resolved Hide resolved
command/agent/testagent.go Outdated Show resolved Hide resolved
@kevinschoonover
Copy link
Contributor Author

Hey @tgross, anything left pending for me on this PR that I can help address?

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @kevinschoonover! I think I missed one last item and that should get this into good shape.

srv.registerHandlers(config.EnableDebug)
// NewHTTPServers starts an HTTP server for every address.http configured in
// the agent.
func NewHTTPServers(agent *Agent, config *Config) ([]HTTPServer, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I think I missed this in the first pass, but we'll want this to return a slice of pointers (ex []*HTTPServer) so that we're not changing the semantics of mutability on the servers. See https://play.golang.com/p/JzckJ_BBiXb as an example of why.

@kevinschoonover
Copy link
Contributor Author

@tgross should be good now and I reran the tests and everything looks good. One thing I did notice is that when I ran the command/agent tests I was seeing:

[WARN] freeport: 2 out of 2 pending ports are still in use; something probably didn't wait around for the port to be closed!

but I also see that on master so I think its unrelated to the changes.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @kevinschoonover!

@tgross tgross merged commit 0873e08 into hashicorp:main Jan 3, 2022
@tgross tgross added this to the 1.2.4 milestone Jan 3, 2022
@github-actions
Copy link

github-actions bot commented Nov 5, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

2 participants