[BREAKINGCHANGE] fetch plugin based on version

[shahrokni]

perses/perses#1186

⚠️ BACKEND NEEDS TO BE UPDAED. EXPLAINED AT THE END OF DESCRIPTION

Different Plugins Versions and Registries

So far a plugin could be imported by its name only.

const remoteEntryURL = baseURL ? `${baseURL}/${name}/mf-manifest.json` : `/plugins/${name}/mf-manifest.json`;

The backend has recently introduced a feature by which multiple versions of the same plugin could be available.
This means if the consumer of the Plugin Registry provided the version and registry, the exact desired plugin could be loaded in the front!

To achieve the mentioned goal the new URL will carry the version and registry as well. If not provided by the consumer, the front uses the default values introduced by the backend which are latest and perses.dev

 const remoteEntryURL = baseURL
      ? `${baseURL}/${name}/${registry || DEFAULT_PLUGIN_REGISTRY}/${version || DEFAULT_PLUGIN_VERSION}/mf-manifest.json`
      : `/plugins/${name}/${registry || DEFAULT_PLUGIN_REGISTRY}/${version || DEFAULT_PLUGIN_VERSION}/mf-manifest.json`;

/*ESAMPLE FROM MY LOCAL MACHINE BY DEFAULT VALUES*/
/* http://localhost:3000/plugins/TimeSeriesChart/perses.dev/latest/mf-manifest.json */

⚠️ Backend needs to do

Backend should be adjusted to manage the new URL. I checked the backend and understood that at the moment the intercepted GET request is translated to the address of the files on the disk. Please take a look at (ui\endpoint.go)
I believe the following shared code should be adjusted for the new URL that I just explained.

if devEnvironment == nil {
		// In that case, we need to read the requested files from the file system.
		// The First thing to do is to replace the URL path with the local path of the plugin.
		localPath := strings.Replace(req.URL.Path, fmt.Sprintf("%s/plugins/%s", f.apiPrefix, pluginName), loaded.LocalPath, 1)

		// Then we just need to rely on the echo router to serve the file.

		// X-Content-Type-Options: nosniff is an HTTP response header that tells browsers: "Do not try to guess the content type — trust the Content-Type header I sent you."
		// Without it, browsers perform "MIME sniffing". For example, a file served as text/plain could be sniffed as text/html and executed as HTML, which could open the door to XSS attacks.
		// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/X-Content-Type-Options
		c.Response().Header().Set("X-Content-Type-Options", "nosniff")
		return c.File(localPath)
	}

There is also a TODO comment in backend that I believe could to be addressed now

// TODO: These hardcoded values should be replaced once the frontend is able to manage multiple registries and plugin versions.
	// This suppose to update the plugin system first.
	// These hardocded values are matching the default behavior of the plugin system and it helps to keep backward compatibility.
	loaded, isLoaded := f.pluginService.GetLoadedPlugin(pluginName, pluginModel.LatestVersion, pluginModel.DefaultRegistry)
	if !isLoaded || !loaded.Module.Status.IsLoaded {
		logrus.Errorf("unable to find the plugin => %s", pluginName)
		return apiinterface.NotFoundError
	}

Checklist

• Pull request has a descriptive title and context useful to a reviewer.
• Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the
  following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, DOC,IGNORE.
• All commits have DCO signoffs.

UI Changes

• Changes that impact the UI include screenshots and/or screencasts of the relevant changes.
• Code follows the UI guidelines.
• E2E tests are stable and unlikely to be flaky.
  See e2e docs for more details. Common issues include:
  • Is the data inconsistent? You need to mock API requests.
  • Does the time change? You need to use consistent time values or mock time utilities.
  • Does it have loading states? You need to wait for loading to complete.

[Nexucis]

Thank you @shahrokni for updating the frontend regarding the plugin multi version management !

Indeed the backend will need to be updated at some point, but hopefully that should be as easy as to update the regexp

capturingPluginName = regexp.MustCompile(`/plugins/([a-zA-Z0-9_-]+)/?.*`)

