Skip to content

Add support for updating specified modules via go get and record as patch to go.mod, go.sum#63

Open
e4t wants to merge 6 commits into
openSUSE:masterfrom
e4t:update_modules
Open

Add support for updating specified modules via go get and record as patch to go.mod, go.sum#63
e4t wants to merge 6 commits into
openSUSE:masterfrom
e4t:update_modules

Conversation

@e4t

@e4t e4t commented Mar 26, 2025

Copy link
Copy Markdown

To address issues with vendored modules - in particular to address CVEs quickly, obs-service-go_modules has been extended to 'patch' go.mod and go.sum.
The changes are described in a YAML file. The service will apply these changes to go.mod and go.sum before the the vendor files are downloaded. A patch will be created that can be applied during the %prep stage, also the changes file is updated with any changes not yet recorded.
This avoids having to touch the source tarball itself and allows to keep it pristine. At the same time, the updates to the YAML file can be done by an automated tool evaluating the results from govulncheck. Multiple updates to the same module should be harmless as long as the update to the later version comes later in the list.
Without additional entries to the _service file, obs-service-go_modules will behave as before.

These changes will complement the ideas discussed on the Go module dependency handling nicely.

e4t added 3 commits March 20, 2025 16:26
libarchive as distributed on SLE 15 does not support the mtime,
ctime and atime arguments. In this case, call back to using
os.utime() instead.

Signed-off-by: Egbert Eich <eich@suse.com>
Older python versions do not support the capture_output argument to the
run comand. In this case stderr is not a string.

Signed-off-by: Egbert Eich <eich@suse.com>
Signed-off-by: Egbert Eich <eich@suse.com>
@e4t e4t force-pushed the update_modules branch from 90f2c05 to ff6f2b2 Compare March 26, 2025 18:00
The module update information is maintained in a YAML file. This service
will read the YAML file, apply the changes, create a patch and update
the changelog file. Once this is done, it will proceed creating the
vendor tarball.
The advantage if this approach over patching go.mod and go.sum directly
is that the YAML file can be created automatically without the need to
untar the source tarball. This is all done by `go_modules`.

Added functionality in detail:
Once the source tar ball is expanded, this performs the following steps:
1. Parse a YAML file with information on module versions that require
   changes,
2. apply these changes using `go get <module>[@<version>`,
3. Add any comments that do not exist yet in .changes file(s) to these.
4. create a patch that can be used with in the spec file. If no changes
   are required, the patch file will be empty.
For this 3 new arguments are introduced:
* `modupdates`: name of the YAML file. Default is 'none'.
* `moddiff`: name of the patch file to generate. The patch file will
   be written to `outdir`. Default: `go-modules.patch`
* 'changesfile': if multiple changes files exist. by default all of
  them are checked for missing comments and comments are added to the
  ones where it is missing. This option allows to specify an individual
  changes file.
The YAML file has the following format:
go_module:
   module_updates:
   - uri: <module>@<version>
     comment: <comment>
the uri list elements are passed to `go get` verbatim. A minimal check for
unsupported characters is performed. The `comment` element is optional. Any
comment is checked against the changes file(s) if it exist already. If not,
a new comment is created with all comment elements that don't already exist.
Multi-line comments are possible, line-breaks need to be indicated by `\n`.

Signed-off-by: Egbert Eich <eich@suse.com>
@e4t e4t force-pushed the update_modules branch from ff6f2b2 to ccea3f0 Compare March 26, 2025 18:01
@jfkw

jfkw commented Apr 21, 2025

Copy link
Copy Markdown
Collaborator

Thanks @e4t for the comprehensive PR and its useful functionality. Apologies for the delay in review, I'm currently working on priority tasks with the go1.x toolchain and it will be another week or two before I can review, test and align with some related functionality in #57 and #59.

This PR's functionality will be reviewed as a potential complement to #57 and #59. We also need to consider the VEX Vulnerability Exchange Format and upcoming its support in govulncheck. Currently purl-spec upstream is discussing whether to evolve the GOPATH-era golang: purl tag or create a modern go: successor tag which is modules-aware. govulncheck upstream is waiting on that decision.

I would prefer to avoid taking on an external dependency on YAML, so we would express config using _service parameters, a file format supported in the standard library, or as a last option a bespoke format e.g. key: value. I do like YAML the format for the essential scalars, lists, and dicts, it would be great here if that subset were in the standard library. Managing C extension different versions across distros is proving unwieldy. On that topic, I will cherry-pick your commit handling the libarchive API change ASAP, thanks for thinking of that run-time check.

Thanks again.

@jfkw jfkw changed the title Update modules Add support for updating specified modules via go get and record as patch to go.mod, go.sum Apr 21, 2025
@jfkw

jfkw commented Apr 21, 2025

Copy link
Copy Markdown
Collaborator

Related use case in #61.

@e4t

e4t commented Apr 25, 2025

Copy link
Copy Markdown
Author

