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

acl token support in setKVValue #237

Open
DanielYWoo opened this issue May 3, 2022 · 0 comments
Open

acl token support in setKVValue #237

DanielYWoo opened this issue May 3, 2022 · 0 comments

Comments

@DanielYWoo
Copy link

Currently the client can use an acl token, e.g.,

	@Override
	public Response<Boolean> setKVBinaryValue(String key, byte[] value, String token, PutParams putParams, QueryParams queryParams) {
		Request request = Request.Builder.newBuilder()
			.setEndpoint("/v1/kv/" + key)
			.setToken(token) <---- here is the acl token as X-Consul-Token header
			.addUrlParameter(queryParams)
			.addUrlParameter(putParams)
			.setBinaryContent(value)
			.build();

The token will be later used by Utils.createTokenMap() and attached to the request header as X-Consul-Token. Good.

But in the string value of the same method, why do we use the token as a request parameter? And the token will not be attached to the request header, instead, it's attached as a request parameter. What is it for?

	@Override
	public Response<Boolean> setKVValue(String key, String value, String token, PutParams putParams, QueryParams queryParams) {
		UrlParameters tokenParam = token != null ? new SingleUrlParameters("token", token) : null; <-- what is this?
		HttpResponse httpResponse = rawClient.makePutRequest("/v1/kv/" + key, value, putParams, tokenParam, queryParams);	

mehiel pushed a commit to mehiel/consul-api that referenced this issue May 5, 2023
Consul now warns aggressively when token is provided as a request param
and the X-Consul-Token is required. Also as already mentioned in Ecwid#186
token request param will be removed in Consul v1.17.

By reading the code and related PRs I've seen that in the so called
"new http architecture" this is addressed although not all clients fully
utilize the new Request class.

This PR uses the Request class for both Catalog and KV clients.

Should address both Ecwid#186 and Ecwid#237.
lucwillems pushed a commit to lucwillems/consul-api that referenced this issue Mar 27, 2024
Consul now warns aggressively when token is provided as a request param
and the X-Consul-Token is required. Also as already mentioned in Ecwid#186
token request param will be removed in Consul v1.17.

By reading the code and related PRs I've seen that in the so called
"new http architecture" this is addressed although not all clients fully
utilize the new Request class.

This PR uses the Request class for both Catalog and KV clients.

Should address both Ecwid#186 and Ecwid#237.
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

No branches or pull requests

1 participant