-
Notifications
You must be signed in to change notification settings - Fork 888
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
Add new advanced settings for vis augmenter #3961
Changes from 4 commits
d19a257
d2e2a48
282a7c6
e179ef5
42bd042
fdaf819
66f7f1f
67276c8
6c5cb9e
7f553f3
0b31c87
d60caa2
bb5dc5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { schema, TypeOf } from '@osd/config-schema'; | ||
|
||
export const configSchema = schema.object({ | ||
pluginAugmentationEnabled: schema.boolean({ defaultValue: true }), | ||
}); | ||
|
||
export type VisAugmenterPluginConfigType = TypeOf<typeof configSchema>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,46 @@ | |
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { getSavedAugmentVisLoader } from '../../services'; | ||
import { ISavedAugmentVis } from '../../types'; | ||
import { get } from 'lodash'; | ||
import { getSavedAugmentVisLoader, getUISettings } from '../../services'; | ||
import { ISavedAugmentVis } from '../types'; | ||
|
||
/** | ||
* Create an augment vis saved object given an object that | ||
* implements the ISavedAugmentVis interface | ||
*/ | ||
export const createAugmentVisSavedObject = async (AugmentVis: ISavedAugmentVis): Promise<any> => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add unit tests here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will follow up in another pr |
||
const loader = getSavedAugmentVisLoader(); | ||
const config = getUISettings(); | ||
|
||
const isAugmentationEnabled = config.get('visualization:enablePluginAugmentation'); | ||
if (!isAugmentationEnabled) { | ||
// eslint-disable-next-line no-throw-literal | ||
throw 'Visualization augmentation is disabled, please enable visualization:enablePluginAugmentation.'; | ||
} | ||
const maxAssociatedCount = config.get('visualization:enablePluginAugmentation.maxPluginObjects'); | ||
|
||
await loader.findAll().then(async (resp) => { | ||
if (resp !== undefined) { | ||
const savedAugmentObjects = get(resp, 'hits', []); | ||
// gets all the saved object for this visualization | ||
const savedObjectsForThisVisualization = savedAugmentObjects.filter( | ||
(savedObj) => get(savedObj, 'visId', '') === AugmentVis.visId | ||
); | ||
|
||
if (maxAssociatedCount <= savedObjectsForThisVisualization.length) { | ||
// eslint-disable-next-line no-throw-literal | ||
throw ( | ||
'Cannot associate the plugin resource to the visualization due to the limit of the' + | ||
'max amount of associated plugin resources (' + | ||
maxAssociatedCount + | ||
') with ' + | ||
savedObjectsForThisVisualization.length + | ||
' associated to the visualization' | ||
); | ||
} | ||
} | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the more I'm thinking about this, I think we will need to make this check on the server-side at the low-level api call made by the saved object client. Otherwise we still aren't covering all scenarios, and the limit could be circumvented by making alternate calls to create & save the object. This function To my knowledge there hasn't been set limitations on creating a saved object from the client, other than the fact that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @ashwin-pc @joshuarrrr for thoughts on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe having this at the loader-level is still appropriate, and if users circumvent by creating these saved objs in unexpected ways (e.g., direct api calls), that is ok. Typically saved objs are quite fragile and are highly recommended not to be created manually, and altering them later on could lead to weird/unexpected behavior. I'd still like to get OSD folks opinion on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea this is a real issue as its bypassing the setting. More enforcement would be hard too since they can bypass the normal saved object api and write to the index directly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the reason to move this to the server side is to prevent the user from circumventing the limit, I'd love to know more about how risky that is. But in general it's better to put a warning near the setting than to move it to the server. Moving it to the server is more useful when we want to reduce latency or access secrets. For real safeguards, that has to be controlled on OS itself. As for saved object safeguards, yeah we don't have many today and that's a sorely needed feature. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the same time if you think it's necessary, feel free to add your own safeguards for your saved objects on the server. It's a judgement call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ashwin-pc thanks for your input! Sounds good. This is helpful in understanding the current state of saved object safeguarding/limits. Agreed it would need to have logic in OpenSearch itself to truly safeguard. If the limit is breached unexpectedly, this does not have extreme impacts. It would just mean errors when trying to link more plugin resources to the particular vis that is over the limit, and potential confusion if users deep dive and see there are In my opinion, based on what's currently available, the overall low likelihood of this manual behavior, and the overall low criticality of this limit being breached unexpectedly, I think I'm ok with keeping this at the saved-object-loader level of checks. Moving to server-side and/or having actual blocking checks on the OpenSearch side I think is a bigger discussion regarding efforts to lock down saved objects. Lmk your thoughts @lezzago There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea I think leaving as is would be best. Later the roadmap is that OSD would have its own data store separate from OS, so having this logic in OS would cause more problems in the future. I believe adding some documentation here would be helpful for the users. |
||
|
||
return await loader.get((AugmentVis as any) as Record<string, unknown>); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,10 @@ | |
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { i18n } from '@osd/i18n'; | ||
import { schema } from '@osd/config-schema'; | ||
import { Observable } from 'rxjs'; | ||
import { first } from 'rxjs/operators'; | ||
import { | ||
PluginInitializerContext, | ||
CoreSetup, | ||
|
@@ -12,6 +16,7 @@ import { | |
} from '../../../core/server'; | ||
import { augmentVisSavedObjectType } from './saved_objects'; | ||
import { capabilitiesProvider } from './capabilities_provider'; | ||
import { VisAugmenterPluginConfigType } from '../config'; | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-empty-interface | ||
export interface VisAugmenterPluginSetup {} | ||
|
@@ -21,15 +26,53 @@ export interface VisAugmenterPluginStart {} | |
export class VisAugmenterPlugin | ||
implements Plugin<VisAugmenterPluginSetup, VisAugmenterPluginStart> { | ||
private readonly logger: Logger; | ||
private readonly config$: Observable<VisAugmenterPluginConfigType>; | ||
|
||
constructor(initializerContext: PluginInitializerContext) { | ||
this.logger = initializerContext.logger.get(); | ||
this.config$ = initializerContext.config.create<VisAugmenterPluginConfigType>(); | ||
} | ||
|
||
public setup(core: CoreSetup) { | ||
public async setup(core: CoreSetup) { | ||
this.logger.debug('VisAugmenter: Setup'); | ||
core.savedObjects.registerType(augmentVisSavedObjectType); | ||
core.capabilities.registerProvider(capabilitiesProvider); | ||
|
||
const config: VisAugmenterPluginConfigType = await this.config$.pipe(first()).toPromise(); | ||
const isAugmentationEnabled = | ||
config.pluginAugmentationEnabled === undefined ? true : config.pluginAugmentationEnabled; | ||
|
||
if (isAugmentationEnabled) { | ||
lezzago marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually pretty neat that you are relying on the missing UI settings to disable it in the plugin. Calling it out in the comment here will be really helpful in the future though since it may not be obvious that you are doing it this way and its anon standard pattern in the repo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will follow up in another pr with the tests |
||
core.uiSettings.register({ | ||
['visualization:enablePluginAugmentation']: { | ||
name: i18n.translate('visualization.enablePluginAugmentationTitle', { | ||
defaultMessage: 'Enable plugin augmentation', | ||
}), | ||
value: true, | ||
description: i18n.translate('visualization.enablePluginAugmentationText', { | ||
defaultMessage: 'Plugin functionality can be accessed from line chart visualizations', | ||
}), | ||
category: ['visualization'], | ||
schema: schema.boolean(), | ||
}, | ||
['visualization:enablePluginAugmentation.maxPluginObjects']: { | ||
name: i18n.translate('visualization.enablePluginAugmentation.maxPluginObjectsTitle', { | ||
defaultMessage: 'Max number of associated augmentations', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This message seems a little weird, should it be something like 'max number of associated plugin objects / plugin resources'? Not sure if this was finalized by tech writer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was based on mockups and we can improve this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure - no blocker here. We can probably follow up with one larger PR to update wording after UX has a full walk through. |
||
}), | ||
value: 10, | ||
description: i18n.translate( | ||
'visualization.enablePluginAugmentation.maxPluginObjectsText', | ||
{ | ||
defaultMessage: | ||
'Associating more than 10 plugin resources per visualization can lead to performance ' + | ||
'issues and increase the cost of running clusters.', | ||
} | ||
), | ||
category: ['visualization'], | ||
schema: schema.number({ min: 0 }), | ||
}, | ||
}); | ||
} | ||
return {}; | ||
} | ||
|
||
|
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.
I don't think we're consistent in this file, but I prefer the form of L232-234; because we provide the commented out default value, the instructions should be about setting it to the non-default:
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.
Oh, I see the default is intended to be true. In that case, keep the text and change the commented value.
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.
I followed the standard of the other settings and the commented value should be false based on that.