Paths

Table of Contentst

Differential D3530

[services] Backup - Fix requests in proto file
ClosedPublic
Actions

Authored by • karol on Mar 29 2022, 4:38 AM.

Tags

None

Referenced Files

	F3377814: D3530.id10997.diff
	Wed, Nov 27, 7:39 AM

	F3377440: D3530.diff
	Wed, Nov 27, 5:42 AM

	Unknown Object (File)
	Tue, Nov 5, 6:05 PM

	Unknown Object (File)
	Oct 13 2024, 11:37 AM

	Unknown Object (File)
	Oct 13 2024, 11:37 AM

	Unknown Object (File)
	Oct 13 2024, 11:37 AM

	Unknown Object (File)
	Oct 13 2024, 11:37 AM

	Unknown Object (File)
	Oct 13 2024, 11:34 AM

Subscribers

Details

Reviewers

tomek
• max
varun
• jim
ashoat

Commits

rCOMMce9608320574: [services] Backup - Fix requests in proto file

Summary

Depends on D3529

We need a user id in every request in the backup service, how else will we know where the request comes from?

Previously the user id was deeper in the authentication types but they were removed and we missed that we removed the user id as well.

Test Plan

cd services
yarn run-backup-service

Diff Detail

Repository

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Mar 29 2022, 4:38 AM

Herald added subscribers: • abosh, • benschac, atul and 3 others. · View Herald TranscriptMar 29 2022, 4:38 AM

• karol edited the summary of this revision. (Show Details)Mar 29 2022, 4:46 AM

• karol edited the test plan for this revision. (Show Details)

• karol added reviewers: tomek, • max, varun, • jim.

• karol added a parent revision: D3529: [services] Reactors - use doneCallback only from OnDone.

Harbormaster completed remote builds in B7667: Diff 10763.Mar 29 2022, 4:46 AM

• karol requested review of this revision.Mar 29 2022, 4:46 AM

• karol added a child revision: D3531: [services] Backup - Apply reading user id in the reactors.Mar 29 2022, 4:48 AM

Adding @ashoat to verify that this is what we need from security point of view

Makes sense to me

native/cpp/CommonCpp/grpc/protos/backup.proto
48	Don't we also need to specify the `backupID` here? I know we say it is part of auth, but I think it is an essential part of the request here, just like `userID`

This revision is now accepted and ready to land.Mar 31 2022, 8:38 PM

• karol added inline comments.Apr 1 2022, 5:34 AM

native/cpp/CommonCpp/grpc/protos/backup.proto
48	We agreed that for now, we handle only assigning to the last backup, so it's going to automatically pick the latest one. Once we do the feature allowing users to choose older backups, `backupID` will be needed here.

ashoat requested changes to this revision.Apr 1 2022, 12:16 PM

ashoat added inline comments.

native/cpp/CommonCpp/grpc/protos/backup.proto
48	That's not my memory. I don't think we agreed on that. I think we explicitly agreed that we should always specify the `backupID` in `SendLog`. I don't have time to dig up the comments about this but I'm pretty sure about it

This revision now requires changes to proceed.Apr 1 2022, 12:16 PM

add backup id

• karol added inline comments.Apr 4 2022, 7:17 AM

native/cpp/CommonCpp/grpc/protos/backup.proto
48	Yes, maybe you're right. Ok, let's go for the `backupID` here too.

Harbormaster completed remote builds in B7829: Diff 10997.Apr 4 2022, 11:51 AM

ashoat accepted this revision.Apr 5 2022, 1:11 AM

This revision is now accepted and ready to land.Apr 5 2022, 1:11 AM

Closed by commit rCOMMce9608320574: [services] Backup - Fix requests in proto file (authored by • karol). · Explain WhyApr 7 2022, 2:37 AM

This revision was automatically updated to reflect the committed changes.

• karol added a commit: rCOMMce9608320574: [services] Backup - Fix requests in proto file.

Revision Contents
Changeset List

Path

Size

native/

cpp/

CommonCpp/

grpc/

_generated/

backup.grpc.pb.h

protos/

12 lines

Diff 10763

native/cpp/CommonCpp/grpc/_generated/backup.grpc.pb.h

Loading...

native/cpp/CommonCpp/grpc/_generated/backup.pb.h

Loading...

native/cpp/CommonCpp/grpc/_generated/backup.pb.cc

Loading...

native/cpp/CommonCpp/grpc/protos/backup.proto

Loading...