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

Plugin ID uniqueness not enforced at registration #3210

Closed
jcordasc opened this issue Apr 12, 2019 · 5 comments · Fixed by #3322
Closed

Plugin ID uniqueness not enforced at registration #3210

jcordasc opened this issue Apr 12, 2019 · 5 comments · Fixed by #3322
Milestone

Comments

@jcordasc
Copy link

Description

Plugin registration does not enforce uniqueness of plugin IDs. This leads to an issue when attempting to disable plugins because when plugin.Graph loops through the registered plugins to match against the disabled list, it only removes the first plugin with a given ID (presumably on the assumption of uniqueness of plugin IDs).

I'm opening this issue to:

  1. have a place to discuss whether or not uniqueness should be enforced in code rather than by convention and
  2. to propose addressing the one occurrence of duplicate plugin names I'm aware of -- the GRPCPlugin and ContentPlugin both named "content".

Steps to reproduce the issue:

  1. List "content" in the disabled_plugins slice in the containerd config file
  2. Start and stop containerd a few times. Observe that because of registration order, it is not deterministic which of the plugins get disabled.

Describe the results you received:
It is not possible to disable a specific one of either the GRPCPlugin or ContentPlugin named "content" reliably.

Describe the results you expected:
Disabling a plugin by providing it's ID in the config file.

Output of containerd --version:

containerd github.com/containerd/containerd v1.2.0-473-g5f4c977b 5f4c977ba0c44d6e12a0907251ca133954f0c82d
@Random-Liu
Copy link
Member

Random-Liu commented Apr 12, 2019

The plugin URI io.containerd.grpc.v1.content should be unique in this case https://github.com/containerd/containerd/blob/master/services/server/server.go#L130.

Maybe we should use plugin URI instead? We may want to keep the required plugin consistent #3197

But please note that we need to handle backward compatibility if we change this. See #3196

@Random-Liu
Copy link
Member

Random-Liu commented Apr 12, 2019

Hm, but we do use ID to find plugin config, so it seems that we do assume ID is unique, maybe we should enforce the uniqueness as is suggested by this issue if we do expect it to be unique.

If we only expect URI to be unique, we should probably indexing plugin config with URI as well.

@Random-Liu
Copy link
Member

Random-Liu commented Apr 18, 2019

I think we may want to finalize this in 1.3, or else it will be even harder to change in the future, given that we introduced a new option required_plugins in 1.3

@Random-Liu Random-Liu added this to the 1.3 milestone Apr 18, 2019
@Random-Liu
Copy link
Member

Given that we've been using plugin ID for config, disabled_plugins and required_plugins, I feel like enforcing uniqueness of plugin ID seems to be an easier solution to this problem.

We should probably:

  1. Enforce the ID uniqueness during containerd start up;
  2. Fix duplicated plugin ID content. Rename "content" gRPC plugin to "content-grpc" #3211

@jcordasc
Copy link
Author

jcordasc commented May 6, 2019

If the approach is to simply enforce uniqueness, and if no one else is already working on this, I'd be happy to take a pass at a PR to do this.

@Random-Liu, @crosbymichael - should I move forward with this, or do we need more discussion on the approach?

crosbymichael added a commit to crosbymichael/containerd that referenced this issue Jun 10, 2019
Closes containerd#3210

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
tussennet pushed a commit to tussennet/containerd that referenced this issue Sep 11, 2020
Closes containerd#3210

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
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

Successfully merging a pull request may close this issue.

2 participants