From b0fc6040f5f50d5f8f98b640e9d683e8ecad4104 Mon Sep 17 00:00:00 2001 From: Graham Plata Date: Wed, 6 Aug 2025 15:07:22 -0400 Subject: [PATCH 1/6] iImprove database name generation and sanitize user/database identifiers --- .../clickhousestatic/provisioner.go | 82 +++++++++---------- .../clickhousestatic/provisioner_test.go | 20 +++++ 2 files changed, 59 insertions(+), 43 deletions(-) diff --git a/admin/provisioner/clickhousestatic/provisioner.go b/admin/provisioner/clickhousestatic/provisioner.go index cb0179820ed..f48d87897e5 100644 --- a/admin/provisioner/clickhousestatic/provisioner.go +++ b/admin/provisioner/clickhousestatic/provisioner.go @@ -130,16 +130,9 @@ func (p *Provisioner) Provision(ctx context.Context, r *provisioner.Resource, op } // Prepare for creating the schema and user. - id := strings.ReplaceAll(r.ID, "-", "") + id := sanitizeName(r.ID) user := fmt.Sprintf("rill_%s", id) - dbName := fmt.Sprintf("rill_%s", id) - - // Use org and project names to create a more human-readable database name. - orgName := sanitizeName(getAnnotationValue(opts.Annotations, "organization_name")) - projectName := sanitizeName(getAnnotationValue(opts.Annotations, "project_name")) - if orgName != "" && projectName != "" { - dbName = fmt.Sprintf("rill_%s_%s_%s", orgName, projectName, id) - } + dbName := generateDatabaseName(id, opts.Annotations) password := newPassword() annotationsJSON, err := json.Marshal(opts.Annotations) @@ -148,13 +141,13 @@ func (p *Provisioner) Provision(ctx context.Context, r *provisioner.Resource, op } // Idempotently create the schema - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE IF NOT EXISTS %s %s COMMENT ?", dbName, p.onCluster()), string(annotationsJSON)) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE IF NOT EXISTS `%s` %s COMMENT ?", dbName, p.onCluster()), string(annotationsJSON)) if err != nil { return nil, fmt.Errorf("failed to create clickhouse database: %w", err) } // Idempotently create the user. - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("CREATE USER IF NOT EXISTS %s %s IDENTIFIED WITH sha256_password BY ? DEFAULT DATABASE %s GRANTEES NONE", user, p.onCluster(), dbName), password) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("CREATE USER IF NOT EXISTS `%s` %s IDENTIFIED WITH sha256_password BY ? DEFAULT DATABASE `%s` GRANTEES NONE", user, p.onCluster(), dbName), password) if err != nil { return nil, fmt.Errorf("failed to create clickhouse user: %w", err) } @@ -162,29 +155,13 @@ func (p *Provisioner) Provision(ctx context.Context, r *provisioner.Resource, op // When creating the user, the password assignment is not idempotent (if there are two concurrent invocations, we don't know which password was used). // By adding the password separately, we ensure all passwords will work. // NOTE: Requires ClickHouse 24.9 or later. - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("ALTER USER %s %s ADD IDENTIFIED WITH sha256_password BY ?", user, p.onCluster()), password) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("ALTER USER `%s` %s ADD IDENTIFIED WITH sha256_password BY ?", user, p.onCluster()), password) if err != nil { return nil, fmt.Errorf("failed to add password for clickhouse user: %w", err) } // Grant privileges on the database to the user - _, err = p.ch.ExecContext(ctx, fmt.Sprintf(` - GRANT %s - SELECT, - INSERT, - ALTER, - CREATE TABLE, - CREATE DICTIONARY, - CREATE VIEW, - DROP TABLE, - DROP DICTIONARY, - DROP VIEW, - TRUNCATE, - OPTIMIZE, - SHOW DICTIONARIES, - dictGet - ON %s.* TO %s - `, p.onCluster(), dbName, user)) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf(`GRANT %s SELECT, INSERT, ALTER, CREATE TABLE, CREATE DICTIONARY, CREATE VIEW, DROP TABLE, DROP DICTIONARY, DROP VIEW, TRUNCATE, OPTIMIZE, SHOW DICTIONARIES, dictGet ON `+"`%s`"+`.* TO `+"`%s`", p.onCluster(), dbName, user)) if err != nil { return nil, fmt.Errorf("failed to grant privileges to clickhouse user: %w", err) } @@ -192,23 +169,13 @@ func (p *Provisioner) Provision(ctx context.Context, r *provisioner.Resource, op // Grant access to system.parts for reporting disk usage. // NOTE 1: ClickHouse automatically adds row filters to restrict result to tables the user has access to. // NOTE 2: We do not need to explicitly grant access to system.tables and system.columns because ClickHouse adds those implicitly. - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("GRANT %s SELECT ON system.parts TO %s", p.onCluster(), user)) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("GRANT %s SELECT ON system.parts TO `%s`", p.onCluster(), user)) if err != nil { return nil, fmt.Errorf("failed to grant system privileges to clickhouse user: %w", err) } // Grant some additional global privileges to the user - _, err = p.ch.ExecContext(ctx, fmt.Sprintf(` - GRANT %s - URL, - REMOTE, - MONGO, - MYSQL, - POSTGRES, - S3, - AZURE - ON *.* TO %s - `, p.onCluster(), user)) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf(`GRANT %s URL, REMOTE, MONGO, MYSQL, POSTGRES, S3, AZURE ON *.* TO `+"`%s`", p.onCluster(), user)) if err != nil { return nil, fmt.Errorf("failed to grant global privileges to clickhouse user: %w", err) } @@ -269,13 +236,13 @@ func (p *Provisioner) Deprovision(ctx context.Context, r *provisioner.Resource) } // Drop the database - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("DROP DATABASE IF EXISTS %s %s", opts.Auth.Database, p.onCluster())) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("DROP DATABASE IF EXISTS `%s` %s", opts.Auth.Database, p.onCluster())) if err != nil { return fmt.Errorf("failed to drop clickhouse database: %w", err) } // Drop the user - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("DROP USER IF EXISTS %s %s", opts.Auth.Username, p.onCluster())) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("DROP USER IF EXISTS `%s` %s", opts.Auth.Username, p.onCluster())) if err != nil { return fmt.Errorf("failed to drop clickhouse user: %w", err) } @@ -357,3 +324,32 @@ func sanitizeName(name string) string { return strings.ToLower(result.String()) } + +// generateDatabaseName creates a predictable, length-controlled database name +func generateDatabaseName(resourceID string, annotations map[string]string) string { + orgName := sanitizeName(getAnnotationValue(annotations, "organization_name")) + projectName := sanitizeName(getAnnotationValue(annotations, "project_name")) + + // Limit component lengths to avoid overly long database names + if len(orgName) > 20 { + orgName = orgName[:20] + } + if len(projectName) > 20 { + projectName = projectName[:20] + } + if len(resourceID) > 20 { + resourceID = resourceID[:20] + } + + // Build database name based on available components for human readability + if orgName != "" && projectName != "" { + return fmt.Sprintf("rill_%s_%s_%s", orgName, projectName, resourceID) + } else if orgName != "" { + return fmt.Sprintf("rill_%s_%s", orgName, resourceID) + } else if projectName != "" { + return fmt.Sprintf("rill_%s_%s", projectName, resourceID) + } + + // Fallback to simple naming + return fmt.Sprintf("rill_%s", resourceID) +} diff --git a/admin/provisioner/clickhousestatic/provisioner_test.go b/admin/provisioner/clickhousestatic/provisioner_test.go index 304f7cd8148..88741bde596 100644 --- a/admin/provisioner/clickhousestatic/provisioner_test.go +++ b/admin/provisioner/clickhousestatic/provisioner_test.go @@ -427,3 +427,23 @@ func TestSanitizeName(t *testing.T) { }) } } + +func TestGenerateDatabaseName(t *testing.T) { + tests := []struct { + id string + annotations map[string]string + expected string + }{ + {"12345", nil, "rill_12345"}, + {"12345", map[string]string{"organization_name": "Acme-Corp"}, "rill_acme_corp_12345"}, + {"12345", map[string]string{"project_name": "My-Project"}, "rill_my_project_12345"}, + {"12345", map[string]string{"organization_name": "Acme-Corp", "project_name": "My-Project"}, "rill_acme_corp_my_project_12345"}, + } + + for _, tt := range tests { + t.Run(tt.id, func(t *testing.T) { + result := generateDatabaseName(tt.id, tt.annotations) + require.Equal(t, tt.expected, result) + }) + } +} From 84cd3f1e14fa303f6fef513490c57dad6a257a4e Mon Sep 17 00:00:00 2001 From: Graham Plata Date: Wed, 6 Aug 2025 15:27:26 -0400 Subject: [PATCH 2/6] use string builder --- .../clickhousestatic/provisioner.go | 38 +++++++++------- .../clickhousestatic/provisioner_test.go | 44 ++++++++++++++++--- 2 files changed, 60 insertions(+), 22 deletions(-) diff --git a/admin/provisioner/clickhousestatic/provisioner.go b/admin/provisioner/clickhousestatic/provisioner.go index f48d87897e5..975d304c83c 100644 --- a/admin/provisioner/clickhousestatic/provisioner.go +++ b/admin/provisioner/clickhousestatic/provisioner.go @@ -330,26 +330,30 @@ func generateDatabaseName(resourceID string, annotations map[string]string) stri orgName := sanitizeName(getAnnotationValue(annotations, "organization_name")) projectName := sanitizeName(getAnnotationValue(annotations, "project_name")) - // Limit component lengths to avoid overly long database names - if len(orgName) > 20 { - orgName = orgName[:20] - } - if len(projectName) > 20 { - projectName = projectName[:20] + var builder strings.Builder + builder.WriteString("rill") + + // Build database name based on available components for human readability + if orgName != "" { + builder.WriteString("_") + builder.WriteString(orgName) } - if len(resourceID) > 20 { - resourceID = resourceID[:20] + if projectName != "" { + builder.WriteString("_") + builder.WriteString(projectName) } + builder.WriteString("_") + builder.WriteString(resourceID) - // Build database name based on available components for human readability - if orgName != "" && projectName != "" { - return fmt.Sprintf("rill_%s_%s_%s", orgName, projectName, resourceID) - } else if orgName != "" { - return fmt.Sprintf("rill_%s_%s", orgName, resourceID) - } else if projectName != "" { - return fmt.Sprintf("rill_%s_%s", projectName, resourceID) + dbName := builder.String() + + // Ensure the total length doesn't exceed 63 characters + if len(dbName) > 63 { + dbName = dbName[:63] } - // Fallback to simple naming - return fmt.Sprintf("rill_%s", resourceID) + // Remove any trailing underscores + dbName = strings.TrimRight(dbName, "_") + + return dbName } diff --git a/admin/provisioner/clickhousestatic/provisioner_test.go b/admin/provisioner/clickhousestatic/provisioner_test.go index 88741bde596..8b7521666c8 100644 --- a/admin/provisioner/clickhousestatic/provisioner_test.go +++ b/admin/provisioner/clickhousestatic/provisioner_test.go @@ -430,20 +430,54 @@ func TestSanitizeName(t *testing.T) { func TestGenerateDatabaseName(t *testing.T) { tests := []struct { + name string id string annotations map[string]string expected string }{ - {"12345", nil, "rill_12345"}, - {"12345", map[string]string{"organization_name": "Acme-Corp"}, "rill_acme_corp_12345"}, - {"12345", map[string]string{"project_name": "My-Project"}, "rill_my_project_12345"}, - {"12345", map[string]string{"organization_name": "Acme-Corp", "project_name": "My-Project"}, "rill_acme_corp_my_project_12345"}, + { + name: "with org and project", + id: "77cf2b72_65ab_4bbe_a10e_627bcff4915e", + annotations: map[string]string{"organization_name": "rilldata", "project_name": "dev-project-1"}, + expected: "rill_rilldata_dev_project_1_77cf2b72_65ab_4bbe_a10e_627bcff4915", + }, + { + name: "with org only", + id: "12345", + annotations: map[string]string{"organization_name": "acme-corp"}, + expected: "rill_acme_corp_12345", + }, + { + name: "with project only", + id: "12345", + annotations: map[string]string{"project_name": "my-project"}, + expected: "rill_my_project_12345", + }, + { + name: "no annotations", + id: "12345", + annotations: map[string]string{}, + expected: "rill_12345", + }, + { + name: "nil annotations", + id: "12345", + annotations: nil, + expected: "rill_12345", + }, + { + name: "long name truncated", + id: "very_long_resource_id_that_will_cause_truncation_12345678", + annotations: map[string]string{"organization_name": "very_long_organization_name", "project_name": "very_long_project_name"}, + expected: "rill_very_long_organization_name_very_long_project_name_very_lo", + }, } for _, tt := range tests { - t.Run(tt.id, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { result := generateDatabaseName(tt.id, tt.annotations) require.Equal(t, tt.expected, result) + require.LessOrEqual(t, len(result), 63, "database name should not exceed 63 characters") }) } } From e221af5a8777c28dbf12a1a1f9e5736fae0645f2 Mon Sep 17 00:00:00 2001 From: Graham Plata Date: Wed, 6 Aug 2025 15:36:00 -0400 Subject: [PATCH 3/6] database and user naming in tests --- admin/provisioner/clickhousestatic/provisioner_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/admin/provisioner/clickhousestatic/provisioner_test.go b/admin/provisioner/clickhousestatic/provisioner_test.go index 8b7521666c8..f43775f6e72 100644 --- a/admin/provisioner/clickhousestatic/provisioner_test.go +++ b/admin/provisioner/clickhousestatic/provisioner_test.go @@ -294,9 +294,9 @@ func TestClickHouseStaticHumanReadableNaming(t *testing.T) { opts2, err := clickhouse.ParseDSN(cfg.DSN) require.NoError(t, err) // Check that the database name follows the expected format - expectedID := strings.ReplaceAll(resourceID, "-", "") + expectedID := sanitizeName(resourceID) expectedDBName := fmt.Sprintf("rill_acme_corp_my_project_%s", expectedID) - expectedUser := fmt.Sprintf("rill_%s", expectedID) // User name remains UUID format + expectedUser := fmt.Sprintf("rill_%s", expectedID) // User name uses sanitized format require.Equal(t, expectedDBName, opts2.Auth.Database) require.Equal(t, expectedUser, opts2.Auth.Username) @@ -377,9 +377,9 @@ func TestClickHouseStaticFallbackNaming(t *testing.T) { opts2, err := clickhouse.ParseDSN(cfg.DSN) require.NoError(t, err) // Check that the database name follows the fallback format - expectedID := strings.ReplaceAll(resourceID, "-", "") + expectedID := sanitizeName(resourceID) expectedDBName := fmt.Sprintf("rill_%s", expectedID) - expectedUser := fmt.Sprintf("rill_%s", expectedID) // User name always uses UUID format + expectedUser := fmt.Sprintf("rill_%s", expectedID) // User name uses sanitized format require.Equal(t, expectedDBName, opts2.Auth.Database) require.Equal(t, expectedUser, opts2.Auth.Username) From 5b57275ffa7adf5a01e76fc582267b31ab04535a Mon Sep 17 00:00:00 2001 From: Graham Plata Date: Thu, 7 Aug 2025 09:23:20 -0400 Subject: [PATCH 4/6] revert --- .../clickhousestatic/provisioner.go | 86 +++++++++---------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/admin/provisioner/clickhousestatic/provisioner.go b/admin/provisioner/clickhousestatic/provisioner.go index 975d304c83c..cb0179820ed 100644 --- a/admin/provisioner/clickhousestatic/provisioner.go +++ b/admin/provisioner/clickhousestatic/provisioner.go @@ -130,9 +130,16 @@ func (p *Provisioner) Provision(ctx context.Context, r *provisioner.Resource, op } // Prepare for creating the schema and user. - id := sanitizeName(r.ID) + id := strings.ReplaceAll(r.ID, "-", "") user := fmt.Sprintf("rill_%s", id) - dbName := generateDatabaseName(id, opts.Annotations) + dbName := fmt.Sprintf("rill_%s", id) + + // Use org and project names to create a more human-readable database name. + orgName := sanitizeName(getAnnotationValue(opts.Annotations, "organization_name")) + projectName := sanitizeName(getAnnotationValue(opts.Annotations, "project_name")) + if orgName != "" && projectName != "" { + dbName = fmt.Sprintf("rill_%s_%s_%s", orgName, projectName, id) + } password := newPassword() annotationsJSON, err := json.Marshal(opts.Annotations) @@ -141,13 +148,13 @@ func (p *Provisioner) Provision(ctx context.Context, r *provisioner.Resource, op } // Idempotently create the schema - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE IF NOT EXISTS `%s` %s COMMENT ?", dbName, p.onCluster()), string(annotationsJSON)) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE IF NOT EXISTS %s %s COMMENT ?", dbName, p.onCluster()), string(annotationsJSON)) if err != nil { return nil, fmt.Errorf("failed to create clickhouse database: %w", err) } // Idempotently create the user. - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("CREATE USER IF NOT EXISTS `%s` %s IDENTIFIED WITH sha256_password BY ? DEFAULT DATABASE `%s` GRANTEES NONE", user, p.onCluster(), dbName), password) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("CREATE USER IF NOT EXISTS %s %s IDENTIFIED WITH sha256_password BY ? DEFAULT DATABASE %s GRANTEES NONE", user, p.onCluster(), dbName), password) if err != nil { return nil, fmt.Errorf("failed to create clickhouse user: %w", err) } @@ -155,13 +162,29 @@ func (p *Provisioner) Provision(ctx context.Context, r *provisioner.Resource, op // When creating the user, the password assignment is not idempotent (if there are two concurrent invocations, we don't know which password was used). // By adding the password separately, we ensure all passwords will work. // NOTE: Requires ClickHouse 24.9 or later. - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("ALTER USER `%s` %s ADD IDENTIFIED WITH sha256_password BY ?", user, p.onCluster()), password) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("ALTER USER %s %s ADD IDENTIFIED WITH sha256_password BY ?", user, p.onCluster()), password) if err != nil { return nil, fmt.Errorf("failed to add password for clickhouse user: %w", err) } // Grant privileges on the database to the user - _, err = p.ch.ExecContext(ctx, fmt.Sprintf(`GRANT %s SELECT, INSERT, ALTER, CREATE TABLE, CREATE DICTIONARY, CREATE VIEW, DROP TABLE, DROP DICTIONARY, DROP VIEW, TRUNCATE, OPTIMIZE, SHOW DICTIONARIES, dictGet ON `+"`%s`"+`.* TO `+"`%s`", p.onCluster(), dbName, user)) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf(` + GRANT %s + SELECT, + INSERT, + ALTER, + CREATE TABLE, + CREATE DICTIONARY, + CREATE VIEW, + DROP TABLE, + DROP DICTIONARY, + DROP VIEW, + TRUNCATE, + OPTIMIZE, + SHOW DICTIONARIES, + dictGet + ON %s.* TO %s + `, p.onCluster(), dbName, user)) if err != nil { return nil, fmt.Errorf("failed to grant privileges to clickhouse user: %w", err) } @@ -169,13 +192,23 @@ func (p *Provisioner) Provision(ctx context.Context, r *provisioner.Resource, op // Grant access to system.parts for reporting disk usage. // NOTE 1: ClickHouse automatically adds row filters to restrict result to tables the user has access to. // NOTE 2: We do not need to explicitly grant access to system.tables and system.columns because ClickHouse adds those implicitly. - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("GRANT %s SELECT ON system.parts TO `%s`", p.onCluster(), user)) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("GRANT %s SELECT ON system.parts TO %s", p.onCluster(), user)) if err != nil { return nil, fmt.Errorf("failed to grant system privileges to clickhouse user: %w", err) } // Grant some additional global privileges to the user - _, err = p.ch.ExecContext(ctx, fmt.Sprintf(`GRANT %s URL, REMOTE, MONGO, MYSQL, POSTGRES, S3, AZURE ON *.* TO `+"`%s`", p.onCluster(), user)) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf(` + GRANT %s + URL, + REMOTE, + MONGO, + MYSQL, + POSTGRES, + S3, + AZURE + ON *.* TO %s + `, p.onCluster(), user)) if err != nil { return nil, fmt.Errorf("failed to grant global privileges to clickhouse user: %w", err) } @@ -236,13 +269,13 @@ func (p *Provisioner) Deprovision(ctx context.Context, r *provisioner.Resource) } // Drop the database - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("DROP DATABASE IF EXISTS `%s` %s", opts.Auth.Database, p.onCluster())) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("DROP DATABASE IF EXISTS %s %s", opts.Auth.Database, p.onCluster())) if err != nil { return fmt.Errorf("failed to drop clickhouse database: %w", err) } // Drop the user - _, err = p.ch.ExecContext(ctx, fmt.Sprintf("DROP USER IF EXISTS `%s` %s", opts.Auth.Username, p.onCluster())) + _, err = p.ch.ExecContext(ctx, fmt.Sprintf("DROP USER IF EXISTS %s %s", opts.Auth.Username, p.onCluster())) if err != nil { return fmt.Errorf("failed to drop clickhouse user: %w", err) } @@ -324,36 +357,3 @@ func sanitizeName(name string) string { return strings.ToLower(result.String()) } - -// generateDatabaseName creates a predictable, length-controlled database name -func generateDatabaseName(resourceID string, annotations map[string]string) string { - orgName := sanitizeName(getAnnotationValue(annotations, "organization_name")) - projectName := sanitizeName(getAnnotationValue(annotations, "project_name")) - - var builder strings.Builder - builder.WriteString("rill") - - // Build database name based on available components for human readability - if orgName != "" { - builder.WriteString("_") - builder.WriteString(orgName) - } - if projectName != "" { - builder.WriteString("_") - builder.WriteString(projectName) - } - builder.WriteString("_") - builder.WriteString(resourceID) - - dbName := builder.String() - - // Ensure the total length doesn't exceed 63 characters - if len(dbName) > 63 { - dbName = dbName[:63] - } - - // Remove any trailing underscores - dbName = strings.TrimRight(dbName, "_") - - return dbName -} From dca1f7b0682f3692be7e29d06fdf50819fef8b58 Mon Sep 17 00:00:00 2001 From: Graham Plata Date: Thu, 7 Aug 2025 09:27:58 -0400 Subject: [PATCH 5/6] review --- .../clickhousestatic/provisioner.go | 51 +++++++------------ .../clickhousestatic/provisioner_test.go | 42 ++++++++------- 2 files changed, 40 insertions(+), 53 deletions(-) diff --git a/admin/provisioner/clickhousestatic/provisioner.go b/admin/provisioner/clickhousestatic/provisioner.go index cb0179820ed..f0b195bae13 100644 --- a/admin/provisioner/clickhousestatic/provisioner.go +++ b/admin/provisioner/clickhousestatic/provisioner.go @@ -9,6 +9,7 @@ import ( "io" "net/url" "os" + "regexp" "strings" "github.com/ClickHouse/clickhouse-go/v2" @@ -17,6 +18,8 @@ import ( "go.uber.org/zap" ) +var nonAlphanumericRegexp = regexp.MustCompile(`[^a-zA-Z0-9]+`) + func init() { provisioner.Register("clickhouse-static", New) } @@ -130,16 +133,8 @@ func (p *Provisioner) Provision(ctx context.Context, r *provisioner.Resource, op } // Prepare for creating the schema and user. - id := strings.ReplaceAll(r.ID, "-", "") - user := fmt.Sprintf("rill_%s", id) - dbName := fmt.Sprintf("rill_%s", id) - - // Use org and project names to create a more human-readable database name. - orgName := sanitizeName(getAnnotationValue(opts.Annotations, "organization_name")) - projectName := sanitizeName(getAnnotationValue(opts.Annotations, "project_name")) - if orgName != "" && projectName != "" { - dbName = fmt.Sprintf("rill_%s_%s_%s", orgName, projectName, id) - } + user := fmt.Sprintf("rill_%s", nonAlphanumericRegexp.ReplaceAllString(r.ID, "")) + dbName := generateDatabaseName(r.ID, opts.Annotations) password := newPassword() annotationsJSON, err := json.Marshal(opts.Annotations) @@ -329,31 +324,19 @@ func newPassword() string { return fmt.Sprintf("1Rr!%x", b[:]) } -// Helper function to get annotation value -func getAnnotationValue(annotations map[string]string, key string) string { - if annotations == nil { - return "" +func generateDatabaseName(resourceID string, annotations map[string]string) string { + name := "rill" + if org, ok := annotations["organization_name"]; ok { + name += "_" + nonAlphanumericRegexp.ReplaceAllString(org, "") } - return annotations[key] -} - -// Helper function to sanitize names for ClickHouse identifiers -func sanitizeName(name string) string { - if name == "" { - return "" + if proj, ok := annotations["project_name"]; ok { + name += "_" + nonAlphanumericRegexp.ReplaceAllString(proj, "") } - - // Replace invalid characters with underscores - name = strings.ReplaceAll(name, "-", "_") - name = strings.ReplaceAll(name, " ", "_") - - // Remove any characters that aren't alphanumeric or underscore - var result strings.Builder - for _, r := range name { - if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '_' { - result.WriteRune(r) - } + name += "_" + nonAlphanumericRegexp.ReplaceAllString(resourceID, "") + // Optionally, trim to 63 chars and remove trailing underscores if needed + if len(name) > 63 { + name = name[:63] } - - return strings.ToLower(result.String()) + name = strings.TrimRight(name, "_") + return name } diff --git a/admin/provisioner/clickhousestatic/provisioner_test.go b/admin/provisioner/clickhousestatic/provisioner_test.go index f43775f6e72..f9cd1c0408a 100644 --- a/admin/provisioner/clickhousestatic/provisioner_test.go +++ b/admin/provisioner/clickhousestatic/provisioner_test.go @@ -294,9 +294,13 @@ func TestClickHouseStaticHumanReadableNaming(t *testing.T) { opts2, err := clickhouse.ParseDSN(cfg.DSN) require.NoError(t, err) // Check that the database name follows the expected format - expectedID := sanitizeName(resourceID) - expectedDBName := fmt.Sprintf("rill_acme_corp_my_project_%s", expectedID) - expectedUser := fmt.Sprintf("rill_%s", expectedID) // User name uses sanitized format + expectedDBName := fmt.Sprintf( + "rill_%s_%s_%s", + nonAlphanumericRegexp.ReplaceAllString(opts.Annotations["organization_name"], ""), + nonAlphanumericRegexp.ReplaceAllString(opts.Annotations["project_name"], ""), + nonAlphanumericRegexp.ReplaceAllString(resourceID, ""), + ) + expectedUser := fmt.Sprintf("rill_%s", nonAlphanumericRegexp.ReplaceAllString(resourceID, "")) require.Equal(t, expectedDBName, opts2.Auth.Database) require.Equal(t, expectedUser, opts2.Auth.Username) @@ -377,9 +381,8 @@ func TestClickHouseStaticFallbackNaming(t *testing.T) { opts2, err := clickhouse.ParseDSN(cfg.DSN) require.NoError(t, err) // Check that the database name follows the fallback format - expectedID := sanitizeName(resourceID) - expectedDBName := fmt.Sprintf("rill_%s", expectedID) - expectedUser := fmt.Sprintf("rill_%s", expectedID) // User name uses sanitized format + expectedDBName := fmt.Sprintf("rill_%s", nonAlphanumericRegexp.ReplaceAllString(resourceID, "")) + expectedUser := fmt.Sprintf("rill_%s", nonAlphanumericRegexp.ReplaceAllString(resourceID, "")) require.Equal(t, expectedDBName, opts2.Auth.Database) require.Equal(t, expectedUser, opts2.Auth.Username) @@ -408,21 +411,22 @@ func TestSanitizeName(t *testing.T) { }{ {"", ""}, {"simple", "simple"}, - {"Simple", "simple"}, - {"UPPERCASE", "uppercase"}, - {"with-dashes", "with_dashes"}, - {"with spaces", "with_spaces"}, + {"Simple", "Simple"}, + {"UPPERCASE", "UPPERCASE"}, + {"with-dashes", "withdashes"}, + {"with spaces", "withspaces"}, {"with@special!chars", "withspecialchars"}, - {"mixed-Case_Name", "mixed_case_name"}, + {"mixed-Case_Name", "mixedCaseName"}, {"123numbers", "123numbers"}, - {"_underscore_", "_underscore_"}, - {"Acme-Corp", "acme_corp"}, - {"My-Project", "my_project"}, + {"_underscore_", "underscore"}, + {"Acme-Corp", "AcmeCorp"}, + {"My-Project", "MyProject"}, + {"name_with_special_characters_1234567890!@#$%^&*()_+{}|:\"<>?[];',./`~", "namewithspecialcharacters1234567890"}, } for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { - result := sanitizeName(tt.input) + result := nonAlphanumericRegexp.ReplaceAllString(tt.input, "") require.Equal(t, tt.expected, result) }) } @@ -439,19 +443,19 @@ func TestGenerateDatabaseName(t *testing.T) { name: "with org and project", id: "77cf2b72_65ab_4bbe_a10e_627bcff4915e", annotations: map[string]string{"organization_name": "rilldata", "project_name": "dev-project-1"}, - expected: "rill_rilldata_dev_project_1_77cf2b72_65ab_4bbe_a10e_627bcff4915", + expected: "rill_rilldata_devproject1_77cf2b7265ab4bbea10e627bcff4915e", }, { name: "with org only", id: "12345", annotations: map[string]string{"organization_name": "acme-corp"}, - expected: "rill_acme_corp_12345", + expected: "rill_acmecorp_12345", }, { name: "with project only", id: "12345", annotations: map[string]string{"project_name": "my-project"}, - expected: "rill_my_project_12345", + expected: "rill_myproject_12345", }, { name: "no annotations", @@ -469,7 +473,7 @@ func TestGenerateDatabaseName(t *testing.T) { name: "long name truncated", id: "very_long_resource_id_that_will_cause_truncation_12345678", annotations: map[string]string{"organization_name": "very_long_organization_name", "project_name": "very_long_project_name"}, - expected: "rill_very_long_organization_name_very_long_project_name_very_lo", + expected: "rill_verylongorganizationname_verylongprojectname_verylongresou", }, } From 0fbbd537d45da90028a10e30dc5bb8a120142a3f Mon Sep 17 00:00:00 2001 From: Graham Plata Date: Thu, 7 Aug 2025 10:07:29 -0400 Subject: [PATCH 6/6] nits --- .../clickhousestatic/provisioner.go | 2 +- .../clickhousestatic/provisioner_test.go | 37 +------------------ 2 files changed, 3 insertions(+), 36 deletions(-) diff --git a/admin/provisioner/clickhousestatic/provisioner.go b/admin/provisioner/clickhousestatic/provisioner.go index f0b195bae13..34ae0b28fc1 100644 --- a/admin/provisioner/clickhousestatic/provisioner.go +++ b/admin/provisioner/clickhousestatic/provisioner.go @@ -338,5 +338,5 @@ func generateDatabaseName(resourceID string, annotations map[string]string) stri name = name[:63] } name = strings.TrimRight(name, "_") - return name + return strings.ToLower(name) } diff --git a/admin/provisioner/clickhousestatic/provisioner_test.go b/admin/provisioner/clickhousestatic/provisioner_test.go index f9cd1c0408a..729c0cdad52 100644 --- a/admin/provisioner/clickhousestatic/provisioner_test.go +++ b/admin/provisioner/clickhousestatic/provisioner_test.go @@ -294,13 +294,8 @@ func TestClickHouseStaticHumanReadableNaming(t *testing.T) { opts2, err := clickhouse.ParseDSN(cfg.DSN) require.NoError(t, err) // Check that the database name follows the expected format - expectedDBName := fmt.Sprintf( - "rill_%s_%s_%s", - nonAlphanumericRegexp.ReplaceAllString(opts.Annotations["organization_name"], ""), - nonAlphanumericRegexp.ReplaceAllString(opts.Annotations["project_name"], ""), - nonAlphanumericRegexp.ReplaceAllString(resourceID, ""), - ) expectedUser := fmt.Sprintf("rill_%s", nonAlphanumericRegexp.ReplaceAllString(resourceID, "")) + expectedDBName := generateDatabaseName(resourceID, opts.Annotations) require.Equal(t, expectedDBName, opts2.Auth.Database) require.Equal(t, expectedUser, opts2.Auth.Username) @@ -381,8 +376,8 @@ func TestClickHouseStaticFallbackNaming(t *testing.T) { opts2, err := clickhouse.ParseDSN(cfg.DSN) require.NoError(t, err) // Check that the database name follows the fallback format - expectedDBName := fmt.Sprintf("rill_%s", nonAlphanumericRegexp.ReplaceAllString(resourceID, "")) expectedUser := fmt.Sprintf("rill_%s", nonAlphanumericRegexp.ReplaceAllString(resourceID, "")) + expectedDBName := generateDatabaseName(resourceID, opts.Annotations) require.Equal(t, expectedDBName, opts2.Auth.Database) require.Equal(t, expectedUser, opts2.Auth.Username) @@ -404,34 +399,6 @@ func TestClickHouseStaticFallbackNaming(t *testing.T) { require.NoError(t, err) } -func TestSanitizeName(t *testing.T) { - tests := []struct { - input string - expected string - }{ - {"", ""}, - {"simple", "simple"}, - {"Simple", "Simple"}, - {"UPPERCASE", "UPPERCASE"}, - {"with-dashes", "withdashes"}, - {"with spaces", "withspaces"}, - {"with@special!chars", "withspecialchars"}, - {"mixed-Case_Name", "mixedCaseName"}, - {"123numbers", "123numbers"}, - {"_underscore_", "underscore"}, - {"Acme-Corp", "AcmeCorp"}, - {"My-Project", "MyProject"}, - {"name_with_special_characters_1234567890!@#$%^&*()_+{}|:\"<>?[];',./`~", "namewithspecialcharacters1234567890"}, - } - - for _, tt := range tests { - t.Run(tt.input, func(t *testing.T) { - result := nonAlphanumericRegexp.ReplaceAllString(tt.input, "") - require.Equal(t, tt.expected, result) - }) - } -} - func TestGenerateDatabaseName(t *testing.T) { tests := []struct { name string