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.

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

Conversation

cgwalters
Copy link
Contributor

See #1829 (comment)

This is prep for adding another option like require-signatures.

Signed-off-by: Colin Walters [email protected]

See containers#1829 (comment)

This is prep for adding another option like `require-signatures`.

Signed-off-by: Colin Walters <[email protected]>
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.

}

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

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

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).

@cgwalters
Copy link
Contributor Author

Thanks for the review! I mentioned this in passing before but I have been vaguely mulling a switch to e.g. Cap'n Proto, which would obviate all of this. Any opinions on that?

If we did it in the obvious way it would involve sending the blob data over the primary channel, but maybe that's OK. (As of lately cpnp does support fd passing, but support is not yet widespread in the bindings; at least not Rust which is mainly what I care about)

@mtrmac
Copy link
Contributor

mtrmac commented Jan 11, 2023

You did mention that — it the website looks nice. I didn’t take a deep look and I don’t really have a strong opinion on RPC mechanisms, I know far too little about the various trade-offs.

https://capnproto.org/otherlang.html seems sufficient for interoperability.

I guess the main concern would be total binary size — that should not be an issue in principle, but I’m just dealing with a Swagger generator that somehow adds a BSON encoder and a telemetry framework as non-avoidable dependencies.


I guess to be clearer: from the stream-of-consciousness thread #1844 (comment) , the only thing I’m really concerned is that the options argument should be a JSON object, not an array. Parsing that JSON object with a hand-crafted parse is, I think, good enough that it doesn’t warrant a complete refactoring.

The other things about that possible refactoring are not blocking this work. It’s just me slowly realizing that the Args []any approach is hard to automate…

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Jan 11, 2023
@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

A friendly reminder that this PR had no activity for 30 days.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental-image-proxy Issues related to experimental-image-proxy kind/feature A request for, or a PR adding, new functionality stale-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants