luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

ui: Don't default to the default namespace, use the token default namespace instead (#10503) 

The default namespace, and the tokens default namespace (or its origin namespace) is slightly more complicated than other things we deal with in the UI, there's plenty of info/docs on this that I've added in this PR.

Previously:

When a namespace was not specified in the URL, we used to default to the default namespace. When you logged in using a token we automatically forward you the namespace URL that your token originates from, so you are then using the namespace for your token by default. You can of course then edit the URL to remove the namespace portion, or perhaps revisit the UI at the root path with you token already set. In these latter cases we would show you information from the default namespace. So if you had no namespace segment/portion in the URL, we would assume default, perform actions against the default namespace and highlight the default namespace in the namespace selector menu. If you wanted to perform actions in your tokens origin namespace you would have to manually select it from the namespace selector menu.

This PR:

Now, when you have no namespace segment/portion in the URL, we use the token's origin namespace instead (and if you don't have a token, we then use the default namespace like it was previously)

Notes/thoughts:

I originally thought we were showing an incorrectly selected namespace in the namespace selector, but it also matched up with what we were doing with the API, so it was in fact correct. The issue was more that we weren't selecting the origin namespace of the token for the user when a namespace segment was omitted from the URL. Seeing as we automatically forward you to the tokens origin namespace when you log in, and we were correctly showing the namespace we were acting on when you had no namespace segment in the URL (in the previous case default), I'm not entirely sure how much of an issue this actually was.

This characteristic of namespace+token+namespace is a little weird and its easy to miss a subtlety or two so I tried to add some documentation in here for future me/someone else (including some in depth code comment around one of the API endpoints where this is very subtle and very hard to miss). I'm not the greatest at words, so would be great to get some edits there if it doesn't seem clear to folks.

The fact that we used to save your previous datacenter and namespace into local storage for reasons also meant the interaction here was slightly more complicated than it needed to be, so whilst we were here we rejigged things slightly to satisfy said reasons still but not use local storage (we try and grab the info from higher up). A lot of the related code here is from before we had our Routlets which I think could probably make all of this a lot less complicated, but I didn't want to do a wholesale replacement in this PR, we can save that for a separate PR on its own at some point.
This commit is contained in:
John Cowen 2021-07-07 11:46:41 +01:00 committed by GitHub
parent 93607fa2ee
commit d93d9d4e39
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
23 changed files with 188 additions and 105 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
.changelog/10503.txt Normal file
View File

@ -0,0 +1,4 @@
```release-note:bug
ui: Use the token's namespace instead of the default namespace when not
specifying a namespace in the URL
```

3
ui/packages/consul-ui/app/components/auth-form/index.hbs
View File

