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

Problematic dependencies for downstream projects #758

Open
klihub opened this issue Nov 17, 2022 · 13 comments
Open

Problematic dependencies for downstream projects #758

klihub opened this issue Nov 17, 2022 · 13 comments

Comments

@klihub
Copy link
Contributor

klihub commented Nov 17, 2022

During the review process of the recently merged Kubernetes Dynamic Resource Allocation feature it was pointed out that runtime-tools has a few problems to vendor for otherwise potential downstream projects.

The first problem is the lack of actively tagged releases, which I guess should be relatively easy to solve. The primary concern with the lack of recent tags is that "it's not clear if HEAD of the repo is always expected to be production-worthy".

The remaining problems are related to dependencies which are problematic to inherit downstream, either because of licensing choices (not a CNCF-approved license), or lack of proper maintenance. Those dependencies are considered problematic even if they are isolated within runtime-tools well enough that vendoring runtime-tools typically does not pull any of the actual code from the dependencies downstream. The list of these packages is the following, together with a brief description of the main issues and some potential remedies:

  • github.com/hashicorp/go-multierror:
    • MPL license not in the CNCF license allowlist
    • if not part of the publicly visible API, could be probably dropped/replaced with some work (we've done something similar recently)
  • github.com/hashicorp/errwrap:
    • MPL license, not in the CNCF license allowlist
    • if not part of the publicly visible API, could be probably dropped/replaced with some work
  • github.com/mndrix/tap-go:
    • no releases/tags
    • no changes since 2017
    • repo marked as archived / read-only
    • maintenance status is unclear
    • this is so little code, and in the PD, that it could be probably simply copied over as an internal package
  • github.com/xeipuuv/gojsonschema:
    • no updates since 2020
    • many open issues, some of which indicate lack of maintenance and correctness (fails tests on current go versions) issues
  • github.com/xeipuuv/gojsonpointer:
    • same as for gojsonschema
  • github.com/xeipuuv/gojsonreference:
    • same as for gojsonschema

Folks who contribute to DRA and CDI would be willing to do all the heavy lifting if we can agree what lifting with be acceptable. The hashicorp and tap-go bits look fairly straightforward.

The xeipuuv bits are less so, mostly because there does not seem to be an easy replacement/alternative implementation for JSON-Schema based verification. One possible solution would be to simply split out the JSON Schema validation function from validate into a new repo, say opencontainers/[runtime]-spec-validation. This would be a backward-incompatible change as anyone using schema-based validation would need then to start importing that repo.

@zvonkok
Copy link

zvonkok commented Nov 18, 2022

/cc @zvonkok

@klihub
Copy link
Contributor Author

klihub commented Nov 21, 2022

/cc @mrunalp @kolyshkin

@rhatdan
Copy link
Contributor

rhatdan commented Nov 21, 2022

@giuseppe PTAL

@elezar
Copy link

elezar commented Nov 29, 2022

@rhatdan @giuseppe have you had a chance to look into this issue? As @klihub mentioned we'll try to do most of the heavy lifting from our side but would require the buy-in from the maintainers.

@giuseppe
Copy link
Member

as much as I like your plan, I am not a maintainer to ack it: https://github.com/opencontainers/runtime-tools/blob/master/MAINTAINERS

I think it is better if any maintainer will ack it first

@elezar
Copy link

elezar commented Nov 29, 2022

Thanks @kolyshkin since you're the latest addition approved by @vbatts and @tianon would you take a look at the proposal?

@mrunalp
Copy link
Contributor

mrunalp commented Dec 1, 2022

I am 👍 on attempting to fix these issues. Thanks @klihub!

@klihub
Copy link
Contributor Author

klihub commented Feb 7, 2023

Just an update/note related to eliminating github.com/hashicorp/go-multierror from dependencies: golang 1.20 has now support for wrapping multiple errors. This hopefully removes the need to rely on an external or internal multierror-like package altogether.

@fmuyassarov
Copy link

I willing to help with this work. I see that go version is 1.16 here, and for multierror dependency dropping we could use go's native support for multi error wrapping but that requires bumping the version to at least 1.20. Would folks here be okay with that change as well?
/cc @mrunalp @kolyshkin

@fmuyassarov
Copy link

/assign

@debarshiray
Copy link

The first problem is the lack of actively tagged releases, which I guess should be relatively easy to solve. The primary concern with the lack of recent tags is that "it's not clear if HEAD of the repo is always expected to be production-worthy".

Any thoughts on tagging a new release?

I find myself looking at this issue because Toolbx picked up a dependency on tags.cncf.io/container-device-interface, and the tags.cncf.io/container-device-interface module requires:

require (
    ...
    github.com/opencontainers/runtime-tools v0.9.1-0.20221107090550-2e043c6bd626
    ...
)

I see that @mrunalp merged commit 2e043c6bd626. So, maybe we can formalize things by tagging it as v0.9.1?

@kolyshkin
Copy link
Contributor

For github.com/xeipuuv/gojsonschema, perhaps https://github.com/santhosh-tekuri/jsonschema will be an adequate replacement.

@elezar
Copy link

elezar commented Jun 14, 2024

One thought is to move validation-specific code to a separate go submodule. This would mean that only clients that opt-in to validation would depend on the transitive dependency.

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

No branches or pull requests

9 participants