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.

Sign up for GitHub

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

proxy: Add optional flags to OpenImage #1844

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

proxy: Add optional flags to OpenImage #1844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 49 additions & 6 deletions cmd/skopeo/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ import (
// 0.2.3: Added GetFullConfig
// 0.2.4: Added OpenImageOptional
// 0.2.5: Added LayerInfoJSON
const protocolVersion = "0.2.5"
// 0.2.6: OpenImage now accepts a second argument with an array of options
const protocolVersion = "0.2.6"

// maxMsgSize is the current limit on a packet size.
// Note that all non-metadata (i.e. payload data) is sent over a pipe.
Expand Down Expand Up @@ -214,8 +215,18 @@ func (h *proxyHandler) Initialize(args []interface{}) (replyBuf, error) {

// OpenImage accepts a string image reference i.e. TRANSPORT:REF - like `skopeo copy`.
// The return value is an opaque integer handle.
//
// A second argument may be present; if so, it is an array of string-valued flags.
//
// - optional: Do not error if the image is not present; instead return a zero imageid. Since: 0.2.6
func (h *proxyHandler) OpenImage(args []interface{}) (replyBuf, error) {
return h.openImageImpl(args, false)
opts := openOptions{}
if len(args) > 1 {
if err := opts.parse(args[1]); err != nil {
return replyBuf{}, err
}
}
return h.openImageImpl(args, opts)
}

// isDockerManifestUnknownError is a copy of code from containers/image,
Expand All @@ -237,15 +248,45 @@ func isNotFoundImageError(err error) bool {
errors.Is(err, ocilayout.ImageNotFoundError{})
}

func (h *proxyHandler) openImageImpl(args []interface{}, allowNotFound bool) (replyBuf, error) {
type openOptions struct {
optional bool
}

func (o *openOptions) parse(argsval interface{}) error {
args, ok := argsval.([]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

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

I’d find map[string]any more natural here — it seems fairly likely we will want to pass non-boolean options in the future.

To go further, (preferably?), there probably is a way to use json.Unmarshal to deal with parsing the options into a Go struct, not writing any manual parser at all. Maybe something vaguely like

type request struct {
	Method string `json:"method"`
	Args []interface{} `json:"args"`
	Options json.RawMessage
}

type requestTableEntry struct {
	handler func(options any /* sadly type-erased */ args []any) (replyBuf, error)
	allocOptions func() any
}
var requestTable map[string]requestTableEntry = {
	…
	"Open": requestTableEntry{
		handler: h.OpenImage, /* FIXME: this can't actually refer to h. here — refactor somehow */
		allocOptions: func() any { return &openOptions{} },
	},
}

…
func (h *proxyHandler) processRequest(readBytes []byte) (rb replyBuf, terminate bool, err error) {
	var req request, options any

	if err = json.Unmarshal(readBytes, &req); err != nil {
		err = fmt.Errorf("invalid request: %v", err)
		return
	}
	handler, ok := requestTable[req.Method]; if !ok { … }
	if handler.allocOptions != nil {
		options = handler.allocOptions()
		if err = json.Unmarshal(req.Options, options); err != nil { … }
	} else if req.Options != nil { /* fail */ }
	return handler.handler(options, req.Args)
}

or some even more elaborate option where the handers have well-typed, not type-erased options

… but that screams YAGNI.

Copy link
Contributor

@mtrmac mtrmac Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

… or, why the … not, we could architecture-astronaut this, and remove the args/options distinction, and all the manual casts

type parsedRequest interface{
    argsDestinations() []any
   /* or have ^^^ even return (mandatoryArgs []any, optionalArgs []any), and then we could farm out validating `len(args)` to the shared `processRequest` entirely */
    run() (replyBuf, error)
}

type openImageRequest{
    imageName string
    opts openOptions /* a struct with named fields, automatically decoded by json.Unmarshal */
}

func (*r openImageRequest) argsDestinations() []any {
   return []any{&r.imageName, &r.opts}
}

func (*r openImageRequest) run() (replyBuf, error)
    /* use r.imageName, r.opts - no manual type checks or decoding */
}

var requestTable map[string]func ()*parsedRequest = {
   "OpenImage": func()*parsedRequest{return &openImageRequest{}
}

func (h *proxyHandler) processRequest(readBytes []byte) (rb replyBuf, terminate bool, err error) {
   /* unmarshal req.Args as []json.RawMessage */
   newRequestState, ok := requestTable[req.Method]; if !ok { … }
   rs := newRequestState()
   argsDest := rs.argsDestinations()
   if len(req.Args) > len(argsDest) { /* fail */ }
   for i := range req.Args {
      if err := json.Unmarshal(Args[i], argsDest[i]); err != nil { … } 
   }
   return rs.run()
}

Honestly I have no idea if this ends up actually shorter. There are not that many operations that avoiding manual parsing of args is clearly a win, and the parsedRequest implementations are just another kind of boilerplate.

The above still feels clumsy, like I’m missing some obvious way to have the standard library do all the work.

(OTOH If we keep going like this, I’m sure we will eventually end up using something like Swagger…)

Copy link
Contributor

Choose a reason for hiding this comment

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

(s/any/interface{}/g, I think we don’t yet require Go 1.18, but that might change soon.)

Copy link
Contributor

@mtrmac mtrmac Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The above still feels clumsy, like I’m missing some obvious way to have the standard library do all the work.

One more thought: Get rid of the untyped Args entirely, use named parameters only (doesn’t help for any existing API with the Args []any)

type sharedRequestHeader struct {
   Method string `json:"method"`
}

type parsedRequest interface{
    run() (replyBuf, error)
}

type openImageRequest{
    sharedRequestHeader   
    ImageName string `json:"imageName"`
    Opts openOptions `json:"opts"`
}

func (*r openImageRequest) run() (replyBuf, error)
    /* use r.imageName, r.opts - no manual type checks or decoding */
}

var requestTable map[string]func ()*parsedRequest = {
   "OpenImage2": func()*parsedRequest{return &openImageRequest{}}
}

func (h *proxyHandler) processRequest(readBytes []byte) (rb replyBuf, terminate bool, err error) {
   var header sharedRequeestHeader
   if err := json.Unmarshal(readBytes, &header); err != nil { … }
   newRequestState, ok := requestTable[header.Method]; if !ok { … }
   rs := newRequestState()
   if err := json.Unmarshal(readBytes, rs); err != nil { … } // re-parse now that we know the request type
   return rs.run()
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Doh, if we unmarshaled the whole request as a method-specific struct, we wouldn’t need the openOptions sub-struct at all (though it wouldn’t hurt).

if !ok {
return fmt.Errorf("expecting array for options, not %T", argsval)
}
for _, argv := range args {
arg, ok := argv.(string)
if !ok {
return fmt.Errorf("expecting string option, not %T", arg)
}
switch arg {
case "optional":
o.optional = true
default:
return fmt.Errorf("unknown option %s", arg)
}
}

return nil
}

func (h *proxyHandler) openImageImpl(args []interface{}, opts openOptions) (replyBuf, error) {
h.lock.Lock()
defer h.lock.Unlock()
var ret replyBuf

if h.sysctx == nil {
return ret, fmt.Errorf("client error: must invoke Initialize")
}
if len(args) != 1 {
switch len(args) {
case 1:
// This is is the default
case 2:
// The second argument, if present should have been parsed by the caller
Copy link
Contributor

Choose a reason for hiding this comment

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

… but OpenImageOptional doesn’t do that…

I think enforcing len(args), and turning args into better-typed data, should be collocated close to each other.

Probably in OpenImage (allowing len(args) in {1,2}) and OpenImageOptional (allowing len(args) == 1), calling already well-typed openImageImpl(imageRef string, opts openOptions); or maybe have openImageImpl fully responsible for args, with the effect of OpenImageOptional also allowing the new option argument. I’d prefer the former one but I don’t feel nearly as strongly about this choice as I do about having the the len check and the code that parses argv close.

default:
return ret, fmt.Errorf("invalid request, expecting one argument")
}
imageref, ok := args[0].(string)
Expand All @@ -259,7 +300,7 @@ func (h *proxyHandler) openImageImpl(args []interface{}, allowNotFound bool) (re
}
imgsrc, err := imgRef.NewImageSource(context.Background(), h.sysctx)
if err != nil {
if allowNotFound && isNotFoundImageError(err) {
if opts.optional && isNotFoundImageError(err) {
ret.value = sentinelImageID
return ret, nil
}
Expand All @@ -283,7 +324,9 @@ func (h *proxyHandler) openImageImpl(args []interface{}, allowNotFound bool) (re
// The return value is an opaque integer handle. If the image does not exist, zero
// is returned.
func (h *proxyHandler) OpenImageOptional(args []interface{}) (replyBuf, error) {
return h.openImageImpl(args, true)
return h.openImageImpl(args, openOptions{
optional: true,
})
}

func (h *proxyHandler) CloseImage(args []interface{}) (replyBuf, error) {
Expand Down
17 changes: 17 additions & 0 deletions integration/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,23 @@ func runTestOpenImageOptionalNotFound(p *proxy, img string) error {
if imgid != 0 {
return fmt.Errorf("Unexpected optional image id %v", imgid)
}

// Also verify the optional options
opts := []string{"optional"}
v, err = p.callNoFd("OpenImage", []interface{}{img, opts})
if err != nil {
return fmt.Errorf("calling OpenImage with flag optional: %w ", err)
}

imgidv, ok = v.(float64)
if !ok {
return fmt.Errorf("OpenImage return value is %T", v)
}
imgid = uint32(imgidv)
if imgid != 0 {
return fmt.Errorf("Unexpected optional image id %v", imgid)
}

return nil
}

Expand Down