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.

Sign up for GitHub

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

Jump to bottom

Review hashicorp license change impact #9181

Closed
2 of 3 tasks
killianmuldoon opened this issue Aug 14, 2023 · 24 comments
Closed
2 of 3 tasks

Review hashicorp license change impact #9181

killianmuldoon opened this issue Aug 14, 2023 · 24 comments
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@killianmuldoon
Copy link
Contributor

killianmuldoon commented Aug 14, 2023 •
edited
Loading

Recently hashicorp announced changes to some of its licenses. This issue is a place to assess the impact on Cluster API.

CNCF issue: cncf/foundation#617

None of the dependencies CAPI imports have been updated to the Business source license.

Note: The original finding in this issue are irrelevant at this point. See below for the current state and plan-of-action.

Initial findings

Generate list of all modules which are importing a hashicorp module. This must be done for each of the three go modules in the CAPI repo.

go mod graph | grep "\s.*hashicorp" > hashicorp_modules
cd test ; go mod graph | grep "\s.*hashicorp" >> ../hashicorp_modules
cd ../hack/tools ; go mod graph | grep "\s.*hashicorp" >> ../../hashicorp_modules
cd ../..

Get a unique sorted list of hashicorp modules that are being imported

cat hashicorp_modules | cut -d ' ' -f 2 | sort -u -o hashicorp_modules

The end result of the above is a list of 27 modules. Some are the same module with a different version.

Outstanding tasks:
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Aug 14, 2023
@killianmuldoon
Copy link
Contributor Author

This list reflects the current state of the master branches. These can change in future versions.

It could be good idea to add license scanning on PRs so that if a version updates to a non-compatible license we can catch it. Not sure what k/k is doing in this area right now.

@killianmuldoon
Copy link
Contributor Author

Looks like trivvy has this functionality - will take a look at how it works and whether it's useful.

@sbueringer
Copy link
Member

Thx for looking into this!

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 14, 2023
@vincepri
Copy link
Member

Thanks @killianmuldoon ! We might want to broadcast the news at this week community meeting, there is usage in other Cluster API repositories as well.

This was referenced Aug 14, 2023
Merged
Closed
@sbueringer
Copy link
Member

@killianmuldoon
https://github.com/kubernetes/kubernetes/blob/master/hack/unwanted-dependencies.json#L30
from https://kubernetes.slack.com/archives/C5P3FE08M/p1691699636105219?thread_ts=1691699636.105219&cid=C5P3FE08M

Just in case it's relevant

@sbueringer
Copy link
Member

sbueringer commented Aug 15, 2023 •
edited
Loading

Looks like they are blocking dependencies because of MPL (because "MPL license not in CNCF allowlist") Would be interesting to know how they enforce this config (and eg if they check only go.mod or also go.sum)

Also would be good to know why we have those dependencies

@killianmuldoon
Copy link
Contributor Author

killianmuldoon commented Aug 15, 2023 •
edited
Loading

Looks like they are blocking dependencies because of MPL (because "MPL license not in CNCF allowlist") Would be interesting to know how they enforce this config (and eg if they check only go.mod or also go.sum)
Also would be good to know why we have those dependencies

Their tool only checks go.mod AFAIU, and they have those dependencies in the go.sum. I'm still doing some research on how the CNCF sees go.sum dependencies, but I haven't found anything concrete yet.

@furkatgofurov7
Copy link
Member

Thanks @killianmuldoon looking into this!

@sbueringer
Copy link
Member

sbueringer commented Aug 15, 2023 •
edited
Loading

I recently implemented a feature in CR for authorized metrics. I ended up implementing it in a way that you only actually get dependencies like k8s.io/apiserver in your go.mod file when you are using it (i.e. importing a certain package). If you don't use it, it will only end up in go.sum.

So my rough guess is that dependencies which are only in go.sum are not actually getting compiled into your binary because you don't need any of their packages. They are probably just part of "what if you include all dependencies of all dependencies without looking at specific packages". But really just a guess.

Probably we should just ask in some Kubernetes Slack channel about the background of only checking go.mod and they can tell us (it seems k/k is aligning to CNCF recommendations)

@mboersma mboersma mentioned this issue Aug 16, 2023
Open
@nrb
Copy link
Contributor

nrb commented Aug 16, 2023

In terms of tooling to help mitigate these issues on an ongoing basis, https://compliance.linuxfoundation.org/references/tools/ mentions some tools for licensing compliance. It appears the LF-sponsored project was spun down in 2020, but Fossology is active.

The downside to Fossology from what I see is that it appears to be designed as a live database system, which would require extra hosting.

@sbueringer
Copy link
Member

I would definitely like to align to upstream k/k if somehow possible

@BenTheElder
Copy link
Member

BenTheElder commented Aug 16, 2023 •
edited
Loading

I would definitely like to align to upstream k/k if somehow possible

