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

[builder] Add strict versioning #9897

Merged
merged 6 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .chloggen/builder-strict-versioning.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: breaking

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: builder

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add strict version checking when using the builder. Add the temporary flag `--skip-strict-versioning `for skipping this check.

# One or more tracking issues or pull requests related to the change
issues: [9896]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext: Strict version checking will error on mismatches between the `otelcol_version` configured and the builder version or versions in the go.mod. This check can be temporarily disabled by using the `--skip-strict-versioning` flag. This flag will be removed in a future minor version.

# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: []
21 changes: 21 additions & 0 deletions cmd/builder/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,24 @@ then commit the code in a git repo. A CI can sync the code and execute
ocb --skip-generate --skip-get-modules --config=config.yaml
```
to only execute the compilation step.

### Strict versioning checks

The builder checks the relevant `go.mod`
file for the following things after `go get`ing all components and calling
`go mod tidy`:

1. The `dist::otelcol_version` field in the build configuration must
match the core library version calculated by the Go toolchain,
considering all components. A mismatch could happen, for example,
when one of the components depends on a newer release of the core
collector library.
2. For each component in the build configuration, the version included
in the `gomod` module specifier must match the one calculated by
the Go toolchain, considering all components. A mismatch could
happen, for example, when the enclosing Go module uses a newer
release of the core collector library.

The `--skip-strict-versioning` flag disables these versioning checks.
This flag is available temporarily and
**will be removed in a future minor version**.
1 change: 1 addition & 0 deletions cmd/builder/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ require (
go.uber.org/goleak v1.3.0
go.uber.org/multierr v1.11.0
go.uber.org/zap v1.27.0
golang.org/x/mod v0.17.0
)

require (
Expand Down
2 changes: 2 additions & 0 deletions cmd/builder/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 7 additions & 6 deletions cmd/builder/internal/builder/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,13 @@ var ErrInvalidGoMod = errors.New("invalid gomod specification for module")

// Config holds the builder's configuration
type Config struct {
Logger *zap.Logger
SkipGenerate bool `mapstructure:"-"`
SkipCompilation bool `mapstructure:"-"`
SkipGetModules bool `mapstructure:"-"`
LDFlags string `mapstructure:"-"`
Verbose bool `mapstructure:"-"`
Logger *zap.Logger
SkipGenerate bool `mapstructure:"-"`
SkipCompilation bool `mapstructure:"-"`
SkipGetModules bool `mapstructure:"-"`
SkipStrictVersioning bool `mapstructure:"-"`
LDFlags string `mapstructure:"-"`
Verbose bool `mapstructure:"-"`

Distribution Distribution `mapstructure:"dist"`
Exporters []Module `mapstructure:"exporters"`
Expand Down
123 changes: 104 additions & 19 deletions cmd/builder/internal/builder/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,42 +4,48 @@
package builder // import "go.opentelemetry.io/collector/cmd/builder/internal/builder"

import (
"bytes"
"errors"
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
"text/template"
"time"

"go.uber.org/zap"
"go.uber.org/zap/zapio"
"golang.org/x/mod/modfile"
)

var (
// ErrGoNotFound is returned when a Go binary hasn't been found
ErrGoNotFound = errors.New("go binary not found")
ErrGoNotFound = errors.New("go binary not found")
ErrStrictMode = errors.New("mismatch in go.mod and builder configuration versions")
errFailedToDownload = errors.New("failed to download go modules")
)

func runGoCommand(cfg Config, args ...string) error {
cfg.Logger.Info("Running go subcommand.", zap.Any("arguments", args))
func runGoCommand(cfg Config, args ...string) ([]byte, error) {
if cfg.Verbose {
cfg.Logger.Info("Running go subcommand.", zap.Any("arguments", args))

Check warning on line 30 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L30

Added line #L30 was not covered by tests
}

// #nosec G204 -- cfg.Distribution.Go is trusted to be a safe path and the caller is assumed to have carried out necessary input validation
cmd := exec.Command(cfg.Distribution.Go, args...)
cmd.Dir = cfg.Distribution.OutputPath

if cfg.Verbose {
writer := &zapio.Writer{Log: cfg.Logger}
defer func() { _ = writer.Close() }()
cmd.Stdout = writer
cmd.Stderr = writer
return cmd.Run()
}
var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr

if out, err := cmd.CombinedOutput(); err != nil {
return fmt.Errorf("go subcommand failed with args '%v': %w. Output:\n%s", args, err, out)
if err := cmd.Run(); err != nil {
return nil, fmt.Errorf("go subcommand failed with args '%v': %w, error message: %s", args, err, stderr.String())

Check warning on line 42 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L42

Added line #L42 was not covered by tests
}
if cfg.Verbose && stderr.Len() != 0 {
cfg.Logger.Info("go subcommand error", zap.String("message", stderr.String()))

Check warning on line 45 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L45

Added line #L45 was not covered by tests
}

return nil
return stdout.Bytes(), nil
}

// GenerateAndCompile will generate the source files based on the given configuration, update go mod, and will compile into a binary
Expand All @@ -64,6 +70,9 @@
}
// create a warning message for non-aligned builder and collector base
if cfg.Distribution.OtelColVersion != defaultOtelColVersion {
if !cfg.SkipStrictVersioning {
return fmt.Errorf("builder version %q does not match build configuration version %q: %w", cfg.Distribution.OtelColVersion, defaultOtelColVersion, ErrStrictMode)
}
cfg.Logger.Info("You're building a distribution with non-aligned version of the builder. Compilation may fail due to API changes. Please upgrade your builder or API", zap.String("builder-version", defaultOtelColVersion))
}
// if the file does not exist, try to create it
Expand Down Expand Up @@ -115,7 +124,7 @@
if cfg.Distribution.BuildTags != "" {
args = append(args, "-tags", cfg.Distribution.BuildTags)
}
if err := runGoCommand(cfg, args...); err != nil {
if _, err := runGoCommand(cfg, args...); err != nil {

Check warning on line 127 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L127

Added line #L127 was not covered by tests
return fmt.Errorf("failed to compile the OpenTelemetry Collector distribution: %w", err)
}
cfg.Logger.Info("Compiled", zap.String("binary", fmt.Sprintf("%s/%s", cfg.Distribution.OutputPath, cfg.Distribution.Name)))
Expand All @@ -131,29 +140,66 @@
}

// ambiguous import: found package cloud.google.com/go/compute/metadata in multiple modules
if err := runGoCommand(cfg, "get", "cloud.google.com/go"); err != nil {
if _, err := runGoCommand(cfg, "get", "cloud.google.com/go"); err != nil {

Check warning on line 143 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L143

Added line #L143 was not covered by tests
return fmt.Errorf("failed to go get: %w", err)
}

if err := runGoCommand(cfg, "mod", "tidy", "-compat=1.21"); err != nil {
if _, err := runGoCommand(cfg, "mod", "tidy", "-compat=1.21"); err != nil {

Check warning on line 147 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L147

Added line #L147 was not covered by tests
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("failed to update go.mod: %w", err)
}

if cfg.SkipStrictVersioning {
return downloadModules(cfg)
}
mx-psi marked this conversation as resolved.
Show resolved Hide resolved

// Perform strict version checking. For each component listed and the
// otelcol core dependency, check that the enclosing go module matches.
modulePath, dependencyVersions, err := cfg.readGoModFile()
if err != nil {

Check warning on line 158 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L158

Added line #L158 was not covered by tests
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
return err
}

corePath, coreVersion := cfg.coreModuleAndVersion()
if dependencyVersions[corePath] != coreVersion {
Copy link
Member

Choose a reason for hiding this comment

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

The error here and below would be a bit confusing if corePath is not on dependencyVersions. While this would be a weird situation that I don't think we can encounter now, IMO for future-proofing we should do the full check (corePathVersion, ok := dependencyVersions[corePath]) and provide a more appropriate error message if ok is false

return fmt.Errorf(
"%w: core collector version calculated by component dependencies %q does not match configured version %q. Use --skip-strict-versioning to temporarily disable this check. This flag will be removed in a future minor version",

Check warning on line 165 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L163-L165

Added lines #L163 - L165 were not covered by tests
ErrStrictMode, dependencyVersions[corePath], coreVersion)
}

for _, mod := range cfg.allComponents() {
module, version, _ := strings.Cut(mod.GoMod, " ")
if module == modulePath {
// No need to check the version of components that are part of the
// module we're building from.

Check warning on line 173 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L173

Added line #L173 was not covered by tests
continue
}

if dependencyVersions[module] != version {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here, IMO we should do the explicit check for presence in dependencyVersions

return fmt.Errorf(
"%w: component %q version calculated by dependencies %q does not match configured version %q. Use --skip-strict-versioning to temporarily disable this check. This flag will be removed in a future minor version",
ErrStrictMode, module, dependencyVersions[module], version)
}
}

return downloadModules(cfg)
}

func downloadModules(cfg Config) error {
cfg.Logger.Info("Getting go modules")
// basic retry if error from go mod command (in case of transient network error). This could be improved
// retry 3 times with 5 second spacing interval
retries := 3
failReason := "unknown"
for i := 1; i <= retries; i++ {
if err := runGoCommand(cfg, "mod", "download"); err != nil {
if _, err := runGoCommand(cfg, "mod", "download"); err != nil {

Check warning on line 194 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L194

Added line #L194 was not covered by tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't been able to add coverage for this because if I would expect this to fail, the previous go get command fails first.

failReason = err.Error()
cfg.Logger.Info("Failed modules download", zap.String("retry", fmt.Sprintf("%d/%d", i, retries)))
time.Sleep(5 * time.Second)
continue
}
return nil
}
return fmt.Errorf("failed to download go modules: %s", failReason)
return fmt.Errorf("%w: %s", errFailedToDownload, failReason)
}

func processAndWrite(cfg Config, tmpl *template.Template, outFile string, tmplParams any) error {
Expand All @@ -165,3 +211,42 @@
defer out.Close()
return tmpl.Execute(out, tmplParams)
}

func (c *Config) coreModuleAndVersion() (string, string) {
module := "go.opentelemetry.io/collector"
if c.Distribution.RequireOtelColModule {

Check warning on line 217 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L217

Added line #L217 was not covered by tests
module += "/otelcol"
}
return module, "v" + c.Distribution.OtelColVersion
}

func (c *Config) allComponents() []Module {
return append(c.Exporters,
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
append(c.Receivers,
append(c.Processors,
append(c.Extensions,
c.Connectors...)...)...)...)
}

func (c *Config) readGoModFile() (string, map[string]string, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't been able to add coverage for this function because in the code path, if I would expect this to fail, the previous go get command fails first.

I can add a separate test that targets this function directly, otherwise not sure how to test this.

var modPath string
stdout, err := runGoCommand(*c, "mod", "edit", "-print")
if err != nil {

Check warning on line 234 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L234

Added line #L234 was not covered by tests
return modPath, nil, err
}
parsedFile, err := modfile.Parse("go.mod", stdout, nil)
if err != nil {

Check warning on line 238 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L238

Added line #L238 was not covered by tests
return modPath, nil, err
}
if parsedFile != nil && parsedFile.Module != nil {
modPath = parsedFile.Module.Mod.Path
}
dependencies := map[string]string{}
for _, req := range parsedFile.Require {
if req == nil {

Check warning on line 246 in cmd/builder/internal/builder/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/builder/internal/builder/main.go#L246

Added line #L246 was not covered by tests
continue
}
dependencies[req.Mod.Path] = req.Mod.Version
}
return modPath, dependencies, nil
}
88 changes: 81 additions & 7 deletions cmd/builder/internal/builder/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,6 @@ func TestGenerateDefault(t *testing.T) {
require.NoError(t, Generate(NewDefaultConfig()))
}

func TestGenerateInvalidCollectorVersion(t *testing.T) {
cfg := NewDefaultConfig()
cfg.Distribution.OtelColVersion = "invalid"
err := Generate(cfg)
require.NoError(t, err)
}

func TestGenerateInvalidOutputPath(t *testing.T) {
cfg := NewDefaultConfig()
cfg.Distribution.OutputPath = "/:invalid"
Expand All @@ -34,6 +27,87 @@ func TestGenerateInvalidOutputPath(t *testing.T) {
require.Contains(t, err.Error(), "failed to create output path")
}

func TestVersioning(t *testing.T) {
tests := []struct {
description string
cfgBuilder func() Config
expectedErr error
}{
{
description: "success",
cfgBuilder: func() Config {
cfg := NewDefaultConfig()
cfg.Distribution.Go = "go"
return cfg
},
expectedErr: nil,
},
{
description: "old otel version",
cfgBuilder: func() Config {
cfg := NewDefaultConfig()
cfg.Distribution.OtelColVersion = "0.90.0"
return cfg
},
expectedErr: ErrStrictMode,
},
{
description: "old otel version without strict mode",
cfgBuilder: func() Config {
cfg := NewDefaultConfig()
cfg.Distribution.Go = "go"
cfg.SkipStrictVersioning = true
cfg.Distribution.OtelColVersion = "0.90.0"
return cfg
},
expectedErr: nil,
},
{
description: "invalid version",
cfgBuilder: func() Config {
cfg := NewDefaultConfig()
cfg.Distribution.OtelColVersion = "invalid"
return cfg
},
expectedErr: ErrStrictMode,
},
{
description: "invalid version without strict mode, only generate",
cfgBuilder: func() Config {
cfg := NewDefaultConfig()
cfg.Distribution.OtelColVersion = "invalid"
cfg.SkipGetModules = true
cfg.SkipCompilation = true
cfg.SkipStrictVersioning = true
return cfg
},
expectedErr: nil,
},
{
description: "old component version",
cfgBuilder: func() Config {
cfg := NewDefaultConfig()
cfg.Distribution.Go = "go"
cfg.Exporters = []Module{
{
Import: "go.opentelemetry.io/collector/receiver/otlpreceiver",
GoMod: "go.opentelemetry.io/collector v0.96.0",
},
}
return cfg
},
expectedErr: ErrStrictMode,
},
}
for _, tc := range tests {
t.Run(tc.description, func(t *testing.T) {
cfg := tc.cfgBuilder()
err := GenerateAndCompile(cfg)
require.ErrorIs(t, err, tc.expectedErr)
})
}
}

func TestSkipGenerate(t *testing.T) {
cfg := NewDefaultConfig()
cfg.Distribution.OutputPath = t.TempDir()
Expand Down
Loading
Loading