api: only set url field in config if previously unset #23785
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In #16872 we added support for configuring the API client with a unix domain socket. In order to set the host correctly, we parse the address before mutating the Address field in the configuration. But this prevents the configuration from being reused across multiple clients, as the next time we parse the address it will no longer be pointing to the socket. This breaks consumers like the autoscaler, which reuse the API config between plugins. Update the `NewClient` constructor to only override the `url` field if it hasn't already been parsed. Include a test demonstrating safe reuse with a unix domain socket. Ref: hashicorp/nomad-autoscaler#944 Ref: hashicorp/nomad-autoscaler#945
tgross
force-pushed
the
api-unix-socket-config
branch
from
155e371 to
c31a760
Compare
tgross
added
backport/1.8.x
gulducat
approved these changes
Aug 9, 2024
tgross
added a commit
to hashicorp/nomad-autoscaler
that referenced
this pull request
Sep 5, 2024
For #944 we fixed the Nomad API package so that it no longer mutated the private `url` field if previously set, which allowed reusing an `api.Config` object between clients when a unix domain socket was in use. However, the autoscaler plugins for Nomad strategy and target don't use the `api.Config` object we parse directly and instead get a map of string->string derived from that config so it can be passed over the go-plugin interface. This mapping did not account for the `Address` field being mutated when unix domain sockets are in use, so the bug was not actually fixed. Update the mapping to use the safe `URL()` method on the config, rather than reading the `Address` field. Fixes: #955 Ref: hashicorp/nomad#23785
tgross
added a commit
to hashicorp/nomad-autoscaler
that referenced
this pull request
Sep 5, 2024
For #944 we fixed the Nomad API package so that it no longer mutated the private `url` field if previously set, which allowed reusing an `api.Config` object between clients when a unix domain socket was in use. However, the autoscaler plugins for Nomad strategy and target don't use the `api.Config` object we parse directly and instead get a map of string->string derived from that config so it can be passed over the go-plugin interface. This mapping did not account for the `Address` field being mutated when unix domain sockets are in use, so the bug was not actually fixed. Update the mapping to use the safe `URL()` method on the config, rather than reading the `Address` field. Fixes: #955 Ref: hashicorp/nomad#23785
tgross
added a commit
to hashicorp/nomad-autoscaler
that referenced
this pull request
Sep 5, 2024
For #944 we fixed the Nomad API package so that it no longer mutated the private `url` field if previously set, which allowed reusing an `api.Config` object between clients when a unix domain socket was in use. However, the autoscaler plugins for Nomad strategy and target don't use the `api.Config` object we parse directly and instead get a map of string->string derived from that config so it can be passed over the go-plugin interface. This mapping did not account for the `Address` field being mutated when unix domain sockets are in use, so the bug was not actually fixed. Update the mapping to use the safe `URL()` method on the config, rather than reading the `Address` field. Fixes: #955 Ref: hashicorp/nomad#23785
tgross
added a commit
to hashicorp/nomad-autoscaler
that referenced
this pull request
Sep 11, 2024
For #944 we fixed the Nomad API package so that it no longer mutated the private `url` field if previously set, which allowed reusing an `api.Config` object between clients when a unix domain socket was in use. However, the autoscaler plugins for Nomad strategy and target don't use the `api.Config` object we parse directly and instead get a map of string->string derived from that config so it can be passed over the go-plugin interface. This mapping did not account for the `Address` field being mutated when unix domain sockets are in use, so the bug was not actually fixed. Update the mapping to use the safe `URL()` method on the config, rather than reading the `Address` field. Fixes: #955 Ref: hashicorp/nomad#23785
tgross
added a commit
that referenced
this pull request
Dec 10, 2024
In #16872 we added support for unix domain sockets, but this required mutating the `Config` when parsing the address. In #23785 we fixed a bug where if the configuration was used across multiple clients that mutation would happen multiple times and the address would be incorrectly parsed. When making `alloc log` or `alloc exec` calls to a region where the region is not "global", we create a new client from the same configuration and then set the address. But in this case we copy the private `url` field and that causes the URL parsing to be skipped for the new client. This results in the region always being set to the string literal `global` (because of mTLS handling code introduced all the way back in 4d3b75d), which fails with an error "no path to region" when the cluster isn't non-global and requests are sent to a non-leader. The "right" way of fixing this would be for `ClientConfig` not to change the region to global in the first place, but as this is a public API and extremely longstanding behavior, it could potentially be a breaking change for some downstream consumers. Instead, we'll avoid copying the private `url` field so that the new address is re-parsed. Fixes: #24635 Fixes: #24609 Ref: #16872 Ref: #23785 Ref: 4d3b75d
tgross
added a commit
that referenced
this pull request
Dec 10, 2024
In #16872 we added support for unix domain sockets, but this required mutating the `Config` when parsing the address so as to remove the port number. In #23785 we fixed a bug where if the configuration was used across multiple clients that mutation would happen multiple times and the address would be incorrectly parsed. When making `alloc log`, `alloc fs`, or `alloc exec` calls where we have line-of-sight to the client, we attempt to make a HTTP API call directly to the client node. So we create a new API client from the same configuration and then set the address. But in this case we copy the private `url` field and that causes the URL parsing to be skipped for the new client. This results in the region always being set to the string literal `"global"` (because of mTLS handling code introduced all the way back in 4d3b75d), unless the user has set the region specifically. This fails with an error "no path to region" when the cluster isn't non-global and requests are sent to a non-leader. Arguably the "right" way of fixing this would be for `ClientConfig` not to change the API client's region to `"global"` in the first place, but as this is a public API and extremely longstanding behavior, it could potentially be a breaking change for some downstream consumers. Instead, we'll avoid copying the private `url` field so that the new address is re-parsed. Fixes: #24635 Fixes: #24609 Ref: #16872 Ref: #23785 Ref: 4d3b75d
tgross
added a commit
that referenced
this pull request
Dec 10, 2024
In #16872 we added support for unix domain sockets, but this required mutating the `Config` when parsing the address so as to remove the port number. In #23785 we fixed a bug where if the configuration was used across multiple clients that mutation would happen multiple times and the address would be incorrectly parsed. When making `alloc log`, `alloc fs`, or `alloc exec` calls where we have line-of-sight to the client, we attempt to make a HTTP API call directly to the client node. So we create a new API client from the same configuration and then set the address. But in this case we copy the private `url` field and that causes the URL parsing to be skipped for the new client. This results in the region always being set to the string literal `"global"` (because of mTLS handling code introduced all the way back in 4d3b75d), unless the user has set the region specifically. This fails with an error "no path to region" when the cluster isn't non-global and requests are sent to a non-leader. Arguably the "right" way of fixing this would be for `ClientConfig` not to change the API client's region to `"global"` in the first place, but as this is a public API and extremely longstanding behavior, it could potentially be a breaking change for some downstream consumers. Instead, we'll avoid copying the private `url` field so that the new address is re-parsed. Fixes: #24635 Fixes: #24609 Ref: #16872 Ref: #23785 Ref: 4d3b75d
tgross
added a commit
that referenced
this pull request
Dec 10, 2024
In #16872 we added support for unix domain sockets, but this required mutating the `Config` when parsing the address so as to remove the port number. In #23785 we fixed a bug where if the configuration was used across multiple clients that mutation would happen multiple times and the address would be incorrectly parsed. When making `alloc log`, `alloc fs`, or `alloc exec` calls where we have line-of-sight to the client, we attempt to make a HTTP API call directly to the client node. So we create a new API client from the same configuration and then set the address. But in this case we copy the private `url` field and that causes the URL parsing to be skipped for the new client. This results in the region always being set to the string literal `"global"` (because of mTLS handling code introduced all the way back in 4d3b75d), unless the user has set the region specifically. This fails with an error "no path to region" when the cluster isn't non-global and requests are sent to a non-leader. Arguably the "right" way of fixing this would be for `ClientConfig` not to change the API client's region to `"global"` in the first place, but as this is a public API and extremely longstanding behavior, it could potentially be a breaking change for some downstream consumers. Instead, we'll avoid copying the private `url` field so that the new address is re-parsed. Fixes: #24635 Fixes: #24609 Ref: #16872 Ref: #23785 Ref: 4d3b75d
tgross
added a commit
that referenced
this pull request
Dec 16, 2024
In #16872 we added support for unix domain sockets, but this required mutating the `Config` when parsing the address so as to remove the port number. In #23785 we fixed a bug where if the configuration was used across multiple clients that mutation would happen multiple times and the address would be incorrectly parsed. When making `alloc log`, `alloc fs`, or `alloc exec` calls where we have line-of-sight to the client, we attempt to make a HTTP API call directly to the client node. So we create a new API client from the same configuration and then set the address. But in this case we copy the private `url` field and that causes the URL parsing to be skipped for the new client. This results in the region always being set to the string literal `"global"` (because of mTLS handling code introduced all the way back in 4d3b75d), unless the user has set the region specifically. This fails with an error "no path to region" when the cluster isn't non-global and requests are sent to a non-leader. Arguably the "right" way of fixing this would be for `ClientConfig` not to change the API client's region to `"global"` in the first place, but as this is a public API and extremely longstanding behavior, it could potentially be a breaking change for some downstream consumers. Instead, we'll avoid copying the private `url` field so that the new address is re-parsed. Fixes: #24635 Fixes: #24609 Ref: #16872 Ref: #23785 Ref: 4d3b75d
tgross
added a commit
that referenced
this pull request
Dec 16, 2024
In #16872 we added support for unix domain sockets, but this required mutating the `Config` when parsing the address so as to remove the port number. In #23785 we fixed a bug where if the configuration was used across multiple clients that mutation would happen multiple times and the address would be incorrectly parsed. When making `alloc log`, `alloc fs`, or `alloc exec` calls where we have line-of-sight to the client, we attempt to make a HTTP API call directly to the client node. So we create a new API client from the same configuration and then set the address. But in this case we copy the private `url` field and that causes the URL parsing to be skipped for the new client. This results in the region always being set to the string literal `"global"` (because of mTLS handling code introduced all the way back in 4d3b75d), unless the user has set the region specifically. This fails with an error "no path to region" when the cluster isn't non-global and requests are sent to a non-leader. Arguably the "right" way of fixing this would be for `ClientConfig` not to change the API client's region to `"global"` in the first place, but as this is a public API and extremely longstanding behavior, it could potentially be a breaking change for some downstream consumers. Instead, we'll avoid copying the private `url` field so that the new address is re-parsed. Fixes: #24635 Fixes: #24609 Ref: #16872 Ref: #23785 Ref: 4d3b75d
tgross
added a commit
that referenced
this pull request
Dec 16, 2024
In #16872 we added support for unix domain sockets, but this required mutating the `Config` when parsing the address so as to remove the port number. In #23785 we fixed a bug where if the configuration was used across multiple clients that mutation would happen multiple times and the address would be incorrectly parsed. When making `alloc log`, `alloc fs`, or `alloc exec` calls where we have line-of-sight to the client, we attempt to make a HTTP API call directly to the client node. So we create a new API client from the same configuration and then set the address. But in this case we copy the private `url` field and that causes the URL parsing to be skipped for the new client. This results in the region always being set to the string literal `"global"` (because of mTLS handling code introduced all the way back in 4d3b75d), unless the user has set the region specifically. This fails with an error "no path to region" when the cluster isn't non-global and requests are sent to a non-leader. Arguably the "right" way of fixing this would be for `ClientConfig` not to change the API client's region to `"global"` in the first place, but as this is a public API and extremely longstanding behavior, it could potentially be a breaking change for some downstream consumers. Instead, we'll avoid copying the private `url` field so that the new address is re-parsed. Fixes: #24635 Fixes: #24609 Ref: #16872 Ref: #23785 Ref: 4d3b75d
tgross
added a commit
that referenced
this pull request
Dec 16, 2024
…ddress (#24644) (#24682) In #16872 we added support for unix domain sockets, but this required mutating the `Config` when parsing the address so as to remove the port number. In #23785 we fixed a bug where if the configuration was used across multiple clients that mutation would happen multiple times and the address would be incorrectly parsed. When making `alloc log`, `alloc fs`, or `alloc exec` calls where we have line-of-sight to the client, we attempt to make a HTTP API call directly to the client node. So we create a new API client from the same configuration and then set the address. But in this case we copy the private `url` field and that causes the URL parsing to be skipped for the new client. This results in the region always being set to the string literal `"global"` (because of mTLS handling code introduced all the way back in 4d3b75d), unless the user has set the region specifically. This fails with an error "no path to region" when the cluster isn't non-global and requests are sent to a non-leader. Arguably the "right" way of fixing this would be for `ClientConfig` not to change the API client's region to `"global"` in the first place, but as this is a public API and extremely longstanding behavior, it could potentially be a breaking change for some downstream consumers. Instead, we'll avoid copying the private `url` field so that the new address is re-parsed. Fixes: #24635 Fixes: #24609 Ref: #16872 Ref: #23785 Ref: 4d3b75d Co-authored-by: Tim Gross <tgross@hashicorp.com>
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #16872 we added support for configuring the API client with a unix domain socket. In order to set the host correctly, we parse the address before mutating the Address field in the configuration. But this prevents the configuration from being reused across multiple clients, as the next time we parse the address it will no longer be pointing to the socket. This breaks consumers like the autoscaler, which reuse the API config between plugins.
Update the
NewClientconstructor to only override theurlfield if it hasn't already been parsed. Include a test demonstrating safe reuse with a unix domain socket.Ref: hashicorp/nomad-autoscaler#944
Ref: hashicorp/nomad-autoscaler#945
Note for reviewers: in some hypothetical "v2" of the API I'd like to have some kind of options or builder pattern for the client config where we can "consume" the config. But for the moment getting the
Configto be safely reusable seems like the best approach to avoid further backwards-incompatibility.