Skip to content

Commit 84e45ce

Browse files
ma-g-22bhshkh
ma-g-22
and
bhshkh
authored
fix(bigtable): Reject misspecified Automated Backup Policies when updating a table (#10226)
Co-authored-by: Baha Aiman <bahaaiman@google.com>
1 parent 7cae242 commit 84e45ce

2 files changed

Lines changed: 17 additions & 27 deletions

File tree

‎bigtable/admin.go‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -446,8 +446,8 @@ const (
446446
deletionProtectionFieldMask = "deletion_protection"
447447
changeStreamConfigFieldMask = "change_stream_config"
448448
automatedBackupPolicyFieldMask = "automated_backup_policy"
449-
retentionPeriodFieldMaskPath = ".retention_period"
450-
frequencyFieldMaskPath = ".frequency"
449+
retentionPeriodFieldMaskPath = "retention_period"
450+
frequencyFieldMaskPath = "frequency"
451451
)
452452

453453
func (ac *AdminClient) newUpdateTableRequestProto(tableID string) (*btapb.UpdateTableRequest, error) {
@@ -500,7 +500,7 @@ func (ac *AdminClient) UpdateTableWithChangeStream(ctx context.Context, tableID
500500
if err != nil {
501501
return err
502502
}
503-
req.UpdateMask.Paths = []string{changeStreamConfigFieldMask + retentionPeriodFieldMaskPath}
503+
req.UpdateMask.Paths = []string{changeStreamConfigFieldMask + "." + retentionPeriodFieldMaskPath}
504504
req.Table.ChangeStreamConfig = &btapb.ChangeStreamConfig{}
505505
req.Table.ChangeStreamConfig.RetentionPeriod = durationpb.New(changeStreamRetention.(time.Duration))
506506
return ac.updateTableAndWait(ctx, req)
@@ -537,13 +537,19 @@ func (ac *AdminClient) UpdateTableWithAutomatedBackupPolicy(ctx context.Context,
537537
if err != nil {
538538
return err
539539
}
540+
// If the AutomatedBackupPolicy is not at least partially specified, or if both fields are 0, then this is an
541+
// incorrect configuration for updating the table, and should be rejected. Both fields could be zero if (1)
542+
// they are set to zero, or (2) neither field was set and the policy was constructed using toProto().
543+
if abc.AutomatedBackupPolicy.RetentionPeriod.Seconds == 0 && abc.AutomatedBackupPolicy.Frequency.Seconds == 0 {
544+
return errors.New("Invalid automated backup policy. If you're intending to disable automated backups, please use the UpdateTableDisableAutomatedBackupPolicy method instead")
545+
}
540546
if abc.AutomatedBackupPolicy.RetentionPeriod.Seconds != 0 {
541547
// Update Retention Period
542-
req.UpdateMask.Paths = append(req.UpdateMask.Paths, automatedBackupPolicyFieldMask+retentionPeriodFieldMaskPath)
548+
req.UpdateMask.Paths = append(req.UpdateMask.Paths, automatedBackupPolicyFieldMask+"."+retentionPeriodFieldMaskPath)
543549
}
544550
if abc.AutomatedBackupPolicy.Frequency.Seconds != 0 {
545551
// Update Frequency
546-
req.UpdateMask.Paths = append(req.UpdateMask.Paths, automatedBackupPolicyFieldMask+frequencyFieldMaskPath)
552+
req.UpdateMask.Paths = append(req.UpdateMask.Paths, automatedBackupPolicyFieldMask+"."+frequencyFieldMaskPath)
547553
}
548554
req.Table.AutomatedBackupConfig = abc
549555
return ac.updateTableAndWait(ctx, req)

‎bigtable/admin_test.go‎

Lines changed: 6 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -647,37 +647,21 @@ func TestTableAdmin_UpdateTableDisableAutomatedBackupPolicy(t *testing.T) {
647647
}
648648
}
649649

650-
func TestTableAdmin_UpdateTableWithConf_DisableAutomatedBackupPolicy_DisableAbp(t *testing.T) {
650+
func TestTableAdmin_UpdateTableWithAutomatedBackupPolicy_NilFields_Invalid(t *testing.T) {
651651
mock := &mockTableAdminClock{}
652652
c := setupTableClient(t, mock)
653653

654-
err := c.UpdateTableDisableAutomatedBackupPolicy(context.Background(), "My-table")
655-
if err != nil {
656-
t.Fatalf("UpdateTableDisableAutomatedBackupPolicy failed: %v", err)
657-
}
658-
updateTableReq := mock.updateTableReq
659-
if !cmp.Equal(updateTableReq.Table.Name, "projects/my-cool-project/instances/my-cool-instance/tables/My-table") {
660-
t.Errorf("UpdateTableRequest does not match, TableID: %v", updateTableReq.Table.Name)
661-
}
662-
if updateTableReq.Table.AutomatedBackupConfig != nil {
663-
t.Errorf("UpdateTableRequest does not match, AutomatedBackupConfig: %v should be empty", updateTableReq.Table.AutomatedBackupConfig)
664-
}
665-
if updateTableReq.Table.GetAutomatedBackupPolicy() != nil {
666-
t.Errorf("UpdateTableRequest does not match, GetAutomatedBackupPolicy: %v should be empty", updateTableReq.Table.GetAutomatedBackupPolicy())
667-
}
668-
if !cmp.Equal(len(updateTableReq.UpdateMask.Paths), 1) {
669-
t.Errorf("UpdateTableRequest does not match, UpdateMask has length of %d, expected 1", len(updateTableReq.UpdateMask.Paths))
670-
}
671-
if !cmp.Equal(updateTableReq.UpdateMask.Paths[0], "automated_backup_policy") {
672-
t.Errorf("UpdateTableRequest does not match, UpdateMask: %v", updateTableReq.UpdateMask.Paths[0])
654+
err := c.UpdateTableWithAutomatedBackupPolicy(context.Background(), "My-table", TableAutomatedBackupPolicy{nil, nil})
655+
if err == nil {
656+
t.Fatalf("Expected UpdateTableDisableAutomatedBackupPolicy to fail due to misspecified AutomatedBackupPolicy")
673657
}
674658
}
675659

676-
func TestTableAdmin_UpdateTableWithConf_AutomatedBackupPolicyNilFields_Invalid(t *testing.T) {
660+
func TestTableAdmin_UpdateTableWithAutomatedBackupPolicy_ZeroFields_Invalid(t *testing.T) {
677661
mock := &mockTableAdminClock{}
678662
c := setupTableClient(t, mock)
679663

680-
err := c.UpdateTableWithAutomatedBackupPolicy(context.Background(), "My-table", TableAutomatedBackupPolicy{nil, nil})
664+
err := c.UpdateTableWithAutomatedBackupPolicy(context.Background(), "My-table", TableAutomatedBackupPolicy{time.Duration(0), time.Duration(0)})
681665
if err == nil {
682666
t.Fatalf("Expected UpdateTableDisableAutomatedBackupPolicy to fail due to misspecified AutomatedBackupPolicy")
683667
}

0 commit comments

Comments
 (0)