Skip to content

Conversation

@fl0Lec
Copy link
Contributor

@fl0Lec fl0Lec commented May 30, 2025

Updated the AddressObject class to inherit from Address::InstanceAccessor instead of StreamInfo::FilterState::Object. This change enhances the address handling capabilities while maintaining the existing functionality. This will allow to do CIDR range matching on AddressObject

Commit Message: AddressObject to inherit from Address::InstanceAccessor
Additional Description: Updated the AddressObject class to inherit from Address::InstanceAccessor instead of StreamInfo::FilterState::Object. This change enhances the address handling capabilities while maintaining the existing functionality. This will allow to do CIDR range matching on AddressObject
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Updated the AddressObject class to inherit from Address::InstanceAccessor instead of StreamInfo::FilterState::Object. This change enhances the address handling capabilities while maintaining the existing functionality.
This will allow to do CIDR range matching on AddressObject

Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #39680 was opened by fl0Lec.

see: more, trace.

@fl0Lec fl0Lec marked this pull request as ready for review May 30, 2025 15:28
nezdolik
nezdolik previously approved these changes Jun 3, 2025
@nezdolik nezdolik self-assigned this Jun 3, 2025
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/assign-from @envoyproxy/senior-maintainers

public:
AddressObject(Network::Address::InstanceConstSharedPtr address) : address_(address) {}
AddressObject(Network::Address::InstanceConstSharedPtr address)
: Address::InstanceAccessor(address), address_(address) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the address is now kept in 2 places: the address_ data member in AddressObject and the ip_ data member in InstanceAccessor.
I suggest ensuring ODR by only keeping the data member in InstanceAccessor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The address_ is now only store in InstanceAccessor but I add to create an additional method for InstanceAccessor to be able to share the InstanceConstSharedPtr

@repokitteh-read-only
Copy link

@envoyproxy/senior-maintainers assignee is @yanavlasov

🐱

Caused by: a #39680 (review) was submitted by @adisuissa.

see: more, trace.

@yanavlasov
Copy link
Contributor

/wait

Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
Copy link
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/wait

Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>
@fl0Lec

This comment was marked as off-topic.

@fl0Lec fl0Lec requested a review from yanavlasov June 6, 2025 07:07
@yanavlasov yanavlasov merged commit 558b43a into envoyproxy:main Jun 6, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants