Licensing review ahead of adding/updating dependencies
Before a dependency import/update is submitted, the submitter should seek a a review of the license of the dependency by an engineering Technical Lead (TL) at CRL.
It’s important to do this first, even in the case of an UPDATE, because the licensing terms may have changed since the previous version.
The CRL TL will follow licensing review / instructions available in our internal wiki.
In most cases, the code licensing follows established industry patterns and this review will be quick. If the licensing is using non-standard terms, this review may last for longer. Expect up to two weeks review if the CRL legal team needs to be involved.
Preparing the change PR
The PR containing the dependency change should be prepared according to the technical instructions in the dependency README on the branch where the dependency is being updated.
Reviewing a dependency addition or update
This is the most important and sensitive part.
The CRL team will review this PR.
Two following aspects will need to be scrutinized carefully: licensing and technical adequacy.
Additionally, it sometimes happens that updating a domain-specific, feature-oriented dependency forces another dependency (or multiple) to be updated as well, and the other dependency is “infrastructure” with impact on many teams. When this happens, we need another layer of review for architectural impact.
A common case is when upgrading certain encoding packages (useful to a few features) forces an upgrade to protobuf or gRPC (of architectural significance to the entire project).
Reviewing dependency licensing
The licensing details for the code, as well as licensing details for any additional dependency indirectly imported by this code, will be reviewed to ensure it adheres to our dependency and licensing standards.
These standards are described in an internal documentation page.
Reviewing technical adequacy of external dependencies
Someone who is expert with the technical domain of the dependency must review the code in the dependency and all its new/updated indirect dependencies to check the level of quality of the code, and its potential impact on the CRL code base. In particular, the following areas will need to be validated:
That the dependency’s size is proportional to its age and reputation: we are careful to only import large dependencies if they have a track record of working well for other production-ready systems. Importing a younger project is OK when it is so small it can be reviewed in its entirety by a CRL engineer during the import/upgrade.
That the dependency has minimal use of global state.
That synchronization and concurrency are used properly.
That it does not leak memory by design.
That it does not contain side effects that cannot be controlled by CRL’s product (e.g. direct writes to stdout/stderr, mutating state in the Go runtime, in other dependencies, etc).
That it has controllable error behavior, either using error objects or recoverable panic objects (in particular it does not terminate the process even when something unexpected happen).
That the dependency is not responsible for accessing network resources or connecting the process to other systems (including other processes on the same machine). If it does, this will need an extra review by our security team.
That the dependency is not using cryptography or other core security functions. If it does, this will need an extra review by our security team.
Generally, dependency updates need much more scrutiny than other changes in CRL’s products. This is because for internal reviews we have the benefit of trust within the team. This trust should not be extended implicitly to authors of third party dependencies, unless they happen to also be CRL team members.
Reviewing for architectural impact of key dependencies
Sometimes a key dependency is upgraded as a side-effect of (attempting to) upgrading a more lightweight dependency. When this happens, more scrutiny is needed from a TL of the team who “owns” the key dependency.
During this review, we will evaluate:
potential impact on stability
potential impact on performance
potential impact on security (especially in other areas than the one that triggered the upgrade in the first place)
cross-checking the bug tracker of the key dependency to see if there are any current issues that may affect the CRL product.
At the time of this writing, the key dependencies that should trigger an advanced review for architectural impact include (but are not limited to):
go.etcd.io/etcd/raft (now go.etcd.io/raft)
Copyright (C) Cockroach Labs.
Attention: This documentation is provided on an "as is" basis, without warranties or conditions of any kind, either express or implied, including, without limitation, any warranties or conditions of title, non-infringement, merchantability, or fitness for a particular purpose.