Skip to content
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

feat: support dual stack requests for aws clients #52

Merged
merged 4 commits into from
Oct 28, 2024

Conversation

ddebko
Copy link
Contributor

@ddebko ddebko commented Oct 16, 2024

Summary

Dual stack access for ipv4/ipv6 is by default disabled in aws clients. This PR allows users to enable dual stack access using the attributes field. The new field dual_stack is a boolean type.

Concerns

Updating the aws dependencies have caused the memory usage in boundary to spike. Before updating the aws dependencies, boundary was utilizing ~460MB while idling. With the updates, boundary is utilizing ~720MB while idling. I think this is concerning, but I'm not sure how to proceed.

Copy link
Contributor

@kheina kheina left a comment

Choose a reason for hiding this comment

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

there might be some unintended sbc state things in this pr, but otherwise looks good imo

plugin/service/host/state.go Show resolved Hide resolved
plugin/service/storage/const.go Show resolved Hide resolved
plugin/service/host/plugin.go Outdated Show resolved Hide resolved
plugin/service/host/state.go Outdated Show resolved Hide resolved
Copy link
Contributor

@louisruch louisruch left a comment

Choose a reason for hiding this comment

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

Great work sorry for taking so long to review. I left one minor comment. Otherwise we have been using the readme's in the services as a source of truth for valid attribute and secret fields in conversations recently. We should therefore add dual_stack to those two readmes until we have a better option for documenting this.

plugin/service/storage/const.go Show resolved Hide resolved
Copy link
Contributor

@kheina kheina left a comment

Choose a reason for hiding this comment

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

👍

@ddebko ddebko requested review from elimt and kheina October 21, 2024 17:18
@ddebko ddebko force-pushed the ddebko-ipv6-compatability branch 2 times, most recently from 1c9d6f1 to 8ab07c3 Compare October 28, 2024 04:11
@ddebko ddebko force-pushed the ddebko-ipv6-compatability branch from 8ab07c3 to 97aae6e Compare October 28, 2024 04:18
Copy link
Contributor

@kheina kheina left a comment

Choose a reason for hiding this comment

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

LGTM

@ddebko ddebko merged commit 899c62c into main Oct 28, 2024
2 checks passed
louisruch added a commit that referenced this pull request Dec 10, 2024
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