From 11339761ca37d89c3eda0815b583b3bc8a18931c Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Fri, 22 May 2020 11:16:01 +0200 Subject: [PATCH] Change the way a context is stored Initially we stored the context data in the `Metadata` of the context but in hindsight this data would be better of in the `Endpoints` because that's what it is used for. Before: ```json { "Name": "aci", "Metadata": { "Type": "aci", "Data": { "key": "value" } }, "Endpoints": { "docker": {} } } ``` After: ```json { "Name": "aci", "Type": "aci", "Metadata": {}, "Endpoints": { "aci": { "key": "value" }, "docker": {} } } ``` With this change the contexts that we create are more in line with the contexts the docker cli creates. It also makes the code less complicated since we don't need to marsal twice any more. The API is nicer too: ```go // Get a context: c, err := store.Get(contextName) // Get the stored endpoint: var aciContext store.AciContext if err := contextStore.GetEndpoint(currentContext, &aciContext); err != nil { return nil, err } ``` --- .github/PULL_REQUEST_TEMPLATE.md | 4 +- azure/backend.go | 12 +- cli/cmd/context/create.go | 6 +- cli/cmd/context/createaci.go | 18 ++- cli/cmd/context/ls.go | 16 ++- cli/cmd/context/show.go | 2 +- cli/cmd/context/use.go | 2 +- cli/cmd/serve.go | 2 +- cli/main.go | 2 +- client/client.go | 7 +- context/store/store.go | 229 +++++++++++++++++-------------- context/store/store_test.go | 38 +++-- context/store/storedefault.go | 13 +- errdefs/errors.go | 3 + tests/framework/clisuite.go | 4 +- 15 files changed, 199 insertions(+), 159 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index ec10ce4d..655d1b93 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,8 +1,6 @@ **What I did** **Related issue** -<-- If this is a bug fix, make sure your description includes "fixes #xxxx", or -"closes #xxxx" ---> +<-- If this is a bug fix, make sure your description includes "fixes #xxxx", or "closes #xxxx" --> **(not mandatory) A picture of a cute animal, if possible in relation with what you did** diff --git a/azure/backend.go b/azure/backend.go index a0b20743..5f678d52 100644 --- a/azure/backend.go +++ b/azure/backend.go @@ -36,19 +36,15 @@ func init() { }) } -func getter() interface{} { - return &store.AciContext{} -} - // New creates a backend that can manage containers func New(ctx context.Context) (backend.Service, error) { currentContext := apicontext.CurrentContext(ctx) contextStore := store.ContextStore(ctx) - metadata, err := contextStore.Get(currentContext, getter) - if err != nil { - return nil, errors.Wrap(err, "wrong context type") + + var aciContext store.AciContext + if err := contextStore.GetEndpoint(currentContext, &aciContext); err != nil { + return nil, err } - aciContext, _ := metadata.Metadata.Data.(store.AciContext) auth, _ := login.NewAuthorizerFromLogin() containerGroupsClient := containerinstance.NewContainerGroupsClient(aciContext.SubscriptionID) diff --git a/cli/cmd/context/create.go b/cli/cmd/context/create.go index 18ab1051..25ffd56c 100644 --- a/cli/cmd/context/create.go +++ b/cli/cmd/context/create.go @@ -67,9 +67,7 @@ func runCreate(ctx context.Context, opts createOpts, name string, contextType st return createACIContext(ctx, name, opts) default: s := store.ContextStore(ctx) - return s.Create(name, store.TypedContext{ - Type: contextType, - Description: opts.description, - }) + // TODO: we need to implement different contexts for known backends + return s.Create(name, contextType, opts.description, store.ExampleContext{}) } } diff --git a/cli/cmd/context/createaci.go b/cli/cmd/context/createaci.go index 02771ea6..ceb23f3c 100644 --- a/cli/cmd/context/createaci.go +++ b/cli/cmd/context/createaci.go @@ -29,19 +29,27 @@ package context import ( "context" + "fmt" "github.com/docker/api/context/store" ) func createACIContext(ctx context.Context, name string, opts createOpts) error { s := store.ContextStore(ctx) - return s.Create(name, store.TypedContext{ - Type: "aci", - Description: opts.description, - Data: store.AciContext{ + + description := fmt.Sprintf("%s@%s", opts.aciResourceGroup, opts.aciLocation) + if opts.description != "" { + description = fmt.Sprintf("%s (%s)", opts.description, description) + } + + return s.Create( + name, + store.AciContextType, + description, + store.AciContext{ SubscriptionID: opts.aciSubscriptionID, Location: opts.aciLocation, ResourceGroup: opts.aciResourceGroup, }, - }) + ) } diff --git a/cli/cmd/context/ls.go b/cli/cmd/context/ls.go index 5c7e7ffb..d72b5a0b 100644 --- a/cli/cmd/context/ls.go +++ b/cli/cmd/context/ls.go @@ -79,7 +79,7 @@ func runList(ctx context.Context) error { fmt.Fprintf(w, format, contextName, - c.Metadata.Type, + c.Type, c.Metadata.Description, getEndpoint("docker", c.Endpoints), getEndpoint("kubernetes", c.Endpoints), @@ -89,15 +89,19 @@ func runList(ctx context.Context) error { return w.Flush() } -func getEndpoint(name string, meta map[string]store.Endpoint) string { - d, ok := meta[name] +func getEndpoint(name string, meta map[string]interface{}) string { + endpoints, ok := meta[name] + if !ok { + return "" + } + data, ok := endpoints.(store.Endpoint) if !ok { return "" } - result := d.Host - if d.DefaultNamespace != "" { - result += fmt.Sprintf(" (%s)", d.DefaultNamespace) + result := data.Host + if data.DefaultNamespace != "" { + result += fmt.Sprintf(" (%s)", data.DefaultNamespace) } return result diff --git a/cli/cmd/context/show.go b/cli/cmd/context/show.go index 20e4aec9..36a8a612 100644 --- a/cli/cmd/context/show.go +++ b/cli/cmd/context/show.go @@ -53,7 +53,7 @@ func runShow(ctx context.Context) error { // Match behavior of existing CLI if name != store.DefaultContextName { s := store.ContextStore(ctx) - if _, err := s.Get(name, nil); err != nil { + if _, err := s.Get(name); err != nil { return err } } diff --git a/cli/cmd/context/use.go b/cli/cmd/context/use.go index 0d1ce2b1..09126685 100644 --- a/cli/cmd/context/use.go +++ b/cli/cmd/context/use.go @@ -52,7 +52,7 @@ func runUse(ctx context.Context, name string) error { s := store.ContextStore(ctx) // Match behavior of existing CLI if name != store.DefaultContextName { - if _, err := s.Get(name, nil); err != nil { + if _, err := s.Get(name); err != nil { return err } } diff --git a/cli/cmd/serve.go b/cli/cmd/serve.go index 119c797e..c4149a04 100644 --- a/cli/cmd/serve.go +++ b/cli/cmd/serve.go @@ -77,7 +77,7 @@ func (cs *cliServer) Contexts(ctx context.Context, request *cliv1.ContextsReques for _, c := range contexts { result.Contexts = append(result.Contexts, &cliv1.Context{ Name: c.Name, - ContextType: c.Metadata.Type, + ContextType: c.Type, }) } return result, nil diff --git a/cli/main.go b/cli/main.go index 328a3a5c..64a60ce2 100644 --- a/cli/main.go +++ b/cli/main.go @@ -182,7 +182,7 @@ func execMoby(ctx context.Context) { currentContext := apicontext.CurrentContext(ctx) s := store.ContextStore(ctx) - _, err := s.Get(currentContext, nil) + _, err := s.Get(currentContext) // Only run original docker command if the current context is not // ours. if err != nil { diff --git a/client/client.go b/client/client.go index 64d68503..44c05a77 100644 --- a/client/client.go +++ b/client/client.go @@ -44,19 +44,18 @@ func New(ctx context.Context) (*Client, error) { currentContext := apicontext.CurrentContext(ctx) s := store.ContextStore(ctx) - cc, err := s.Get(currentContext, nil) + cc, err := s.Get(currentContext) if err != nil { return nil, err } - contextType := s.GetType(cc) - service, err := backend.Get(ctx, contextType) + service, err := backend.Get(ctx, cc.Type) if err != nil { return nil, err } return &Client{ - backendType: contextType, + backendType: cc.Type, bs: service, }, nil diff --git a/context/store/store.go b/context/store/store.go index 9583d6c3..3adb8e47 100644 --- a/context/store/store.go +++ b/context/store/store.go @@ -72,18 +72,66 @@ func ContextStore(ctx context.Context) Store { type Store interface { // Get returns the context with name, it returns an error if the context // doesn't exist - Get(name string, getter func() interface{}) (*Metadata, error) - // GetType returns the type of the context (docker, aci etc) - GetType(meta *Metadata) string + Get(name string) (*Metadata, error) + // GetEndpoint sets the `v` parameter to the value of the endpoint for a + // particular context type + GetEndpoint(name string, v interface{}) error // Create creates a new context, it returns an error if a context with the // same name exists already. - Create(name string, data TypedContext) error + Create(name string, contextType string, description string, data interface{}) error // List returns the list of created contexts List() ([]*Metadata, error) // Remove removes a context by name from the context store Remove(name string) error } +// Endpoint holds the Docker or the Kubernetes endpoint, they both have the +// `Host` property, only kubernetes will have the `DefaultNamespace` +type Endpoint struct { + Host string `json:",omitempty"` + DefaultNamespace string `json:",omitempty"` +} + +const ( + // AciContextType is the endpoint key in the context endpoints for an ACI + // backend + AciContextType = "aci" + // MobyContextType is the endpoint key in the context endpoints for a moby + // backend + MobyContextType = "moby" + // ExampleContextType is the endpoint key in the context endpoints for an + // example backend + ExampleContextType = "example" +) + +// Metadata represents the docker context metadata +type Metadata struct { + Name string `json:",omitempty"` + Type string `json:",omitempty"` + Metadata ContextMetadata `json:",omitempty"` + Endpoints map[string]interface{} `json:",omitempty"` +} + +// ContextMetadata is represtentation of the data we put in a context +// metadata +type ContextMetadata struct { + Description string `json:",omitempty"` + StackOrchestrator string `json:",omitempty"` +} + +// AciContext is the context for the ACI backend +type AciContext struct { + SubscriptionID string `json:",omitempty"` + Location string `json:",omitempty"` + ResourceGroup string `json:",omitempty"` +} + +// MobyContext is the context for the moby backend +type MobyContext struct{} + +// ExampleContext is the context for the example backend +type ExampleContext struct{} + type store struct { root string } @@ -127,9 +175,9 @@ func New(opts ...Opt) (Store, error) { } // Get returns the context with the given name -func (s *store) Get(name string, getter func() interface{}) (*Metadata, error) { +func (s *store) Get(name string) (*Metadata, error) { meta := filepath.Join(s.root, contextsDir, metadataDir, contextDirOf(name), metaFile) - m, err := read(meta, getter) + m, err := read(meta) if os.IsNotExist(err) { return nil, errors.Wrap(errdefs.ErrNotFound, objectName(name)) } else if err != nil { @@ -139,73 +187,75 @@ func (s *store) Get(name string, getter func() interface{}) (*Metadata, error) { return m, nil } -func read(meta string, getter func() interface{}) (*Metadata, error) { +func (s *store) GetEndpoint(name string, data interface{}) error { + meta, err := s.Get(name) + if err != nil { + return err + } + if _, ok := meta.Endpoints[meta.Type]; !ok { + return errors.Wrapf(errdefs.ErrNotFound, "endpoint of type %q", meta.Type) + } + + dstPtrValue := reflect.ValueOf(data) + dstValue := reflect.Indirect(dstPtrValue) + + val := reflect.ValueOf(meta.Endpoints[meta.Type]) + valIndirect := reflect.Indirect(val) + + if dstValue.Type() != valIndirect.Type() { + return errdefs.ErrWrongContextType + } + + dstValue.Set(valIndirect) + + return nil +} + +func read(meta string) (*Metadata, error) { bytes, err := ioutil.ReadFile(meta) if err != nil { return nil, err } - var um untypedMetadata - if err := json.Unmarshal(bytes, &um); err != nil { + var metadata Metadata + if err := json.Unmarshal(bytes, &metadata); err != nil { return nil, err } - var uc untypedContext - if err := json.Unmarshal(um.Metadata, &uc); err != nil { + metadata.Endpoints, err = toTypedEndpoints(metadata.Endpoints) + if err != nil { return nil, err } - if uc.Type == "" { - uc.Type = "docker" - } - var data interface{} - if uc.Data != nil { - data, err = parse(uc.Data, getter) + return &metadata, nil +} + +func toTypedEndpoints(endpoints map[string]interface{}) (map[string]interface{}, error) { + result := map[string]interface{}{} + for k, v := range endpoints { + bytes, err := json.Marshal(v) if err != nil { return nil, err } - } + typeGetters := getters() + if _, ok := typeGetters[k]; !ok { + result[k] = v + continue + } - return &Metadata{ - Name: um.Name, - Endpoints: um.Endpoints, - Metadata: TypedContext{ - StackOrchestrator: uc.StackOrchestrator, - Description: uc.Description, - Type: uc.Type, - Data: data, - }, - }, nil -} - -func parse(payload []byte, getter func() interface{}) (interface{}, error) { - if getter == nil { - var res map[string]interface{} - if err := json.Unmarshal(payload, &res); err != nil { + val := typeGetters[k]() + err = json.Unmarshal(bytes, &val) + if err != nil { return nil, err } - return res, nil + + result[k] = val } - typed := getter() - if err := json.Unmarshal(payload, &typed); err != nil { - return nil, err - } - - return reflect.ValueOf(typed).Elem().Interface(), nil + return result, nil } -func (s *store) GetType(meta *Metadata) string { - for k := range meta.Endpoints { - if k != dockerEndpointKey { - return k - } - } - - return dockerEndpointKey -} - -func (s *store) Create(name string, data TypedContext) error { +func (s *store) Create(name string, contextType string, description string, data interface{}) error { if name == DefaultContextName { return errors.Wrap(errdefs.ErrAlreadyExists, objectName(name)) } @@ -220,16 +270,15 @@ func (s *store) Create(name string, data TypedContext) error { return err } - if data.Data == nil { - data.Data = dummyContext{} - } - meta := Metadata{ - Name: name, - Metadata: data, - Endpoints: map[string]Endpoint{ - (dockerEndpointKey): {}, - (data.Type): {}, + Name: name, + Type: contextType, + Metadata: ContextMetadata{ + Description: description, + }, + Endpoints: map[string]interface{}{ + (dockerEndpointKey): data, + (contextType): data, }, } @@ -252,7 +301,7 @@ func (s *store) List() ([]*Metadata, error) { for _, fi := range c { if fi.IsDir() { meta := filepath.Join(root, fi.Name(), metaFile) - r, err := read(meta, nil) + r, err := read(meta) if err != nil { return nil, err } @@ -303,45 +352,19 @@ func createDirIfNotExist(dir string) error { return nil } -type dummyContext struct{} - -// Endpoint holds the Docker or the Kubernetes endpoint -type Endpoint struct { - Host string `json:",omitempty"` - DefaultNamespace string `json:",omitempty"` -} - -// Metadata represents the docker context metadata -type Metadata struct { - Name string `json:",omitempty"` - Metadata TypedContext `json:",omitempty"` - Endpoints map[string]Endpoint `json:",omitempty"` -} - -type untypedMetadata struct { - Name string `json:",omitempty"` - Metadata json.RawMessage `json:",omitempty"` - Endpoints map[string]Endpoint `json:",omitempty"` -} - -type untypedContext struct { - StackOrchestrator string `json:",omitempty"` - Type string `json:",omitempty"` - Description string `json:",omitempty"` - Data json.RawMessage `json:",omitempty"` -} - -// TypedContext is a context with a type (moby, aci, etc...) -type TypedContext struct { - StackOrchestrator string `json:",omitempty"` - Type string `json:",omitempty"` - Description string `json:",omitempty"` - Data interface{} `json:",omitempty"` -} - -// AciContext is the context for ACI -type AciContext struct { - SubscriptionID string `json:",omitempty"` - Location string `json:",omitempty"` - ResourceGroup string `json:",omitempty"` +// Different context types managed by the store. +// TODO(rumpl): we should make this extensible in the future if we want to +// be able to manage other contexts. +func getters() map[string]func() interface{} { + return map[string]func() interface{}{ + "aci": func() interface{} { + return &AciContext{} + }, + "moby": func() interface{} { + return &MobyContext{} + }, + "example": func() interface{} { + return &ExampleContext{} + }, + } } diff --git a/context/store/store_test.go b/context/store/store_test.go index c901afaa..90b6d806 100644 --- a/context/store/store_test.go +++ b/context/store/store_test.go @@ -33,6 +33,7 @@ import ( "os" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -62,42 +63,55 @@ func (suite *StoreTestSuite) AfterTest(suiteName, testName string) { } func (suite *StoreTestSuite) TestCreate() { - err := suite.store.Create("test", TypedContext{}) + err := suite.store.Create("test", "test", "description", ContextMetadata{}) require.Nil(suite.T(), err) - err = suite.store.Create("test", TypedContext{}) + err = suite.store.Create("test", "test", "descrsiption", ContextMetadata{}) require.EqualError(suite.T(), err, `context "test": already exists`) require.True(suite.T(), errdefs.IsAlreadyExistsError(err)) } +func (suite *StoreTestSuite) TestGetEndpoint() { + err := suite.store.Create("aci", "aci", "description", AciContext{ + Location: "eu", + }) + require.Nil(suite.T(), err) + + var ctx AciContext + err = suite.store.GetEndpoint("aci", &ctx) + assert.Equal(suite.T(), nil, err) + assert.Equal(suite.T(), "eu", ctx.Location) + + var exampleCtx ExampleContext + err = suite.store.GetEndpoint("aci", &exampleCtx) + assert.EqualError(suite.T(), err, "wrong context type") +} + func (suite *StoreTestSuite) TestGetUnknown() { - meta, err := suite.store.Get("unknown", nil) + meta, err := suite.store.Get("unknown") require.Nil(suite.T(), meta) require.EqualError(suite.T(), err, `context "unknown": not found`) require.True(suite.T(), errdefs.IsNotFoundError(err)) } func (suite *StoreTestSuite) TestGet() { - err := suite.store.Create("test", TypedContext{ - Type: "type", - Description: "description", - }) + err := suite.store.Create("test", "type", "description", ContextMetadata{}) require.Nil(suite.T(), err) - meta, err := suite.store.Get("test", nil) + meta, err := suite.store.Get("test") require.Nil(suite.T(), err) require.NotNil(suite.T(), meta) require.Equal(suite.T(), "test", meta.Name) require.Equal(suite.T(), "description", meta.Metadata.Description) - require.Equal(suite.T(), "type", meta.Metadata.Type) + require.Equal(suite.T(), "type", meta.Type) } func (suite *StoreTestSuite) TestList() { - err := suite.store.Create("test1", TypedContext{}) + err := suite.store.Create("test1", "type", "description", ContextMetadata{}) require.Nil(suite.T(), err) - err = suite.store.Create("test2", TypedContext{}) + err = suite.store.Create("test2", "type", "description", ContextMetadata{}) require.Nil(suite.T(), err) contexts, err := suite.store.List() @@ -116,7 +130,7 @@ func (suite *StoreTestSuite) TestRemoveNotFound() { } func (suite *StoreTestSuite) TestRemove() { - err := suite.store.Create("testremove", TypedContext{}) + err := suite.store.Create("testremove", "type", "description", ContextMetadata{}) require.Nil(suite.T(), err) contexts, err := suite.store.List() require.Nil(suite.T(), err) diff --git a/context/store/storedefault.go b/context/store/storedefault.go index 61283d3f..5555fdb0 100644 --- a/context/store/storedefault.go +++ b/context/store/storedefault.go @@ -10,7 +10,7 @@ import ( // Represents a context as created by the docker cli type defaultContext struct { - Metadata TypedContext + Metadata ContextMetadata Endpoints endpoints } @@ -54,20 +54,19 @@ func dockerDefaultContext() (*Metadata, error) { meta := Metadata{ Name: "default", - Endpoints: map[string]Endpoint{ - "docker": { + Type: "docker", + Endpoints: map[string]interface{}{ + "docker": Endpoint{ Host: defaultCtx.Endpoints.Docker.Host, }, - "kubernetes": { + "kubernetes": Endpoint{ Host: defaultCtx.Endpoints.Kubernetes.Host, DefaultNamespace: defaultCtx.Endpoints.Kubernetes.DefaultNamespace, }, }, - Metadata: TypedContext{ + Metadata: ContextMetadata{ Description: "Current DOCKER_HOST based configuration", - Type: "docker", StackOrchestrator: defaultCtx.Metadata.StackOrchestrator, - Data: defaultCtx.Metadata, }, } diff --git a/errdefs/errors.go b/errdefs/errors.go index 3a4e2984..f1933f35 100644 --- a/errdefs/errors.go +++ b/errdefs/errors.go @@ -47,6 +47,9 @@ var ( ErrNotImplemented = errors.New("not implemented") // ErrParsingFailed is returned when a string cannot be parsed ErrParsingFailed = errors.New("parsing failed") + // ErrWrongContextType is returned when the caller tries to get a context + // with the wrong type + ErrWrongContextType = errors.New("wrong context type") ) // IsNotFoundError returns true if the unwrapped error is ErrNotFound diff --git a/tests/framework/clisuite.go b/tests/framework/clisuite.go index fafae504..00e87c7c 100644 --- a/tests/framework/clisuite.go +++ b/tests/framework/clisuite.go @@ -35,9 +35,7 @@ func (sut *CliSuite) BeforeTest(suiteName, testName string) { ) require.Nil(sut.T(), err) - err = s.Create("example", store.TypedContext{ - Type: "example", - }) + err = s.Create("example", "example", "", store.ContextMetadata{}) require.Nil(sut.T(), err) sut.storeRoot = dir