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

Jump to bottom

api/server: drop unused Config struct #45083

Merged
merged 1 commit into from
Mar 1, 2023
Merged

api/server: drop unused Config struct #45083

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 0 additions & 20 deletions api/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package server // import "github.com/docker/docker/api/server"

import (
"context"
"crypto/tls"
"net"
"net/http"
"strings"
Expand All @@ -22,32 +21,13 @@ import (
// when a request is about to be served.
const versionMatcher = "/v{version:[0-9.]+}"

// Config provides the configuration for the API server
type Config struct {
CorsHeaders string
Version string
SocketGroup string
TLSConfig *tls.Config
// Hosts is a list of addresses for the API to listen on.
Hosts []string
}

// Server contains instance details for the server
type Server struct {
cfg *Config
servers []*HTTPServer
routers []router.Router
middlewares []middleware.Middleware
}

// New returns a new instance of the server based on the specified configuration.
// It allocates resources which will be needed for ServeAPI(ports, unix-sockets).
func New(cfg *Config) *Server {
return &Server{
cfg: cfg,
}
}

// UseMiddleware appends a new middleware to the request chain.
// This needs to be called before the API routes are configured.
func (s *Server) UseMiddleware(m middleware.Middleware) {
Expand Down
7 changes: 1 addition & 6 deletions api/server/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,7 @@ import (
)

func TestMiddlewares(t *testing.T) {
cfg := &Config{
Version: "0.1omega2",
}
srv := &Server{
cfg: cfg,
}
srv := &Server{}

srv.UseMiddleware(middleware.NewVersionMiddleware("0.1omega2", api.DefaultVersion, api.MinVersion))

Expand Down
42 changes: 17 additions & 25 deletions cmd/dockerd/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ type DaemonCli struct {
configFile *string
flags *pflag.FlagSet

api *apiserver.Server
api apiserver.Server
d *daemon.Daemon
authzMiddleware *authorization.Middleware // authzMiddleware enables to dynamically reload the authorization plugins
}
Expand All @@ -80,7 +80,7 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
return err
}

serverConfig, err := newAPIServerConfig(cli.Config)
tlsConfig, err := newAPIServerTLSConfig(cli.Config)
if err != nil {
return err
}
Expand Down Expand Up @@ -161,9 +161,7 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
}
}

cli.api = apiserver.New(serverConfig)
Copy link
Member

Choose a reason for hiding this comment

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

Where does the actual initialisation of the Server struct happen now? (i should probably check out the code locally 😂)

Also was scratching my head a bit why the "constructor" didn't actually construct everything, other than setting the "config" (but none of the other fields). Is that something we would "want" to be more of an actual constructor instead? Or not worth bothering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where does the actual initialisation of the Server struct happen now?

Initialization of the apiserver.Server struct happens in the cli.initMiddlewares and initRouter calls. That is true on master and it is true in this PR branch. The only meaningful change is that cli.api is no longer a pointer.

Also was scratching my head a bit why the "constructor" didn't actually construct everything, other than setting the "config" (but none of the other fields). Is that something we would "want" to be more of an actual constructor instead? Or not worth bothering?

I'm not convinced it's worth bothering. apiserver.Server implements the builder pattern, which seems to work well enough for this use case. I wish that its build method was named something more intuitive than Wait(), but that's out of scope for this PR.


hosts, err := loadListeners(cli, serverConfig)
hosts, err := loadListeners(cli, tlsConfig)
if err != nil {
return errors.Wrap(err, "failed to load listeners")
}
Expand Down Expand Up @@ -192,7 +190,7 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {

pluginStore := plugin.NewStore()

if err := cli.initMiddlewares(cli.api, serverConfig, pluginStore); err != nil {
if err := cli.initMiddlewares(&cli.api, pluginStore); err != nil {
logrus.Fatalf("Error creating middlewares: %v", err)
}

Expand Down Expand Up @@ -230,7 +228,7 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
if err != nil {
return err
}
routerOptions.api = cli.api
routerOptions.api = &cli.api
routerOptions.cluster = c

initRouter(routerOptions)
Expand Down Expand Up @@ -546,17 +544,17 @@ func initRouter(opts routerOptions) {
}

// TODO: remove this from cli and return the authzMiddleware
func (cli *DaemonCli) initMiddlewares(s *apiserver.Server, cfg *apiserver.Config, pluginStore plugingetter.PluginGetter) error {
v := cfg.Version
func (cli *DaemonCli) initMiddlewares(s *apiserver.Server, pluginStore plugingetter.PluginGetter) error {
v := dockerversion.Version

exp := middleware.NewExperimentalMiddleware(cli.Config.Experimental)
s.UseMiddleware(exp)

vm := middleware.NewVersionMiddleware(v, api.DefaultVersion, api.MinVersion)
s.UseMiddleware(vm)

if cfg.CorsHeaders != "" {
c := middleware.NewCORSMiddleware(cfg.CorsHeaders)
if cli.Config.CorsHeaders != "" {
c := middleware.NewCORSMiddleware(cli.Config.CorsHeaders)
s.UseMiddleware(c)
}

Expand Down Expand Up @@ -597,7 +595,7 @@ func (cli *DaemonCli) getContainerdDaemonOpts() ([]supervisor.DaemonOpt, error)
return opts, nil
}

func newAPIServerConfig(config *config.Config) (*apiserver.Config, error) {
func newAPIServerTLSConfig(config *config.Config) (*tls.Config, error) {
var tlsConfig *tls.Config
if config.TLS != nil && *config.TLS {
var (
Expand All @@ -620,13 +618,7 @@ func newAPIServerConfig(config *config.Config) (*apiserver.Config, error) {
}
}

return &apiserver.Config{
SocketGroup: config.SocketGroup,
Version: dockerversion.Version,
CorsHeaders: config.CorsHeaders,
TLSConfig: tlsConfig,
Hosts: config.Hosts,
}, nil
return tlsConfig, nil
}

// checkTLSAuthOK checks basically for an explicitly disabled TLS/TLSVerify
Expand Down Expand Up @@ -654,21 +646,21 @@ func checkTLSAuthOK(c *config.Config) bool {
return true
}

func loadListeners(cli *DaemonCli, serverConfig *apiserver.Config) ([]string, error) {
if len(serverConfig.Hosts) == 0 {
func loadListeners(cli *DaemonCli, tlsConfig *tls.Config) ([]string, error) {
if len(cli.Config.Hosts) == 0 {
return nil, errors.New("no hosts configured")
}
var hosts []string

for i := 0; i < len(serverConfig.Hosts); i++ {
protoAddr := serverConfig.Hosts[i]
for i := 0; i < len(cli.Config.Hosts); i++ {
protoAddr := cli.Config.Hosts[i]
proto, addr, ok := strings.Cut(protoAddr, "://")
if !ok || addr == "" {
return nil, fmt.Errorf("bad format %s, expected PROTO://ADDR", protoAddr)
}

// It's a bad idea to bind to TCP without tlsverify.
authEnabled := serverConfig.TLSConfig != nil && serverConfig.TLSConfig.ClientAuth == tls.RequireAndVerifyClientCert
authEnabled := tlsConfig != nil && tlsConfig.ClientAuth == tls.RequireAndVerifyClientCert
if proto == "tcp" && !authEnabled {
logrus.WithField("host", protoAddr).Warn("Binding to IP address without --tlsverify is insecure and gives root access on this machine to everyone who has access to your network.")
logrus.WithField("host", protoAddr).Warn("Binding to an IP address, even on localhost, can also give access to scripts run in a browser. Be safe out there!")
Expand Down Expand Up @@ -712,7 +704,7 @@ func loadListeners(cli *DaemonCli, serverConfig *apiserver.Config) ([]string, er
return nil, err
}
}
ls, err := listeners.Init(proto, addr, serverConfig.SocketGroup, serverConfig.TLSConfig)
ls, err := listeners.Init(proto, addr, cli.Config.SocketGroup, tlsConfig)
if err != nil {
return nil, err
}
Expand Down