From 6097459130065b992c9c438ea73639eabdeae789 Mon Sep 17 00:00:00 2001 From: "lukasz.widera@vshn.ch" Date: Tue, 10 Sep 2024 13:08:28 +0200 Subject: [PATCH 1/4] draft: adding webhook check to detect forbidden keys in VSHNPostgreSQL config --- pkg/controller/webhooks/postgresql.go | 155 ++++++++++++++++++++++++++ 1 file changed, 155 insertions(+) diff --git a/pkg/controller/webhooks/postgresql.go b/pkg/controller/webhooks/postgresql.go index b4f677d9bc..fc86b6da2f 100644 --- a/pkg/controller/webhooks/postgresql.go +++ b/pkg/controller/webhooks/postgresql.go @@ -2,6 +2,7 @@ package webhooks import ( "context" + "encoding/json" "fmt" vshnv1 "github.com/vshn/appcat/v4/apis/vshn/v1" @@ -105,6 +106,11 @@ func (p *PostgreSQLWebhookHandler) ValidateCreate(ctx context.Context, obj runti }) } + errList := validatePgConf(pg) + if errList != nil { + allErrs = append(allErrs, errList...) + } + if len(allErrs) != 0 { return nil, apierrors.NewInvalid( pgGK, @@ -167,6 +173,11 @@ func (p *PostgreSQLWebhookHandler) ValidateUpdate(ctx context.Context, oldObj, n }) } + errList := validatePgConf(pg) + if errList != nil { + allErrs = append(allErrs, errList...) + } + // We aggregate and return all errors at the same time. // So the user is aware of all broken parameters. // But at the same time, if any of these fail we cannot do proper quota checks anymore. @@ -287,3 +298,147 @@ func validateVacuumRepack(vacuum, repack bool) error { } return nil } + +func validatePgConf(pg *vshnv1.VSHNPostgreSQL) (fErros field.ErrorList) { + + pgConfBytes := pg.Spec.Parameters.Service.PostgreSQLSettings + + pgConf := map[string]string{} + if pgConfBytes.Raw != nil { + err := json.Unmarshal(pgConfBytes.Raw, &pgConf) + if err != nil { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings", + Detail: fmt.Sprintf("Error parsing pgConf: %s", err.Error()), + Type: field.ErrorTypeInvalid, + BadValue: pgConfBytes, + }) + return fErros + } + } + + for key := range pgConf { + if key == "listen_addresses" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[listen_addresses]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "port" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[port]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "cluster_name" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[cluster_name]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "hot_standby" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[hot_standby]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "fsync" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[fsync]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "full_page_writes" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[full_page_writes]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "log_destination" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[log_destination]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "logging_collector" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[logging_collector]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "max_replication_slots" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[max_replication_slots]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "max_wal_senders" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[max_wal_senders]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "wal_keep_segments" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[wal_keep_segments]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "wal_level" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[wal_level]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "wal_log_hints" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[wal_log_hints]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "archive_mode" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[archive_mode]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + if key == "archive_command" { + fErros = append(fErros, &field.Error{ + Field: "spec.parameters.service.postgresqlSettings[archive_command]", + Type: field.ErrorTypeForbidden, + BadValue: key, + Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", + }) + } + return fErros + } + return nil +} From 5db61cb43445fcaab74855aa9505e714c8fb2f66 Mon Sep 17 00:00:00 2001 From: "lukasz.widera@vshn.ch" Date: Tue, 10 Sep 2024 17:20:27 +0200 Subject: [PATCH 2/4] adding webhook to validate PgSettings --- pkg/controller/webhooks/postgresql.go | 5 +- pkg/controller/webhooks/postgresql_test.go | 98 ++++++++++++++++++++++ 2 files changed, 100 insertions(+), 3 deletions(-) diff --git a/pkg/controller/webhooks/postgresql.go b/pkg/controller/webhooks/postgresql.go index fc86b6da2f..9127cf6f8a 100644 --- a/pkg/controller/webhooks/postgresql.go +++ b/pkg/controller/webhooks/postgresql.go @@ -317,7 +317,7 @@ func validatePgConf(pg *vshnv1.VSHNPostgreSQL) (fErros field.ErrorList) { } } - for key := range pgConf { + for key, _ := range pgConf { if key == "listen_addresses" { fErros = append(fErros, &field.Error{ Field: "spec.parameters.service.postgresqlSettings[listen_addresses]", @@ -438,7 +438,6 @@ func validatePgConf(pg *vshnv1.VSHNPostgreSQL) (fErros field.ErrorList) { Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", }) } - return fErros } - return nil + return fErros } diff --git a/pkg/controller/webhooks/postgresql_test.go b/pkg/controller/webhooks/postgresql_test.go index d329243e85..5602a3f3a9 100644 --- a/pkg/controller/webhooks/postgresql_test.go +++ b/pkg/controller/webhooks/postgresql_test.go @@ -12,6 +12,7 @@ import ( "github.com/vshn/appcat/v4/pkg/common/utils" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -231,6 +232,103 @@ func TestPostgreSQLWebhookHandler_ValidateCreate(t *testing.T) { pgInvalid.Spec.Parameters.Service.ServiceLevel = "guaranteed" _, err = handler.ValidateCreate(ctx, pgInvalid) assert.Error(t, err) + + // check pgSettings + pgValid := pgOrig.DeepCopy() + pgValid.Spec.Parameters.Service.PostgreSQLSettings = runtime.RawExtension{ + Raw: []byte(`{"foo": "bar"}`), + } + _, err = handler.ValidateCreate(ctx, pgValid) + assert.NoError(t, err) + + // check pgSettings + pgInvalid = pgOrig.DeepCopy() + pgInvalid.Spec.Parameters.Service.PostgreSQLSettings = runtime.RawExtension{ + Raw: []byte(`{"fsync": "bar"}`), + } + _, err = handler.ValidateCreate(ctx, pgInvalid) + assert.Error(t, err) + + // check pgSettings + pgInvalid = pgOrig.DeepCopy() + pgInvalid.Spec.Parameters.Service.PostgreSQLSettings = runtime.RawExtension{ + Raw: []byte(`{"fsync": "bar", "wal_level": "foo", "max_connections": "bar"}`), + } + + _, err = handler.ValidateCreate(ctx, pgInvalid) + assert.Error(t, err) +} + +func TestPostgreSQLWebhookHandler_ValidateUpdate(t *testing.T) { + ctx := context.TODO() + claimNS := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "claimns", + Labels: map[string]string{ + utils.OrgLabelName: "myorg", + }, + }, + } + fclient := fake.NewClientBuilder(). + WithScheme(pkg.SetupScheme()). + WithObjects(claimNS). + Build() + + viper.Set("PLANS_NAMESPACE", "testns") + viper.AutomaticEnv() + + handler := PostgreSQLWebhookHandler{ + DefaultWebhookHandler: DefaultWebhookHandler{ + client: fclient, + log: logr.Discard(), + withQuota: false, + obj: &vshnv1.VSHNPostgreSQL{}, + name: "postgresql", + }, + } + pgOrig := &vshnv1.VSHNPostgreSQL{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myinstance", + Namespace: "claimns", + }, + Spec: vshnv1.VSHNPostgreSQLSpec{ + Parameters: vshnv1.VSHNPostgreSQLParameters{ + Size: vshnv1.VSHNSizeSpec{ + CPU: "500m", + Memory: "1Gi", + }, + Instances: 1, + Service: vshnv1.VSHNPostgreSQLServiceSpec{ + RepackEnabled: true, + }, + }, + }, + } + + // check pgSettings with single good setting + pgValid := pgOrig.DeepCopy() + pgValid.Spec.Parameters.Service.PostgreSQLSettings = runtime.RawExtension{ + Raw: []byte(`{"foo": "bar"}`), + } + _, err := handler.ValidateUpdate(ctx, pgOrig, pgValid) + assert.NoError(t, err) + + // check pgSettings with single bad setting + pgInvalid := pgOrig.DeepCopy() + pgInvalid.Spec.Parameters.Service.PostgreSQLSettings = runtime.RawExtension{ + Raw: []byte(`{"fsync": "bar"}`), + } + _, err = handler.ValidateUpdate(ctx, pgOrig, pgInvalid) + assert.Error(t, err) + + // check pgSettings, startiong with valid settings + pgInvalid = pgOrig.DeepCopy() + pgInvalid.Spec.Parameters.Service.PostgreSQLSettings = runtime.RawExtension{ + Raw: []byte(`{"foo": "bar", "fsync": "bar", "wal_level": "foo", "max_connections": "bar"}`), + } + + _, err = handler.ValidateUpdate(ctx, pgOrig, pgInvalid) + assert.Error(t, err) } func TestPostgreSQLWebhookHandler_ValidateDelete(t *testing.T) { From fa730c52375d4ad229809ecec73eb9ad41b9e385 Mon Sep 17 00:00:00 2001 From: "lukasz.widera@vshn.ch" Date: Thu, 12 Sep 2024 12:09:49 +0200 Subject: [PATCH 3/4] switching from If to map --- pkg/controller/webhooks/postgresql.go | 136 ++++---------------------- 1 file changed, 21 insertions(+), 115 deletions(-) diff --git a/pkg/controller/webhooks/postgresql.go b/pkg/controller/webhooks/postgresql.go index 9127cf6f8a..ffde9d7862 100644 --- a/pkg/controller/webhooks/postgresql.go +++ b/pkg/controller/webhooks/postgresql.go @@ -317,122 +317,28 @@ func validatePgConf(pg *vshnv1.VSHNPostgreSQL) (fErros field.ErrorList) { } } - for key, _ := range pgConf { - if key == "listen_addresses" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[listen_addresses]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "port" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[port]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "cluster_name" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[cluster_name]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "hot_standby" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[hot_standby]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "fsync" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[fsync]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "full_page_writes" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[full_page_writes]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "log_destination" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[log_destination]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "logging_collector" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[logging_collector]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "max_replication_slots" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[max_replication_slots]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "max_wal_senders" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[max_wal_senders]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "wal_keep_segments" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[wal_keep_segments]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "wal_level" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[wal_level]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "wal_log_hints" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[wal_log_hints]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "archive_mode" { - fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[archive_mode]", - Type: field.ErrorTypeForbidden, - BadValue: key, - Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", - }) - } - if key == "archive_command" { + blocklist := map[string]string{ + "listen_addresses": "", + "port": "", + "cluster_name": "", + "hot_standby": "", + "fsync": "", + "full_page_writes": "", + "log_destination": "", + "logging_collector": "", + "max_replication_slots": "", + "max_wal_senders": "", + "wal_keep_segments": "", + "wal_level": "", + "wal_log_hints": "", + "archive_mode": "", + "archive_command": "", + } + + for key := range pgConf { + if _, ok := blocklist[key]; ok { fErros = append(fErros, &field.Error{ - Field: "spec.parameters.service.postgresqlSettings[archive_command]", + Field: fmt.Sprintf("spec.parameters.service.postgresqlSettings[%s]", key), Type: field.ErrorTypeForbidden, BadValue: key, Detail: "https://stackgres.io/doc/latest/api/responses/error/#postgres-blocklist", From b6031146c4dc2bcd549697789493573574f113c8 Mon Sep 17 00:00:00 2001 From: "lukasz.widera@vshn.ch" Date: Thu, 12 Sep 2024 13:36:19 +0200 Subject: [PATCH 4/4] moving variable to be global --- pkg/controller/webhooks/postgresql.go | 36 +++++++++++++-------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/pkg/controller/webhooks/postgresql.go b/pkg/controller/webhooks/postgresql.go index ffde9d7862..21c50f4f92 100644 --- a/pkg/controller/webhooks/postgresql.go +++ b/pkg/controller/webhooks/postgresql.go @@ -35,6 +35,24 @@ var ( var _ webhook.CustomValidator = &PostgreSQLWebhookHandler{} +var blocklist = map[string]string{ + "listen_addresses": "", + "port": "", + "cluster_name": "", + "hot_standby": "", + "fsync": "", + "full_page_writes": "", + "log_destination": "", + "logging_collector": "", + "max_replication_slots": "", + "max_wal_senders": "", + "wal_keep_segments": "", + "wal_level": "", + "wal_log_hints": "", + "archive_mode": "", + "archive_command": "", +} + // PostgreSQLWebhookHandler handles all quota webhooks concerning postgresql by vshn. type PostgreSQLWebhookHandler struct { DefaultWebhookHandler @@ -317,24 +335,6 @@ func validatePgConf(pg *vshnv1.VSHNPostgreSQL) (fErros field.ErrorList) { } } - blocklist := map[string]string{ - "listen_addresses": "", - "port": "", - "cluster_name": "", - "hot_standby": "", - "fsync": "", - "full_page_writes": "", - "log_destination": "", - "logging_collector": "", - "max_replication_slots": "", - "max_wal_senders": "", - "wal_keep_segments": "", - "wal_level": "", - "wal_log_hints": "", - "archive_mode": "", - "archive_command": "", - } - for key := range pgConf { if _, ok := blocklist[key]; ok { fErros = append(fErros, &field.Error{