TrueCloudLab/frostfs-node
21
1
Fork 27
Code Issues 108 Pull requests 3 Projects Releases 1 Activity Actions

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 +207 -40
aarifullin commented 2024-05-20 13:36:20 +00:00
Member
Copy link
  • 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 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
Copy link

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
Copy link

@dkirillov

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

We need only IP

We need only IP
aarifullin commented 2024-05-21 15:44:19 +00:00
Author
Member
Copy link

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
Copy link

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
Author
Member
Copy link

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
Author
Member
Copy link

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
Member
Copy link

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
Member
Copy link

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
Author
Member
Copy link

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 labels
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 items
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
No description provided.
Delete branch "aarifullin/frostfs-node:feat/ape_sourceip"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?