@ -75,8 +75,9 @@
<em>Contact your administrator for login credentials.</em>
</form>
{{#if (env 'CONSUL_SSO_ENABLED')}}
{{!-- This `or` can be completely removed post 1.10 as 1.10 has optional params with default values --}}
<DataSource
@src={{concat '/' (or nspace 'default') '/' dc '/oidc/providers'}}
@src={{concat '/' (or nspace '') '/' dc '/oidc/providers'}}
@onchange={{queue (action (mut providers) value="data")}}
@onerror={{queue (action (mut error) value="error.errors.firstObject")}}
@loading="lazy"

2
ui/packages/consul-ui/app/components/child-selector/index.hbs
View File

@ -9,7 +9,7 @@
<span><YieldSlot @name="label">{{yield}}</YieldSlot></span>
{{#if isOpen}}
<DataSource
@src={{concat '/' (or nspace 'default') '/' dc '/' (pluralize type)}}
@src={{concat '/' nspace '/' dc '/' (pluralize type)}}
@onchange={{action (mut allOptions) value="data"}}
/>
{{/if}}

3
ui/packages/consul-ui/app/components/token-source/index.hbs
View File

@ -1,6 +1,7 @@
<StateChart @src={{chart}} @initial={{if (eq type 'oidc') 'provider' 'secret'}} as |State Guard Action dispatch state|>
<Guard @name="isSecret" @cond={{action 'isSecret'}} />
{{#let (concat '/' (or nspace 'default') '/' dc) as |path|}}
{{!-- This `or` can be completely removed post 1.10 as 1.10 has optional params with default values --}}
{{#let (concat '/' (or nspace '') '/' dc) as |path|}}
<State @matches="secret">
<DataSource
@src={{concat path '/token/self/' value}}

26
ui/packages/consul-ui/app/routes/dc.js
View File

@ -5,23 +5,6 @@ import { get, action } from '@ember/object';
// TODO: We should potentially move all these nspace related things
// up a level to application.js
const findActiveNspace = function(nspaces, nspace) {
let found = nspaces.find(function(item) {
return item.Name === nspace.Name;
});
if (typeof found === 'undefined') {
// if we can't find the nspace that was saved
// try default
found = nspaces.find(function(item) {
return item.Name === 'default';
});
// if there is no default just choose the first
if (typeof found === 'undefined') {
found = nspaces.firstObject;
}
}
return found;
};
export default class DcRoute extends Route {
@service('repository/dc') repo;
@service('repository/permission') permissionsRepo;
@ -29,18 +12,11 @@ export default class DcRoute extends Route {
@service('settings') settingsRepo;
async model(params) {
const app = this.modelFor('application');
let [token, nspace, dc] = await Promise.all([
this.settingsRepo.findBySlug('token'),
this.nspacesRepo.getActive(this.optionalParams().nspace),
this.repo.findBySlug(params.dc, app.dcs),
this.repo.findBySlug(params.dc),
]);
// if there is only 1 namespace then use that
// otherwise find the namespace object that corresponds
// to the active one
nspace =
app.nspaces.length > 1 ? findActiveNspace(app.nspaces, nspace) : app.nspaces.firstObject;
// When disabled nspaces is [], so nspace is undefined
const permissions = await this.permissionsRepo.findAll({

2
ui/packages/consul-ui/app/routes/dc/kv/edit.js
View File

@ -21,7 +21,7 @@ export default class EditRoute extends Route {
const nspace = this.optionalParams().nspace;
return hash({
dc: dc,
nspace: nspace || 'default',
nspace: nspace,
parent:
typeof key !== 'undefined'
? this.repo.findBySlug({

61
ui/packages/consul-ui/app/routes/settings.js
View File

@ -1,33 +1,50 @@
import { inject as service } from '@ember/service';
import Route from 'consul-ui/routing/route';
import { hash } from 'rsvp';
import { get, set, action } from '@ember/object';
import { inject as service } from '@ember/service';
import { get, action } from '@ember/object';
export default class SettingsRoute extends Route {
@service('client/http')
client;
@service('client/http') client;
@service('settings')
repo;
@service('settings') repo;
@service('repository/dc') dcRepo;
@service('repository/permission') permissionsRepo;
@service('repository/nspace/disabled') nspacesRepo;
@service('repository/dc')
dcRepo;
async model(params) {
// reach up and grab things from the application route/controller
const app = this.controllerFor('application');
@service('repository/nspace/disabled')
nspacesRepo;
// figure out if we have anything missing for menus etc and get them if
// so, otherwise just use what they already are
const [item, dc] = await Promise.all([
this.repo.findAll(),
typeof app.dc === 'undefined' ? this.dcRepo.getActive() : app.dc,
]);
const nspace =
typeof app.nspace === 'undefined'
? await this.nspacesRepo.getActive(item.nspace)
: app.nspace;
const permissions =
typeof app.permissions === 'undefined'
? await this.permissionsRepo.findAll({
dc: dc.Name,
nspace: nspace.Name,
})
: app.permissions;
model(params) {
const app = this.modelFor('application');
return hash({
item: this.repo.findAll(),
dc: this.dcRepo.getActive(undefined, app.dcs),
nspace: this.nspacesRepo.getActive(),
}).then(model => {
if (typeof get(model.item, 'client.blocking') === 'undefined') {
set(model, 'item.client', { blocking: true });
}
return model;
// reset the things higher up in the application if they were already set
// this won't do anything
this.controllerFor('application').setProperties({
dc: dc,
nspace: nspace,
token: item.token,
permissions: permissions,
});
if (typeof get(item, 'client.blocking') === 'undefined') {
item.client = { blocking: true };
}
return { item };
}
setupController(controller, model) {

2
ui/packages/consul-ui/app/serializers/application.js
View File

@ -11,7 +11,7 @@ import { FOREIGN_KEY as DATACENTER_KEY } from 'consul-ui/models/dc';
import { NSPACE_KEY } from 'consul-ui/models/nspace';
import createFingerprinter from 'consul-ui/utils/create-fingerprinter';
const DEFAULT_NSPACE = 'default';
const DEFAULT_NSPACE = '';
const map = function(obj, cb) {
if (!Array.isArray(obj)) {

18
ui/packages/consul-ui/app/services/repository/dc.js
View File

@ -18,11 +18,11 @@ export default class DcService extends RepositoryService {
return this.store.query(this.getModelName(), {});
}
async findBySlug(name, items) {
async findBySlug(name) {
const items = this.store.peekAll('dc');
if (name != null) {
const item = await items.findBy('Name', name);
if (typeof item !== 'undefined') {
await this.settings.persist({ dc: get(item, 'Name') });
return item;
}
}
@ -32,17 +32,13 @@ export default class DcService extends RepositoryService {
}
async getActive(name, items) {
return Promise.all([name || this.settings.findBySlug('dc'), items || this.findAll()]).then(
([name, items]) => {
return Promise.all([name, items || this.findAll()]).then(([name, items]) => {
return this.findBySlug(name, items).catch(async e => {
const item =
items.findBy('Name', this.env.var('CONSUL_DATACENTER_LOCAL')) ||
get(items, 'firstObject');
await this.settings.persist({ dc: get(item, 'Name') });
return item;
});
}
return (
items.findBy('Name', this.env.var('CONSUL_DATACENTER_LOCAL')) || get(items, 'firstObject')
);
});
});
}
async clearActive() {

3
ui/packages/consul-ui/app/services/repository/nspace/disabled.js
View File

@ -2,7 +2,6 @@ import RepositoryService from 'consul-ui/services/repository';
import { PRIMARY_KEY, SLUG_KEY } from 'consul-ui/models/nspace';
const modelName = 'nspace';
const DEFAULT_NSPACE = 'default';
export default class NspaceDisabledService extends RepositoryService {
getPrimaryKey() {
return PRIMARY_KEY;
@ -22,7 +21,7 @@ export default class NspaceDisabledService extends RepositoryService {
getActive() {
return {
Name: DEFAULT_NSPACE,
Name: 'default',
};
}

44
ui/packages/consul-ui/app/services/repository/nspace/enabled.js
View File

@ -1,7 +1,27 @@
import { inject as service } from '@ember/service';
import RepositoryService from 'consul-ui/services/repository';
import { PRIMARY_KEY, SLUG_KEY } from 'consul-ui/models/nspace';
import { runInDebug } from '@ember/debug';
const findActiveNspace = function(nspaces, nspace) {
let found = nspaces.find(function(item) {
return item.Name === nspace.Name;
});
if (typeof found === 'undefined') {
runInDebug(_ =>
console.info(`${nspace.Name} not found in [${nspaces.map(item => item.Name).join(', ')}]`)
);
// if we can't find the nspace that was specified try default
found = nspaces.find(function(item) {
return item.Name === 'default';
});
// if there is no default just choose the first
if (typeof found === 'undefined') {
found = nspaces[0];
}
}
return found;
};
const modelName = 'nspace';
export default class NspaceEnabledService extends RepositoryService {
@service('router') router;
@ -46,20 +66,14 @@ export default class NspaceEnabledService extends RepositoryService {
});
}
getActive(paramsNspace) {
return this.settings
.findBySlug('nspace')
.then(function(nspace) {
// If we can't figure out the nspace from the URL use
// the previously saved nspace and if thats not there
// then just use default
return paramsNspace || nspace || 'default';
})
.then(nspace => this.settings.persist({ nspace: nspace }))
.then(function(item) {
return {
Name: item.nspace,
};
});
async getActive(paramsNspace = '') {
const nspaces = this.store.peekAll('nspace').toArray();
if (paramsNspace.length === 0) {
const token = await this.settings.findBySlug('token');
paramsNspace = token.Namespace || 'default';
}
// if there is only 1 namespace then use that, otherwise find the
// namespace object that corresponds to the active one
return nspaces.length === 1 ? nspaces[0] : findActiveNspace(nspaces, { Name: paramsNspace });
}
}

31
ui/packages/consul-ui/app/services/repository/oidc-provider.js
View File

@ -7,8 +7,8 @@ import dataSource from 'consul-ui/decorators/data-source';
const modelName = 'oidc-provider';
const OAUTH_PROVIDER_NAME = 'oidc-with-url';
export default class OidcProviderService extends RepositoryService {
@service('torii')
manager;
@service('torii') manager;
@service('repository/settings') settings;
init() {
super.init(...arguments);
@ -25,8 +25,31 @@ export default class OidcProviderService extends RepositoryService {
}
@dataSource('/:ns/:dc/oidc/provider/:id')
async findBySlug() {
return super.findBySlug(...arguments);
async findBySlug(params) {
// This addition is mainly due to ember-data book-keeping This is one of
// the only places where Consul w/namespaces enabled doesn't return a
// response with a Namespace property, but in order to keep ember-data
// id's happy we need to fake one. Usually when we make a request to consul
// with an empty `ns=` Consul will use the namespace that is assigned to
// the token, and when we get the response we can pick that back off the
// responses `Namespace` property. As we don't receive a `Namespace`
// property here, we have to figure this out ourselves. Biut we also want
// to make this completely invisible to 'the application engineer/a
// template engineer'. This feels like the best place/way to do it as we
// are already in a asynchronous method, and we avoid adding extra 'just
// for us' parameters to the query object. There is a chance that as we
// are discovering the tokens default namespace on the frontend and
// assigning that to the ns query param, the token default namespace 'may'
// have changed by the time the request hits the backend. As this is
// extremely unlikely and in the scheme of things not a big deal, we
// decided that doing this here is ok and avoids doing this in a more
// complicated manner.
const token = (await this.settings.findBySlug('token')) || {};
return super.findBySlug({
ns: params.ns || token.Namespace || 'default',
dc: params.dc,
id: params.id,
});
}
@dataSource('/:ns/:dc/oidc/authorize/:id/:code/:state')

57
ui/packages/consul-ui/docs/significant-patterns.mdx
View File

@ -1,16 +1,18 @@
# Significant patterns
## Ember specific
The Consul UI tries to follow typical conventional ember but differs
significantly in several places:
## Routing
### Routing
We use a declarative configuration format for our routing rather than Ember's
DSL. The format is closely modeled on Ember's DSL and if you need to generate
Ember's DSL from this for some reason you can use one of our Debug Utilities
to do this.
## Routes
### Routes
We use a specific BaseRoute as a parent Route for **all** our Routes. This contains project wide
functionality for several important additions. If you use the normal `ember
@ -27,7 +29,7 @@ has no query parameters configured they are copied over from the Route.
Preferably we don't use Controllers, but this doesn't mean you shouldn't if
you need to.
## Routlets
### Routlets
We use have a concept of 'routlets', the combination of a `<Route />` and
`<Outlet />` components that we use within our route templates to achieve
@ -38,7 +40,7 @@ Every route template should be wrapped in a `<Route @name={{routeName}}>` compon
The `routeName` variable is made available in every single route template and
it equal to the routeName of the current template e.g. `dc.services.index`
## DataSources
### DataSources
In order to support our live updating long-polling blocking queries we use
'DataSources' which come in two flavours. A service backed
@ -79,7 +81,7 @@ And a component based approach for use within templates.
See the relavant component/service documentation for more detail.
## Components
### Components
You could group our components into two different groups:
@ -94,7 +96,7 @@ We currently use a mix of named/block slots and contextual components and its
fair to say that we use more named/block slots, but both are fine depending on
your use case.
## Significant Addons
### Significant Addons
- `ember-data` - model layer
- `ember-stargate` - wormhole/portal/put this DOM over there functionality
@ -105,4 +107,47 @@ your use case.
Please see our package.json file dependencies.
## Consul specific
### Namespace support
Whilst Consul namespaces are a OSS feature of Consul, we almost always build
things 'pretending' that namespaces are always enabled. Then there is code
within the data layer of the application that removes any namespace related
query parameters.
Namespaces are a little more complex to think about than other things in
Consul as we have the idea of 'the default' namespace, and then additionally
an ACL token can have 'a default' namespace (not the same as 'the default'
namespace) which is a little like a namespace that is assigned to the token,
or where the token originated from. The Consul backend can then use the tokens
default/origin/assigned namespace to figure out whether and how to filter any
results by namespace before sending those results to the UI.
When you are requesting data from the API you should always include a
namespace (if applicable to the API endpoint), but you should never default
the namespace value to 'the default' namespace - `default`. Conversly, when
receiving data from an API response, there are some places where we do default
a namespace to 'the default' namespace `default`. The easiest way to think
about this is: If its on the way out, don't add a default ever, if its on the
way in you may need to default any potentially empty Namespace parameters to
`default` (due to using Consul without namespaces enabled), there is a
NspaceAbility and/or an `env` feature flag to help you to discover this.
We've made various decisions in the UI to make it hard to omit a namespace
when you shouldn't, for example using our DataSource component requires you to
include a defined namespace parameter (even if the value for that is `''`),
and the route parameter `nspace` value will always default to `''`. It also
reminds you to be continually thinking about namespaces whilst adding new
features.
Lastly, we always refer to namespaces in code as the word `nspace` or
`nspaces`. Both `ns` and `namespace` have other purposes in Ember and Ember
Data (in various places one is significant but not the other and vice versa).
Always using the term `nspace` means we avoid any strange bugs or
clashes/overwrites with anything in Ember or Ember Data core. The only place
where we use `ns` or `Namespace` is to construct API requests/responses.
Sometimes when using this term in CSS or on plain javascript objects, this may
seem unnecessary but it just helps smooth over writing code around namespaces
as you don't have the split second decision of whether you write `ns` or
`namespace` - just use `nspace` and everything will be fine :)

2
ui/packages/consul-ui/tests/integration/serializers/acl-test.js
View File

@ -49,7 +49,7 @@ module('Integration | Serializer | acl', function(hooks) {
Datacenter: dc,
[META]: {
[DC.toLowerCase()]: dc,
[NSPACE.toLowerCase()]: nspace,
[NSPACE.toLowerCase()]: '',
},
// TODO: default isn't required here, once we've
// refactored out our Serializer this can go

2
ui/packages/consul-ui/tests/integration/serializers/kv-test.js
View File

@ -57,7 +57,7 @@ module('Integration | Serializer | kv', function(hooks) {
Datacenter: dc,
[META]: {
[DC.toLowerCase()]: dc,
[NSPACE.toLowerCase()]: payload[0].Namespace || undefinedNspace,
[NSPACE.toLowerCase()]: nspace || '',
},
Namespace: payload[0].Namespace || undefinedNspace,
uid: `["${payload[0].Namespace || undefinedNspace}","${dc}","${id}"]`,

2
ui/packages/consul-ui/tests/integration/serializers/node-test.js
View File

@ -80,7 +80,7 @@ module('Integration | Serializer | node', function(hooks) {
Port: '8500',
[META]: {
[DC.toLowerCase()]: dc,
[NSPACE.toLowerCase()]: nspace,
[NSPACE.toLowerCase()]: '',
},
};
const actual = serializer.respondForQueryLeader(

15
ui/packages/consul-ui/tests/integration/serializers/oidc-provider-test.js
View File

@ -44,18 +44,24 @@ module('Integration | Serializer | oidc-provider', function(hooks) {
const dc = 'dc-1';
const id = 'slug';
const request = {
url: `/v1/acl/oidc/auth-url?dc=${dc}`,
url: `/v1/acl/oidc/auth-url?dc=${dc}${
typeof nspace !== 'undefined' ? `&ns=${nspace || undefinedNspace}` : ``
}`,
};
return get(request.url).then(function(payload) {
// The response here never has a Namespace property so its ok to just
// use the query parameter as the expected nspace value. See
// implementation of this method for info on why this is slightly
// different to other tests
const expected = Object.assign({}, payload, {
Name: id,
Datacenter: dc,
[META]: {
[DC.toLowerCase()]: dc,
[NSPACE.toLowerCase()]: payload.Namespace || undefinedNspace,
[NSPACE.toLowerCase()]: nspace || '',
},
Namespace: payload.Namespace || undefinedNspace,
uid: `["${payload.Namespace || undefinedNspace}","${dc}","${id}"]`,
Namespace: nspace || undefinedNspace,
uid: `["${nspace || undefinedNspace}","${dc}","${id}"]`,
});
const actual = serializer.respondForQueryRecord(
function(cb) {
@ -66,6 +72,7 @@ module('Integration | Serializer | oidc-provider', function(hooks) {
{
dc: dc,
id: id,
ns: nspace,
}
);
assert.deepEqual(actual, expected);

2
ui/packages/consul-ui/tests/integration/serializers/policy-test.js
View File

@ -49,7 +49,7 @@ module('Integration | Serializer | policy', function(hooks) {
Datacenter: dc,
[META]: {
[DC.toLowerCase()]: dc,
[NSPACE.toLowerCase()]: payload.Namespace || undefinedNspace,
[NSPACE.toLowerCase()]: nspace || '',
},
Namespace: payload.Namespace || undefinedNspace,
uid: `["${payload.Namespace || undefinedNspace}","${dc}","${id}"]`,

2
ui/packages/consul-ui/tests/integration/serializers/role-test.js
View File

@ -53,7 +53,7 @@ module('Integration | Serializer | role', function(hooks) {
Policies: createPolicies(payload),
[META]: {
[DC.toLowerCase()]: dc,
[NSPACE.toLowerCase()]: payload.Namespace || undefinedNspace,
[NSPACE.toLowerCase()]: nspace || '',
},
Namespace: payload.Namespace || undefinedNspace,
uid: `["${payload.Namespace || undefinedNspace}","${dc}","${id}"]`,

2
ui/packages/consul-ui/tests/integration/serializers/service-instance-test.js
View File

@ -28,7 +28,7 @@ module('Integration | Serializer | service-instance', function(hooks) {
Datacenter: dc,
[META]: {
[DC.toLowerCase()]: dc,
[NSPACE.toLowerCase()]: payload[0].Service.Namespace || undefinedNspace,
[NSPACE.toLowerCase()]: nspace || '',
},
Namespace: payload[0].Service.Namespace || undefinedNspace,
uid: `["${payload[0].Service.Namespace || undefinedNspace}","${dc}","${node}","${id}"]`,

2
ui/packages/consul-ui/tests/integration/serializers/session-test.js
View File

@ -54,7 +54,7 @@ module('Integration | Serializer | session | response', function(hooks) {
Datacenter: dc,
[META]: {
[DC.toLowerCase()]: dc,
[NSPACE.toLowerCase()]: payload[0].Namespace || undefinedNspace,
[NSPACE.toLowerCase()]: nspace || '',
},
Namespace: payload[0].Namespace || undefinedNspace,
uid: `["${payload[0].Namespace || undefinedNspace}","${dc}","${id}"]`,

2
ui/packages/consul-ui/tests/integration/serializers/token-test.js
View File

@ -53,7 +53,7 @@ module('Integration | Serializer | token', function(hooks) {
Datacenter: dc,
[META]: {
[DC.toLowerCase()]: dc,
[NSPACE.toLowerCase()]: payload.Namespace || undefinedNspace,
[NSPACE.toLowerCase()]: nspace || '',
},
Namespace: payload.Namespace || undefinedNspace,
uid: `["${payload.Namespace || undefinedNspace}","${dc}","${id}"]`,

2
ui/packages/consul-ui/tests/unit/serializers/application-test.js
View File

@ -67,7 +67,7 @@ module('Unit | Serializer | application', function(hooks) {
Name: 'name',
[META]: {
[DC.toLowerCase()]: 'dc-1',
[NSPACE.toLowerCase()]: 'default',
[NSPACE.toLowerCase()]: '',
},
'primary-key-name': 'name',
};