diff --git a/CHANGELOG.md b/CHANGELOG.md index 00d9c7a8..c7643af9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ - Fix `ClickhouseGrant` invalid remote and local privileges comparison - Fix `ClickhouseGrant`: doesn't escape role name to grant - Fix `ClickhouseUser`: password was reset due to an incorrect processing cycle +- Fix `ClickhouseGrant`: grant privileges for an unknown table +- Fix `ClickhouseGrant`: track the state to revoke privileges ## v0.22.0 - 2024-07-02 diff --git a/api/v1alpha1/clickhousegrant_types.go b/api/v1alpha1/clickhousegrant_types.go index 73a76142..0a54d74b 100644 --- a/api/v1alpha1/clickhousegrant_types.go +++ b/api/v1alpha1/clickhousegrant_types.go @@ -59,19 +59,35 @@ type RoleGrant struct { WithAdminOption bool `json:"withAdminOption,omitempty"` } -// ClickhouseGrantSpec defines the desired state of ClickhouseGrant -type ClickhouseGrantSpec struct { - ServiceDependant `json:",inline,omitempty"` - +type Grants struct { // Configuration to grant a privilege. Privileges not in the manifest are revoked. Existing privileges are retained; new ones are granted. PrivilegeGrants []PrivilegeGrant `json:"privilegeGrants,omitempty"` // Configuration to grant a role. Role grants not in the manifest are revoked. Existing role grants are retained; new ones are granted. RoleGrants []RoleGrant `json:"roleGrants,omitempty"` } +func (in *Grants) BuildStatements(statementType chUtils.StatementType) []string { + stmts := make([]string, 0, len(in.PrivilegeGrants)+len(in.RoleGrants)) + for _, g := range in.PrivilegeGrants { + stmts = append(stmts, buildStatement(statementType, g)) + } + for _, g := range in.RoleGrants { + stmts = append(stmts, buildStatement(statementType, g)) + } + return stmts +} + +// ClickhouseGrantSpec defines the desired state of ClickhouseGrant +type ClickhouseGrantSpec struct { + ServiceDependant `json:",inline,omitempty"` + Grants `json:",inline,omitempty"` +} + // ClickhouseGrantStatus defines the observed state of ClickhouseGrant type ClickhouseGrantStatus struct { Conditions []metav1.Condition `json:"conditions"` + // Stores previous (before update) and current state (after update) + State Grants `json:"state,omitempty"` } //+kubebuilder:object:root=true @@ -89,28 +105,6 @@ type ClickhouseGrant struct { Status ClickhouseGrantStatus `json:"status,omitempty"` } -func (in ClickhouseGrantSpec) buildStatements(statementType chUtils.StatementType) []string { - stmts := make([]string, 0, len(in.PrivilegeGrants)+len(in.RoleGrants)) - for _, g := range in.PrivilegeGrants { - stmts = append(stmts, buildStatement(statementType, g)) - } - for _, g := range in.RoleGrants { - stmts = append(stmts, buildStatement(statementType, g)) - } - return stmts -} - -func (in ClickhouseGrantSpec) ExecuteStatements(ctx context.Context, avnGen avngen.Client, statementType chUtils.StatementType) (bool, error) { - statements := in.buildStatements(statementType) - for _, stmt := range statements { - _, err := chUtils.ExecuteClickHouseQuery(ctx, avnGen, in.Project, in.ServiceName, stmt) - if err != nil { - return false, err - } - } - return true, nil -} - func (in ClickhouseGrantSpec) CollectGrantees() []string { allGrantees := []string{} processGrantee := func(grantees []Grantee) { diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 83500160..0359f743 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -394,20 +394,7 @@ func (in *ClickhouseGrantList) DeepCopyObject() runtime.Object { func (in *ClickhouseGrantSpec) DeepCopyInto(out *ClickhouseGrantSpec) { *out = *in in.ServiceDependant.DeepCopyInto(&out.ServiceDependant) - if in.PrivilegeGrants != nil { - in, out := &in.PrivilegeGrants, &out.PrivilegeGrants - *out = make([]PrivilegeGrant, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } - } - if in.RoleGrants != nil { - in, out := &in.RoleGrants, &out.RoleGrants - *out = make([]RoleGrant, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } - } + in.Grants.DeepCopyInto(&out.Grants) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClickhouseGrantSpec. @@ -430,6 +417,7 @@ func (in *ClickhouseGrantStatus) DeepCopyInto(out *ClickhouseGrantStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + in.State.DeepCopyInto(&out.State) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClickhouseGrantStatus. @@ -1014,6 +1002,35 @@ func (in *Grantee) DeepCopy() *Grantee { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Grants) DeepCopyInto(out *Grants) { + *out = *in + if in.PrivilegeGrants != nil { + in, out := &in.PrivilegeGrants, &out.PrivilegeGrants + *out = make([]PrivilegeGrant, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } + if in.RoleGrants != nil { + in, out := &in.RoleGrants, &out.RoleGrants + *out = make([]RoleGrant, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Grants. +func (in *Grants) DeepCopy() *Grants { + if in == nil { + return nil + } + out := new(Grants) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Kafka) DeepCopyInto(out *Kafka) { *out = *in diff --git a/charts/aiven-operator-crds/templates/aiven.io_clickhousegrants.yaml b/charts/aiven-operator-crds/templates/aiven.io_clickhousegrants.yaml index d5218d63..55811cc1 100644 --- a/charts/aiven-operator-crds/templates/aiven.io_clickhousegrants.yaml +++ b/charts/aiven-operator-crds/templates/aiven.io_clickhousegrants.yaml @@ -260,6 +260,116 @@ spec: - type type: object type: array + state: + description: + Stores previous (before update) and current state (after + update) + properties: + privilegeGrants: + description: + Configuration to grant a privilege. Privileges not + in the manifest are revoked. Existing privileges are retained; + new ones are granted. + items: + description: |- + PrivilegeGrant represents the privileges to be granted to users or roles. + See https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax. + properties: + columns: + description: The column that the grant refers to. + items: + type: string + type: array + database: + description: The database that the grant refers to. + type: string + grantees: + description: + List of grantees (users or roles) to grant + the privilege to. + items: + description: + Grantee represents a user or a role to which + privileges or roles are granted. + properties: + role: + type: string + user: + type: string + type: object + minItems: 1 + type: array + privileges: + description: |- + The privileges to grant, i.e. `INSERT`, `SELECT`. + See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax. + items: + type: string + type: array + table: + description: + 'The tables that the grant refers to. To grant + a privilege on all tables in a database, omit this field + instead of writing `table: "*"`.' + type: string + withGrantOption: + description: |- + If true, then the grantee (user or role) get the permission to execute the `GRANT` query. + Users can grant privileges of the same scope they have and less. + See https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax + type: boolean + required: + - database + - grantees + - privileges + type: object + x-kubernetes-validations: + - message: "`table` must be set if `columns` are set" + rule: "!has(self.columns) || (has(self.columns) && has(self.table))" + type: array + roleGrants: + description: + Configuration to grant a role. Role grants not in + the manifest are revoked. Existing role grants are retained; + new ones are granted. + items: + description: |- + RoleGrant represents the roles to be assigned to users or roles. + See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax. + properties: + grantees: + description: + List of grantees (users or roles) to grant + the privilege to. + items: + description: + Grantee represents a user or a role to which + privileges or roles are granted. + properties: + role: + type: string + user: + type: string + type: object + minItems: 1 + type: array + roles: + description: List of roles to grant to the grantees. + items: + type: string + minItems: 1 + type: array + withAdminOption: + description: |- + If true, the grant is executed with `ADMIN OPTION` privilege. + See https://clickhouse.com/docs/en/sql-reference/statements/grant#admin-option. + type: boolean + required: + - grantees + - roles + type: object + type: array + type: object required: - conditions type: object diff --git a/config/crd/bases/aiven.io_clickhousegrants.yaml b/config/crd/bases/aiven.io_clickhousegrants.yaml index d5218d63..55811cc1 100644 --- a/config/crd/bases/aiven.io_clickhousegrants.yaml +++ b/config/crd/bases/aiven.io_clickhousegrants.yaml @@ -260,6 +260,116 @@ spec: - type type: object type: array + state: + description: + Stores previous (before update) and current state (after + update) + properties: + privilegeGrants: + description: + Configuration to grant a privilege. Privileges not + in the manifest are revoked. Existing privileges are retained; + new ones are granted. + items: + description: |- + PrivilegeGrant represents the privileges to be granted to users or roles. + See https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax. + properties: + columns: + description: The column that the grant refers to. + items: + type: string + type: array + database: + description: The database that the grant refers to. + type: string + grantees: + description: + List of grantees (users or roles) to grant + the privilege to. + items: + description: + Grantee represents a user or a role to which + privileges or roles are granted. + properties: + role: + type: string + user: + type: string + type: object + minItems: 1 + type: array + privileges: + description: |- + The privileges to grant, i.e. `INSERT`, `SELECT`. + See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax. + items: + type: string + type: array + table: + description: + 'The tables that the grant refers to. To grant + a privilege on all tables in a database, omit this field + instead of writing `table: "*"`.' + type: string + withGrantOption: + description: |- + If true, then the grantee (user or role) get the permission to execute the `GRANT` query. + Users can grant privileges of the same scope they have and less. + See https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax + type: boolean + required: + - database + - grantees + - privileges + type: object + x-kubernetes-validations: + - message: "`table` must be set if `columns` are set" + rule: "!has(self.columns) || (has(self.columns) && has(self.table))" + type: array + roleGrants: + description: + Configuration to grant a role. Role grants not in + the manifest are revoked. Existing role grants are retained; + new ones are granted. + items: + description: |- + RoleGrant represents the roles to be assigned to users or roles. + See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax. + properties: + grantees: + description: + List of grantees (users or roles) to grant + the privilege to. + items: + description: + Grantee represents a user or a role to which + privileges or roles are granted. + properties: + role: + type: string + user: + type: string + type: object + minItems: 1 + type: array + roles: + description: List of roles to grant to the grantees. + items: + type: string + minItems: 1 + type: array + withAdminOption: + description: |- + If true, the grant is executed with `ADMIN OPTION` privilege. + See https://clickhouse.com/docs/en/sql-reference/statements/grant#admin-option. + type: boolean + required: + - grantees + - roles + type: object + type: array + type: object required: - conditions type: object diff --git a/controllers/clickhousegrant_controller.go b/controllers/clickhousegrant_controller.go index ef6c27c8..4173e86b 100644 --- a/controllers/clickhousegrant_controller.go +++ b/controllers/clickhousegrant_controller.go @@ -4,16 +4,13 @@ package controllers import ( "context" - "encoding/json" "fmt" "net/http" - "slices" "strconv" "github.com/aiven/aiven-go-client/v2" avngen "github.com/aiven/go-client-codegen" - "github.com/aiven/go-client-codegen/handler/clickhouse" - "github.com/google/go-cmp/cmp" + "github.com/hashicorp/go-multierror" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -57,26 +54,14 @@ func (h *ClickhouseGrantHandler) createOrUpdate(ctx context.Context, avn *aiven. return err } - flatPrivilegeGrants := v1alpha1.FlattenPrivilegeGrants(g.Spec.PrivilegeGrants) - flatRoleGrants := v1alpha1.FlattenRoleGrants(g.Spec.RoleGrants) - apiPrivilegeGrants, apiRoleGrants, err := getGrantsFromApi(ctx, avnGen, g) + // Revokes existing grants + err = revokeGrants(ctx, avnGen, g) if err != nil { return err } - privilegeGrantsToRevoke, _, roleGrantsToRevoke, _ := diffClickhouseGrantSpecToApi(flatPrivilegeGrants, apiPrivilegeGrants, flatRoleGrants, apiRoleGrants) - - // Issue revoke grant statements for privilege and role grants not found in the spec - projectName := g.Spec.Project - serviceName := g.Spec.ServiceName - for _, grantToRevoke := range privilegeGrantsToRevoke { - v1alpha1.ExecuteGrant(ctx, avnGen, chUtils.REVOKE, grantToRevoke, projectName, serviceName) //nolint:errcheck - } - for _, grantToRevoke := range roleGrantsToRevoke { - v1alpha1.ExecuteGrant(ctx, avnGen, chUtils.REVOKE, grantToRevoke, projectName, serviceName) //nolint:errcheck - } - - _, err = g.Spec.ExecuteStatements(ctx, avnGen, chUtils.GRANT) + // Grants new privileges + err = grantSpecGrants(ctx, avnGen, g) if err != nil { return err } @@ -97,13 +82,8 @@ func (h *ClickhouseGrantHandler) delete(ctx context.Context, avn *aiven.Client, return false, err } - _, err = g.Spec.ExecuteStatements(ctx, avnGen, chUtils.REVOKE) - if isAivenError(err, http.StatusBadRequest) { - // "not found in user directories", "There is no role", etc - return true, nil - } - - return isDeleted(err) + err = revokeGrants(ctx, avnGen, g) + return err == nil, err } func (h *ClickhouseGrantHandler) get(ctx context.Context, avn *aiven.Client, avnGen avngen.Client, obj client.Object) (*corev1.Secret, error) { @@ -112,18 +92,8 @@ func (h *ClickhouseGrantHandler) get(ctx context.Context, avn *aiven.Client, avn return nil, err } - flatPrivilegeGrants := v1alpha1.FlattenPrivilegeGrants(g.Spec.PrivilegeGrants) - flatRoleGrants := v1alpha1.FlattenRoleGrants(g.Spec.RoleGrants) - apiPrivilegeGrants, apiRoleGrants, err := getGrantsFromApi(ctx, avnGen, g) - if err != nil { - return nil, err - } - - _, privilegeGrantsToAd, _, roleGrantsToAdd := diffClickhouseGrantSpecToApi(flatPrivilegeGrants, apiPrivilegeGrants, flatRoleGrants, apiRoleGrants) - - if len(privilegeGrantsToAd) > 0 || len(roleGrantsToAdd) > 0 { - return nil, fmt.Errorf("missing grants defined in spec: %+v (privilege grants) %+v (role grants)", privilegeGrantsToAd, roleGrantsToAdd) - } + // Stores previous state + g.Status.State = *g.Spec.Grants.DeepCopy() meta.SetStatusCondition(&g.Status.Conditions, getRunningCondition(metav1.ConditionTrue, "CheckRunning", @@ -133,95 +103,6 @@ func (h *ClickhouseGrantHandler) get(ctx context.Context, avn *aiven.Client, avn return nil, nil } -func getGrantsFromApi(ctx context.Context, avnGen avngen.Client, g *v1alpha1.ClickhouseGrant) ([]v1alpha1.PrivilegeGrant, []v1alpha1.RoleGrant, error) { - privileges, err := chUtils.QueryPrivileges(ctx, avnGen, g.Spec.Project, g.Spec.ServiceName) - if err != nil { - return nil, nil, err - } - roleGrants, err := chUtils.QueryRoleGrants(ctx, avnGen, g.Spec.Project, g.Spec.ServiceName) - if err != nil { - return nil, nil, err - } - - apiPrivilegeGrants, err := processGrantsFromApiResponse(privileges, PrivilegeGrantType, convertPrivilegeGrantFromApiStruct) - if err != nil { - return nil, nil, err - } - apiRoleGrants, err := processGrantsFromApiResponse(roleGrants, RoleGrantType, convertRoleGrantFromApiStruct) - if err != nil { - return nil, nil, err - } - return apiPrivilegeGrants, apiRoleGrants, nil -} - -func diffClickhouseGrantSpecToApi(specPrivilegeGrants []v1alpha1.PrivilegeGrant, apiPrivilegeGrants []v1alpha1.PrivilegeGrant, specRoleGrants []v1alpha1.RoleGrant, apiRoleGrants []v1alpha1.RoleGrant) ([]v1alpha1.PrivilegeGrant, []v1alpha1.PrivilegeGrant, []v1alpha1.RoleGrant, []v1alpha1.RoleGrant) { - var privilegeGrantsToRevoke, privilegeGrantsToAdd []v1alpha1.PrivilegeGrant - var roleGrantsToRevoke, roleGrantsToAdd []v1alpha1.RoleGrant - - for _, apiGrant := range apiPrivilegeGrants { - if !containsGrant(specPrivilegeGrants, apiGrant) { - privilegeGrantsToRevoke = append(privilegeGrantsToRevoke, apiGrant) - } - } - - for _, specGrant := range specPrivilegeGrants { - if !containsGrant(apiPrivilegeGrants, specGrant) { - privilegeGrantsToAdd = append(privilegeGrantsToAdd, specGrant) - } - } - - for _, apiGrant := range apiRoleGrants { - if !containsGrant(specRoleGrants, apiGrant) { - roleGrantsToRevoke = append(roleGrantsToRevoke, apiGrant) - } - } - - for _, specGrant := range specRoleGrants { - if !containsGrant(apiRoleGrants, specGrant) { - roleGrantsToAdd = append(roleGrantsToAdd, specGrant) - } - } - - return privilegeGrantsToRevoke, privilegeGrantsToAdd, roleGrantsToRevoke, roleGrantsToAdd -} - -// 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 m -} - -func containsGrant[T any](grants []T, grant T) bool { - s := normalizeGrant(grant) - for _, g := range grants { - if cmp.Equal(s, normalizeGrant(g)) { - return true - } - } - return false -} - func (h *ClickhouseGrantHandler) checkPreconditions(ctx context.Context, avn *aiven.Client, avnGen avngen.Client, obj client.Object) (bool, error) { /** Preconditions for ClickhouseGrant: * @@ -256,12 +137,6 @@ func (h *ClickhouseGrantHandler) checkPreconditions(ctx context.Context, avn *ai return false, err } - // Check that tables specified in spec exist - _, err = checkPrecondition(ctx, g, avnGen, g.Spec.CollectTables, chUtils.QueryTables, "missing tables defined in spec: %+v") - if err != nil { - return false, err - } - // Remove previous error conditions meta.RemoveStatusCondition(&g.Status.Conditions, "Error") @@ -295,190 +170,44 @@ func (h *ClickhouseGrantHandler) convert(i client.Object) (*v1alpha1.ClickhouseG return g, nil } -type ApiPrivilegeGrant struct { - Grantee v1alpha1.Grantee - Privilege string - Database string - Table string - Column string - WithGrantOption bool -} - -type ApiRoleGrant struct { - Grantee v1alpha1.Grantee - Role string - WithAdminOption bool -} - -type GrantType string - -const ( - PrivilegeGrantType GrantType = "PrivilegeGrant" - RoleGrantType GrantType = "RoleGrant" -) - -// GrantColumns defines the columns required for each grant type -var GrantColumns = map[GrantType][]string{ - PrivilegeGrantType: {"user_name", "role_name", "access_type", "database", "table", "column", "is_partial_revoke", "grant_option"}, - RoleGrantType: {"user_name", "role_name", "granted_role_name", "with_admin_option"}, -} - -// Process either privilege or role grants from the API response -func processGrantsFromApiResponse[U any, T any](r *clickhouse.ServiceClickHouseQueryOut, grantType GrantType, processFn func([]U) []T) ([]T, error) { - requiredColumns := GrantColumns[grantType] - columnNameMap, err := validateColumns(r.Meta, requiredColumns) - if err != nil { - return nil, err - } - - var grants []U - for _, dataRow := range r.Data { - grant, err := extractGrant(dataRow, columnNameMap, grantType) +func executeStatements( + ctx context.Context, + avnGen avngen.Client, + project, serviceName string, + statements []string, +) []error { + errors := make([]error, 0) + for _, stmt := range statements { + _, err := chUtils.ExecuteClickHouseQuery(ctx, avnGen, project, serviceName, stmt) if err != nil { - return nil, err - } - g, ok := grant.(U) - if !ok { - return nil, fmt.Errorf("failed to convert grant to type %T", grant) + errors = append(errors, err) } - grants = append(grants, g) } - - return processFn(grants), nil + return errors } -// validateColumns checks if all required columns are present in the metadata -func validateColumns(meta []clickhouse.MetaOut, requiredColumns []string) (map[string]int, error) { - columnNameMap := make(map[string]int) - for i, md := range meta { - columnNameMap[md.Name] = i - } - for _, columnName := range requiredColumns { - if _, ok := columnNameMap[columnName]; !ok { - return nil, fmt.Errorf("'system.grants' metadata is missing the '%s' column", columnName) - } +func revokeGrants(ctx context.Context, avnGen avngen.Client, g *v1alpha1.ClickhouseGrant) error { + var statements []string + statements = append(statements, g.Status.State.BuildStatements(chUtils.REVOKE)...) + statements = append(statements, g.Spec.Grants.BuildStatements(chUtils.REVOKE)...) + if len(statements) == 0 { + return nil } - return columnNameMap, nil -} -func extractGrant(dataRow []interface{}, columnNameMap map[string]int, grantType GrantType) (interface{}, error) { - switch grantType { - case PrivilegeGrantType: - grant := ApiPrivilegeGrant{ - Grantee: v1alpha1.Grantee{ - User: getString(dataRow, columnNameMap, "user_name"), - Role: getString(dataRow, columnNameMap, "role_name"), - }, - Privilege: getString(dataRow, columnNameMap, "access_type"), - Database: getString(dataRow, columnNameMap, "database"), - Table: getString(dataRow, columnNameMap, "table"), - Column: getString(dataRow, columnNameMap, "column"), - WithGrantOption: getBool(dataRow, columnNameMap, "grant_option"), - } - return grant, nil - case RoleGrantType: - grant := ApiRoleGrant{ - Grantee: v1alpha1.Grantee{ - User: getString(dataRow, columnNameMap, "user_name"), - Role: getString(dataRow, columnNameMap, "role_name"), - }, - Role: getString(dataRow, columnNameMap, "granted_role_name"), - WithAdminOption: getBool(dataRow, columnNameMap, "with_admin_option"), + errors := executeStatements(ctx, avnGen, g.Spec.Project, g.Spec.ServiceName, statements) + for _, err := range errors { + if isAivenError(err, http.StatusBadRequest) || isAivenError(err, http.StatusNotFound) { + // "not found in user directories", "There is no role", "database does not exist" etc + continue } - return grant, nil - default: - return nil, fmt.Errorf("unsupported grant type: %s", grantType) - } -} - -// getString and getBool are helper functions to extract string and boolean values from dataRow based on columnNameMap -func getString(dataRow []interface{}, columnNameMap map[string]int, columnName string) string { - if index, ok := columnNameMap[columnName]; ok && index < len(dataRow) { - if value, ok := dataRow[index].(string); ok { - return value - } - } - return "" -} - -func getBool(dataRow []interface{}, columnNameMap map[string]int, columnName string) bool { - if index, ok := columnNameMap[columnName]; ok && index < len(dataRow) { - return dataRow[index] != float64(0) - } - return false -} - -func convertPrivilegeGrantFromApiStruct(clickhouseGrants []ApiPrivilegeGrant) []v1alpha1.PrivilegeGrant { - grantMap := make(map[string]*v1alpha1.PrivilegeGrant) - for _, chGrant := range clickhouseGrants { - key := chGrant.Grantee.User + chGrant.Grantee.Role + chGrant.Database + chGrant.Table - if grant, exists := grantMap[key]; exists { - // If the grant already exists, append the privilege and column if not empty. - grant.Privileges = appendUnique(grant.Privileges, chGrant.Privilege) - if chGrant.Column != "" { - grant.Columns = appendUnique(grant.Columns, chGrant.Column) - } - } else { - // Create a new PrivilegeGrant if it does not exist. - newGrant := v1alpha1.PrivilegeGrant{ - Grantees: []v1alpha1.Grantee{{User: chGrant.Grantee.User, Role: chGrant.Grantee.Role}}, - Privileges: []string{chGrant.Privilege}, - Database: chGrant.Database, - Table: chGrant.Table, - Columns: nil, - WithGrantOption: chGrant.WithGrantOption, - } - if chGrant.Column != "" { - newGrant.Columns = append(newGrant.Columns, chGrant.Column) - } - grantMap[key] = &newGrant - } - } - - // Extract the values from the map to a slice - var privilegeGrants []v1alpha1.PrivilegeGrant - for _, grant := range grantMap { - privilegeGrants = append(privilegeGrants, *grant) + return err } - return privilegeGrants -} - -func appendUnique(slice []string, item string) []string { - for _, elem := range slice { - if elem == item { - return slice - } - } - return append(slice, item) + return nil } -func convertRoleGrantFromApiStruct(clickhouseRoleGrants []ApiRoleGrant) []v1alpha1.RoleGrant { - grantMap := make(map[string]*v1alpha1.RoleGrant) - - for _, chRoleGrant := range clickhouseRoleGrants { - key := chRoleGrant.Grantee.User + chRoleGrant.Grantee.Role - if grant, exists := grantMap[key]; exists { - if !slices.Contains(grant.Roles, chRoleGrant.Role) { - grant.Roles = append(grant.Roles, chRoleGrant.Role) - } - } else { - // Create a new RoleGrant and add it to the map - grantMap[key] = &v1alpha1.RoleGrant{ - Grantees: []v1alpha1.Grantee{{ - User: chRoleGrant.Grantee.User, - Role: chRoleGrant.Grantee.Role, - }}, - Roles: []string{chRoleGrant.Role}, - WithAdminOption: chRoleGrant.WithAdminOption, - } - } - } - - var roleGrants []v1alpha1.RoleGrant - for _, grant := range grantMap { - roleGrants = append(roleGrants, *grant) - } - - return roleGrants +func grantSpecGrants(ctx context.Context, avnGen avngen.Client, g *v1alpha1.ClickhouseGrant) error { + errors := executeStatements(ctx, avnGen, g.Spec.Project, g.Spec.ServiceName, g.Spec.Grants.BuildStatements(chUtils.GRANT)) + var err error + return multierror.Append(err, errors...).ErrorOrNil() } diff --git a/tests/clickhousegrant_test.go b/tests/clickhousegrant_test.go index 6592464a..2faee6da 100644 --- a/tests/clickhousegrant_test.go +++ b/tests/clickhousegrant_test.go @@ -11,7 +11,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/meta" "github.com/aiven/aiven-operator/api/v1alpha1" chUtils "github.com/aiven/aiven-operator/utils/clickhouse" @@ -142,7 +141,6 @@ func TestClickhouseGrant(t *testing.T) { // Waits kube objects ch := new(v1alpha1.Clickhouse) db := new(v1alpha1.ClickhouseDatabase) - grant := new(v1alpha1.ClickhouseGrant) require.NoError(t, s.GetRunning(ch, chName)) require.NoError(t, s.GetRunning(db, dbName)) @@ -154,26 +152,9 @@ func TestClickhouseGrant(t *testing.T) { require.NoError(t, err, "failed to connect to ClickHouse") // THEN - // Initially grant isn't created because the table that it is targeting doesn't exist - err = s.GetRunning(grant, "test-grant") - require.ErrorContains(t, err, "unable to wait for preconditions: missing tables defined in spec: [{Database:clickhouse-db Table:example-table}]") - - // Create example-table in clickhouse-db - createTableQuery := fmt.Sprintf("CREATE TABLE `%s`.`example-table` (col1 String, col2 Int32) ENGINE = MergeTree() ORDER BY col1", dbName) - _, err = conn.Query(ctx, createTableQuery) - require.NoError(t, err) - - // Clear conditions to stop erroring out in GetRunning. The condition is also removed in ClickhouseGrant - // checkPreconditions but due to timing issues the tests may fail if we don't remove it here. - meta.RemoveStatusCondition(grant.Conditions(), "Error") - errStatus := k8sClient.Status().Update(ctx, grant) - require.NoError(t, errStatus) - - grant = new(v1alpha1.ClickhouseGrant) - // ... and wait for the grant to be created and running + grant := new(v1alpha1.ClickhouseGrant) require.NoError(t, s.GetRunning(grant, "test-grant")) - // THEN // Query and collect ClickhouseGrant results results, err := queryAndCollectResults[ClickhouseGrant](ctx, conn, chUtils.QueryNonAivenPrivileges) require.NoError(t, err) @@ -338,6 +319,62 @@ func fromPtr[T any](v *T) T { return *v } +func clickhouseGrantExampleExtra(project, serviceName, db, user, role, grant string) string { + return fmt.Sprintf(` +apiVersion: aiven.io/v1alpha1 +kind: ClickhouseUser +metadata: + name: %[4]s +spec: + authSecretRef: + name: aiven-token + key: token + + project: %[1]s + serviceName: %[2]s +--- +apiVersion: aiven.io/v1alpha1 +kind: ClickhouseRole +metadata: + name: writer +spec: + authSecretRef: + name: aiven-token + key: token + + project: %[1]s + serviceName: %[2]s + role: %[5]s +--- +apiVersion: aiven.io/v1alpha1 +kind: ClickhouseGrant +metadata: + name: %[6]s +spec: + authSecretRef: + name: aiven-token + key: token + + project: %[1]s + serviceName: %[2]s + privilegeGrants: + - grantees: + - user: %[4]s + - role: %[5]s + privileges: + - SELECT + - INSERT + database: %[3]s + withGrantOption: true + roleGrants: + - roles: + - %[5]s + grantees: + - user: %[4]s + withAdminOption: true +`, project, serviceName, db, user, role, grant) +} + func TestClickhouseGrantExample(t *testing.T) { t.Parallel() defer recoverPanic(t) @@ -349,26 +386,33 @@ func TestClickhouseGrantExample(t *testing.T) { chName := randName("clickhouse-service") dbName := randName("clickhouse-db") userName := randName("clickhouse-user") - grantName := randName("clickhouse-grant") roleName := randName("clickhouse-role") + grantName := randName("clickhouse-grant") 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, + "my-clickhouse-grant": grantName, }) require.NoError(t, err) + + extraUserName := randName("clickhouse-extra-user") + extraRoleName := randName("clickhouse-extra-role") + extraGrantName := randName("clickhouse-extra-grant") + extraYml := clickhouseGrantExampleExtra(cfg.Project, chName, dbName, extraUserName, extraRoleName, extraGrantName) + s := NewSession(ctx, k8sClient, cfg.Project) // Cleans test afterward - defer s.Destroy() + defer s.Destroy(t) // WHEN // Applies given manifest - require.NoError(t, s.Apply(yml)) + allYml := fmt.Sprintf("%s\n---\n%s", yml, extraYml) + require.NoError(t, s.Apply(allYml)) ch := new(v1alpha1.Clickhouse) require.NoError(t, s.GetRunning(ch, chName)) @@ -395,6 +439,9 @@ func TestClickhouseGrantExample(t *testing.T) { results, err := queryAndCollectResults[ClickhouseGrant](ctx, conn, chUtils.QueryNonAivenPrivileges) require.NoError(t, err) + // Validates the state + assert.Equal(t, grant.Spec.Grants, grant.Status.State) + // Privileges validation expected := map[string]bool{ fmt.Sprintf("%s/%s/INSERT", dbName, roleName): true, @@ -411,4 +458,8 @@ func TestClickhouseGrantExample(t *testing.T) { // Nothing left == all found assert.Empty(t, expected) + + // Validates concurrency + extraGrant := new(v1alpha1.ClickhouseGrant) + require.NoError(t, s.GetRunning(extraGrant, grantName)) }