From b5dc1b82622eb08d29c115b18d07968f528d546a Mon Sep 17 00:00:00 2001 From: Murad Biashimov Date: Thu, 11 Jul 2024 11:24:47 +0200 Subject: [PATCH] fix(clickhousegrant): remote and local states comparison (#780) --- CHANGELOG.md | 2 + api/v1alpha1/clickhousegrant_types.go | 10 +- controllers/clickhousegrant_controller.go | 44 ++++++--- docs/docs/api-reference/clickhousegrant.md | 92 ++++++++++++++++++- .../examples/clickhousegrant.example_2.yaml | 40 ++++++++ .../examples/clickhousegrant.yaml | 92 ++++++++++++++----- tests/clickhousegrant_test.go | 89 +++++++++++++++++- 7 files changed, 326 insertions(+), 43 deletions(-) create mode 100644 docs/docs/api-reference/examples/clickhousegrant.example_2.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d81d75d..3c5fe564 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ - Ignore `http.StatusBadRequest` on `ClickhouseGrant` deletion - Retry conflict error when k8s object saved to the storage +- Fix `ClickhouseGrant` invalid remote and local privileges comparison +- Fix `ClickhouseGrant`: doesn't escape role name to grant ## v0.22.0 - 2024-07-02 diff --git a/api/v1alpha1/clickhousegrant_types.go b/api/v1alpha1/clickhousegrant_types.go index f1515a87..73a76142 100644 --- a/api/v1alpha1/clickhousegrant_types.go +++ b/api/v1alpha1/clickhousegrant_types.go @@ -237,7 +237,7 @@ func constructPrivilegesPart(g PrivilegeGrant) string { } func (g RoleGrant) ConstructParts(t chUtils.StatementType) (string, string, string) { - rolesPart := strings.Join(g.Roles, ", ") + rolesPart := strings.Join(escapeList(g.Roles), ", ") granteesPart := constructGranteePart(g.Grantees) options := "" if g.WithAdminOption && t == chUtils.GRANT { @@ -315,3 +315,11 @@ func escape(identifier string) string { replacer := strings.NewReplacer("`", "\\`", "\\", "\\\\") return "`" + replacer.Replace(identifier) + "`" } + +func escapeList(list []string) []string { + result := make([]string, len(list)) + for i, v := range list { + result[i] = escape(v) + } + return result +} diff --git a/controllers/clickhousegrant_controller.go b/controllers/clickhousegrant_controller.go index a0ce655a..ef6c27c8 100644 --- a/controllers/clickhousegrant_controller.go +++ b/controllers/clickhousegrant_controller.go @@ -4,10 +4,11 @@ package controllers import ( "context" + "encoding/json" "fmt" + "net/http" "slices" "strconv" - "strings" "github.com/aiven/aiven-go-client/v2" avngen "github.com/aiven/go-client-codegen" @@ -158,25 +159,25 @@ func diffClickhouseGrantSpecToApi(specPrivilegeGrants []v1alpha1.PrivilegeGrant, var roleGrantsToRevoke, roleGrantsToAdd []v1alpha1.RoleGrant for _, apiGrant := range apiPrivilegeGrants { - if !containsPrivilegeGrant(specPrivilegeGrants, apiGrant) { + if !containsGrant(specPrivilegeGrants, apiGrant) { privilegeGrantsToRevoke = append(privilegeGrantsToRevoke, apiGrant) } } for _, specGrant := range specPrivilegeGrants { - if !containsPrivilegeGrant(apiPrivilegeGrants, specGrant) { + if !containsGrant(apiPrivilegeGrants, specGrant) { privilegeGrantsToAdd = append(privilegeGrantsToAdd, specGrant) } } for _, apiGrant := range apiRoleGrants { - if !containsRoleGrant(specRoleGrants, apiGrant) { + if !containsGrant(specRoleGrants, apiGrant) { roleGrantsToRevoke = append(roleGrantsToRevoke, apiGrant) } } for _, specGrant := range specRoleGrants { - if !containsRoleGrant(apiRoleGrants, specGrant) { + if !containsGrant(apiRoleGrants, specGrant) { roleGrantsToAdd = append(roleGrantsToAdd, specGrant) } } @@ -184,18 +185,37 @@ func diffClickhouseGrantSpecToApi(specPrivilegeGrants []v1alpha1.PrivilegeGrant, return privilegeGrantsToRevoke, privilegeGrantsToAdd, roleGrantsToRevoke, roleGrantsToAdd } -func containsPrivilegeGrant(grants []v1alpha1.PrivilegeGrant, grant chUtils.Grant) bool { - for _, g := range grants { - if cmp.Equal(g, grant) { - return true +// normalizeGrant v1alpha1.PrivilegeGrant and []v1alpha1.RoleGrant +// have array fields with scalars and objects. +// To compare two grants list fields must be sorted +func normalizeGrant(grant any) (result map[string]any) { + b, _ := json.Marshal(grant) + _ = json.Unmarshal(b, &result) + return sortValues(result) +} + +func sortValues[T map[string]any](m T) T { + for k, v := range m { + switch val := v.(type) { + case T: + m[k] = sortValues(val) + case []any: + sorted := make([]string, len(val)) + for i, s := range val { + sorted[i] = fmt.Sprintf("%v", s) + } + + slices.Sort(sorted) + m[k] = sorted } } - return false + return m } -func containsRoleGrant(grants []v1alpha1.RoleGrant, grant v1alpha1.RoleGrant) bool { +func containsGrant[T any](grants []T, grant T) bool { + s := normalizeGrant(grant) for _, g := range grants { - if cmp.Equal(g, grant) { + if cmp.Equal(s, normalizeGrant(g)) { return true } } diff --git a/docs/docs/api-reference/clickhousegrant.md b/docs/docs/api-reference/clickhousegrant.md index d4d1c9bc..96e7e6be 100644 --- a/docs/docs/api-reference/clickhousegrant.md +++ b/docs/docs/api-reference/clickhousegrant.md @@ -2,9 +2,9 @@ title: "ClickhouseGrant" --- -## Usage example +## Usage examples -??? example +??? example "example_2" ```yaml apiVersion: aiven.io/v1alpha1 kind: ClickhouseGrant @@ -46,6 +46,94 @@ title: "ClickhouseGrant" - role: my-role ``` +??? example + ```yaml + apiVersion: aiven.io/v1alpha1 + kind: ClickhouseGrant + metadata: + name: my-clickhouse-grant + spec: + authSecretRef: + name: aiven-token + key: token + + project: aiven-project-name + serviceName: my-clickhouse-service + + privilegeGrants: + - grantees: + - role: my-clickhouse-role + privileges: + - INSERT + - SELECT + - CREATE TABLE + - CREATE VIEW + database: my-clickhouse-db + roleGrants: + - grantees: + - user: my-clickhouse-user + roles: + - my-clickhouse-role + + --- + + apiVersion: aiven.io/v1alpha1 + kind: Clickhouse + metadata: + name: my-clickhouse-service + spec: + authSecretRef: + name: aiven-token + key: token + + project: aiven-project-name + cloudName: google-europe-west1 + plan: startup-16 + + --- + + apiVersion: aiven.io/v1alpha1 + kind: ClickhouseDatabase + metadata: + name: my-clickhouse-db + spec: + authSecretRef: + name: aiven-token + key: token + + project: aiven-project-name + serviceName: my-clickhouse-service + + --- + + apiVersion: aiven.io/v1alpha1 + kind: ClickhouseUser + metadata: + name: my-clickhouse-user + spec: + authSecretRef: + name: aiven-token + key: token + + project: aiven-project-name + serviceName: my-clickhouse-service + + --- + + apiVersion: aiven.io/v1alpha1 + kind: ClickhouseRole + metadata: + name: my-clickhouse-role + spec: + authSecretRef: + name: aiven-token + key: token + + project: aiven-project-name + serviceName: my-clickhouse-service + role: my-clickhouse-role + ``` + !!! info To create this resource, a `Secret` containing Aiven token must be [created](/aiven-operator/authentication.html) first. diff --git a/docs/docs/api-reference/examples/clickhousegrant.example_2.yaml b/docs/docs/api-reference/examples/clickhousegrant.example_2.yaml new file mode 100644 index 00000000..9fc67a1c --- /dev/null +++ b/docs/docs/api-reference/examples/clickhousegrant.example_2.yaml @@ -0,0 +1,40 @@ + +apiVersion: aiven.io/v1alpha1 +kind: ClickhouseGrant +metadata: + name: demo-ch-grant +spec: + authSecretRef: + name: aiven-token + key: token + + project: my-aiven-project + serviceName: my-clickhouse + + privilegeGrants: + - grantees: + - user: user1 + - user: my-clickhouse-user-🦄 + privileges: + - SELECT + - INSERT + database: my-db + # If table is omitted, the privileges are granted on all tables in the database + # If columns is omitted, the privileges are granted on all columns in the table + - grantees: + - role: my-role + privileges: + - SELECT + database: my-db + table: my-table + columns: + - col1 + - col2 + + roleGrants: + - roles: + - other-role + grantees: + - user: my-user + - role: my-role + diff --git a/docs/docs/api-reference/examples/clickhousegrant.yaml b/docs/docs/api-reference/examples/clickhousegrant.yaml index 9fc67a1c..2a9bb33e 100644 --- a/docs/docs/api-reference/examples/clickhousegrant.yaml +++ b/docs/docs/api-reference/examples/clickhousegrant.yaml @@ -1,40 +1,84 @@ - apiVersion: aiven.io/v1alpha1 kind: ClickhouseGrant metadata: - name: demo-ch-grant + name: my-clickhouse-grant spec: authSecretRef: name: aiven-token key: token - project: my-aiven-project - serviceName: my-clickhouse + project: aiven-project-name + serviceName: my-clickhouse-service privilegeGrants: - grantees: - - user: user1 - - user: my-clickhouse-user-🦄 + - role: my-clickhouse-role privileges: - - SELECT - INSERT - database: my-db - # If table is omitted, the privileges are granted on all tables in the database - # If columns is omitted, the privileges are granted on all columns in the table - - grantees: - - role: my-role - privileges: - SELECT - database: my-db - table: my-table - columns: - - col1 - - col2 - + - CREATE TABLE + - CREATE VIEW + database: my-clickhouse-db roleGrants: - - roles: - - other-role - grantees: - - user: my-user - - role: my-role + - grantees: + - user: my-clickhouse-user + roles: + - my-clickhouse-role + +--- + +apiVersion: aiven.io/v1alpha1 +kind: Clickhouse +metadata: + name: my-clickhouse-service +spec: + authSecretRef: + name: aiven-token + key: token + + project: aiven-project-name + cloudName: google-europe-west1 + plan: startup-16 + +--- + +apiVersion: aiven.io/v1alpha1 +kind: ClickhouseDatabase +metadata: + name: my-clickhouse-db +spec: + authSecretRef: + name: aiven-token + key: token + + project: aiven-project-name + serviceName: my-clickhouse-service + +--- + +apiVersion: aiven.io/v1alpha1 +kind: ClickhouseUser +metadata: + name: my-clickhouse-user +spec: + authSecretRef: + name: aiven-token + key: token + + project: aiven-project-name + serviceName: my-clickhouse-service + +--- + +apiVersion: aiven.io/v1alpha1 +kind: ClickhouseRole +metadata: + name: my-clickhouse-role +spec: + authSecretRef: + name: aiven-token + key: token + project: aiven-project-name + serviceName: my-clickhouse-service + role: my-clickhouse-role diff --git a/tests/clickhousegrant_test.go b/tests/clickhousegrant_test.go index 853b050e..47eb8578 100644 --- a/tests/clickhousegrant_test.go +++ b/tests/clickhousegrant_test.go @@ -17,7 +17,7 @@ import ( chUtils "github.com/aiven/aiven-operator/utils/clickhouse" ) -func chConnFromSecret(ctx context.Context, secret *corev1.Secret) (clickhouse.Conn, error) { +func chConnFromSecret(secret *corev1.Secret) (clickhouse.Conn, error) { c, err := clickhouse.Open(&clickhouse.Options{ Addr: []string{fmt.Sprintf("%s:%s", string(secret.Data["HOST"]), string(secret.Data["PORT"]))}, Auth: clickhouse.Auth{ @@ -141,18 +141,16 @@ func TestClickhouseGrant(t *testing.T) { // Waits kube objects ch := new(v1alpha1.Clickhouse) - // user := new(v1alpha1.ClickhouseUser) db := new(v1alpha1.ClickhouseDatabase) grant := new(v1alpha1.ClickhouseGrant) require.NoError(t, s.GetRunning(ch, chName)) - // require.NoError(t, s.GetRunning(user, userName)) require.NoError(t, s.GetRunning(db, dbName)) // Constructs connection to ClickHouse from service secret secret, err := s.GetSecret(ch.GetName()) require.NoError(t, err, "Failed to get secret") - conn, err := chConnFromSecret(ctx, secret) + conn, err := chConnFromSecret(secret) require.NoError(t, err, "failed to connect to ClickHouse") // THEN @@ -331,3 +329,86 @@ var expectedRoleGrants = []ClickhouseRoleGrant{ } func ptr(s string) *string { return &s } + +func fromPtr[T any](v *T) T { + if v == nil { + var empty T + return empty + } + return *v +} + +func TestClickhouseGrantExample(t *testing.T) { + t.Parallel() + defer recoverPanic(t) + + // GIVEN + ctx, cancel := testCtx() + defer cancel() + + chName := randName("clickhouse-service") + dbName := randName("clickhouse-db") + userName := randName("clickhouse-user") + grantName := randName("clickhouse-grant") + roleName := randName("clickhouse-role") + + yml, err := loadExampleYaml("clickhousegrant.yaml", map[string]string{ + "aiven-project-name": cfg.Project, + "my-clickhouse-service": chName, + "my-clickhouse-db": dbName, + "my-clickhouse-user": userName, + "my-clickhouse-grant": grantName, + "my-clickhouse-role": roleName, + }) + require.NoError(t, err) + s := NewSession(ctx, k8sClient, cfg.Project) + + // Cleans test afterward + defer s.Destroy() + + // WHEN + // Applies given manifest + require.NoError(t, s.Apply(yml)) + + ch := new(v1alpha1.Clickhouse) + require.NoError(t, s.GetRunning(ch, chName)) + + user := new(v1alpha1.ClickhouseUser) + require.NoError(t, s.GetRunning(user, userName)) + + db := new(v1alpha1.ClickhouseDatabase) + require.NoError(t, s.GetRunning(db, dbName)) + + role := new(v1alpha1.ClickhouseRole) + require.NoError(t, s.GetRunning(role, roleName)) + + grant := new(v1alpha1.ClickhouseGrant) + require.NoError(t, s.GetRunning(grant, grantName)) + + // Creates connection + secret, err := s.GetSecret(ch.GetName()) + require.NoError(t, err, "Failed to get secret") + + conn, err := chConnFromSecret(secret) + require.NoError(t, err, "failed to connect to ClickHouse") + + results, err := queryAndCollectResults[ClickhouseGrant](ctx, conn, chUtils.QueryNonAivenPrivileges) + require.NoError(t, err) + + // Privileges validation + expected := map[string]bool{ + fmt.Sprintf("%s/%s/INSERT", dbName, roleName): true, + fmt.Sprintf("%s/%s/SELECT", dbName, roleName): true, + fmt.Sprintf("%s/%s/CREATE TABLE", dbName, roleName): true, + fmt.Sprintf("%s/%s/CREATE VIEW", dbName, roleName): true, + } + + // Finds and removes grants from the expected list + for _, r := range results { + key := fmt.Sprintf("%s/%s/%s", fromPtr(r.Database), fromPtr(r.RoleName), r.AccessType) + delete(expected, key) + } + + // Nothing left == all found + assert.Empty(t, expected) +}