-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
UI: [BUGFIX] Cope with service names that contain slashes #4756
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, test looks good to me so approved but was curious how this new method is being called.
return this.appendURL('health/service', [query.id], this.cleanQuery(query)); | ||
return this.appendURL(URL_PREFIX_SINGLE, [query.id], this.cleanQuery(query)); | ||
}, | ||
isQueryRecord: function(url, method) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you overriding an adapter method here? Curious why this new function wasn't called and couldn't find it in the adapter source after a quick look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it overrides a 'generic' method in the ApplicationAdapter. The ApplicationAdapter has a generic assumption for knowing whether a URL was to query a record (as oppose to query records). This Adapter inherits from that and therefore previously used the 'generic' isQueryRecord
, using this 'generic' method was the root of this bug.
The 'generic' assumption was that the service name was always the last 'part' of the URL (so /health/service/service-name
). That was the wrong assumption to make :) and the root of this bug, as people could have services with slashes in them meaning the last 'part' is no longer the full service name.
Curious why this new function wasn't called and couldn't find it in the adapter source after a quick look.
It's down in the handleResponse
method
consul/ui-v2/app/adapters/service.js
Lines 23 to 37 in d8c9ab3
handleResponse: function(status, headers, payload, requestData) { | |
let response = payload; | |
const method = requestData.method; | |
if (status === HTTP_OK) { | |
const url = this.parseURL(requestData.url); | |
switch (true) { | |
case this.isQueryRecord(url, method): | |
response = this.handleSingleResponse(url, { Nodes: response }, PRIMARY_KEY, SLUG_KEY); | |
break; | |
default: | |
response = this.handleBatchResponse(url, response, PRIMARY_KEY, SLUG_KEY); | |
} | |
} | |
return this._super(status, headers, response, requestData); | |
}, |
Previously we'd assumed that the last part of a slash split URL was the service name, services can contain slashes, so this is not the case.
We now detect a 'single service URL' by looking for the
health/service
prefix.Note: we still have to do this detection to be able to create proper cluster-wide unique id's using the
dc=dc-name
from the URL.We also updated/bumped
consul-api-double
to make it use the same rule (use everything followinghealth/service
as the service name instead of just the following 'dirname')Fixes: #4755