TrueCloudLab
/
frostfs-node
17
1
Fork 22
Code Issues 90 Pull Requests 8 Actions Projects Releases Activity

Fill APE Request proprties with source IP in services #1142

Merged
fyrchik merged 3 commits from aarifullin/frostfs-node:feat/ape_sourceip into master 2024-05-27 10:17:21 +00:00
Conversation 0 Commits 3 Files Changed 8
aarifullin commented 2024-05-20 13:36:20 +00:00
Collaborator
  • container: Fill APE-request property with source IP
  • object: Fill APE-request with source IP property
  • tree: Fill APE-request with source IP property
* container: Fill APE-request property with source IP * object: Fill APE-request with source IP property * tree: Fill APE-request with source IP property
aarifullin added 3 commits 2024-05-20 13:36:25 +00:00
49472befce [#XX] container: Fill APE-request property with source IP 
Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
49b3c58edf [#XX] object: Fill APE-request with source IP property 
Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
Vulncheck / Vulncheck (pull_request) Successful in 4m54s Details
DCO action / DCO (pull_request) Failing after 5m24s Details
Build / Build Components (1.21) (pull_request) Successful in 6m48s Details
Tests and linters / gopls check (pull_request) Successful in 7m5s Details
Build / Build Components (1.22) (pull_request) Successful in 7m4s Details
Tests and linters / Staticcheck (pull_request) Successful in 8m17s Details
Tests and linters / Lint (pull_request) Successful in 8m46s Details
Tests and linters / Tests (1.22) (pull_request) Failing after 10m5s Details
Tests and linters / Tests (1.21) (pull_request) Successful in 13m6s Details
Tests and linters / Tests with -race (pull_request) Successful in 14m48s Details
Pre-commit hooks / Pre-commit (pull_request) Successful in 18m7s Details
356c6c1cb7 [#XX] tree: Fill APE-request with source IP property 
Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
aarifullin force-pushed feat/ape_sourceip from 356c6c1cb7 to 80f28b82a7 2024-05-20 13:37:09 +00:00 Compare
aarifullin requested review from storage-core-committers 2024-05-20 13:37:13 +00:00
aarifullin requested review from storage-core-developers 2024-05-20 13:37:30 +00:00
fyrchik reviewed 2024-05-21 09:51:48 +00:00
pkg/services/container/ape.go Outdated
@ -378,2 +386,4 @@
return nil, nil, err
}
if p, ok := peer.FromContext(ctx); ok {
reqProps[commonschema.PropertyKeyFrostFSSourceIP] = p.Addr.String()
fyrchik commented 2024-05-21 09:51:30 +00:00
Owner

net.Addr is an interface and it can contain port. The port is random for outgoing connections and we are probably interested in the IP only.
Is it explicit in the spec?

`net.Addr` is an interface and it can contain port. The port is random for outgoing connections and we are probably interested in the IP only. Is it explicit in the spec?
fyrchik commented 2024-05-21 14:49:35 +00:00
Owner

@dkirillov

@dkirillov
dkirillov commented 2024-05-21 14:55:56 +00:00

We need only IP

We need only IP
aarifullin commented 2024-05-21 15:44:19 +00:00
Poster
Collaborator

I have fixed that. Please, check this out

if p, ok := peer.FromContext(ctx); ok {
   addr, err := net.ResolveTCPAddr("", p.Addr.String())
   if err != nil {
			return nil, nil, err
    }
	reqProps[commonschema.PropertyKeyFrostFSSourceIP] = addr.IP.String()
}

Since, tcp-address is parsed and IP is put to the property

I have fixed that. Please, check this out ```go if p, ok := peer.FromContext(ctx); ok { addr, err := net.ResolveTCPAddr("", p.Addr.String()) if err != nil { return nil, nil, err } reqProps[commonschema.PropertyKeyFrostFSSourceIP] = addr.IP.String() } ``` Since, tcp-address is parsed and IP is put to the property
fyrchik commented 2024-05-22 08:47:39 +00:00
Owner

Have you checked what type is returned here? I believe it shoud be TCPAddr as we provide TCP listener to the gRPC server. ResolveTCPAddr can consult DNS, which is definitely what we do not want to do.

Have you checked what type is returned here? I believe it shoud be `TCPAddr` as we provide TCP listener to the gRPC server. `ResolveTCPAddr` can consult DNS, which is definitely what we do not want to do.
aarifullin commented 2024-05-22 12:41:22 +00:00
Poster
Collaborator

ResolveTCPAddr can consult DNS, which is definitely what we do not want to do.

Sorry, I don't get your point. Let it be able to consult DNS, but

we provide TCP listener to the gRPC server.

so, peer.Addr is coming within context as tcp 192.92.9.2:4032. So, it successfully parses the tcp address and we keep IP in addr.IP

> ResolveTCPAddr can consult DNS, which is definitely what we do not want to do. Sorry, I don't get your point. Let it be able to consult DNS, but >we provide TCP listener to the gRPC server. so, `peer.Addr` is coming within context as tcp `192.92.9.2:4032`. So, it successfully parses the tcp address and we keep `IP` in `addr.IP`
aarifullin commented 2024-05-24 11:31:51 +00:00
Poster
Collaborator

Okay, I got it. Fixed as you requested 👍

Okay, I got it. Fixed as you requested 👍
fyrchik requested review from dkirillov 2024-05-21 14:49:18 +00:00
aarifullin force-pushed feat/ape_sourceip from 80f28b82a7 to 409c3ae2ff 2024-05-21 15:42:54 +00:00 Compare
dkirillov approved these changes 2024-05-23 08:11:07 +00:00
dkirillov left a comment

LGTM

LGTM
aarifullin force-pushed feat/ape_sourceip from 409c3ae2ff to 297e4b4984 2024-05-24 11:17:33 +00:00 Compare
aarifullin requested review from dkirillov 2024-05-24 11:20:03 +00:00
fyrchik approved these changes 2024-05-27 07:31:26 +00:00
acid-ant approved these changes 2024-05-27 08:00:38 +00:00
pkg/services/tree/signature_test.go Outdated
@ -133,3 +134,3 @@
t.Run("missing signature, no panic", func(t *testing.T) {
require.Error(t, s.verifyClient(req, cid2, nil, op))
require.Error(t, s.verifyClient(context.TODO(), req, cid2, nil, op))
acid-ant commented 2024-05-27 07:59:41 +00:00
Collaborator

You are planning to extend these tests with proper context? Why not to use Background() here?

You are planning to extend these tests with proper context? Why not to use `Background()` here?
aarifullin commented 2024-05-27 09:06:08 +00:00
Poster
Collaborator

Fixed

Fixed
aarifullin force-pushed feat/ape_sourceip from 297e4b4984 to 75881cf908 2024-05-27 09:06:01 +00:00 Compare
acid-ant approved these changes 2024-05-27 09:33:41 +00:00
fyrchik merged commit 3627b44e92 into master 2024-05-27 10:17:21 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
dkirillov
fyrchik
acid-ant
TrueCloudLab/storage-core-developers
Labels
Clear labels   
P0

Highest   
P1

High   
P2

Medium   
P3

Low   
badger

Badger DB related issues   
frostfs-adm
   
frostfs-cli
   
frostfs-ir
   
frostfs-lens
   
frostfs-node
   
good first issue

Good for newcomers   
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff   
blocked

The issue or PR is blocked by something   
bug

Something is not working   
config

Configuration format update or breaking change   
discussion

Talking about something in order to reach a decision or to exchange ideas   
documentation

Documentation related   
duplicate

This issue or pull request already exists   
enhancement

New feature   
go

Language features adoption   
help wanted

Extra attention is needed   
internal
   
invalid

Something is wrong   
kludge

This is a kludge and we know it   
observability

Related to metrics, tracing and logs   
perfomance

Perfomance related   
question

More information is needed   
refactoring

Refactoring   
wontfix

This won't be fixed
No Label
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#1142
There is no content yet.
Delete Branch "aarifullin/frostfs-node:feat/ape_sourceip"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?