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

dependencies: github.com/xeipuuv pulled into Kubernetes #80

Closed
pohly opened this issue Sep 1, 2022 · 9 comments
Closed

dependencies: github.com/xeipuuv pulled into Kubernetes #80

pohly opened this issue Sep 1, 2022 · 9 comments

Comments

@pohly
Copy link
Contributor

pohly commented Sep 1, 2022

When using github.com/container-orchestrated-devices/container-device-interface in Kubernetes without enabling validation, packages from github.com/xeipuuv still get pulled in as new dependencies. It would be good to avoid that when it's not needed.

See kubernetes/kubernetes#111023, commit "kubelet: add support for dynamic resource allocation".

@klihub
Copy link
Contributor

klihub commented Sep 6, 2022

I looked into this, and compared the situation with containerd. In containerd, which declares a dependency on CDI v0.3.1, make vendor (go mod tidy && go mod vendor && go mod verify) pulls in code from all 3 of the xeipuuv repos, which also get added as indirect deps in go.mod.

Updating the CDI dependencies to v0.3.2 or beyond, where Schema-based validation has been split out to an isolated subpackage, this is not the case any more. No source code gets pullled in from any of the xeipuuv repos. However, those still show up as indirect dependencies in go.mod.

If this is something we need to prevent from happening and if this is happening because of how go mod works, then the only remaining thing we can try is to split out the validation bits to a separate repository of its own under container-orchestrated-devices. Or maybe adding go.mods to their subdirs in the repo would also help. We need to test it.

@bart0sh @pohly Any thoughts ?

@pohly
Copy link
Contributor Author

pohly commented Sep 6, 2022

Kubernetes pulls it in like this:

$ go mod why -m github.com/xeipuuv/gojsonpointer
# github.com/xeipuuv/gojsonpointer
k8s.io/kubernetes/pkg/kubelet/cm/dra
github.com/container-orchestrated-devices/container-device-interface/pkg/cdi
github.com/opencontainers/runtime-tools/generate
github.com/opencontainers/runtime-tools/validate
github.com/xeipuuv/gojsonschema
github.com/xeipuuv/gojsonreference
github.com/xeipuuv/gojsonpointer

$ git grep github.com/opencontainers/runtime-tools/generate vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/
vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/container-edits.go: ocigen "github.com/opencontainers/runtime-tools/generate"

That looks like a hard dependency to me, even if it is indirect. When you developed #62, did you or someone else verify that container-device-interface/pkg/cdi can be used without pulling in github.com/xeipuuv/gojsonschema? Perhaps something has changed in the meantime and the dependency is back?

@pohly
Copy link
Contributor Author

pohly commented Sep 6, 2022

Updating the CDI dependencies to v0.3.2 or beyond, where Schema-based validation has been split out to an isolated subpackage, this is not the case any more. No source code gets pullled in from any of the xeipuuv repos.

I cannot reproduce that. I checked out github.com/containerd/containerd main (currently 217aa160cff03c6ffc1d242f871c5171ba1e8e3d), then ran:

go get github.com/container-orchestrated-devices/[email protected]
go mod tidy
go mod vendor

The xeipuuv repos are still in vendor:

$ git status
On branch main
Your branch is up to date with 'origin/main'.

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   go.mod
	modified:   go.sum
	modified:   vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/cache.go
	modified:   vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/container-edits.go
	modified:   vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/doc.go
	modified:   vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/qualified-device.go
	modified:   vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/registry.go
	deleted:    vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/schema.go
	modified:   vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/spec-dirs.go
	modified:   vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/spec.go
	deleted:    vendor/github.com/container-orchestrated-devices/container-device-interface/schema/Makefile
	deleted:    vendor/github.com/container-orchestrated-devices/container-device-interface/schema/defs.json
	deleted:    vendor/github.com/container-orchestrated-devices/container-device-interface/schema/schema.go
	deleted:    vendor/github.com/container-orchestrated-devices/container-device-interface/schema/schema.json
	modified:   vendor/github.com/container-orchestrated-devices/container-device-interface/specs-go/config.go
	modified:   vendor/github.com/container-orchestrated-devices/container-device-interface/specs-go/oci.go
	modified:   vendor/modules.txt

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/spec_linux.go
	vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/spec_other.go

@klihub
Copy link
Contributor

klihub commented Sep 7, 2022

