Skip to content

feat: add image_pinned to skip registry rewrite for custom images#425

Open
zeroasterisk wants to merge 2 commits into
GoogleCloudPlatform:mainfrom
zeroasterisk:feat/image-pinned
Open

feat: add image_pinned to skip registry rewrite for custom images#425
zeroasterisk wants to merge 2 commits into
GoogleCloudPlatform:mainfrom
zeroasterisk:feat/image-pinned

Conversation

@zeroasterisk

Copy link
Copy Markdown
Contributor

Summary

  • Adds image_pinned field to ScionConfig (scion-agent.yaml)
  • When image_pinned: true, the image_registry rewrite is skipped, preserving the exact image reference from the template
  • No behavior change for existing templates (opt-in only)

Motivation

Templates that specify a fully-qualified custom scion-* image (e.g. ghcr.io/myorg/scion-elixir-image:latest) get their registry prefix rewritten by RewriteImageRegistry() to match the broker's image_registry setting. This makes it impossible to use custom images hosted in external registries without push access to the broker's registry.

The image_pinned flag lets template authors signal that their image reference is intentional and should not be rewritten.

Usage

In a template's scion-agent.yaml:

image: ghcr.io/myorg/scion-custom-image:latest
image_pinned: true

Changes

  • pkg/api/types.go: Add ImagePinned bool field to ScionConfig
  • pkg/agent/run.go: Check ImagePinned before applying registry rewrite

Test plan

  • go build ./... passes
  • go test ./pkg/agent/... ./pkg/api/... ./pkg/config/... — all image-related tests pass
  • Manual: create template with image_pinned: true and custom image, verify no rewrite in debug logs

Templates that specify a fully-qualified custom image (e.g.
ghcr.io/myorg/scion-myimage:latest) currently get their registry
prefix rewritten by the broker's image_registry setting. This makes
it impossible to use custom scion-* images hosted in external
registries without push access to the broker's registry.

Add an `image_pinned` field to ScionConfig. When set to true in a
template's scion-agent.yaml, the image is used as-is without
registry rewriting.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces an ImagePinned configuration option to ScionConfig to allow skipping the image registry rewrite process when resolving agent images. The review feedback suggests formatting the struct fields in pkg/api/types.go to align with standard gofmt style, and simplifying the conditional logic in pkg/agent/run.go to eliminate a single-use variable and prevent misleading debug logs when no image is resolved.

Comment thread pkg/api/types.go Outdated
Comment on lines +450 to +452
Image string `json:"image,omitempty" yaml:"image,omitempty"`
ImagePinned bool `json:"image_pinned,omitempty" yaml:"image_pinned,omitempty"`
Services []ServiceSpec `json:"services,omitempty" yaml:"services,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The newly added and modified struct fields are not aligned with the existing fields in ScionConfig. In Go, struct fields (types and tags) should be aligned consistently using tabs and spaces as per standard gofmt formatting.

Suggested change
Image string `json:"image,omitempty" yaml:"image,omitempty"`
ImagePinned bool `json:"image_pinned,omitempty" yaml:"image_pinned,omitempty"`
Services []ServiceSpec `json:"services,omitempty" yaml:"services,omitempty"`
Image string `json:"image,omitempty" yaml:"image,omitempty"`
ImagePinned bool `json:"image_pinned,omitempty" yaml:"image_pinned,omitempty"`
Services []ServiceSpec `json:"services,omitempty" yaml:"services,omitempty"`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed — ran gofmt -w pkg/api/types.go to align the struct fields consistently.

Comment thread pkg/agent/run.go Outdated
Comment on lines +314 to +317
imagePinned := finalScionCfg != nil && finalScionCfg.ImagePinned
if imagePinned {
util.Debugf("image resolution: image_pinned=true, skipping registry rewrite")
} else if settings != nil && resolvedImage != "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

We can make this more idiomatic Go by eliminating the single-use local variable imagePinned and combining the check directly into the if statement. Additionally, adding a guard for resolvedImage != "" prevents printing a misleading debug log about skipping registry rewrite when no image was actually resolved.

Suggested change
imagePinned := finalScionCfg != nil && finalScionCfg.ImagePinned
if imagePinned {
util.Debugf("image resolution: image_pinned=true, skipping registry rewrite")
} else if settings != nil && resolvedImage != "" {
if finalScionCfg != nil && finalScionCfg.ImagePinned && resolvedImage != "" {
util.Debugf("image resolution: image_pinned=true, skipping registry rewrite")
} else if settings != nil && resolvedImage != "" {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed — inlined the imagePinned variable directly into the if condition.

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.

1 participant