From f9724b4fa59111f99fabb463369f134b31504682 Mon Sep 17 00:00:00 2001 From: Anna Vitova Date: Tue, 17 Oct 2023 15:35:48 +0200 Subject: [PATCH] fix(HMS-2757): Do not cascade on pubkey delete --- cmd/spec/example_reservation.go | 12 ++++++------ internal/dao/dao_errors.go | 3 +++ internal/dao/pgx/reservation_pgx.go | 14 +++++++++++++- internal/dao/tests/reservation_test.go | 5 +++-- internal/jobs/launch_instance_aws_test.go | 2 +- internal/jobs/launch_instance_azure_test.go | 2 +- internal/jobs/launch_instance_gcp_test.go | 2 +- .../migrations/sql/021_remove_delete_cascade.sql | 14 ++++++++++++++ internal/migrations/sql/022_pubkey_nullable.sql | 3 +++ internal/models/reservation_model.go | 6 +++--- internal/payloads/reservation_payload.go | 6 +++--- internal/services/aws_reservation_service.go | 10 +++++++--- internal/services/azure_reservation_service.go | 2 +- internal/services/gcp_reservation_service.go | 14 +++++++++----- internal/services/reservations_service.go | 1 + internal/services/reservations_service_test.go | 2 +- 16 files changed, 70 insertions(+), 28 deletions(-) create mode 100644 internal/migrations/sql/021_remove_delete_cascade.sql create mode 100644 internal/migrations/sql/022_pubkey_nullable.sql diff --git a/cmd/spec/example_reservation.go b/cmd/spec/example_reservation.go index 11e97a60..339fe685 100644 --- a/cmd/spec/example_reservation.go +++ b/cmd/spec/example_reservation.go @@ -86,7 +86,7 @@ var AwsReservationRequestPayloadExample = payloads.AWSReservationRequest{ } var AwsReservationResponsePayloadPendingExample = payloads.AWSReservationResponse{ - PubkeyID: 42, + PubkeyID: ptr.ToInt64(42), SourceID: "654321", Region: "us-east-1", InstanceType: "t3.small", @@ -99,7 +99,7 @@ var AwsReservationResponsePayloadPendingExample = payloads.AWSReservationRespons var AwsReservationResponsePayloadDoneExample = payloads.AWSReservationResponse{ ID: 1305, - PubkeyID: 42, + PubkeyID: ptr.ToInt64(42), SourceID: "654321", Region: "us-east-1", InstanceType: "t3.small", @@ -133,7 +133,7 @@ var AzureReservationRequestPayloadExample = payloads.AzureReservationRequest{ var AzureReservationResponsePayloadPendingExample = payloads.AzureReservationResponse{ ID: 1310, - PubkeyID: 42, + PubkeyID: ptr.ToInt64(42), SourceID: "654321", ResourceGroup: "myCustom Azure RG", Location: "useast", @@ -147,7 +147,7 @@ var AzureReservationResponsePayloadPendingExample = payloads.AzureReservationRes var AzureReservationResponsePayloadDoneExample = payloads.AzureReservationResponse{ ID: 1310, - PubkeyID: 42, + PubkeyID: ptr.ToInt64(42), SourceID: "654321", ResourceGroup: "myCustom Azure RG", Location: "useast", @@ -179,7 +179,7 @@ var GCPReservationRequestPayloadExample = payloads.GCPReservationRequest{ var GCPReservationResponsePayloadPendingExample = payloads.GCPReservationResponse{ ID: 1305, - PubkeyID: 42, + PubkeyID: ptr.ToInt64(42), SourceID: "654321", Zone: "us-east-4", MachineType: "e2-micro", @@ -193,7 +193,7 @@ var GCPReservationResponsePayloadPendingExample = payloads.GCPReservationRespons var GCPReservationResponsePayloadDoneExample = payloads.GCPReservationResponse{ ID: 1305, - PubkeyID: 42, + PubkeyID: ptr.ToInt64(42), SourceID: "654321", Zone: "us-east-4", MachineType: "e2-micro", diff --git a/internal/dao/dao_errors.go b/internal/dao/dao_errors.go index 64439df7..fee24b1f 100644 --- a/internal/dao/dao_errors.go +++ b/internal/dao/dao_errors.go @@ -38,4 +38,7 @@ var ( // ErrReservationRateExceeded is returned when SQL constraint does not allow to insert more reservations ErrReservationRateExceeded = usrerr.New(429, "rate limit exceeded", "too many reservations, wait and retry") + + // ErrPubkeyNotFound is returned when a nil pointer to a pubkey is used for reservation detail + ErrPubkeyNotFound = usrerr.New(404, "pubkey not found", "no pubkey found, it may have been already deleted") ) diff --git a/internal/dao/pgx/reservation_pgx.go b/internal/dao/pgx/reservation_pgx.go index 0e581acc..0c7c9815 100644 --- a/internal/dao/pgx/reservation_pgx.go +++ b/internal/dao/pgx/reservation_pgx.go @@ -49,11 +49,15 @@ func (x *reservationDao) CreateAWS(ctx context.Context, reservation *models.AWSR return err } + if reservation.PubkeyID == nil { + return fmt.Errorf("pgx error: %w", dao.ErrPubkeyNotFound) + } + awsQuery := `INSERT INTO aws_reservation_details (reservation_id, pubkey_id, source_id, image_id, detail) VALUES ($1, $2, $3, $4, $5)` tag, err := tx.Exec(ctx, awsQuery, reservation.ID, - reservation.PubkeyID, + &reservation.PubkeyID, reservation.SourceID, reservation.ImageID, reservation.Detail) @@ -80,6 +84,10 @@ func (x *reservationDao) CreateAzure(ctx context.Context, reservation *models.Az return err } + if reservation.PubkeyID == nil { + return fmt.Errorf("pgx error: %w", dao.ErrPubkeyNotFound) + } + azureQuery := `INSERT INTO azure_reservation_details (reservation_id, pubkey_id, source_id, image_id, detail) VALUES ($1, $2, $3, $4, $5)` tag, err := tx.Exec(ctx, azureQuery, @@ -111,6 +119,10 @@ func (x *reservationDao) CreateGCP(ctx context.Context, reservation *models.GCPR return err } + if reservation.PubkeyID == nil { + return fmt.Errorf("pgx error: %w", dao.ErrPubkeyNotFound) + } + gcpQuery := `INSERT INTO gcp_reservation_details (reservation_id, pubkey_id, source_id, image_id, detail) VALUES ($1, $2, $3, $4, $5)` tag, err := tx.Exec(ctx, gcpQuery, diff --git a/internal/dao/tests/reservation_test.go b/internal/dao/tests/reservation_test.go index e946ae8b..8467800e 100644 --- a/internal/dao/tests/reservation_test.go +++ b/internal/dao/tests/reservation_test.go @@ -12,6 +12,7 @@ import ( "github.com/RHEnVision/provisioning-backend/internal/dao" "github.com/RHEnVision/provisioning-backend/internal/db" "github.com/RHEnVision/provisioning-backend/internal/models" + "github.com/RHEnVision/provisioning-backend/internal/ptr" "github.com/RHEnVision/provisioning-backend/internal/testing/identity" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -47,7 +48,7 @@ func newAWSReservation() *models.AWSReservation { AccountID: 1, Status: "Created", }, - PubkeyID: 1, + PubkeyID: ptr.ToInt64(1), } } @@ -58,7 +59,7 @@ func newGCPReservation() *models.GCPReservation { AccountID: 1, Status: "Created", }, - PubkeyID: 1, + PubkeyID: ptr.ToInt64(1), } } diff --git a/internal/jobs/launch_instance_aws_test.go b/internal/jobs/launch_instance_aws_test.go index b5fa32ce..fd6926ef 100644 --- a/internal/jobs/launch_instance_aws_test.go +++ b/internal/jobs/launch_instance_aws_test.go @@ -39,7 +39,7 @@ func prepareAWSReservation(t *testing.T, ctx context.Context, pk *models.Pubkey) PowerOff: false, } reservation := &models.AWSReservation{ - PubkeyID: pk.ID, + PubkeyID: &pk.ID, SourceID: "irrelevant", ImageID: "irrelevant", Detail: detail, diff --git a/internal/jobs/launch_instance_azure_test.go b/internal/jobs/launch_instance_azure_test.go index b77ab349..869606be 100644 --- a/internal/jobs/launch_instance_azure_test.go +++ b/internal/jobs/launch_instance_azure_test.go @@ -38,7 +38,7 @@ func prepareAzureReservation(t *testing.T, ctx context.Context, pk *models.Pubke PowerOff: false, } reservation := &models.AzureReservation{ - PubkeyID: pk.ID, + PubkeyID: &pk.ID, SourceID: "irrelevant", ImageID: "irrelevant", Detail: detail, diff --git a/internal/jobs/launch_instance_gcp_test.go b/internal/jobs/launch_instance_gcp_test.go index 62c52ddd..2ffcdbc9 100644 --- a/internal/jobs/launch_instance_gcp_test.go +++ b/internal/jobs/launch_instance_gcp_test.go @@ -38,7 +38,7 @@ func prepareGCPReservation(t *testing.T, ctx context.Context, pk *models.Pubkey) PowerOff: false, } reservation := &models.GCPReservation{ - PubkeyID: pk.ID, + PubkeyID: &pk.ID, SourceID: "irrelevant", ImageID: "irrelevant", Detail: detail, diff --git a/internal/migrations/sql/021_remove_delete_cascade.sql b/internal/migrations/sql/021_remove_delete_cascade.sql new file mode 100644 index 00000000..9dba3f4d --- /dev/null +++ b/internal/migrations/sql/021_remove_delete_cascade.sql @@ -0,0 +1,14 @@ +ALTER TABLE aws_reservation_details +DROP CONSTRAINT aws_reservation_details_pubkey_id_fkey, +ADD CONSTRAINT aws_reservation_details_pubkey_id_fkey +FOREIGN key (pubkey_id) REFERENCES pubkeys(id) ON DELETE SET NULL; + +ALTER TABLE gcp_reservation_details +DROP CONSTRAINT gcp_reservation_details_pubkey_id_fkey, +ADD CONSTRAINT gcp_reservation_details_pubkey_id_fkey +FOREIGN key (pubkey_id) REFERENCES pubkeys(id) ON DELETE SET NULL; + +ALTER TABLE azure_reservation_details +DROP CONSTRAINT azure_reservation_details_pubkey_id_fkey, +ADD CONSTRAINT azure_reservation_details_pubkey_id_fkey +FOREIGN key (pubkey_id) REFERENCES pubkeys(id) ON DELETE SET NULL; diff --git a/internal/migrations/sql/022_pubkey_nullable.sql b/internal/migrations/sql/022_pubkey_nullable.sql new file mode 100644 index 00000000..601404d6 --- /dev/null +++ b/internal/migrations/sql/022_pubkey_nullable.sql @@ -0,0 +1,3 @@ +ALTER TABLE aws_reservation_details ALTER COLUMN pubkey_id DROP NOT NULL; +ALTER TABLE gcp_reservation_details ALTER COLUMN pubkey_id DROP NOT NULL; +ALTER TABLE azure_reservation_details ALTER COLUMN pubkey_id DROP NOT NULL; diff --git a/internal/models/reservation_model.go b/internal/models/reservation_model.go index 2759e62e..407f405a 100644 --- a/internal/models/reservation_model.go +++ b/internal/models/reservation_model.go @@ -76,7 +76,7 @@ type AWSReservation struct { Reservation // Pubkey ID. - PubkeyID int64 `db:"pubkey_id" json:"pubkey_id"` + PubkeyID *int64 `db:"pubkey_id" json:"pubkey_id,omitempty"` // Source ID. SourceID string `db:"source_id" json:"source_id"` @@ -118,7 +118,7 @@ type GCPReservation struct { Reservation // Pubkey ID. - PubkeyID int64 `db:"pubkey_id" json:"pubkey_id"` + PubkeyID *int64 `db:"pubkey_id" json:"pubkey_id,omitempty"` // Source ID. SourceID string `db:"source_id" json:"source_id"` @@ -158,7 +158,7 @@ type AzureReservation struct { Reservation // Pubkey ID. - PubkeyID int64 `db:"pubkey_id" json:"pubkey_id"` + PubkeyID *int64 `db:"pubkey_id" json:"pubkey_id,omitempty"` // Source ID. SourceID string `db:"source_id" json:"source_id"` diff --git a/internal/payloads/reservation_payload.go b/internal/payloads/reservation_payload.go index e2ff8415..5fc5f854 100644 --- a/internal/payloads/reservation_payload.go +++ b/internal/payloads/reservation_payload.go @@ -55,7 +55,7 @@ type AWSReservationResponse struct { ID int64 `json:"reservation_id" yaml:"reservation_id"` // Pubkey ID. - PubkeyID int64 `json:"pubkey_id" yaml:"pubkey_id"` + PubkeyID *int64 `json:"pubkey_id,omitempty" yaml:"pubkey_id,omitempty"` // Source ID. SourceID string `json:"source_id" yaml:"source_id"` @@ -91,7 +91,7 @@ type AWSReservationResponse struct { type AzureReservationResponse struct { ID int64 `json:"reservation_id" yaml:"reservation_id"` - PubkeyID int64 `json:"pubkey_id" yaml:"pubkey_id"` + PubkeyID *int64 `json:"pubkey_id,omitempty" yaml:"pubkey_id,omitempty"` SourceID string `json:"source_id" yaml:"source_id"` @@ -123,7 +123,7 @@ type GCPReservationResponse struct { ID int64 `json:"reservation_id" yaml:"reservation_id"` // Pubkey ID. - PubkeyID int64 `json:"pubkey_id" yaml:"pubkey_id"` + PubkeyID *int64 `json:"pubkey_id,omitempty" yaml:"pubkey_id,omitempty"` // Source ID. SourceID string `json:"source_id" yaml:"source_id"` diff --git a/internal/services/aws_reservation_service.go b/internal/services/aws_reservation_service.go index a84d6ab3..177c89d5 100644 --- a/internal/services/aws_reservation_service.go +++ b/internal/services/aws_reservation_service.go @@ -75,7 +75,7 @@ func CreateAWSReservation(w http.ResponseWriter, r *http.Request) { PowerOff: payload.PowerOff, } reservation := &models.AWSReservation{ - PubkeyID: payload.PubkeyID, + PubkeyID: &payload.PubkeyID, SourceID: payload.SourceID, ImageID: payload.ImageID, Detail: detail, @@ -89,8 +89,12 @@ func CreateAWSReservation(w http.ResponseWriter, r *http.Request) { reservation.Detail.Name = newName // validate pubkey - must be always present because of data integrity (foreign keys) - logger.Debug().Msgf("Validating existence of pubkey %d for this account", reservation.PubkeyID) - pk, err := pkDao.GetById(r.Context(), reservation.PubkeyID) + if reservation.PubkeyID == nil { + renderError(w, r, payloads.NewNotFoundError(r.Context(), "could not create AWS reservation", ErrPubkeyNotFound)) + } + + logger.Debug().Msgf("Validating existence of pubkey %d for this account", *reservation.PubkeyID) + pk, err := pkDao.GetById(r.Context(), *reservation.PubkeyID) if err != nil { message := fmt.Sprintf("get pubkey with id %d", reservation.PubkeyID) renderNotFoundOrDAOError(w, r, err, message) diff --git a/internal/services/azure_reservation_service.go b/internal/services/azure_reservation_service.go index 5d5c7772..92a0110c 100644 --- a/internal/services/azure_reservation_service.go +++ b/internal/services/azure_reservation_service.go @@ -125,7 +125,7 @@ func CreateAzureReservation(w http.ResponseWriter, r *http.Request) { Name: name, } reservation := &models.AzureReservation{ - PubkeyID: payload.PubkeyID, + PubkeyID: &payload.PubkeyID, SourceID: payload.SourceID, ImageID: payload.ImageID, Detail: detail, diff --git a/internal/services/gcp_reservation_service.go b/internal/services/gcp_reservation_service.go index 5a89c668..34430f61 100644 --- a/internal/services/gcp_reservation_service.go +++ b/internal/services/gcp_reservation_service.go @@ -67,7 +67,7 @@ func CreateGCPReservation(w http.ResponseWriter, r *http.Request) { LaunchTemplateID: payload.LaunchTemplateID, } reservation := &models.GCPReservation{ - PubkeyID: payload.PubkeyID, + PubkeyID: &payload.PubkeyID, ImageID: payload.ImageID, SourceID: payload.SourceID, Detail: detail, @@ -79,10 +79,14 @@ func CreateGCPReservation(w http.ResponseWriter, r *http.Request) { reservation.Steps = 2 reservation.StepTitles = jobs.LaunchInstanceGCPSteps - logger.Debug().Msgf("Validating existence of pubkey %d for this account", reservation.PubkeyID) - pk, err := pkDao.GetById(r.Context(), reservation.PubkeyID) + if reservation.PubkeyID == nil { + renderError(w, r, payloads.NewNotFoundError(r.Context(), "could not create AWS reservation", ErrPubkeyNotFound)) + } + + logger.Debug().Msgf("Validating existence of pubkey %d for this account", *reservation.PubkeyID) + pk, err := pkDao.GetById(r.Context(), *reservation.PubkeyID) if err != nil { - message := fmt.Sprintf("get pubkey with id %d", reservation.PubkeyID) + message := fmt.Sprintf("get pubkey with id %d", *reservation.PubkeyID) renderNotFoundOrDAOError(w, r, err, message) return } @@ -147,7 +151,7 @@ func CreateGCPReservation(w http.ResponseWriter, r *http.Request) { Args: jobs.LaunchInstanceGCPTaskArgs{ ReservationID: reservation.ID, Zone: reservation.Detail.Zone, - PubkeyID: reservation.PubkeyID, + PubkeyID: *reservation.PubkeyID, Detail: reservation.Detail, ImageName: name, ProjectID: authentication, diff --git a/internal/services/reservations_service.go b/internal/services/reservations_service.go index dcf1cb1b..cd791628 100644 --- a/internal/services/reservations_service.go +++ b/internal/services/reservations_service.go @@ -23,6 +23,7 @@ var ( ErrBothTypeAndTemplateMissing = errors.New("instance type or launch template not set") ErrUnsupportedRegion = errors.New("unknown region/location/zone") ErrInvalidNamePattern = errors.New("name pattern is not RFC-1035 compatible") + ErrPubkeyNotFound = errors.New("no pubkey found") ) // CreateReservation dispatches requests to type provider specific handlers diff --git a/internal/services/reservations_service_test.go b/internal/services/reservations_service_test.go index bc8fdc93..ec5a5d88 100644 --- a/internal/services/reservations_service_test.go +++ b/internal/services/reservations_service_test.go @@ -43,7 +43,7 @@ func TestGetReservationDetail(t *testing.T) { PowerOff: true, } reservation := &models.AWSReservation{ - PubkeyID: pk.ID, + PubkeyID: &pk.ID, SourceID: "1", ImageID: "ami-random", Detail: detail,