Updating the CDI dependencies to v0.3.2 or beyond, where Schema-based validation has been split out to an isolated subpackage, this is not the case any more. No source code gets pullled in from any of the xeipuuv repos.

Yes, sorry. You're right and I was wrong. The statement should have been 'No code that would depend directly on any of the xeipuuv repos gets pulled in from CDI`.

Looking at the output of go mod why, xeipuuv repos/code gets pulled in because of runtime-tools/generate, which depends on runtime-tools/validate, which in turn depends on xeipuuv/gojsonschema.

@pohly
Copy link
Contributor Author

pohly commented Sep 7, 2022

Can runtime-tools/generate be changed to not have those dependencies, perhaps similar to how you did it in cdi?

@klihub
Copy link
Contributor

klihub commented Sep 7, 2022

It looks like as it should/would be possible in an even simpler way. JSON schema validation is pulled in merely due to how the code is organised. It is not used by runtime-tools/generate. It only calls into runtime-tools/validate to get the list of capabilites, check if a capability is valid, and to check what is the last capability.

I rolled a quick test to split the generator-relevant few functions out from validate to validate/capabilities and replace all validate. refs in generator with refs to validate/capabilities. Added a containerd go.mod replace rule to use my forked/patched repo and now make vendor produces these changes (relevant for this discussion):

klitkey1-mobl containerd $ git diff go.mod | grep xeipuuv; git diff | egrep -A1 '^diff' | grep -A1 xeipuuv
-       github.com/xeipuuv/gojsonpointer v0.0.0-20190809123943-df4f5c81cb3b // indirect
-       github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect
-       github.com/xeipuuv/gojsonschema v1.2.0 // indirect
diff --git a/vendor/github.com/xeipuuv/gojsonpointer/LICENSE-APACHE-2.0.txt b/vendor/github.com/xeipuuv/gojsonpointer/LICENSE-APACHE-2.0.txt
deleted file mode 100644
--
diff --git a/vendor/github.com/xeipuuv/gojsonpointer/README.md b/vendor/github.com/xeipuuv/gojsonpointer/README.md
deleted file mode 100644
--
diff --git a/vendor/github.com/xeipuuv/gojsonpointer/pointer.go b/vendor/github.com/xeipuuv/gojsonpointer/pointer.go
deleted file mode 100644
--
diff --git a/vendor/github.com/xeipuuv/gojsonreference/LICENSE-APACHE-2.0.txt b/vendor/github.com/xeipuuv/gojsonreference/LICENSE-APACHE-2.0.txt
deleted file mode 100644
--
diff --git a/vendor/github.com/xeipuuv/gojsonreference/README.md b/vendor/github.com/xeipuuv/gojsonreference/README.md
deleted file mode 100644
...
diff --git a/vendor/github.com/xeipuuv/gojsonschema/utils.go b/vendor/github.com/xeipuuv/gojsonschema/utils.go
deleted file mode 100644
--
diff --git a/vendor/github.com/xeipuuv/gojsonschema/validation.go b/vendor/github.com/xeipuuv/gojsonschema/validation.go
deleted file mode 100644

And indeed vendor/github.com/xeipuuv is completely gone.

So technically clearly possible. Whether politically achievable (IOW if such a PR would be accepted upstream) is a different story.

If we can come up with a properly, politely and politically correctly formulated justification of why we want to avoid that dependency, then I guess filing a PR and testing the reception in practice is just a matter of giving it a go.

@pohly
Copy link
Contributor Author

pohly commented Sep 7, 2022

If we can come up with a properly, politely and politically correctly formulated justification of why we want to avoid that dependency,

The justification is simply "the less dependencies, the better". It isn't about these particular dependencies, just that any additional dependency introduces work (for review in Kubernetes) and a potential source for future CVEs that would have to be addressed.

is just a matter of giving it a go

Can you please try?

@klihub
Copy link
Contributor

klihub commented Sep 8, 2022

Can you please try?

Filed PR to upstream repo.

@pohly
Copy link
Contributor Author

pohly commented Nov 1, 2022

The upstream fix was merged and updating those dependencies in Kubernetes removes the JSON validation packages. No new release of CDI is needed.

@pohly pohly closed this as completed Nov 1, 2022
@pohly pohly moved this from 🆕 New to ✅ Done in SIG Node: Dynamic Resource Allocation Nov 5, 2022
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