Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Scylla API for backup #4169
Use Scylla API for backup #4169
Changes from all commits
c7c7130
ef3b968
5cfe08c
d2c391c
2def7c1
3e04748
c55fb1d
27af74c
dc6acb8
19b00bf
4d5bee4
007313b
ecfeb80
49c62e9
0731050
d80e48f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when there is an error raised in the Directory that is expected to validate&modify the incoming request ?
I understand that the error is just logged, but proceeds with the call to the scylla server with the current implementation.
This behavior means that the inproper request is proxied still to the scylla server.
It's much more efficient to stop processing the invalid request straight in the proxy instead of forwarding it.
To do that, you can introduce the middleware that is reponsible for validation +
The middleware should evaluate the correct endpoint and save it to conext.
Then, the director just checks the context to see if there is something to change.
Director, can skip the validation
This comment is only valid if you proceed with resolving endpoint in agent. The topic to discuss is that is can be safer/better to have the identifiers of endpoints in object storage and use it in sm configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that if I call agent with endpoint query param = "http://169.254.169.254/storage_service/backup" then "169.254.169.254" is going to be passed to scylla server as the AWS endpoint to scylla server ?
Then this IP is consumed by scylla server to query S3 API, right ?
I think it's a threat called SSRF.
This default must be either change to return explicit error, or Scylla Manager Agent must be aware of whitelisted IP addresses (passed with "endpoint" query param).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see it's actually the main route. The input must be validated then. Is it possible to provide whitelisted IPs/Hosts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, does it mean that the S3 enpoint url is not known by Scylla ?
Why ?
Isn't it stored in some scylla configuration ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@regevran why ScyllaAPI must be informed about the endpoint ? Cannot it be read by scylla directly ? Using this info https://github.com/scylladb/scylladb/blob/92db2eca0b8ab0a4fa2571666a7fe2d2b07c697b/docs/dev/object_storage.md?plain=1#L29-L39 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we covered those points during the meeting, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. These are comments from before the sync. Waiting for the output from the meeting then.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's safer and more accurate to force config cache to update cluster configuration first ? And then call to Read All.