From b5b5e034a25bfe8faf645b90900ac8cdbf67c381 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 3 May 2026 06:55:16 -0500 Subject: [PATCH 1/2] feat(feature): add OCI pull retry with URL sanitization Add retry logic (3 attempts, exponential backoff) to OCI feature pulls so transient registry failures don't break workspace creation. Strip registry URLs to hostname-only in error messages to avoid exposing internal paths. --- pkg/devcontainer/feature/features.go | 25 +++- pkg/devcontainer/feature/features_oci_test.go | 36 +++++ pkg/devcontainer/feature/oci_retry.go | 59 ++++++++ pkg/devcontainer/feature/oci_retry_test.go | 133 ++++++++++++++++++ pkg/devcontainer/feature/sanitize.go | 18 +++ pkg/devcontainer/feature/sanitize_test.go | 50 +++++++ 6 files changed, 316 insertions(+), 5 deletions(-) create mode 100644 pkg/devcontainer/feature/features_oci_test.go create mode 100644 pkg/devcontainer/feature/oci_retry.go create mode 100644 pkg/devcontainer/feature/oci_retry_test.go create mode 100644 pkg/devcontainer/feature/sanitize.go create mode 100644 pkg/devcontainer/feature/sanitize_test.go diff --git a/pkg/devcontainer/feature/features.go b/pkg/devcontainer/feature/features.go index 9ff3dc42e..cdceabc84 100644 --- a/pkg/devcontainer/feature/features.go +++ b/pkg/devcontainer/feature/features.go @@ -147,6 +147,23 @@ func checkFeatureCache(id string) (string, bool) { return "", false } +func pullOCIImage(ref name.Reference) (v1.Image, error) { + var img v1.Image + err := retryOCIPull(func() error { + log.Debugf("fetching OCI image: reference=%s", ref.String()) + var fetchErr error + img, fetchErr = remote.Image(ref, remote.WithAuthFromKeychain(authn.DefaultKeychain)) + return fetchErr + }) + if err != nil { + err = image.SanitizeRegistryError(err) + registry := sanitizeURL(ref.Context().RegistryStr()) + log.Debugf("failed to fetch OCI image: error=%v, registry=%s", err, registry) + return nil, fmt.Errorf("pull from %s: %w", registry, err) + } + return img, nil +} + func processOCIFeature(id string) (string, error) { log.Debugf("processing OCI feature: featureId=%s", id) @@ -167,19 +184,17 @@ func processOCIFeature(id string) (string, error) { return "", err } - log.Debugf("fetching OCI image: reference=%s", ref.String()) - img, err := remote.Image(ref, remote.WithAuthFromKeychain(authn.DefaultKeychain)) + img, err := pullOCIImage(ref) if err != nil { - err = image.SanitizeRegistryError(err) - log.Debugf("failed to fetch OCI image: error=%v, reference=%s", err, ref.String()) return "", err } destFile := filepath.Join(featureFolder, "feature.tgz") + registry := sanitizeURL(ref.Context().RegistryStr()) err = downloadLayer(img, id, destFile) if err != nil { log.Debugf("failed to download feature layer: error=%v, featureId=%s", err, id) - return "", err + return "", fmt.Errorf("download layer from %s: %w", registry, err) } file, err := os.Open(destFile) diff --git a/pkg/devcontainer/feature/features_oci_test.go b/pkg/devcontainer/feature/features_oci_test.go new file mode 100644 index 000000000..8d297ca78 --- /dev/null +++ b/pkg/devcontainer/feature/features_oci_test.go @@ -0,0 +1,36 @@ +package feature + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/suite" +) + +type OCIFeatureTestSuite struct { + suite.Suite +} + +func TestOCIFeatureTestSuite(t *testing.T) { + if testing.Short() { + t.Skip("skipping OCI pull test in short mode") + } + suite.Run(t, new(OCIFeatureTestSuite)) +} + +func (s *OCIFeatureTestSuite) TestProcessOCIFeature_HappyPath() { + dir := s.T().TempDir() + orig := os.Getenv("XDG_CACHE_HOME") + s.T().Setenv("XDG_CACHE_HOME", dir) + defer func() { + if orig != "" { + _ = os.Setenv("XDG_CACHE_HOME", orig) + } + }() + + result, err := processOCIFeature("ghcr.io/devcontainers/features/go:1") + s.Require().NoError(err) + s.DirExists(result) + s.FileExists(filepath.Join(result, "devcontainer-feature.json")) +} diff --git a/pkg/devcontainer/feature/oci_retry.go b/pkg/devcontainer/feature/oci_retry.go new file mode 100644 index 000000000..e7510bf10 --- /dev/null +++ b/pkg/devcontainer/feature/oci_retry.go @@ -0,0 +1,59 @@ +package feature + +import ( + "errors" + "net/http" + "time" + + "github.com/devsy-org/devsy/pkg/log" + "github.com/google/go-containerregistry/pkg/v1/remote/transport" +) + +const ( + ociMaxRetries = 3 + ociBaseDelay = 1 * time.Second + ociRetryExponent = 2 +) + +// isTransientError returns true for errors that may resolve on retry: +// network timeouts, connection resets, and 5xx server errors. +func isTransientError(err error) bool { + if err == nil { + return false + } + + var terr *transport.Error + if errors.As(err, &terr) { + return terr.StatusCode >= http.StatusInternalServerError + } + + return true +} + +// retryOCIPull executes fn up to ociMaxRetries times with exponential backoff. +// It only retries when isTransientError returns true. +func retryOCIPull(fn func() error) error { + var lastErr error + delay := ociBaseDelay + + for attempt := range ociMaxRetries { + if attempt > 0 { + log.Debugf("OCI pull retry: attempt=%d, delay=%v", attempt+1, delay) + time.Sleep(delay) + delay *= ociRetryExponent + } + + lastErr = fn() + if lastErr == nil { + return nil + } + + if !isTransientError(lastErr) { + return lastErr + } + + log.Debugf("OCI pull transient failure: attempt=%d, error=%v", attempt+1, lastErr) + } + + return lastErr +} diff --git a/pkg/devcontainer/feature/oci_retry_test.go b/pkg/devcontainer/feature/oci_retry_test.go new file mode 100644 index 000000000..1611cc33e --- /dev/null +++ b/pkg/devcontainer/feature/oci_retry_test.go @@ -0,0 +1,133 @@ +package feature + +import ( + "fmt" + "net/http" + "testing" + "time" + + "github.com/google/go-containerregistry/pkg/v1/remote/transport" + "github.com/stretchr/testify/suite" +) + +type OCIRetryTestSuite struct { + suite.Suite +} + +func TestOCIRetryTestSuite(t *testing.T) { + suite.Run(t, new(OCIRetryTestSuite)) +} + +func (s *OCIRetryTestSuite) TestRetrySucceedsOnThirdAttempt() { + attempts := 0 + err := retryOCIPull(func() error { + attempts++ + if attempts < 3 { + return fmt.Errorf("connection reset") + } + return nil + }) + + s.NoError(err) + s.Equal(3, attempts) +} + +func (s *OCIRetryTestSuite) TestRetryGivesUpAfterMaxAttempts() { + attempts := 0 + err := retryOCIPull(func() error { + attempts++ + return fmt.Errorf("persistent network error") + }) + + s.Error(err) + s.Equal(ociMaxRetries, attempts) + s.Contains(err.Error(), "persistent network error") +} + +func (s *OCIRetryTestSuite) TestNoRetryOn4xx() { + attempts := 0 + err := retryOCIPull(func() error { + attempts++ + return &transport.Error{StatusCode: http.StatusNotFound} + }) + + s.Error(err) + s.Equal(1, attempts) +} + +func (s *OCIRetryTestSuite) TestNoRetryOn401() { + attempts := 0 + err := retryOCIPull(func() error { + attempts++ + return &transport.Error{StatusCode: http.StatusUnauthorized} + }) + + s.Error(err) + s.Equal(1, attempts) +} + +func (s *OCIRetryTestSuite) TestRetryOn5xx() { + attempts := 0 + err := retryOCIPull(func() error { + attempts++ + if attempts < 3 { + return &transport.Error{StatusCode: http.StatusServiceUnavailable} + } + return nil + }) + + s.NoError(err) + s.Equal(3, attempts) +} + +func (s *OCIRetryTestSuite) TestSucceedsFirstTry() { + attempts := 0 + err := retryOCIPull(func() error { + attempts++ + return nil + }) + + s.NoError(err) + s.Equal(1, attempts) +} + +func (s *OCIRetryTestSuite) TestExponentialBackoff() { + attempts := 0 + var timestamps []time.Time + err := retryOCIPull(func() error { + timestamps = append(timestamps, time.Now()) + attempts++ + if attempts < 3 { + return fmt.Errorf("timeout") + } + return nil + }) + + s.NoError(err) + s.Require().Len(timestamps, 3) + + firstGap := timestamps[1].Sub(timestamps[0]) + secondGap := timestamps[2].Sub(timestamps[1]) + + s.GreaterOrEqual(firstGap.Milliseconds(), int64(900)) + s.GreaterOrEqual(secondGap.Milliseconds(), int64(1800)) +} + +func (s *OCIRetryTestSuite) TestIsTransientError_Nil() { + s.False(isTransientError(nil)) +} + +func (s *OCIRetryTestSuite) TestIsTransientError_5xx() { + err := &transport.Error{StatusCode: http.StatusInternalServerError} + s.True(isTransientError(err)) +} + +func (s *OCIRetryTestSuite) TestIsTransientError_4xx() { + err := &transport.Error{StatusCode: http.StatusForbidden} + s.False(isTransientError(err)) +} + +func (s *OCIRetryTestSuite) TestIsTransientError_NetworkError() { + err := fmt.Errorf("dial tcp: connection refused") + s.True(isTransientError(err)) +} diff --git a/pkg/devcontainer/feature/sanitize.go b/pkg/devcontainer/feature/sanitize.go new file mode 100644 index 000000000..bbd8c6d90 --- /dev/null +++ b/pkg/devcontainer/feature/sanitize.go @@ -0,0 +1,18 @@ +package feature + +import "net/url" + +// sanitizeURL returns only the hostname from a URL string. +// For malformed or empty inputs, returns the input unchanged. +func sanitizeURL(rawURL string) string { + if rawURL == "" { + return "" + } + + parsed, err := url.Parse(rawURL) + if err != nil || parsed.Host == "" { + return rawURL + } + + return parsed.Hostname() +} diff --git a/pkg/devcontainer/feature/sanitize_test.go b/pkg/devcontainer/feature/sanitize_test.go new file mode 100644 index 000000000..d34a2197e --- /dev/null +++ b/pkg/devcontainer/feature/sanitize_test.go @@ -0,0 +1,50 @@ +package feature + +import ( + "testing" + + "github.com/stretchr/testify/suite" +) + +type SanitizeTestSuite struct { + suite.Suite +} + +func TestSanitizeTestSuite(t *testing.T) { + suite.Run(t, new(SanitizeTestSuite)) +} + +func (s *SanitizeTestSuite) TestStripsPath() { + result := sanitizeURL("https://ghcr.io/devcontainers/features/go:1.2.3") + s.Equal("ghcr.io", result) +} + +func (s *SanitizeTestSuite) TestStripsPathWithPort() { + result := sanitizeURL("https://registry.example.com:5000/org/repo/image:latest") + s.Equal("registry.example.com", result) +} + +func (s *SanitizeTestSuite) TestHostnameOnly() { + result := sanitizeURL("https://ghcr.io") + s.Equal("ghcr.io", result) +} + +func (s *SanitizeTestSuite) TestEmptyString() { + result := sanitizeURL("") + s.Equal("", result) +} + +func (s *SanitizeTestSuite) TestNoScheme() { + result := sanitizeURL("ghcr.io/devcontainers/features/go") + s.Equal("ghcr.io/devcontainers/features/go", result) +} + +func (s *SanitizeTestSuite) TestHTTP() { + result := sanitizeURL("http://my-registry.local/v2/feature") + s.Equal("my-registry.local", result) +} + +func (s *SanitizeTestSuite) TestMalformedURL() { + result := sanitizeURL("://broken") + s.Equal("://broken", result) +} From 6ae81316c55423b317299e7897b7d0447d382923 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 3 May 2026 07:34:33 -0500 Subject: [PATCH 2/2] fix(feature): resolve lint violations and reduce complexity Move exported Run method before unexported methods in cmd/up.go to fix funcorder violations. Extract pullAndExtractOCIFeature helper to reduce processOCIFeature cyclomatic complexity. Add testOtherCacheImage constant to fix goconst violation in options_test.go. --- cmd/up.go | 56 +++++++++++++------------- pkg/devcontainer/build/options_test.go | 11 ++--- pkg/devcontainer/feature/features.go | 40 +++++++++--------- 3 files changed, 56 insertions(+), 51 deletions(-) diff --git a/cmd/up.go b/cmd/up.go index 9fc1d8cd8..6d48fb88d 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -66,6 +66,34 @@ func NewUpCmd(f *flags.GlobalFlags) *cobra.Command { return upCmd } +// Run runs the command logic. +func (cmd *UpCmd) Run( + ctx context.Context, + devsyConfig *config.Config, + client client2.BaseWorkspaceClient, + args []string, +) error { + cmd.prepareWorkspace(client) + + wctx, err := cmd.executeDevsyUp(ctx, devsyConfig, client) + if err != nil { + return err + } + if wctx == nil { + return nil // Platform mode + } + + if cmd.Prebuild { + return nil + } + + if err := cmd.configureWorkspace(devsyConfig, client, wctx); err != nil { + return err + } + + return cmd.openIDE(ctx, devsyConfig, client, wctx) +} + func (cmd *UpCmd) execute(cobraCmd *cobra.Command, args []string) error { if err := cmd.validate(); err != nil { return err @@ -255,34 +283,6 @@ func (cmd *UpCmd) registerTestingFlags(upCmd *cobra.Command) { _ = upCmd.Flags().MarkHidden("force-dockerless") } -// Run runs the command logic. -func (cmd *UpCmd) Run( - ctx context.Context, - devsyConfig *config.Config, - client client2.BaseWorkspaceClient, - args []string, -) error { - cmd.prepareWorkspace(client) - - wctx, err := cmd.executeDevsyUp(ctx, devsyConfig, client) - if err != nil { - return err - } - if wctx == nil { - return nil // Platform mode - } - - if cmd.Prebuild { - return nil - } - - if err := cmd.configureWorkspace(devsyConfig, client, wctx); err != nil { - return err - } - - return cmd.openIDE(ctx, devsyConfig, client, wctx) -} - // workspaceContext holds the result of workspace preparation. type workspaceContext struct { result *config2.Result diff --git a/pkg/devcontainer/build/options_test.go b/pkg/devcontainer/build/options_test.go index 69db75619..715358e89 100644 --- a/pkg/devcontainer/build/options_test.go +++ b/pkg/devcontainer/build/options_test.go @@ -9,8 +9,9 @@ import ( ) const ( - testCacheImage = "myregistry.io/cache:latest" - testCLICacheImage = "cli-override:v1" + testCacheImage = "myregistry.io/cache:latest" + testCLICacheImage = "cli-override:v1" + testOtherCacheImage = "other:tag" ) func substitutedConfig(cfg *config.DevContainerConfig) *config.SubstitutedConfig { @@ -22,7 +23,7 @@ func TestNewOptions_CacheFrom_ConfigOnly(t *testing.T) { ParsedConfig: substitutedConfig(&config.DevContainerConfig{ DockerfileContainer: config.DockerfileContainer{ Build: &config.ConfigBuildOptions{ - CacheFrom: types.StrArray{testCacheImage, "other:tag"}, + CacheFrom: types.StrArray{testCacheImage, testOtherCacheImage}, }, }, }), @@ -35,7 +36,7 @@ func TestNewOptions_CacheFrom_ConfigOnly(t *testing.T) { if len(opts.CacheFrom) != 2 { t.Fatalf("expected 2 CacheFrom entries, got %d: %v", len(opts.CacheFrom), opts.CacheFrom) } - if opts.CacheFrom[0] != testCacheImage || opts.CacheFrom[1] != "other:tag" { + if opts.CacheFrom[0] != testCacheImage || opts.CacheFrom[1] != testOtherCacheImage { t.Fatalf("unexpected CacheFrom: %v", opts.CacheFrom) } if _, ok := opts.BuildArgs["BUILDKIT_INLINE_CACHE"]; ok { @@ -100,7 +101,7 @@ func TestNewOptions_CacheFrom_CLIOverridesConfig(t *testing.T) { ParsedConfig: substitutedConfig(&config.DevContainerConfig{ DockerfileContainer: config.DockerfileContainer{ Build: &config.ConfigBuildOptions{ - CacheFrom: types.StrArray{testCacheImage, "other:tag"}, + CacheFrom: types.StrArray{testCacheImage, testOtherCacheImage}, }, }, }), diff --git a/pkg/devcontainer/feature/features.go b/pkg/devcontainer/feature/features.go index cdceabc84..25877c0e4 100644 --- a/pkg/devcontainer/feature/features.go +++ b/pkg/devcontainer/feature/features.go @@ -184,9 +184,22 @@ func processOCIFeature(id string) (string, error) { return "", err } + if err := pullAndExtractOCIFeature(ref, id, featureFolder, featureExtractedFolder); err != nil { + return "", err + } + + log.Infof( + "OCI feature processed successfully: featureId=%s, path=%s", + id, + featureExtractedFolder, + ) + return featureExtractedFolder, nil +} + +func pullAndExtractOCIFeature(ref name.Reference, id, featureFolder, destDir string) error { img, err := pullOCIImage(ref) if err != nil { - return "", err + return err } destFile := filepath.Join(featureFolder, "feature.tgz") @@ -194,33 +207,24 @@ func processOCIFeature(id string) (string, error) { err = downloadLayer(img, id, destFile) if err != nil { log.Debugf("failed to download feature layer: error=%v, featureId=%s", err, id) - return "", fmt.Errorf("download layer from %s: %w", registry, err) + return fmt.Errorf("download layer from %s: %w", registry, err) } file, err := os.Open(destFile) if err != nil { - return "", err + return err } defer func() { _ = file.Close() }() - log.Debugf("extract feature: destination=%s", featureExtractedFolder) - err = extract.Extract(file, featureExtractedFolder) + log.Debugf("extract feature: destination=%s", destDir) + err = extract.Extract(file, destDir) if err != nil { - log.Debugf( - "failed to extract feature: error=%v, destination=%s", - err, - featureExtractedFolder, - ) - _ = os.RemoveAll(featureExtractedFolder) - return "", err + log.Debugf("failed to extract feature: error=%v, destination=%s", err, destDir) + _ = os.RemoveAll(destDir) + return err } - log.Infof( - "OCI feature processed successfully: featureId=%s, path=%s", - id, - featureExtractedFolder, - ) - return featureExtractedFolder, nil + return nil } func validateImageManifest(img v1.Image) (*v1.Manifest, error) {