@jfkw - The YAML file allows for an ordered list where each list element contains two entries: uri and comment. This was chosen to support automation:

  1. The fact that the list is ordered is important: it can be amended with new updates, potentially overriding older ones (without having to remove the previous ones).
  2. An (optional) description can be added which will will be used to generate a changelog entry. This will relieve a tool from having to deal with potentially multiple changes files.
    The service infrastructure in osc only knows service names and parameters. It does seem to be possible to specify multiple parameter entries with the same name which will be added to the command line in the same order, but it is unclear whether this is a guaranteed. Also, a there is no concept of multiple elements per named parameter.
    Therefore, a
go_module:
   module_updates:
   - uri: golang.org/x/net@v0.23.0
     comment: "Update golang.org/x/net to v0.23 to fix CVE-2023-45288\n
(bnc#1236528)."
   - uri: github.com/containers/image/v5@v5.30.2
     comment: "Fixes an interoperability issue while listing tags from JFrog Artifactory."

would look somehwat like:

<services>
  <service name="go_modules" mode="disabled">
    <param name="updateuri">github.com/containers/image/v5@v5.30.2</param>
    <param name="updatecomment">Update golang.org/x/net to v0.23 to fix CVE-2023-45288\n
    (bnc#1236528).</param>
    < param name="updateuri">github.com/containers/image/v5@v5.30.2</param>
    <param name="updatecomment">Fixes an interoperability issue while listing tags from JFrog Artifactory.</param>
    <param name="moddiff">go-module.patch</param>
  </service>
[...]
</services>

which would then be converted to:

/usr/lib/obs/service/go_modules [...] --updateuri github.com/containers/image/v5@v5.30.2 \
   --updatecomment "Update golang.org/x/net to v0.23 to fix CVE-2023-45288\n(bnc#1236528)." \
   --updateuri github.com/containers/image/v5@v5.30.2 \
   --updatecomment "Fixes an interoperability issue while listing tags from JFrog Artifactory."

(AFAICT, the spaces on the entry text will be handled correctly as the entire text will be passed as a single entry of a list.)
Until here, the updateuri and updatecomment elements are associated. This association is lost when go_modules has parsed the command line: each command line argument will be stored in its own list. This would require to have as many comments as update URIs (or none at all).
I'm afraid the simplicity of the _service file model will limit what we are able to do. Using YAML will get us around it.
Using some bespoke key: value format in the xml text element would be ugly. If we are able to assume that the full URI will never contain spaces, we may get by with:

 < param name="update">github.com/containers/image/v5@v5.30.2 Fixes an interoperability issue while listing tags from JFrog Artifactory.</param>

and just treat text after the URI as a comment.

This PR's functionality will be reviewed as a potential complement to #57 and #59.

Both documents talk about running go_modules during the 'online' phase - are you referring to running this service 'server side'? Could you elaborate how this would would work, considering that vendoring will require access to the upstream repos while obs workers do not have access to the internet - at least as long as we do not maintain our own mirrors of these repositories.

We also need to consider the VEX Vulnerability Exchange Format and upcoming its support in govulncheck. Currently purl-spec upstream is discussing whether to evolve the GOPATH-era golang: purl tag or create a modern go: successor tag which is modules-aware. govulncheck upstream is waiting on that decision.

The version of govulncheck I've tested seems to use neither format. For our purposes the use of a full PURL would be redundant in the YAML of _service file.
I would like to suggest to not make the integration of govulncheck into obs-service-go_modules mandatory: for both of my use cases it does not work with the package sources (as these require configuration outside of the Go eco system). It does, however, work with the binaries. These may not be available, though, when the source service is run.

@mcepl mcepl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you consider my comments?

Comment thread README.md
<services>
<service name="go_modules" mode="disabled">
<param name="modupdates">go-module-updates.yaml</param>
<param name="moddiff">go-module.patch</param>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that line here when it is the default value? Is there some hidden purpose for it?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an example of / template for the service file. It's not uncommon to use the default value in examples, is it? There's no hidden agenda, no.

Comment thread README.md

3. a `vendor.tar.gz` file with updated vendor modules.

Note, that the `moddiff` parameter in the service file is set to the default

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, why not just omit it there, and omit this paragraph here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to avoid confusion. I can move the comment up - in front of the example - but I would like to leave this with all options in place to serve as a template.

@jfkw

jfkw commented May 19, 2025

Copy link
Copy Markdown
Collaborator

@e4t thanks for configuring the PR branch to be open for collaborator push. I added one small fix for a merge conflict. There will be more pre-merge fixup commits in the coming days as new commits land. If there are any suggested changes to how the PR works, they'll be discussed here before pushing.

@e4t

e4t commented May 19, 2025

Copy link
Copy Markdown
Author

@jfkw - Thank you! Please let me know if you want me to try and convert the use of an external YAML file to xml - using the existing infrastructure of the _service file.
I may not be able to do it this week though as I'm at the SUSE Labs conference.

@jfkw

jfkw commented May 19, 2025

Copy link
Copy Markdown
Collaborator

@e4t. No changes needed at the moment, enjoy the conference.

In order to allow users of distribution with old libarchive to use this
service, catch the exception generated in this case and fallback to non
reproducible behavior.

A message is logged to inform user of the situation

@mcepl mcepl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I feel persuaded.

@mcepl

mcepl commented Jun 17, 2025

Copy link
Copy Markdown

The need for this has been replaced by (merged) #57, wasn’t it?

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 this pull request may close these issues.

3 participants