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 9ff3dc42e..25877c0e4 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,45 +184,47 @@ 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)) - if err != nil { - err = image.SanitizeRegistryError(err) - log.Debugf("failed to fetch OCI image: error=%v, reference=%s", err, ref.String()) + 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 + } + 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) 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) { 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) +}