fix: ensure image oci config is included when building actor oci spec (#301)

**Summary**
- Fixes a bug where image oci config is not included when building the
actor oci spec by ensuring that we extract the image supplied env from
the config manifest and propagate it to the `buildActorOCISpec`
function.

**Testing**
- Followed the steps listed in issue
#300 and confirmed
that the image env is now captured correctly in the logs and that they
are correctly overridden by the actor template env for duplicate keys.

Fixes #300

- [ ] Tests pass
- [x] Appropriate changes to documentation are included in the PR

---------

Signed-off-by: JM Huibonhoa <jm.huibonhoa@solo.io>

Author: jmhbh

cmd/atelet/internal/memorypullcache/memorypullcache.go

@@ -37,11 +37,15 @@ type MemoryPullCache struct {
 
 	localhostRegistryReplacement string
 
-	// Map from hexadecimal sha256 hash of image to byte contents of composed
-	// tarball
+	// Map from hexadecimal sha256 hash of image to the cached image (tarball + config env).
 	cache *lru.Cache
 }
 
+type cachedImage struct {
+	tar []byte
+	cfg v1.Config
+}
+
 func NewMemoryPullCache(ctx context.Context, gcpAuthenticator authn.Authenticator, localhostRegistryReplacement string) (*MemoryPullCache, error) {
 	c := &MemoryPullCache{
 		// TODO: Need a smarter cache with bounds on total consumed size, not
@@ -65,7 +69,8 @@ func NewMemoryPullCache(ctx context.Context, gcpAuthenticator authn.Authenticato
 	return c, nil
 }
 
-func (c *MemoryPullCache) Fetch(ctx context.Context, ref string) (io.ReadCloser, error) {
+// Fetch returns the image's extracted filesystem tarball and its OCI image config.
+func (c *MemoryPullCache) Fetch(ctx context.Context, ref string) (io.ReadCloser, *v1.Config, error) {
 	// when running in kind we need to rewrite the registry endpoint similar to the
 	// containerd mirror config used in https://kind.sigs.k8s.io/docs/user/local-registry/
 	// for now we have simple opt-in support to rewrite local registries
@@ -86,7 +91,7 @@ func (c *MemoryPullCache) Fetch(ctx context.Context, ref string) (io.ReadCloser,
 
 	parsedRef, err := name.ParseReference(ref, nameOpts...)
 	if err != nil {
-		return nil, fmt.Errorf("while parsing reference: %w", err)
+		return nil, nil, fmt.Errorf("while parsing reference: %w", err)
 	}
 
 	// If the image ref included a digest, check for a hit in the pull cache.
@@ -106,7 +111,8 @@ func (c *MemoryPullCache) Fetch(ctx context.Context, ref string) (io.ReadCloser,
 				slog.String("ref", ref),
 				slog.String("digest", requestedDigest.DigestStr()),
 			)
-			return io.NopCloser(bytes.NewReader(vAny.([]byte))), nil
+			img := vAny.(*cachedImage)
+			return io.NopCloser(bytes.NewReader(img.tar)), &img.cfg, nil
 		}
 	}
 
@@ -142,36 +148,44 @@ func (c *MemoryPullCache) Fetch(ctx context.Context, ref string) (io.ReadCloser,
 
 	img, err := remote.Image(parsedRef, remoteOptions...)
 	if err != nil {
-		return nil, fmt.Errorf("in remote.Image: %w", err)
+		return nil, nil, fmt.Errorf("in remote.Image: %w", err)
 	}
 
+	var imageCfg v1.Config
+	cfg, cfgErr := img.ConfigFile()
+	if cfgErr != nil {
+		return nil, nil, fmt.Errorf("while reading image config: %w", cfgErr)
+	}
+	imageCfg = cfg.Config
+
 	size, err := img.Size()
 	if err != nil {
-		return nil, fmt.Errorf("in img.Size(): %w", err)
+		return nil, nil, fmt.Errorf("in img.Size(): %w", err)
 	}
+
 	if size > 100*1024*1024 {
 		slog.InfoContext(ctx,
 			"Image is too large to cache",
 			slog.String("ref", ref),
 			slog.Int64("size", size),
 		)
-		return mutate.Extract(img), err
+		return mutate.Extract(img), &imageCfg, err
 	}
 
 	tarData := mutate.Extract(img)
 	defer tarData.Close()
 
 	memData, err := io.ReadAll(tarData)
 	if err != nil {
-		return nil, fmt.Errorf("while reading image: %w", err)
+		return nil, nil, fmt.Errorf("while reading image: %w", err)
 	}
 
 	if digestWasIncluded {
 		// If the user requested multi-arch image, the digest they request will
 		// not be the same as the digest of the image we actually downloaded
 		// from the registry.  We need to place the cache entry under the digest
 		// they requested.
-		c.cache.Add(requestedDigest.DigestStr(), memData)
+		c.cache.Add(requestedDigest.DigestStr(), &cachedImage{tar: memData, cfg: imageCfg})
 		slog.InfoContext(
 			ctx,
 			"Populated image cache",
@@ -180,7 +194,7 @@ func (c *MemoryPullCache) Fetch(ctx context.Context, ref string) (io.ReadCloser,
 		)
 	}
 
-	return io.NopCloser(bytes.NewReader(memData)), nil
+	return io.NopCloser(bytes.NewReader(memData)), &imageCfg, nil
 }
 
 func registryHost(ref string) string {

cmd/atelet/oci.go

@@ -29,6 +29,7 @@ import (
 
 	"github.com/agent-substrate/substrate/cmd/atelet/internal/memorypullcache"
 	"github.com/agent-substrate/substrate/internal/ateompath"
+	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/attribute"
@@ -71,7 +72,7 @@ func prepareOCIDirectory(ctx context.Context, pullCache *memorypullcache.MemoryP
 		return fmt.Errorf("in os.MkdirAll for container bundle dir: %w", err)
 	}
 
-	tarData, err := pullCache.Fetch(ctx, ref)
+	tarData, imageCfg, err := pullCache.Fetch(ctx, ref)
 	if err != nil {
 		return fmt.Errorf("in pullCache.Fetch: %w", err)
 	}
@@ -90,7 +91,7 @@ func prepareOCIDirectory(ctx context.Context, pullCache *memorypullcache.MemoryP
 		}
 	}
 
-	ociSpec := buildActorOCISpec(atespace, actorID, args, env, annotations, netns, identityDir, durableDirVolumeMounts)
+	ociSpec := buildActorOCISpec(atespace, actorID, imageCfg, args, env, annotations, netns, identityDir, durableDirVolumeMounts)
 	ociSpecBytes, err := json.MarshalIndent(ociSpec, "", "  ")
 	if err != nil {
 		return fmt.Errorf("while marshaling OCI spec: %w", err)
@@ -103,15 +104,42 @@ func prepareOCIDirectory(ctx context.Context, pullCache *memorypullcache.MemoryP
 	return nil
 }
 
+// mergeActorEnv merges the ActorTemplate env and the image's ENV, with the template taking precedence.
+// duplicated keys are removed in favor of the following precedence template env > image env.
+// default PATH stands in for an image config with no env
+func mergeActorEnv(imageEnv, templateEnv []string) []string {
+	seen := make(map[string]struct{})
+	var out []string
+	add := func(entries ...string) {
+		for _, e := range entries {
+			key, _, _ := strings.Cut(e, "=")
+			if key == "" {
+				continue
+			}
+			if _, ok := seen[key]; ok {
+				continue
+			}
+			seen[key] = struct{}{}
+			out = append(out, e)
+		}
+	}
+
+	add(templateEnv...)
+	add(imageEnv...)
+	add("PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin")
+	return out
+}
+
 // buildActorOCISpec assembles the OCI runtime spec for an actor container.
 // When identityDir is non-empty it adds a read-only bind mount of that host
 // directory at IdentityMountPath so the actor can read its own ID (see
 // IdentityMountPath for why this is a bind mount rather than env vars).
-func buildActorOCISpec(atespace string, actorID string, args []string, env []string, annotations map[string]string, netns string, identityDir string, durableDirVolumeMounts []*ateletpb.VolumeMount) *specs.Spec {
-	envVars := []string{
-		"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
+func buildActorOCISpec(atespace string, actorID string, imageCfg *v1.Config, args []string, env []string, annotations map[string]string, netns string, identityDir string, durableDirVolumeMounts []*ateletpb.VolumeMount) *specs.Spec {
+	var imageEnv []string
+	if imageCfg != nil {
+		imageEnv = imageCfg.Env
 	}
-	envVars = append(envVars, env...)
+	envVars := mergeActorEnv(imageEnv, env)
 
 	mounts := []specs.Mount{
 		{

cmd/atelet/oci_test.go

@@ -88,6 +88,7 @@ func runUntar(t *testing.T, entries []tarEntry) (string, error) {
 func TestBuildActorOCISpec_IdentityMount(t *testing.T) {
 	spec := buildActorOCISpec(
 		"atespace", "id",
+		nil,
 		[]string{"/app"},
 		[]string{"FOO=bar"},
 		map[string]string{"k": "v"},
@@ -116,9 +117,57 @@ func TestBuildActorOCISpec_IdentityMount(t *testing.T) {
 	}
 }
 
+func TestMergeActorEnv(t *testing.T) {
+	defaultPath := "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
+
+	tests := []struct {
+		name        string
+		imageEnv    []string
+		templateEnv []string
+		want        []string
+	}{
+		{
+			name:        "template overrides image by key",
+			imageEnv:    []string{"FOO=image"},
+			templateEnv: []string{"FOO=template"},
+			want:        []string{"FOO=template", defaultPath},
+		},
+		{
+			name:        "default PATH applies when neither sets it",
+			imageEnv:    []string{"FOO=image"},
+			templateEnv: []string{"BAR=template"},
+			want:        []string{"BAR=template", "FOO=image", defaultPath},
+		},
+		{
+			name:     "image PATH overrides default",
+			imageEnv: []string{"PATH=/image/bin"},
+			want:     []string{"PATH=/image/bin"},
+		},
+		{
+			name:        "template PATH overrides default",
+			templateEnv: []string{"PATH=/template/bin"},
+			want:        []string{"PATH=/template/bin"},
+		},
+		{
+			name:     "blank and keyless entries are dropped",
+			imageEnv: []string{"", "=novalue"},
+			want:     []string{defaultPath},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := mergeActorEnv(tc.imageEnv, tc.templateEnv)
+			if !slices.Equal(got, tc.want) {
+				t.Errorf("mergeActorEnv(%v, %v) =\n  %v\nwant:\n  %v", tc.imageEnv, tc.templateEnv, got, tc.want)
+			}
+		})
+	}
+}
+
 // Without an identity dir (the pause container), no identity mount appears.
 func TestBuildActorOCISpec_NoIdentityMountForPause(t *testing.T) {
-	bare := buildActorOCISpec("atespace", "id", []string{"/pause"}, nil, nil, "/run/netns/x", "", nil)
+	bare := buildActorOCISpec("atespace", "id", nil, []string{"/pause"}, nil, nil, "/run/netns/x", "", nil)
 	for _, m := range bare.Mounts {
 		if m.Destination == IdentityMountPath {
 			t.Errorf("identity mount must be absent when identityDir is empty")
@@ -136,7 +185,7 @@ func TestBuildActorOCISpec_DurableDirVolumeMounts(t *testing.T) {
 	}
 	spec := buildActorOCISpec(
 		atespace, id,
-		[]string{"/app"}, nil, nil,
+		nil, []string{"/app"}, nil, nil,
 		"/run/netns/x",
 		"",
 		durableDirs,