From 8476fb899f44e28658f4f3510379b434e6bc1956 Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 11 May 2022 10:17:17 +0200 Subject: [PATCH 1/9] chore(server): remove rendundant if --- server/sources.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/server/sources.go b/server/sources.go index ed3422c51e..6908e37d28 100644 --- a/server/sources.go +++ b/server/sources.go @@ -544,11 +544,6 @@ func (s *Service) NewSourceUser(w http.ResponseWriter, r *http.Request) { return } - if err != nil { - Error(w, http.StatusBadRequest, err.Error(), s.Logger) - return - } - su := newSourceUserResponse(srcID, res.Name).WithPermissions(res.Permissions) if _, hasRoles := s.hasRoles(ctx, ts); hasRoles { su.WithRoles(srcID, res.Roles) From 36090224fbc5c75abb3337867eb51544fc5fa73a Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 11 May 2022 10:36:07 +0200 Subject: [PATCH 2/9] chore(enterprise): add AddUserPerms to meta --- enterprise/meta.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/enterprise/meta.go b/enterprise/meta.go index 2eb4d51aa0..8d927be394 100644 --- a/enterprise/meta.go +++ b/enterprise/meta.go @@ -206,6 +206,18 @@ func (m *MetaClient) RemoveUserPerms(ctx context.Context, name string, perms Per return m.Post(ctx, "/user", a, nil) } +// RemoveUserPerms revokes permissions for a user in Influx Enterprise +func (m *MetaClient) AddUserPerms(ctx context.Context, name string, perms Permissions) error { + a := &UserAction{ + Action: "add-permissions", + User: &User{ + Name: name, + Permissions: perms, + }, + } + return m.Post(ctx, "/user", a, nil) +} + // SetUserPerms removes permissions not in set and then adds the requested perms func (m *MetaClient) SetUserPerms(ctx context.Context, name string, perms Permissions) error { user, err := m.User(ctx, name) @@ -226,14 +238,7 @@ func (m *MetaClient) SetUserPerms(ctx context.Context, name string, perms Permis // ... next, add any permissions the user should have if len(add) > 0 { - a := &UserAction{ - Action: "add-permissions", - User: &User{ - Name: name, - Permissions: add, - }, - } - return m.Post(ctx, "/user", a, nil) + return m.AddUserPerms(ctx, name, add) } return nil } From 32c7984f5482b70365352d1b1774e34a7a9f535a Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 11 May 2022 10:37:14 +0200 Subject: [PATCH 3/9] fix(enterprise): optimize create user without permissions --- enterprise/users.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/enterprise/users.go b/enterprise/users.go index d16f160841..9f8b2ab03d 100644 --- a/enterprise/users.go +++ b/enterprise/users.go @@ -20,8 +20,10 @@ func (c *UserStore) Add(ctx context.Context, u *chronograf.User) (*chronograf.Us } perms := ToEnterprise(u.Permissions) - if err := c.Ctrl.SetUserPerms(ctx, u.Name, perms); err != nil { - return nil, err + if len(perms) > 0 { + if err := c.Ctrl.SetUserPerms(ctx, u.Name, perms); err != nil { + return nil, err + } } for _, role := range u.Roles { if err := c.Ctrl.AddRoleUsers(ctx, role.Name, []string{u.Name}); err != nil { From c35603ac9f06cd84048c38bff088819392097bd1 Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 11 May 2022 12:19:23 +0200 Subject: [PATCH 4/9] fix(enterprise): wait for the user creation --- enterprise/users.go | 25 ++++++++++++++++++++++- enterprise/users_test.go | 43 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/enterprise/users.go b/enterprise/users.go index 9f8b2ab03d..50cbff9305 100644 --- a/enterprise/users.go +++ b/enterprise/users.go @@ -3,6 +3,7 @@ package enterprise import ( "context" "fmt" + "time" "github.com/influxdata/chronograf" ) @@ -18,13 +19,35 @@ func (c *UserStore) Add(ctx context.Context, u *chronograf.User) (*chronograf.Us if err := c.Ctrl.CreateUser(ctx, u.Name, u.Passwd); err != nil { return nil, err } - perms := ToEnterprise(u.Permissions) + // fix #5840: eventual consistency can cause delays in user creation + // wait for the user to become available + _, err := c.Ctrl.User(ctx, u.Name) + timer := time.NewTimer(2_000_000_000) // retry at most 2 seconds + defer timer.Stop() + for err != nil { + if err.Error() != "user not found" { + return nil, err + } + // wait before the next attempt + select { + case <-ctx.Done(): + return nil, err + case <-timer.C: + return nil, err + case <-time.After(50_000_000): + break + } + _, err = c.Ctrl.User(ctx, u.Name) + } + // add permissions + perms := ToEnterprise(u.Permissions) if len(perms) > 0 { if err := c.Ctrl.SetUserPerms(ctx, u.Name, perms); err != nil { return nil, err } } + // add roles for _, role := range u.Roles { if err := c.Ctrl.AddRoleUsers(ctx, role.Name, []string{u.Name}); err != nil { return nil, err diff --git a/enterprise/users_test.go b/enterprise/users_test.go index c715287ef0..0229351ce1 100644 --- a/enterprise/users_test.go +++ b/enterprise/users_test.go @@ -2,6 +2,7 @@ package enterprise_test import ( "context" + "errors" "fmt" "reflect" "testing" @@ -175,6 +176,48 @@ func TestClient_Add(t *testing.T) { } } +// TestClient_Add_UserNotFound check fix for #5840, the API waits +// for the creation of the user (up to 1 second) OOTB +func TestClient_Add_UserNotFound(t *testing.T) { + notFoundAttempts := 1 + c := &enterprise.UserStore{ + Ctrl: &mockCtrl{ + createUser: func(ctx context.Context, name, passwd string) error { + return nil + }, + user: func(ctx context.Context, name string) (*enterprise.User, error) { + if notFoundAttempts > 0 { + notFoundAttempts-- + return nil, errors.New("user not found") + } + return &enterprise.User{ + Name: "pavel", + Permissions: map[string][]string{}, + }, nil + }, + userRoles: func(ctx context.Context) (map[string]enterprise.Roles, error) { + return map[string]enterprise.Roles{}, nil + }, + }, + } + got, err := c.Add(context.Background(), &chronograf.User{ + Name: "pavel", + Passwd: "levap", + }) + if err != nil { + t.Errorf("Client.Add() error = %v", err) + return + } + want := &chronograf.User{ + Name: "pavel", + Permissions: chronograf.Permissions{}, + Roles: []chronograf.Role{}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("Client.Add() = \n%#v\n, want \n%#v\n", got, want) + } +} + func TestClient_Delete(t *testing.T) { type fields struct { Ctrl *mockCtrl From 1f2a8e61b91c414ee069e01c4ae0de70d213c790 Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 11 May 2022 14:38:57 +0200 Subject: [PATCH 5/9] chore(enterprise): improve meta client --- enterprise/meta.go | 11 +- enterprise/meta_test.go | 215 +++++++++------------------------------- 2 files changed, 51 insertions(+), 175 deletions(-) diff --git a/enterprise/meta.go b/enterprise/meta.go index 8d927be394..ef2a8c6e07 100644 --- a/enterprise/meta.go +++ b/enterprise/meta.go @@ -36,12 +36,11 @@ var ( ) type client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } // MetaClient represents a Meta node in an Influx Enterprise cluster type MetaClient struct { - URL *url.URL client client authorizer influx.Authorizer } @@ -49,8 +48,8 @@ type MetaClient struct { // NewMetaClient represents a meta node in an Influx Enterprise cluster func NewMetaClient(url *url.URL, InsecureSkipVerify bool, authorizer influx.Authorizer) *MetaClient { return &MetaClient{ - URL: url, client: &defaultClient{ + URL: url, InsecureSkipVerify: InsecureSkipVerify, }, authorizer: authorizer, @@ -480,14 +479,16 @@ func (m *MetaClient) Post(ctx context.Context, path string, action interface{}, type defaultClient struct { InsecureSkipVerify bool + URL *url.URL } // Do is a helper function to interface with Influx Enterprise's Meta API -func (d *defaultClient) Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) { +func (d *defaultClient) Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) { p := url.Values{} for k, v := range params { p.Add(k, v) } + URL := *d.URL URL.Path = path URL.RawQuery = p.Encode() @@ -563,7 +564,7 @@ func (m *MetaClient) Do(ctx context.Context, path, method string, authorizer inf resps := make(chan (result)) go func() { - resp, err := m.client.Do(m.URL, path, method, authorizer, params, body) + resp, err := m.client.Do(path, method, authorizer, params, body) resps <- result{resp, err} }() diff --git a/enterprise/meta_test.go b/enterprise/meta_test.go index 9f0dd8ff54..517cbec254 100644 --- a/enterprise/meta_test.go +++ b/enterprise/meta_test.go @@ -18,9 +18,8 @@ import ( func TestMetaClient_ShowCluster(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } tests := []struct { @@ -32,10 +31,6 @@ func TestMetaClient_ShowCluster(t *testing.T) { { name: "Successful Show Cluster", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"data":[{"id":2,"version":"1.1.0-c1.1.0","tcpAddr":"data-1.twinpinesmall.net:8088","httpAddr":"data-1.twinpinesmall.net:8086","httpScheme":"https","status":"joined"}],"meta":[{"id":1,"addr":"meta-0.twinpinesmall.net:8091","httpScheme":"http","tcpAddr":"meta-0.twinpinesmall.net:8089","version":"1.1.0-c1.1.0"}]}`), @@ -66,10 +61,6 @@ func TestMetaClient_ShowCluster(t *testing.T) { { name: "Failed Show Cluster", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusBadGateway, nil, @@ -82,10 +73,6 @@ func TestMetaClient_ShowCluster(t *testing.T) { { name: "Bad JSON from Show Cluster", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{data}`), @@ -98,7 +85,6 @@ func TestMetaClient_ShowCluster(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } got, err := m.ShowCluster(context.Background()) @@ -129,9 +115,8 @@ func TestMetaClient_ShowCluster(t *testing.T) { func TestMetaClient_Users(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -148,10 +133,6 @@ func TestMetaClient_Users(t *testing.T) { { name: "Successful Show users", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"users":[{"name":"admin","hash":"1234","permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -179,10 +160,6 @@ func TestMetaClient_Users(t *testing.T) { { name: "Successful Show users single user", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"users":[{"name":"admin","hash":"1234","permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -210,10 +187,6 @@ func TestMetaClient_Users(t *testing.T) { { name: "Failure Show users", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"users":[{"name":"admin","hash":"1234","permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -230,10 +203,6 @@ func TestMetaClient_Users(t *testing.T) { { name: "Bad JSON from Show users", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{foo}`), @@ -250,7 +219,6 @@ func TestMetaClient_Users(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } got, err := m.Users(tt.args.ctx, tt.args.name) @@ -266,9 +234,8 @@ func TestMetaClient_Users(t *testing.T) { func TestMetaClient_User(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -285,10 +252,6 @@ func TestMetaClient_User(t *testing.T) { { name: "Successful Show users", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"users":[{"name":"admin","hash":"1234","permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -312,10 +275,6 @@ func TestMetaClient_User(t *testing.T) { { name: "No such user", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusNotFound, []byte(`{"error":"user not found"}`), @@ -332,10 +291,6 @@ func TestMetaClient_User(t *testing.T) { { name: "Bad JSON", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusNotFound, []byte(`{BAD}`), @@ -351,7 +306,6 @@ func TestMetaClient_User(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } got, err := m.User(tt.args.ctx, tt.args.name) @@ -367,9 +321,8 @@ func TestMetaClient_User(t *testing.T) { func TestMetaClient_CreateUser(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -387,10 +340,6 @@ func TestMetaClient_CreateUser(t *testing.T) { { name: "Successful Create User", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, nil, @@ -408,7 +357,6 @@ func TestMetaClient_CreateUser(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } if err := m.CreateUser(tt.args.ctx, tt.args.name, tt.args.passwd); (err != nil) != tt.wantErr { @@ -438,9 +386,8 @@ func TestMetaClient_CreateUser(t *testing.T) { func TestMetaClient_ChangePassword(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -458,10 +405,6 @@ func TestMetaClient_ChangePassword(t *testing.T) { { name: "Successful Change Password", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, nil, @@ -479,7 +422,6 @@ func TestMetaClient_ChangePassword(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } if err := m.ChangePassword(tt.args.ctx, tt.args.name, tt.args.passwd); (err != nil) != tt.wantErr { @@ -510,9 +452,8 @@ func TestMetaClient_ChangePassword(t *testing.T) { func TestMetaClient_DeleteUser(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -529,10 +470,6 @@ func TestMetaClient_DeleteUser(t *testing.T) { { name: "Successful delete User", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, nil, @@ -549,7 +486,6 @@ func TestMetaClient_DeleteUser(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } if err := m.DeleteUser(tt.args.ctx, tt.args.name); (err != nil) != tt.wantErr { @@ -579,9 +515,8 @@ func TestMetaClient_DeleteUser(t *testing.T) { func TestMetaClient_SetUserPerms(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -600,10 +535,6 @@ func TestMetaClient_SetUserPerms(t *testing.T) { { name: "Remove all permissions for a user", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"users":[{"name":"admin","hash":"1234","permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -620,10 +551,6 @@ func TestMetaClient_SetUserPerms(t *testing.T) { { name: "Remove some permissions and add others", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"users":[{"name":"admin","hash":"1234","permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -646,7 +573,6 @@ func TestMetaClient_SetUserPerms(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } if err := m.SetUserPerms(tt.args.ctx, tt.args.name, tt.args.perms); (err != nil) != tt.wantErr { @@ -700,9 +626,8 @@ func TestMetaClient_SetUserPerms(t *testing.T) { func TestMetaClient_Roles(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -719,10 +644,6 @@ func TestMetaClient_Roles(t *testing.T) { { name: "Successful Show role", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"roles":[{"name":"admin","users":["marty"],"permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -751,10 +672,6 @@ func TestMetaClient_Roles(t *testing.T) { { name: "Successful Show role single role", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"roles":[{"name":"admin","users":["marty"],"permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -783,7 +700,6 @@ func TestMetaClient_Roles(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } got, err := m.Roles(tt.args.ctx, tt.args.name) @@ -799,9 +715,8 @@ func TestMetaClient_Roles(t *testing.T) { func TestMetaClient_Role(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -818,10 +733,6 @@ func TestMetaClient_Role(t *testing.T) { { name: "Successful Show role", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"roles":[{"name":"admin","users":["marty"],"permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -846,10 +757,6 @@ func TestMetaClient_Role(t *testing.T) { { name: "No such role", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusNotFound, []byte(`{"error":"user not found"}`), @@ -866,7 +773,6 @@ func TestMetaClient_Role(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } got, err := m.Role(tt.args.ctx, tt.args.name) @@ -882,9 +788,8 @@ func TestMetaClient_Role(t *testing.T) { func TestMetaClient_UserRoles(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -901,10 +806,6 @@ func TestMetaClient_UserRoles(t *testing.T) { { name: "Successful Show all roles", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"roles":[{"name":"timetravelers","users":["marty","docbrown"],"permissions":{"":["ViewAdmin","ViewChronograf"]}},{"name":"mcfly","users":["marty","george"],"permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -970,7 +871,6 @@ func TestMetaClient_UserRoles(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } got, err := m.UserRoles(tt.args.ctx) @@ -986,9 +886,8 @@ func TestMetaClient_UserRoles(t *testing.T) { func TestMetaClient_CreateRole(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -1005,10 +904,6 @@ func TestMetaClient_CreateRole(t *testing.T) { { name: "Successful Create Role", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, nil, @@ -1025,7 +920,6 @@ func TestMetaClient_CreateRole(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } if err := m.CreateRole(tt.args.ctx, tt.args.name); (err != nil) != tt.wantErr { @@ -1052,9 +946,8 @@ func TestMetaClient_CreateRole(t *testing.T) { func TestMetaClient_DeleteRole(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -1071,10 +964,6 @@ func TestMetaClient_DeleteRole(t *testing.T) { { name: "Successful delete role", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, nil, @@ -1091,7 +980,6 @@ func TestMetaClient_DeleteRole(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } if err := m.DeleteRole(tt.args.ctx, tt.args.name); (err != nil) != tt.wantErr { @@ -1121,9 +1009,8 @@ func TestMetaClient_DeleteRole(t *testing.T) { func TestMetaClient_SetRolePerms(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -1142,10 +1029,6 @@ func TestMetaClient_SetRolePerms(t *testing.T) { { name: "Remove all roles from user", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"roles":[{"name":"admin","users":["marty"],"permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -1162,10 +1045,6 @@ func TestMetaClient_SetRolePerms(t *testing.T) { { name: "Remove some users and add permissions to other", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"roles":[{"name":"admin","users":["marty"],"permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -1188,7 +1067,6 @@ func TestMetaClient_SetRolePerms(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } if err := m.SetRolePerms(tt.args.ctx, tt.args.name, tt.args.perms); (err != nil) != tt.wantErr { @@ -1242,9 +1120,8 @@ func TestMetaClient_SetRolePerms(t *testing.T) { func TestMetaClient_SetRoleUsers(t *testing.T) { type fields struct { - URL *url.URL client interface { - Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) + Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) } } type args struct { @@ -1262,10 +1139,6 @@ func TestMetaClient_SetRoleUsers(t *testing.T) { { name: "Successful set users role (remove user from role)", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"roles":[{"name":"admin","users":["marty"],"permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -1282,10 +1155,6 @@ func TestMetaClient_SetRoleUsers(t *testing.T) { { name: "Successful set single user role", fields: fields{ - URL: &url.URL{ - Host: "twinpinesmall.net:8091", - Scheme: "https", - }, client: NewMockClient( http.StatusOK, []byte(`{"roles":[{"name":"admin","users":[],"permissions":{"":["ViewAdmin","ViewChronograf"]}}]}`), @@ -1305,7 +1174,6 @@ func TestMetaClient_SetRoleUsers(t *testing.T) { } for _, tt := range tests { m := &MetaClient{ - URL: tt.fields.URL, client: tt.fields.client, } if err := m.SetRoleUsers(tt.args.ctx, tt.args.name, tt.args.users); (err != nil) != tt.wantErr { @@ -1346,6 +1214,7 @@ func TestMetaClient_SetRoleUsers(t *testing.T) { } type MockClient struct { + URL *url.URL Code int // HTTP Status code Body []byte HeaderMap http.Header @@ -1356,6 +1225,10 @@ type MockClient struct { func NewMockClient(code int, body []byte, headers http.Header, err error) *MockClient { return &MockClient{ + URL: &url.URL{ + Host: "twinpinesmall.net:8091", + Scheme: "https", + }, Code: code, Body: body, HeaderMap: headers, @@ -1364,13 +1237,10 @@ func NewMockClient(code int, body []byte, headers http.Header, err error) *MockC } } -func (c *MockClient) Do(URL *url.URL, path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) { +func (c *MockClient) Do(path, method string, authorizer influx.Authorizer, params map[string]string, body io.Reader) (*http.Response, error) { if c == nil { return nil, fmt.Errorf("NIL MockClient") } - if URL == nil { - return nil, fmt.Errorf("NIL url") - } if c.Err != nil { return nil, c.Err } @@ -1381,6 +1251,7 @@ func (c *MockClient) Do(URL *url.URL, path, method string, authorizer influx.Aut p.Add(k, v) } + URL := *c.URL URL.Path = path URL.RawQuery = p.Encode() @@ -1401,17 +1272,25 @@ func (c *MockClient) Do(URL *url.URL, path, method string, authorizer influx.Aut }, nil } +type mockAuthorizer struct { + set func(req *http.Request) error +} + +func (a *mockAuthorizer) Set(req *http.Request) error { + return a.set(req) +} + func Test_AuthedCheckRedirect_Do(t *testing.T) { - var ts2URL string + var ts2URL *url.URL ts1 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { want := http.Header{ - "Referer": []string{ts2URL}, + "Referer": []string{ts2URL.String()}, "Accept-Encoding": []string{"gzip"}, "Authorization": []string{"hunter2"}, } for k, v := range want { if !reflect.DeepEqual(r.Header[k], v) { - t.Errorf("Request.Header = %#v; want %#v", r.Header[k], v) + t.Errorf("Request.Header[%s] = %#v; want %#v", k, r.Header[k], v) } } if t.Failed() { @@ -1426,23 +1305,19 @@ func Test_AuthedCheckRedirect_Do(t *testing.T) { http.Redirect(w, r, ts1.URL, http.StatusFound) })) defer ts2.Close() - ts2URL = ts2.URL + ts2URL, _ = url.Parse(ts2.URL) - tr := &http.Transport{} - defer tr.CloseIdleConnections() - d := &defaultClient{} - c := &http.Client{ - Transport: tr, - CheckRedirect: d.AuthedCheckRedirect, + d := &defaultClient{InsecureSkipVerify: true, URL: ts2URL} + authorizer := &mockAuthorizer{ + set: func(req *http.Request) error { + req.Header.Add("Cookie", "foo=bar") + req.Header.Add("Authorization", "hunter2") + req.Header.Add("Howdy", "doody") + req.Header.Set("User-Agent", "Darth Vader, an extraterrestrial from the Planet Vulcan") + return nil + }, } - - req, _ := http.NewRequest("GET", ts2.URL, nil) - req.Header.Add("Cookie", "foo=bar") - req.Header.Add("Authorization", "hunter2") - req.Header.Add("Howdy", "doody") - req.Header.Set("User-Agent", "Darth Vader, an extraterrestrial from the Planet Vulcan") - - res, err := c.Do(req) + res, err := d.Do("", "GET", authorizer, nil, nil) if err != nil { t.Fatal(err) } @@ -1514,9 +1389,9 @@ func Test_defaultClient_Do(t *testing.T) { })) defer ts.Close() - d := &defaultClient{} u, _ := url.Parse(ts.URL) - _, err := d.Do(u, tt.args.path, tt.args.method, tt.args.authorizer, tt.args.params, tt.args.body) + d := &defaultClient{URL: u} + _, err := d.Do(tt.args.path, tt.args.method, tt.args.authorizer, tt.args.params, tt.args.body) if (err != nil) != tt.wantErr { t.Errorf("defaultClient.Do() error = %v, wantErr %v", err, tt.wantErr) return From 47bc1a24494bb241812a0d6f7b032db822853848 Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 11 May 2022 15:32:54 +0200 Subject: [PATCH 6/9] fix(enterprise): remember and use master URL in communication with meta nodes --- enterprise/meta.go | 36 +++++++++++++++++++++++++++++++----- enterprise/meta_test.go | 40 ++++++++++++++++++++++++++++------------ 2 files changed, 59 insertions(+), 17 deletions(-) diff --git a/enterprise/meta.go b/enterprise/meta.go index ef2a8c6e07..946a542f65 100644 --- a/enterprise/meta.go +++ b/enterprise/meta.go @@ -12,6 +12,7 @@ import ( "net" "net/http" "net/url" + "sync" "time" "github.com/influxdata/chronograf" @@ -48,10 +49,7 @@ type MetaClient struct { // NewMetaClient represents a meta node in an Influx Enterprise cluster func NewMetaClient(url *url.URL, InsecureSkipVerify bool, authorizer influx.Authorizer) *MetaClient { return &MetaClient{ - client: &defaultClient{ - URL: url, - InsecureSkipVerify: InsecureSkipVerify, - }, + client: newDefaultClient(url, InsecureSkipVerify), authorizer: authorizer, } } @@ -480,6 +478,33 @@ func (m *MetaClient) Post(ctx context.Context, path string, action interface{}, type defaultClient struct { InsecureSkipVerify bool URL *url.URL + + masterURL *url.URL + mu sync.Mutex +} + +func newDefaultClient(URL *url.URL, InsecureSkipVerify bool) *defaultClient { + return &defaultClient{ + URL: URL, + InsecureSkipVerify: InsecureSkipVerify, + } +} + +func (d *defaultClient) setMasterURL(URL *url.URL) { + if URL.Host != "" && URL.Scheme != "" { + d.mu.Lock() + defer d.mu.Unlock() + d.masterURL = &url.URL{Host: URL.Host, Scheme: URL.Scheme} + } +} + +func (d *defaultClient) getMasterURL() url.URL { + d.mu.Lock() + defer d.mu.Unlock() + if d.masterURL != nil { + return *d.masterURL + } + return *d.URL } // Do is a helper function to interface with Influx Enterprise's Meta API @@ -488,7 +513,7 @@ func (d *defaultClient) Do(path, method string, authorizer influx.Authorizer, pa for k, v := range params { p.Add(k, v) } - URL := *d.URL + URL := d.getMasterURL() URL.Path = path URL.RawQuery = p.Encode() @@ -543,6 +568,7 @@ func (d *defaultClient) Do(path, method string, authorizer influx.Authorizer, pa // AuthedCheckRedirect tries to follow the Influx Enterprise pattern of // redirecting to the leader but preserving authentication headers. func (d *defaultClient) AuthedCheckRedirect(req *http.Request, via []*http.Request) error { + d.setMasterURL(req.URL) if len(via) >= 10 { return errors.New("too many redirects") } else if len(via) == 0 { diff --git a/enterprise/meta_test.go b/enterprise/meta_test.go index 517cbec254..34c57c59ef 100644 --- a/enterprise/meta_test.go +++ b/enterprise/meta_test.go @@ -1281,13 +1281,19 @@ func (a *mockAuthorizer) Set(req *http.Request) error { } func Test_AuthedCheckRedirect_Do(t *testing.T) { + path := "/test" var ts2URL *url.URL + ts1Called := 0 ts1 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ts1Called++ want := http.Header{ - "Referer": []string{ts2URL.String()}, "Accept-Encoding": []string{"gzip"}, "Authorization": []string{"hunter2"}, } + if ts1Called == 1 { + // referer is filled when doing a first redirect + want.Add("Referer", ts2URL.String()+path) + } for k, v := range want { if !reflect.DeepEqual(r.Header[k], v) { t.Errorf("Request.Header[%s] = %#v; want %#v", k, r.Header[k], v) @@ -1301,13 +1307,15 @@ func Test_AuthedCheckRedirect_Do(t *testing.T) { })) defer ts1.Close() + ts2Called := 0 ts2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ts2Called++ http.Redirect(w, r, ts1.URL, http.StatusFound) })) defer ts2.Close() ts2URL, _ = url.Parse(ts2.URL) - d := &defaultClient{InsecureSkipVerify: true, URL: ts2URL} + d := newDefaultClient(ts2URL, true) authorizer := &mockAuthorizer{ set: func(req *http.Request) error { req.Header.Add("Cookie", "foo=bar") @@ -1317,18 +1325,26 @@ func Test_AuthedCheckRedirect_Do(t *testing.T) { return nil }, } - res, err := d.Do("", "GET", authorizer, nil, nil) - if err != nil { - t.Fatal(err) + repetitions := 3 + for i := 0; i < repetitions; i++ { + res, err := d.Do(path, "GET", authorizer, nil, nil) + if err != nil { + t.Fatal(err) + } + defer res.Body.Close() + if res.StatusCode != 200 { + t.Fatal(res.Status) + } + if got := res.Header.Get("Result"); got != "ok" { + t.Errorf("result = %q; want ok", got) + } } - defer res.Body.Close() - if res.StatusCode != 200 { - t.Fatal(res.Status) + if ts1Called != repetitions { + t.Errorf("Master server called %v; expected %v", ts1Called, repetitions) } - - if got := res.Header.Get("Result"); got != "ok" { - t.Errorf("result = %q; want ok", got) + if ts2Called != 1 { + t.Errorf("Follower server called %v; expected 1", ts2Called) } } @@ -1390,7 +1406,7 @@ func Test_defaultClient_Do(t *testing.T) { defer ts.Close() u, _ := url.Parse(ts.URL) - d := &defaultClient{URL: u} + d := newDefaultClient(u, true) _, err := d.Do(tt.args.path, tt.args.method, tt.args.authorizer, tt.args.params, tt.args.body) if (err != nil) != tt.wantErr { t.Errorf("defaultClient.Do() error = %v, wantErr %v", err, tt.wantErr) From 01ad36cae8d3dc13ec539aaeb5cb3ce035b20a80 Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Wed, 11 May 2022 20:20:09 +0200 Subject: [PATCH 7/9] chore: improve docs --- enterprise/meta.go | 2 +- enterprise/users_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/enterprise/meta.go b/enterprise/meta.go index 946a542f65..a90f6166fc 100644 --- a/enterprise/meta.go +++ b/enterprise/meta.go @@ -203,7 +203,7 @@ func (m *MetaClient) RemoveUserPerms(ctx context.Context, name string, perms Per return m.Post(ctx, "/user", a, nil) } -// RemoveUserPerms revokes permissions for a user in Influx Enterprise +// AddUserPerms adds permissions for a user in Influx Enterprise func (m *MetaClient) AddUserPerms(ctx context.Context, name string, perms Permissions) error { a := &UserAction{ Action: "add-permissions", diff --git a/enterprise/users_test.go b/enterprise/users_test.go index 0229351ce1..076bd6fd3b 100644 --- a/enterprise/users_test.go +++ b/enterprise/users_test.go @@ -176,8 +176,8 @@ func TestClient_Add(t *testing.T) { } } -// TestClient_Add_UserNotFound check fix for #5840, the API waits -// for the creation of the user (up to 1 second) OOTB +// TestClient_Add_UserNotFound verifies fix of defect #5840, the API has to wait +// for the creation of the user OOTB func TestClient_Add_UserNotFound(t *testing.T) { notFoundAttempts := 1 c := &enterprise.UserStore{ From c1c6a1d2b8b50efab385f61bf284cd70f9876a97 Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Thu, 12 May 2022 06:22:03 +0200 Subject: [PATCH 8/9] chore: improve inline doc --- enterprise/meta.go | 3 +++ enterprise/users.go | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/enterprise/meta.go b/enterprise/meta.go index a90f6166fc..7f28a67bd0 100644 --- a/enterprise/meta.go +++ b/enterprise/meta.go @@ -479,6 +479,8 @@ type defaultClient struct { InsecureSkipVerify bool URL *url.URL + // masterURL is setup when doing redirects, communication with a master + // node prevents stale reads from follower nodes after master node modifications masterURL *url.URL mu sync.Mutex } @@ -513,6 +515,7 @@ func (d *defaultClient) Do(path, method string, authorizer influx.Authorizer, pa for k, v := range params { p.Add(k, v) } + // prefer communication with the master node, to avoid stale reads URL := d.getMasterURL() URL.Path = path diff --git a/enterprise/users.go b/enterprise/users.go index 50cbff9305..45a5b51268 100644 --- a/enterprise/users.go +++ b/enterprise/users.go @@ -19,7 +19,7 @@ func (c *UserStore) Add(ctx context.Context, u *chronograf.User) (*chronograf.Us if err := c.Ctrl.CreateUser(ctx, u.Name, u.Passwd); err != nil { return nil, err } - // fix #5840: eventual consistency can cause delays in user creation + // fix #5840: eventual consistency can cause delays in user creation, // wait for the user to become available _, err := c.Ctrl.User(ctx, u.Name) timer := time.NewTimer(2_000_000_000) // retry at most 2 seconds From bd1ff6ba0886e904c8f17c26771377c5c8659a1b Mon Sep 17 00:00:00 2001 From: Pavel Zavora Date: Thu, 12 May 2022 06:25:28 +0200 Subject: [PATCH 9/9] chore: update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 553c0e02a6..e627576152 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ 1. [#5882](https://github.com/influxdata/chronograf/pull/5882): Repair table visualization of string values. 1. [#5913](https://github.com/influxdata/chronograf/pull/5913): Improve InfluxDB Enterprise detection. +1. [#5917](https://github.com/influxdata/chronograf/pull/5917): Improve InfluxDB Enterprise user creation process. +1. [#5917](https://github.com/influxdata/chronograf/pull/5917): Avoid stale reads in communication with InfluxDB Enterprise meta nodes. ### Other