search

cloudfoundry / community

Governance and contact information for Cloud Foundry
Apache License 2.0
38 stars 168 forks source link

Proposal for removing replace directive in go modules #921

Open winkingturtle-vmw opened 2 months ago

winkingturtle-vmw commented 2 months ago

[!NOTE] This issue is NOT an RFC. The only reason this issue is opened here instead of a specific repo is because it applies to multiple areas (Diego, Garden Containers, Networking) under app-runtime-platform and it was mentioned in monthly working-group meeting July 11th 2024

Problem

Over the years, we had to lock go dependencies to an older version of a component when we didn’t have the capacity to keep dependencies up-to-date. Now that we can keep up with updating required dependencies, we need to revisit those replace directives and remove them so that we can get the latest version of a dependent module.

This PR for groot-windows is one of latest issues we discovered when we decided to bump to go1.22. Thankfully this issue was resolved with minimal changes to groot-windows, but most of the time this is not the case. The problem arises when we need to do the bump (due to keeping up with security patches) and the work to do the bump is quite significant. This proposal aims to front-load all of the bump issues to now so that when the time comes for patching a security fix, the work has already been done.

Proposed Solution

We would start by removing every single replace directive to see what will fail in CI or at runtime. There are 3 possible scenarios that can happen:

  1. Newer dependency just works. This is the happy path and we would simply remove the replace directive.
  2. Newer dependency fails CI and the fix is in our codebase. We would spend the time fixing our code so that it will be compatible with newer versions of the library.
  3. Newer dependency fails CI and the fix is outside of our control. This would be the case when the library is used inside of another library authored by someone else outside of the org. There are 2 possible ways we could approach this
    1. Outer library is maintained and had a commit within the last year. This is the case where we would open an issue with the owner or try to fix the issue within the library.
    2. Outer library is deprecated and not maintained. This is the case where we would look for alternative libraries or re-writing the logic in our codebase to not depend on this library.

If a dependency had to be kept around for the time being (Option 3i), we would create a replace-directive.yml file in CI under the repo dir with the following format:

---
replace_exceptions:
- module: code.cloudfoundry.org
  exceptions:
  - name: http://github.com/spf13/cobra
    reason:
       type: github_issue or github_pr
       refrence: https://github.com/link-to-issue
    expires_at: 2026-03-01

Additionally, there will be a concourse job for each pipeline that will validate the list defined in replace-directive.yml against what’s defined in the go.mod file of each repo. The job will go red when expires_at or closing of the issue/PR has been reached. For now, we would only care about github_issue and github_pr types, but maybe down the line there is a need to allow more types.

Since some of the pipelines contain submodules that are in itself a go-module (e.g. garden in garden-runc-release), replace-directive.yml is scoped by a module to accommodate defining multiple modules.

Development

Above proposal would require writing a new go binary that would utilize modfile package and github golang API to check whether an issue or PR is closed or not. Since we would probably get rate-limited by Github for unauthenticated access, the binary would require providing a token to authenticate requests. The input for the new binary would be a go.mod and replace-directive.yml and the output would be an error in stderr if issue/pr has been closed or expires_at has been reached.

winkingturtle-vmw commented 2 months ago

cc @maxmoehl

maxmoehl commented 2 months ago

Our list of dependencies which need to be bumped or replaced:

/cc @plowin @Soha-Albaghdady