The #k8s-code-organization group is a good group to talk to about go dependency management, it's usually mostly focused on kubernetes/kubernetes under sig architecture but more general dependency management comes up.

So my rough guess is that dependencies which are only in go.sum are not actually getting compiled into your binary because you don't need any of their packages. They are probably just part of "what if you include all dependencies of all dependencies without looking at specific packages". But really just a guess.

go.sum contains all observed dependencies in the module graph. When go computes what versions should be used it looks at the entire dependency graph of all transitive deps, even if they're not going to wind up in the main module / binary.

In go1.17+ modules (NOTE: that's NOT the go compiler version, it's $x in go $x statement in the go.mod file, controlling the language version of the module), go.mod contains all required dependencies to build and test the main module. Reference: https://tip.golang.org/doc/go1.18#go-mod-download

As for tooling, see https://kubernetes.slack.com/archives/CHGFYJVAN/p1692213758848149

go-licenses can collect the license info for dependencies https://github.com/google/go-licenses, though it's an open question what kubernetes / code organization would recommend (currently discussing), the main repo has checked in vendor and a bunch of custom scripts to audit and manage dependencies / vendor.

@BenTheElder
Copy link
Member

See also: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/vendor.md#reviewing-and-approving-dependency-changes

@killianmuldoon
Copy link
Contributor Author

Looks like our last exposure here is the indirect dependency (through viper) on github.com/hashicorp/hcl

cluster-api/go.mod

Line 94 in 932cdc4

github.com/hashicorp/hcl v1.0.0 // indirect

This dependency doesn't have an approved license by the CNCF rules. It is on an exemption list from 2022 . I need to figure out whether or not that exemption carries forward to today.

@killianmuldoon
Copy link
Contributor Author

It seems like the exceptions in those files are cumulative - as evidenced by this PR: cncf/foundation#621 and some other information on that CNCF repo.

Because of this I'm inclined to close this issue after merging #9184. At that point we will have the automation in place to prevent this from happening in future.

@BenTheElder
Copy link
Member

Because of this I'm inclined to close this issue after merging #9184. At that point we will have the automation in place to prevent this from happening in future.

Maybe I missed something, but my quick skim suggests that PR only covers container images, while this repo also releases binaries and may develop dependencies in other places (test code? clusterctl? etc.)

@killianmuldoon
Copy link
Contributor Author

killianmuldoon commented Aug 24, 2023 •
edited
Loading

The trivy scan is looking at files in the repo - so it catches the go.mod + scripts etc. I think that should be sufficient for what's in the binaries. What's definitely missing is our container images which have binaries + tools etc. from the base images.

I think you're right that we should also setup something for image scanning - maybe snyk like the sig-security scan for k/k.

@sbueringer
Copy link
Member

sbueringer commented Aug 25, 2023 •
edited
Loading

I might be wrong but I thought for CVEs Trivy can scan based on source code and image (we definitely scan for CVE's based on images). Can't it do the same for licenses?

Also wondering what we actually have in our base images, I think we only use gcr.io/distroless (so no alpine or something like that). But never looked into what distroless contains, I assumed almost nothing

@killianmuldoon
Copy link
Contributor Author

I might be wrong but I thought for CVEs Trivy can scan based on source code and image (we definitely scan for CVE's based on images). Can't it do the same for licenses?

It can - we could do a periodic license scan, but that's not covered in #9184 which is for blocking forbidden licenses incoming in PRs.

I'll open a separate issue for image scanninng, I think there's probably a number of choices to be made there.

@killianmuldoon killianmuldoon mentioned this issue Sep 14, 2023
Open
@killianmuldoon
Copy link
Contributor Author

I've opened #9436 to track periodic license scanning.

IMO we should ignore "Remove the indirect dependency on hashicorp HCL." for now as this dependency is accepted at the CNCF level. Using current tooling we will be able to catch a change in its license. If we really want to remove that dependency we could track it in a follow up.

Removing the dependency from our code involves removing viper which is a lot of work for little gain in this instance, at least in my opinion.

/close

@k8s-ci-robot
Copy link
Contributor

@killianmuldoon: Closing this issue.

In response to this:

I've opened #9436 to track periodic license scanning.

IMO we should ignore "Remove the indirect dependency on hashicorp HCL." for now as this dependency is accepted at the CNCF level. Using current tooling we will be able to catch a change in its license. If we really want to remove that dependency we could track it in a follow up.

Removing the dependency from our code involves removing viper which is a lot of work for little gain in this instance, at least in my opinion.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sbueringer
Copy link
Member

Let's just wait and see. Maybe viper just drops the dependency sooner or later.

@sbueringer
Copy link
Member

sbueringer commented Sep 14, 2023 •
edited
Loading

Pretty sure viper itself can't accept a change to a more problematic license

@BenTheElder
Copy link
Member

IIRC we previously removed viper from k/k for reasons like this ....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@nrb @BenTheElder @vincepri @sbueringer @k8s-ci-robot @furkatgofurov7 @killianmuldoon