dperny/NEW_api_server_router_container_container_routes.go

## api_problems.md

      
    Raw
  

              api_problems.md
            
          
    Related PR is: moby/moby#31952
Relevant Snippets below have been heavily elided to include only the important parts and ignore irrelevant complexity.
So, this is the problem: We're writing onto a writer we passed in all of these log lines in this basically-undocumented log line byte string format. This happens in daemon/logs.go, see the for loop. There is no reason for this to occur on the daemon, the daemon shouldn't be responsible for reconstituting a response to be sent over the HTTP response. That should be the job of container_routes.go, just like how it's every other Handler's responsibility to take structs and turn them into json. Ideally, we would bubble up the channel from daemon/logger/logger.go and access the raw messages from that channel, and then mogrify them into a byte string in the routes (by removing all of that formatting and writing from ContainerLogs
My major hangup is that right now the api server is isolated from the daemon through the backend. I understand why this is the case and I straight up totally agree that that's an awesome design. The PROBLEM is that we can't access the raw log Message structs, which are defined in the daemon/logger/ package without importing that and totally breaking the isolation through backend.
My proposed solution is the 4th through final files, which refactors Messages out of logger, moves the bytestring construction to container_routes.go, and changes ContainerLogs() to write to a channel.
My major hangup, if this design looks good, is how do I handle the LogAttributes field, which has useful methods defined on it.

  
## api_server_router_container_container_routes.go
// ORIGINAL container routes

package container

import (
	// none of this is important, it literally ONLY imports things like
	"github.com/docker/docker/api/types/whatever"
	// there are 0 dependencies on the daemon. GOOD DESIGN.
)

func (s *containerRouter) getContainersLogs(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
	// some validation happens here, you can ignore it

	containerName := vars["name"]
	logsConfig := &backend.ContainerLogsConfig{
		ContainerLogsOptions: types.ContainerLogsOptions{
			// bunch of options are set here from the Form, don't worry about it
		},
		// only this bit is important to show how the writer gets into ContainerLogs
		OutStream: w,
	}

	chStarted := make(chan struct{})
	if err := s.backend.ContainerLogs(ctx, containerName, logsConfig, chStarted); err != nil {
		select {
		// started channel gets closed when ContainerLogs starts writing to the response
		case <-chStarted:
			// The client may be expecting all of the data we're sending to
			// be multiplexed, so mux it through the Systemerr stream, which
			// will cause the client to throw an error when demuxing

			// THIS IS BROKEN. I commited this a couple of months ago and it doesn't properly handle errors for
			// TTY logs, which is part of the reasoning for this refactoring
			stdwriter := stdcopy.NewStdWriter(logsConfig.OutStream, stdcopy.Systemerr)
			fmt.Fprintf(stdwriter, "Error running logs job: %v\n", err)
		default:
			return err
		}
	}

	return nil
}

## daemon_logger_logger.go
// ORIGINAL logger.go, modified to fit your screen

package logger

// daemon/logger/logger.go line 54

// Message is datastructure that represents piece of output produced by some
// container.  The Line member is a slice of an array whose contents can be
// changed after a log driver's Log() method returns.
// Any changes made to this struct must also be updated in the `reset` function
type Message struct {
	Line      []byte
	Source    string
	Timestamp time.Time
	Attrs     LogAttributes
	Partial   bool
}

func (*Message) reset() {
	// sets all the fields in a message to thier zero value
}

type LogAttributes map[string]string
// skip some internal types


func(a LogAttributes) String() string {
	// Does some conversions to internal types
	// prints out LogAttributes in the form "key=value,key=value,key=value" sorted by key
}

// Eliding some more details, there is a thing that eventually returns
func NewLogWatcher() *LogWatcher { // LogWatcher type is never explicitly used in the caller, just implicitly
	return &LogWatcher{
		// This Chan here, this is the important part
		Msg: make(chan *Message, logWatcherBufferSize),
		// There are more fields here but they're not important
	}
}

## daemon_logs.go
// ORIGINAL logs.go

package daemon

// lots of imports ignored, only the important ones included
import (
	"github.com/docker/docker/api/types/backend"
	"github.com/docker/docker/daemon/logger"
)

// This implements a method defined on the backend interface
func (d *Daemon) ContainerLogs(ctx context.Context, containerName string, config *backend.ContainerLogsConfig, started chan struct{}) error {
  // Basically, the end result is it gets the chan from the logwatcher and uses it to construct the binary format
	// eliding a LOT of the function, the important part is this:
	// daemon/logs.go line 93
	for {
		select {
		// not actually in the code, included so you can see where the exits were
		case <-someExitConditions:
			return nil
		case msg, ok := <-logs.Msg:
			// This part is the problem part where we build the bytestring, see above
			if !ok { // indicates logs are done
				return nil
			}
			logLine := msg.Line
			if config.Details {
				logLine = append([]byte(msg.Attrs.String()+" "), logLine...)
				//                                ^^^ this call is that LogAttributes.String() included above
			}
			// here it would add the timestamps, skipped
			if msg.Source == "stdout" && config.ShowStdout {
				outStream.Write(logLine)
			}
			if msg.Source == "stderr" && config.ShowStderr {
				errStream.Write(logLine)
			}
		}
	}
}

## NEW_api_server_router_container_container_routes.go
// ORIGINAL container routes

package container

import (
	// none of this is important, it literally ONLY imports things like
	// "github.com/docker/docker/api/types/whatever"
	// there are 0 dependencies on the daemon. GOOD DESIGN.

  "github.com/docker/docker/api/types/backend"
)

func (s *containerRouter) getContainersLogs(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
	// some validation happens here, you can ignore it

	containerName := vars["name"]
	logsConfig := &backend.ContainerLogsConfig{
		ContainerLogsOptions: types.ContainerLogsOptions{
			// bunch of options are set here from the Form, don't worry about it
		},
		// LOOK. NO MORE WRITER HERE.
		Out: make(chan *backend.LogMessage, 1)
	}

	// skip a bunch of junk, ripped from daemon/logs.go, that initializes the writers. it's mostly copied verbatim.

	// do a bit of goroutine wrangling to correctly handle errors
	chErr := make(chan error, 1)
	go func() {
		chErr <- s.backend.ContainerLogs(ctx, containerName, logsConfig, chStarted)
	}

	// now WE do the writing, here
	for {
		select {
		// there may be some other cases, but they're not important to illustrate the point
		case err := <-chErr:
			// if we've not started writing, we can just return here
			if weHaveStartedWritingUseYourImaginationForHowThisLooksItsNotComplicated() {
				ewriter := w
				if !container.HasTTY() { // or whatever that function is called, this isn't real code, remember?
					// THIS is where we would mux the error in, fixing the bug i mentioned in the old file
					ewriter := stdcopy.NewStdWriter(w, stdcopy.Systemerr)
				}
				fmt.Fprintf(ewriter, "Error running logs job: %v\n", err)
				return nil
			}
			// before we've started writing if we just return the error then the HTTP server takes care of it
			return err
		case msg, ok := <-config.Out:
			// THIS is where we make the byte stream
			if !ok { // indicates logs are done
				return nil
			}
			logLine := msg.Line
			if config.Details {
				logLine = append([]byte(msg.Attrs.String()+" "), logLine...)
				//                                ^^^ this call is that LogAttributes.String() that i need to fix
			}
			// here it would add the timestamps, skipped
			if msg.Source == "stdout" && config.ShowStdout {
				outStream.Write(logLine)
			}
			if msg.Source == "stderr" && config.ShowStderr {
				errStream.Write(logLine)
			}
		}
	}
}

## NEW_api_types_backend_backend.go
// NEW backend.go, with proposed changes.

// MOVE this struct def from logger to here.
// It goes in backend.go because it's not needed on the client side

type ContainerLogsConfig struct {
  // keep all of those bits the same
  // LEAVE containerlogoptions alone, we don't need to change it
  ContainerLogOptions
  // BUT, don't pass around a writer anymore. Pass a channel instead
  Out chan *LogMessage
}

// Message is datastructure that represents piece of output produced by some
// container.  The Line member is a slice of an array whose contents can be
// changed after a log driver's Log() method returns.
type LogMessage struct {
	Line      []byte
	Source    string
	Timestamp time.Time
	Attrs     LogAttributes
	Partial   bool
}

type LogAttributes map[string]string
// !!! PROBLEM !!! what do i do with the methods defined on LogAttributes? Specifically, strings?
// It doesn't feel like they belong here!

## NEW_daemon_logger_logger.go
// NEW logger.go
package logger

import (
	// all of the imports is the same EXCEPT we add:
	"github.com/docker/docker/api/types/backend"
)

// Redefine Message. `Message` has dependencies all through the logger package and
// I think that changing all those definitions isn't just a pain, it's wrong. We
// don't need the implementation details of a log message on the logger side outside
// of places that explicitly import logger. When we return messages to places that
// are outside of the daemon, we should cast them back to backend.LogMessage
//
// There is some weird stuff with a logging ring buffer or something and these
// messages. IDK how that factors in
//
// Any changes made to this struct must also be updated in the `reset` function
type Message backend.LogMessage

func (*Message) reset() {
	// sets all the fields in a message to their zero value
}

// HERE is another part I'm not sure of. We need the String() method when we're
// mogrifying the logmessages into bytestrings. the methods defined on LogAttributes
// are totally unimportant otherwise inside of the logger package. if we just nuke
// the whole `logger` package definition of LogAttributes, it has minimal impact
type LogAttributes backend.LogAttributes
// skip some internal types

// This is... like... idk where to put this method, so that it can be accessed without importing logger?
func(a LogAttributes) String() string {
	// Does some conversions to internal types
	// prints out LogAttributes in the form "key=value,key=value,key=value" sorted by key
}

## NEW_daemon_logs.go
// NEW daemon/logs.go

package daemon

// lots of imports ignored, only the important ones included
import (
  // SAME IMPORTS, no changes here
	"github.com/docker/docker/api/types/backend"
	"github.com/docker/docker/daemon/logger"
)

// The signature of this method changes! We drop the started channel, because we don't care if the writer
// has been writen to yet. we don't have a writer. not our problem. also, we can return errors
func (d *Daemon) ContainerLogs(ctx context.Context, containerName string, config *backend.ContainerLogsConfig) error {
  // Basically, the end result is it gets the chan from the logwatcher and uses it to construct the binary format
	// eliding a LOT of the function, the important part is this:
	// daemon/logs.go line 93
	for {
		select {
		// not actually in the code, included so you can see where the exits were
		case <-someExitConditions:
			return nil
		case msg, ok := <-logs.Msg:
			// This part is the problem part where we build the bytestring, see above
			if !ok { // indicates logs are done
        // so close the out channel
        close(config.Out)
			}
      // drop ALL that bytewrangling, just typecast and channel write
      // this isn't the right way to typecast because pointers. just pretend i did it right
      config.Out <- msg.(backend.LogMessage)
    }
	}
}
	// ORIGINAL container routes

	package container

	import (
	// none of this is important, it literally ONLY imports things like
	"github.com/docker/docker/api/types/whatever"
	// there are 0 dependencies on the daemon. GOOD DESIGN.
	)

	func (s containerRouter) getContainersLogs(ctx context.Context, w http.ResponseWriter, r http.Request, vars map[string]string) error {
	// some validation happens here, you can ignore it

	containerName := vars["name"]
	logsConfig := &backend.ContainerLogsConfig{
	ContainerLogsOptions: types.ContainerLogsOptions{
	// bunch of options are set here from the Form, don't worry about it
	},
	// only this bit is important to show how the writer gets into ContainerLogs
	OutStream: w,
	}

	chStarted := make(chan struct{})
	if err := s.backend.ContainerLogs(ctx, containerName, logsConfig, chStarted); err != nil {
	select {
	// started channel gets closed when ContainerLogs starts writing to the response
	case <-chStarted:
	// The client may be expecting all of the data we're sending to
	// be multiplexed, so mux it through the Systemerr stream, which
	// will cause the client to throw an error when demuxing

	// THIS IS BROKEN. I commited this a couple of months ago and it doesn't properly handle errors for
	// TTY logs, which is part of the reasoning for this refactoring
	stdwriter := stdcopy.NewStdWriter(logsConfig.OutStream, stdcopy.Systemerr)
	fmt.Fprintf(stdwriter, "Error running logs job: %v\n", err)
	default:
	return err
	}
	}

	return nil
	}
	// ORIGINAL logger.go, modified to fit your screen

	package logger

	// daemon/logger/logger.go line 54

	// Message is datastructure that represents piece of output produced by some
	// container. The Line member is a slice of an array whose contents can be
	// changed after a log driver's Log() method returns.
	// Any changes made to this struct must also be updated in the `reset` function
	type Message struct {
	Line []byte
	Source string
	Timestamp time.Time
	Attrs LogAttributes
	Partial bool
	}

	func (*Message) reset() {
	// sets all the fields in a message to thier zero value
	}

	type LogAttributes map[string]string
	// skip some internal types


	func(a LogAttributes) String() string {
	// Does some conversions to internal types
	// prints out LogAttributes in the form "key=value,key=value,key=value" sorted by key
	}

	// Eliding some more details, there is a thing that eventually returns
	func NewLogWatcher() *LogWatcher { // LogWatcher type is never explicitly used in the caller, just implicitly
	return &LogWatcher{
	// This Chan here, this is the important part
	Msg: make(chan *Message, logWatcherBufferSize),
	// There are more fields here but they're not important
	}
	}
	// ORIGINAL logs.go

	package daemon

	// lots of imports ignored, only the important ones included
	import (
	"github.com/docker/docker/api/types/backend"
	"github.com/docker/docker/daemon/logger"
	)

	// This implements a method defined on the backend interface
	func (d Daemon) ContainerLogs(ctx context.Context, containerName string, config backend.ContainerLogsConfig, started chan struct{}) error {
	// Basically, the end result is it gets the chan from the logwatcher and uses it to construct the binary format
	// eliding a LOT of the function, the important part is this:
	// daemon/logs.go line 93
	for {
	select {
	// not actually in the code, included so you can see where the exits were
	case <-someExitConditions:
	return nil
	case msg, ok := <-logs.Msg:
	// This part is the problem part where we build the bytestring, see above
	if !ok { // indicates logs are done
	return nil
	}
	logLine := msg.Line
	if config.Details {
	logLine = append([]byte(msg.Attrs.String()+" "), logLine...)
	// ^^^ this call is that LogAttributes.String() included above
	}
	// here it would add the timestamps, skipped
	if msg.Source == "stdout" && config.ShowStdout {
	outStream.Write(logLine)
	}
	if msg.Source == "stderr" && config.ShowStderr {
	errStream.Write(logLine)
	}
	}
	}
	}
	// NEW backend.go, with proposed changes.

	// MOVE this struct def from logger to here.
	// It goes in backend.go because it's not needed on the client side

	type ContainerLogsConfig struct {
	// keep all of those bits the same
	// LEAVE containerlogoptions alone, we don't need to change it
	ContainerLogOptions
	// BUT, don't pass around a writer anymore. Pass a channel instead
	Out chan *LogMessage
	}

	// Message is datastructure that represents piece of output produced by some
	// container. The Line member is a slice of an array whose contents can be
	// changed after a log driver's Log() method returns.
	type LogMessage struct {
	Line []byte
	Source string
	Timestamp time.Time
	Attrs LogAttributes
	Partial bool
	}

	type LogAttributes map[string]string
	// !!! PROBLEM !!! what do i do with the methods defined on LogAttributes? Specifically, strings?
	// It doesn't feel like they belong here!
	// NEW logger.go
	package logger

	import (
	// all of the imports is the same EXCEPT we add:
	"github.com/docker/docker/api/types/backend"
	)

	// Redefine Message. `Message` has dependencies all through the logger package and
	// I think that changing all those definitions isn't just a pain, it's wrong. We
	// don't need the implementation details of a log message on the logger side outside
	// of places that explicitly import logger. When we return messages to places that
	// are outside of the daemon, we should cast them back to backend.LogMessage
	//
	// There is some weird stuff with a logging ring buffer or something and these
	// messages. IDK how that factors in
	//
	// Any changes made to this struct must also be updated in the `reset` function
	type Message backend.LogMessage

	func (*Message) reset() {
	// sets all the fields in a message to their zero value
	}

	// HERE is another part I'm not sure of. We need the String() method when we're
	// mogrifying the logmessages into bytestrings. the methods defined on LogAttributes
	// are totally unimportant otherwise inside of the logger package. if we just nuke
	// the whole `logger` package definition of LogAttributes, it has minimal impact
	type LogAttributes backend.LogAttributes
	// skip some internal types

	// This is... like... idk where to put this method, so that it can be accessed without importing logger?
	func(a LogAttributes) String() string {
	// Does some conversions to internal types
	// prints out LogAttributes in the form "key=value,key=value,key=value" sorted by key
	}
	// NEW daemon/logs.go

	package daemon

	// lots of imports ignored, only the important ones included
	import (
	// SAME IMPORTS, no changes here
	"github.com/docker/docker/api/types/backend"
	"github.com/docker/docker/daemon/logger"
	)

	// The signature of this method changes! We drop the started channel, because we don't care if the writer
	// has been writen to yet. we don't have a writer. not our problem. also, we can return errors
	func (d Daemon) ContainerLogs(ctx context.Context, containerName string, config backend.ContainerLogsConfig) error {
	// Basically, the end result is it gets the chan from the logwatcher and uses it to construct the binary format
	// eliding a LOT of the function, the important part is this:
	// daemon/logs.go line 93
	for {
	select {
	// not actually in the code, included so you can see where the exits were
	case <-someExitConditions:
	return nil
	case msg, ok := <-logs.Msg:
	// This part is the problem part where we build the bytestring, see above
	if !ok { // indicates logs are done
	// so close the out channel
	close(config.Out)
	}
	// drop ALL that bytewrangling, just typecast and channel write
	// this isn't the right way to typecast because pointers. just pretend i did it right
	config.Out <- msg.(backend.LogMessage)
	}
	}
	}