to catch the registry and the version.

[jgbernalp]

is this needed? the plugin versions are defined inside dashboards, if not defined it will use whatever is available in the backend.

[shahrokni]

Thank you for this great comment! The comment makes me wonder, if I understood the requirements clearly. I explain the strategy I have in mind. Please correct me if I am wrong.
CC: @Nexucis

My Assumption

We have modules and under them we have plugins. Example, Prometheus > PrometheusTimeseriesQuery and ...

Now what we want to do is to give the consumer the chance to pick a plugin from a module with a specific registry and version. Now, before we proceed, I want to repeat again that my change sends the version and registry of the module through the URL. (just a reminder to have the whole picture)

visual cue from : api/v1/plugins

Back to Current implementation

The getPlugin which is exposed from the Plugin-Registry receives two params to get a plugin. The first one is the kind and the second one is the module name.

 const getPlugin = useCallback(
    async <T extends PluginType>(kind: T, name: string): Promise<PluginImplementation<T>> => {
    /* HERE WE HAVE THE LOADING LOGIC => WICH COULD HAVE THE VERSION AND REGISTRY SET BY CONSUMER  */
    /* AND FINALLY TRIES TO PICK ITS DESIRED PLUGIN FROM THE MODULE */
    // We currently assume that plugin modules will have named exports that match the kinds they handle
      const plugin = pluginModule[name];

Applying the version and registry

So how could we let the consumer of the registry to set the version and registry ❔ (They don't want the default one)

One way would be to add the two missing parameters to the getPlugin which in my opinion is not a good idea.
Because we need to to take care of new parameters wherever getPlugin has been called. The second option is to have the desired versions from the props of the plugin-registry. For instance, I prepare my desired versions once from perses/app

Now, regardless of how we do it, we need to do it, right? Otherwise, how could the consumers set the version and registry? Does that make sense?

export interface PluginRegistryProps {
  pluginLoader: PluginLoader;
  defaultPluginKinds?: DefaultPluginKinds;
  children?: ReactNode;
  /* ADDING THE PROP*/
/* MAYBE THE NAME IS MISSLEADING? DOES IT MAKE SENSE TO BE MODULE PREF?*/
  pluginPreferences?: PluginPreference[];
}

Something is probably missing from my PR

As the backend gives me the entire module (not the plugin), if I am looking for a plugin with a specific version, I need to provide that version. Right?
This line gives me the impression that we need a plugin_version as well

    // We currently assume that plugin modules will have named exports that match the kinds they handle
      const plugin = pluginModule[name];

[jgbernalp]

Not sure I follow. The consumer of the registry should not define pluginPreferences, this is driven by the version you want to load in your dashboard. We cannot change all the host apps (Perses UI, OpenShift) and hardcode preferred versions. The dashboards will be the ones containing that in the panel specs. This means, we do require a version and registry in the getPlugin function. By default both undefined, meaning use whatever you have of this plugin kind (for backwards compatibility). If one or both are passed, the registry will check if the plugin with the specific version and registry is available, falling back to the current behavior. This also means that the registry now has to track also plugin modules using the version and registry, something like:

let plugin = null;

const modulekey = getModuleKey(name, registry, version)
plugin = pluginModule[modulekey];

if(!plugin){
   plugin = pluginModule[name];
}

...

return plugin;

when registering the plugin modules we need to register both pluginModule[name] "versionless" (pointing to the latest version available) and "versioned" pluginModule[keyWithRegistryAndVersion]

WDYT?

[jgbernalp]

I don't think this branch is necessary, if no version nor registry are provided we fallback to the current implementation, the backend already should return the latest installed version of this plugin.

[shahrokni]

We need it. Even to load the default ones front should add them to the registry the very first time.
Adding it to registry fills the pluginModule and for that the following steps are taken:

From the Legacy Code

1- loadPluginModule is called with a resource. In the legacy code the resource is provided (deduced) from the get installed plugins. The installed plugins are provided by the backend, yet nothing has been registered in front
2- importPluginModule is called with the resource, and tries to find it from cache or (next step)
3- importPluginModule gets the list of plugins from resource.spec.plugins and calls loadPlugin for individual plugins per module. This will call the backend and say 'hey give me what you have with the following information name:registry:version'
4- (3.1) The pluginModule (the dictionary) is built here => Front recieves the module and goes through its plugins and add them into the dictionary. (The key used to be a name only)
5- Now if this is the first time, the default ones have been requested from front and loaded into the registry. It is not like storing them at frontend boot_up. It is an on_demand process even for the default ones you need explicitly ask backend to return them first time (for registry)

I think the code has made the process super complex. Please, if you are not satisfied with what I explained. we can discuss it through a call, but I assure you it won't work if you remove the branch.

[jgbernalp]

This is not backwards compatible it will generate wrong urls like /plugins/test///mf-manifest.json

[shahrokni]

This is not backwards compatible it will generate wrong urls like /plugins/test///mf-manifest.json

@jgbernalp

This is actually not wrong. When intercepted by backend, the backend drops the gaps and it becomes

/plugins/test/mf-manifest.json

I tested and it is working!

But now, it makes me wonder,
either with both version and register, or if even one of them does not exist we should drop them both? Because, how could backend (simply) guess the order of a 3_parts URL? And even more, a version is bound to a registry, without the registry the version does not make any sense. Right?

WDYT?

[jgbernalp]

while the URL is technically valid it might cause problems, for example if Perses is behind a proxy that does not like these urls or cache layers might not cache the data. I trust that works on your scenario but this might not work in other contexts. Is there something blocking us from having a proper URL?

The backend could detect if a semantic version exist in the url parts. the other part "should" be the registry.

/plugins/test/v0.1.0/mf-manifest.json -> v0.1.0, default-registry
/plugins/test/registry/v0.2.0/mf-manifest.json -> v0.2.0, registry
/plugins/test/registry/mf-manifest.json  -> latest, registry
/plugins/test/mf-manifest.json -> latest, default-registry

[jgbernalp]

instead of mutating directly we should probably load from a copy.

[jgbernalp]

We are breaking the cache as the remotes is now tracked by registryName

[jgbernalp]

this search is non deterministic, it might find an older version rather than the latest which should be the default

[shahrokni]

It doesn't really matter if it finds an old one or new one.
We just need a found object of PluginModuleResource which starts with the kind:name: pattern.
As you can see we are crafting our own object by setting both version and registry to empty and undefined respectively. We use this object as a kind of payload to get to a point to see those information are not available.
Since they are not available, the fallback mechanism works and returns the default one.

Why do we need it anyway? Because of the list of plugins it provides. There is a step which loops through the list of plugins to register them individually per module

     const resourceKey = Array.from(pluginIndexes.pluginResourcesByNameAndKind.keys()).find((k) =>
        k.startsWith(`${kind}:${name}:`, 0)
      );
      resource = pluginIndexes.pluginResourcesByNameAndKind.get(resourceKey || '');

      if (!resource) {
        throw new Error(`A ${name} plugin for kind '${kind}' is not installed`);
      }

      const unversionedResource: PluginModuleResource = {
        ...resource,
        metadata: { ...resource.metadata, version: '', registry: undefined },
      };

Again, I would like to draw your attention to how the legacy code gets the default plugin for the very first time
#128 (comment)

[jgbernalp]

Suggested change

	`The ${name} plugin for kind '${kind}' is missing from the ${resource!.metadata.name} plugin module`
	`The ${name} plugin for kind '${kind}' is missing from the ${resource.metadata.name} plugin module`

[jgbernalp]

this type is internal only from what I can see.

Suggested change

	export type PluginPreference = { kind: PluginType; name: string; version: string; registry: string };
	type PluginPreference = { kind: PluginType; name: string; version: string; registry: string };

[jgbernalp]

This comment does not match with the results

[jgbernalp]

maybe is better to add the rationale rather than a link to a discussion.