From 7bde427c026eefacb789940472e365fa6013cfe1 Mon Sep 17 00:00:00 2001 From: Murad Biashimov Date: Fri, 12 Jul 2024 13:21:39 +0200 Subject: [PATCH] fix(clickhousegrant): revoke known privileges only --- CHANGELOG.md | 2 + api/v1alpha1/clickhousegrant_types.go | 133 ++----- api/v1alpha1/zz_generated.deepcopy.go | 49 ++- .../templates/aiven.io_clickhousegrants.yaml | 144 ++++++- .../crd/bases/aiven.io_clickhousegrants.yaml | 144 ++++++- controllers/clickhousegrant_controller.go | 354 +++--------------- docs/docs/api-reference/clickhousedatabase.md | 5 +- docs/docs/api-reference/clickhousegrant.md | 26 +- docs/docs/api-reference/clickhouseuser.md | 5 +- tests/clickhousegrant_test.go | 97 +++-- 10 files changed, 486 insertions(+), 473 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f5b933f..3a999478 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,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 (Clickhouse can do that) +- Fix `ClickhouseGrant`: track the state to revoke only known privileges ## v0.22.0 - 2024-07-02 diff --git a/api/v1alpha1/clickhousegrant_types.go b/api/v1alpha1/clickhousegrant_types.go index 73a76142..cf4026cf 100644 --- a/api/v1alpha1/clickhousegrant_types.go +++ b/api/v1alpha1/clickhousegrant_types.go @@ -3,12 +3,9 @@ package v1alpha1 import ( - "context" "fmt" "strings" - avngen "github.com/aiven/go-client-codegen" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/aiven/aiven-operator/utils" @@ -18,20 +15,24 @@ import ( // TODO: use oneOf in Grantee if https://github.com/kubernetes-sigs/controller-tools/issues/461 is resolved // Grantee represents a user or a role to which privileges or roles are granted. +// Warning "Ambiguity in the `GRANT` syntax": +// Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the GRANT syntax in ClickHouse, +// users and roles should not share the same name. +// It is unclear whether a grant applies to the user or the role. type Grantee struct { User string `json:"user,omitempty"` Role string `json:"role,omitempty"` } // PrivilegeGrant represents the privileges to be granted to users or roles. -// See https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax. +// [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax). // +kubebuilder:validation:XValidation:rule="!has(self.columns) || (has(self.columns) && has(self.table))",message="`table` must be set if `columns` are set" type PrivilegeGrant struct { // List of grantees (users or roles) to grant the privilege to. // +kubebuilder:validation:MinItems=1 Grantees []Grantee `json:"grantees"` // The privileges to grant, i.e. `INSERT`, `SELECT`. - // See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax. + // [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax). Privileges []string `json:"privileges"` // The database that the grant refers to. Database string `json:"database"` @@ -41,12 +42,12 @@ type PrivilegeGrant struct { Columns []string `json:"columns,omitempty"` // 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 + // [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax) WithGrantOption bool `json:"withGrantOption,omitempty"` } // RoleGrant represents the roles to be assigned to users or roles. -// See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax. +// [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax) type RoleGrant struct { // List of grantees (users or roles) to grant the privilege to. // +kubebuilder:validation:MinItems=1 @@ -55,30 +56,46 @@ type RoleGrant struct { // +kubebuilder:validation:MinItems=1 Roles []string `json:"roles"` // If true, the grant is executed with `ADMIN OPTION` privilege. - // See https://clickhouse.com/docs/en/sql-reference/statements/grant#admin-option. + // [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#admin-option) 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"` + // The previous applied grants to revoke. Do not edit + State *Grants `json:"state,omitempty"` } //+kubebuilder:object:root=true //+kubebuilder:subresource:status // ClickhouseGrant is the Schema for the ClickhouseGrants API -// Warning "Ambiguity in the `GRANT` syntax": Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the `GRANT` syntax in Clickhouse, you should not have users and roles with the same name. It is not clear if a grant refers to the user or the role. +// Warning: Due to the way ClickHouse operates, updating this resource first revokes the existing privileges. // +kubebuilder:printcolumn:name="Project",type="string",JSONPath=".spec.project" // +kubebuilder:printcolumn:name="Service Name",type="string",JSONPath=".spec.serviceName" type ClickhouseGrant struct { @@ -89,30 +106,8 @@ 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{} +func (in *ClickhouseGrantSpec) CollectGrantees() []string { + var allGrantees []string processGrantee := func(grantees []Grantee) { for _, grantee := range grantees { allGrantees = append(allGrantees, userOrRole(grantee)) @@ -128,7 +123,7 @@ func (in ClickhouseGrantSpec) CollectGrantees() []string { return utils.UniqueSliceElements(allGrantees) } -func (in ClickhouseGrantSpec) CollectDatabases() []string { +func (in *ClickhouseGrantSpec) CollectDatabases() []string { allDatabases := []string{} for _, grant := range in.PrivilegeGrants { if grant.Database != "" { @@ -138,7 +133,7 @@ func (in ClickhouseGrantSpec) CollectDatabases() []string { return utils.UniqueSliceElements(allDatabases) } -func (in ClickhouseGrantSpec) CollectTables() []chUtils.DatabaseAndTable { +func (in *ClickhouseGrantSpec) CollectTables() []chUtils.DatabaseAndTable { allTables := []chUtils.DatabaseAndTable{} for _, grant := range in.PrivilegeGrants { if grant.Table != "" { @@ -175,44 +170,7 @@ func init() { SchemeBuilder.Register(&ClickhouseGrant{}, &ClickhouseGrantList{}) } -// Takes a slice of PrivilegeGrant and returns a new slice -// where each grantee has its own PrivilegeGrant entry. -func FlattenPrivilegeGrants(grants []PrivilegeGrant) []PrivilegeGrant { - var flattened []PrivilegeGrant - for _, grant := range grants { - for _, grantee := range grant.Grantees { - newGrant := PrivilegeGrant{ - Grantees: []Grantee{grantee}, - Privileges: grant.Privileges, - Database: grant.Database, - Table: grant.Table, - Columns: grant.Columns, - WithGrantOption: grant.WithGrantOption, - } - flattened = append(flattened, newGrant) - } - } - return flattened -} - -// Takes a slice of RoleGrant and returns a new slice -// where each grantee has its own RoleGrant entry. -func FlattenRoleGrants(grants []RoleGrant) []RoleGrant { - var flattened []RoleGrant - for _, grant := range grants { - for _, grantee := range grant.Grantees { - newGrant := RoleGrant{ - Grantees: []Grantee{grantee}, - Roles: grant.Roles, - WithAdminOption: grant.WithAdminOption, - } - flattened = append(flattened, newGrant) - } - } - return flattened -} - -func (g PrivilegeGrant) ConstructParts(t chUtils.StatementType) (string, string, string) { +func (g *PrivilegeGrant) ConstructParts(t chUtils.StatementType) (string, string, string) { privilegesPart := constructPrivilegesPart(g) granteesPart := constructGranteePart(g.Grantees) options := "" @@ -223,7 +181,7 @@ func (g PrivilegeGrant) ConstructParts(t chUtils.StatementType) (string, string, } // Helper function to construct the privileges part of the statement -func constructPrivilegesPart(g PrivilegeGrant) string { +func constructPrivilegesPart(g *PrivilegeGrant) string { privileges := make([]string, 0, len(g.Privileges)) for _, privilege := range g.Privileges { if (privilege == "SELECT" || privilege == "INSERT") && len(g.Columns) > 0 { @@ -236,7 +194,7 @@ func constructPrivilegesPart(g PrivilegeGrant) string { return strings.Join(privileges, ", ") } -func (g RoleGrant) ConstructParts(t chUtils.StatementType) (string, string, string) { +func (g *RoleGrant) ConstructParts(t chUtils.StatementType) (string, string, string) { rolesPart := strings.Join(escapeList(g.Roles), ", ") granteesPart := constructGranteePart(g.Grantees) options := "" @@ -246,16 +204,7 @@ func (g RoleGrant) ConstructParts(t chUtils.StatementType) (string, string, stri return rolesPart, granteesPart, options } -func ExecuteGrant[T chUtils.Grant](ctx context.Context, avnGen avngen.Client, t chUtils.StatementType, grant T, projectName string, serviceName string) error { - stmt := buildStatement(t, grant) - _, err := chUtils.ExecuteClickHouseQuery(ctx, avnGen, projectName, serviceName, stmt) - if err != nil { - return err - } - return nil -} - -// Generates a ClickHouse GRANT or REVOKE statement for privilege or role grants. See https://clickhouse.com/docs/en/sql-reference/statements/grant. +// Generates a ClickHouse GRANT or REVOKE statement for privilege or role grants. [See](https://clickhouse.com/docs/en/sql-reference/statements/grant). func buildStatement[T chUtils.Grant](t chUtils.StatementType, grant T) string { mainPart, granteesPart, options := grant.ConstructParts(t) @@ -268,7 +217,7 @@ func buildStatement[T chUtils.Grant](t chUtils.StatementType, grant T) string { } // Adjust the format string based on the type of grant (PrivilegeGrant needs "ON %s" part) - if p, ok := any(grant).(PrivilegeGrant); ok { + if p, ok := any(grant).(*PrivilegeGrant); ok { // ON part is constructed only for PrivilegeGrant onPart := constructOnPart(p) if t == chUtils.GRANT { @@ -283,7 +232,7 @@ func buildStatement[T chUtils.Grant](t chUtils.StatementType, grant T) string { } // Helper function to construct the ON part of the statement -func constructOnPart(grant PrivilegeGrant) string { +func constructOnPart(grant *PrivilegeGrant) string { switch { case grant.Table != "": return escape(grant.Database) + "." + escape(grant.Table) @@ -311,7 +260,7 @@ func userOrRole(grantee Grantee) string { // Escapes database identifiers like table or column names func escape(identifier string) string { - // See https://github.com/ClickHouse/clickhouse-go/blob/8ad6ec6b95d8b0c96d00115bc2d69ff13083f94b/lib/column/column.go#L32 + // [See](https://github.com/ClickHouse/clickhouse-go/blob/8ad6ec6b95d8b0c96d00115bc2d69ff13083f94b/lib/column/column.go#L32) replacer := strings.NewReplacer("`", "\\`", "\\", "\\\\") return "`" + replacer.Replace(identifier) + "`" } diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 83500160..4f9480b4 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,11 @@ func (in *ClickhouseGrantStatus) DeepCopyInto(out *ClickhouseGrantStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.State != nil { + in, out := &in.State, &out.State + *out = new(Grants) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClickhouseGrantStatus. @@ -1014,6 +1006,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..55db9584 100644 --- a/charts/aiven-operator-crds/templates/aiven.io_clickhousegrants.yaml +++ b/charts/aiven-operator-crds/templates/aiven.io_clickhousegrants.yaml @@ -26,7 +26,7 @@ spec: openAPIV3Schema: description: |- ClickhouseGrant is the Schema for the ClickhouseGrants API - Warning "Ambiguity in the `GRANT` syntax": Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the `GRANT` syntax in Clickhouse, you should not have users and roles with the same name. It is not clear if a grant refers to the user or the role. + Warning: Due to the way ClickHouse operates, updating this resource first revokes the existing privileges. properties: apiVersion: description: |- @@ -69,7 +69,7 @@ spec: 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. + [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax). properties: columns: description: The column that the grant refers to. @@ -84,9 +84,12 @@ spec: 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. + description: |- + Grantee represents a user or a role to which privileges or roles are granted. + Warning "Ambiguity in the `GRANT` syntax": + Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the GRANT syntax in ClickHouse, + users and roles should not share the same name. + It is unclear whether a grant applies to the user or the role. properties: role: type: string @@ -98,7 +101,7 @@ spec: privileges: description: |- The privileges to grant, i.e. `INSERT`, `SELECT`. - See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax. + [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax). items: type: string type: array @@ -112,7 +115,7 @@ spec: 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 + [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax) type: boolean required: - database @@ -139,16 +142,19 @@ spec: 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. + [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. + description: |- + Grantee represents a user or a role to which privileges or roles are granted. + Warning "Ambiguity in the `GRANT` syntax": + Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the GRANT syntax in ClickHouse, + users and roles should not share the same name. + It is unclear whether a grant applies to the user or the role. properties: role: type: string @@ -166,7 +172,7 @@ spec: withAdminOption: description: |- If true, the grant is executed with `ADMIN OPTION` privilege. - See https://clickhouse.com/docs/en/sql-reference/statements/grant#admin-option. + [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#admin-option) type: boolean required: - grantees @@ -260,6 +266,120 @@ spec: - type type: object type: array + state: + description: The previous applied grants to revoke. Do not edit + 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. + Warning "Ambiguity in the `GRANT` syntax": + Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the GRANT syntax in ClickHouse, + users and roles should not share the same name. + It is unclear whether a grant applies to the user or the role. + 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. + Warning "Ambiguity in the `GRANT` syntax": + Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the GRANT syntax in ClickHouse, + users and roles should not share the same name. + It is unclear whether a grant applies to the user or the role. + 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..55db9584 100644 --- a/config/crd/bases/aiven.io_clickhousegrants.yaml +++ b/config/crd/bases/aiven.io_clickhousegrants.yaml @@ -26,7 +26,7 @@ spec: openAPIV3Schema: description: |- ClickhouseGrant is the Schema for the ClickhouseGrants API - Warning "Ambiguity in the `GRANT` syntax": Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the `GRANT` syntax in Clickhouse, you should not have users and roles with the same name. It is not clear if a grant refers to the user or the role. + Warning: Due to the way ClickHouse operates, updating this resource first revokes the existing privileges. properties: apiVersion: description: |- @@ -69,7 +69,7 @@ spec: 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. + [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax). properties: columns: description: The column that the grant refers to. @@ -84,9 +84,12 @@ spec: 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. + description: |- + Grantee represents a user or a role to which privileges or roles are granted. + Warning "Ambiguity in the `GRANT` syntax": + Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the GRANT syntax in ClickHouse, + users and roles should not share the same name. + It is unclear whether a grant applies to the user or the role. properties: role: type: string @@ -98,7 +101,7 @@ spec: privileges: description: |- The privileges to grant, i.e. `INSERT`, `SELECT`. - See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax. + [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax). items: type: string type: array @@ -112,7 +115,7 @@ spec: 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 + [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax) type: boolean required: - database @@ -139,16 +142,19 @@ spec: 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. + [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. + description: |- + Grantee represents a user or a role to which privileges or roles are granted. + Warning "Ambiguity in the `GRANT` syntax": + Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the GRANT syntax in ClickHouse, + users and roles should not share the same name. + It is unclear whether a grant applies to the user or the role. properties: role: type: string @@ -166,7 +172,7 @@ spec: withAdminOption: description: |- If true, the grant is executed with `ADMIN OPTION` privilege. - See https://clickhouse.com/docs/en/sql-reference/statements/grant#admin-option. + [See](https://clickhouse.com/docs/en/sql-reference/statements/grant#admin-option) type: boolean required: - grantees @@ -260,6 +266,120 @@ spec: - type type: object type: array + state: + description: The previous applied grants to revoke. Do not edit + 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. + Warning "Ambiguity in the `GRANT` syntax": + Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the GRANT syntax in ClickHouse, + users and roles should not share the same name. + It is unclear whether a grant applies to the user or the role. + 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. + Warning "Ambiguity in the `GRANT` syntax": + Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the GRANT syntax in ClickHouse, + users and roles should not share the same name. + It is unclear whether a grant applies to the user or the role. + 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..05a9ab26 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,16 @@ 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) - 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 + // Revokes previous grants + if g.Status.State != nil { + err = revokeGrants(ctx, avnGen, g, g.Status.State) + if err != nil { + return err + } } - _, err = g.Spec.ExecuteStatements(ctx, avnGen, chUtils.GRANT) + // Grants new privileges + err = grantSpecGrants(ctx, avnGen, g) if err != nil { return err } @@ -97,13 +84,12 @@ 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) + // Revokes latest grants. + // Doesn't revoke previous grants (state) here. + // If the object hasn't been committed yet, it might have an empty state. + // Must revoke from the spec. + err = revokeGrants(ctx, avnGen, g, &g.Spec.Grants) + 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 +98,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,102 +109,12 @@ 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: * * 1. The service is running * 2. All users and roles specified in spec exist * 3. All databases specified in spec exist - * 4. All tables specified in spec exist */ g, err := h.convert(obj) @@ -256,12 +142,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 +175,42 @@ 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 + errors = append(errors, err) } - g, ok := grant.(U) - if !ok { - return nil, fmt.Errorf("failed to convert grant to type %T", grant) - } - 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, grant *v1alpha1.Grants) error { + statements := grant.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/docs/docs/api-reference/clickhousedatabase.md b/docs/docs/api-reference/clickhousedatabase.md index a1f9771d..746d1c74 100644 --- a/docs/docs/api-reference/clickhousedatabase.md +++ b/docs/docs/api-reference/clickhousedatabase.md @@ -67,7 +67,10 @@ ClickhouseDatabaseSpec defines the desired state of ClickhouseDatabase. - [`authSecretRef`](#spec.authSecretRef-property){: name='spec.authSecretRef-property'} (object). Authentication reference to Aiven token in a secret. See below for [nested schema](#spec.authSecretRef). - [`databaseName`](#spec.databaseName-property){: name='spec.databaseName-property'} (string, Immutable, MaxLength: 63). Specifies the Clickhouse database name. Defaults to `metadata.name` if omitted. -Note: `metadata.name` is ASCII-only. For UTF-8 names, use `spec.databaseName`, but ASCII is advised for compatibility. + +!!! Note + + `metadata.name` is ASCII-only. For UTF-8 names, use `spec.databaseName`, but ASCII is advised for compatibility. ## authSecretRef {: #spec.authSecretRef } diff --git a/docs/docs/api-reference/clickhousegrant.md b/docs/docs/api-reference/clickhousegrant.md index 5660208c..b25fb7d7 100644 --- a/docs/docs/api-reference/clickhousegrant.md +++ b/docs/docs/api-reference/clickhousegrant.md @@ -159,9 +159,9 @@ demo-ch-grant my-aiven-project my-clickhouse ClickhouseGrant is the Schema for the ClickhouseGrants API -!!! Warning "Ambiguity in the `GRANT` syntax" +!!! Warning - Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the `GRANT` syntax in Clickhouse, you should not have users and roles with the same name. It is not clear if a grant refers to the user or the role. + Due to the way ClickHouse operates, updating this resource first revokes the existing privileges. **Required** @@ -203,14 +203,14 @@ Authentication reference to Aiven token in a secret. _Appears on [`spec`](#spec)._ PrivilegeGrant represents the privileges to be granted to users or roles. -See https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax. +[See](https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax). **Required** - [`database`](#spec.privilegeGrants.database-property){: name='spec.privilegeGrants.database-property'} (string). The database that the grant refers to. - [`grantees`](#spec.privilegeGrants.grantees-property){: name='spec.privilegeGrants.grantees-property'} (array of objects, MinItems: 1). List of grantees (users or roles) to grant the privilege to. See below for [nested schema](#spec.privilegeGrants.grantees). - [`privileges`](#spec.privilegeGrants.privileges-property){: name='spec.privilegeGrants.privileges-property'} (array of strings). The privileges to grant, i.e. `INSERT`, `SELECT`. -See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax. +[See](https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax). **Optional** @@ -218,7 +218,7 @@ See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role - [`table`](#spec.privilegeGrants.table-property){: name='spec.privilegeGrants.table-property'} (string). The tables that the grant refers to. To grant a privilege on all tables in a database, omit this field instead of writing `table: "*"`. - [`withGrantOption`](#spec.privilegeGrants.withGrantOption-property){: name='spec.privilegeGrants.withGrantOption-property'} (boolean). 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. +[See](https://clickhouse.com/docs/en/sql-reference/statements/grant#granting-privilege-syntax). ### grantees {: #spec.privilegeGrants.grantees } @@ -226,6 +226,12 @@ _Appears on [`spec.privilegeGrants`](#spec.privilegeGrants)._ Grantee represents a user or a role to which privileges or roles are granted. +!!! Warning "Ambiguity in the `GRANT` syntax" + + Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the GRANT syntax in ClickHouse, + users and roles should not share the same name. + It is unclear whether a grant applies to the user or the role. + **Optional** - [`role`](#spec.privilegeGrants.grantees.role-property){: name='spec.privilegeGrants.grantees.role-property'} (string). @@ -236,7 +242,7 @@ Grantee represents a user or a role to which privileges or roles are granted. _Appears on [`spec`](#spec)._ RoleGrant represents the roles to be assigned to users or roles. -See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax. +[See](https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role-syntax). **Required** @@ -246,7 +252,7 @@ See https://clickhouse.com/docs/en/sql-reference/statements/grant#assigning-role **Optional** - [`withAdminOption`](#spec.roleGrants.withAdminOption-property){: name='spec.roleGrants.withAdminOption-property'} (boolean). If true, the grant is executed with `ADMIN OPTION` privilege. -See https://clickhouse.com/docs/en/sql-reference/statements/grant#admin-option. +[See](https://clickhouse.com/docs/en/sql-reference/statements/grant#admin-option). ### grantees {: #spec.roleGrants.grantees } @@ -254,6 +260,12 @@ _Appears on [`spec.roleGrants`](#spec.roleGrants)._ Grantee represents a user or a role to which privileges or roles are granted. +!!! Warning "Ambiguity in the `GRANT` syntax" + + Due to [an ambiguity](https://github.com/aiven/ospo-tracker/issues/350) in the GRANT syntax in ClickHouse, + users and roles should not share the same name. + It is unclear whether a grant applies to the user or the role. + **Optional** - [`role`](#spec.roleGrants.grantees.role-property){: name='spec.roleGrants.grantees.role-property'} (string). diff --git a/docs/docs/api-reference/clickhouseuser.md b/docs/docs/api-reference/clickhouseuser.md index 6418ada0..792694bf 100644 --- a/docs/docs/api-reference/clickhouseuser.md +++ b/docs/docs/api-reference/clickhouseuser.md @@ -117,7 +117,10 @@ ClickhouseUserSpec defines the desired state of ClickhouseUser. - [`connInfoSecretTarget`](#spec.connInfoSecretTarget-property){: name='spec.connInfoSecretTarget-property'} (object). Secret configuration. See below for [nested schema](#spec.connInfoSecretTarget). - [`connInfoSecretTargetDisabled`](#spec.connInfoSecretTargetDisabled-property){: name='spec.connInfoSecretTargetDisabled-property'} (boolean, Immutable). When true, the secret containing connection information will not be created, defaults to false. This field cannot be changed after resource creation. - [`username`](#spec.username-property){: name='spec.username-property'} (string, Immutable, MaxLength: 63). Name of the Clickhouse user. Defaults to `metadata.name` if omitted. -Note: `metadata.name` is ASCII-only. For UTF-8 names, use `spec.username`, but ASCII is advised for compatibility. + +!!! Note + + `metadata.name` is ASCII-only. For UTF-8 names, use `spec.username`, but ASCII is advised for compatibility. ## authSecretRef {: #spec.authSecretRef } diff --git a/tests/clickhousegrant_test.go b/tests/clickhousegrant_test.go index 6c68e67c..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,18 +386,24 @@ 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 @@ -368,7 +411,8 @@ func TestClickhouseGrantExample(t *testing.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)) }