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

Convert AWS API calls to use paginators #14491

Merged
merged 1 commit into from
Dec 29, 2020
Merged

Convert AWS API calls to use paginators #14491

merged 1 commit into from
Dec 29, 2020

Conversation

ungureanuvladvictor
Copy link
Member

Some of the AWS API calls that we do are not doing correct pagination. This means that we return partial output to the calling functions. Those ones are (from the ec2 internal package):

  • describeVpcs
  • describeSubnets
  • describeSecurityGroups

The ones that implement manually pagination are:

  • describeNetworkInterfaces
  • GetInstanceTypes

Overall it might not be important to implement pagination for all EC2 API calls but is good to standardize on it. Happy to revisit this if the reviewers think this is complicating things too much.

Signed-off-by: Vlad Ungureanu vladu@palantir.com

@ungureanuvladvictor ungureanuvladvictor added pending-review area/eni Impacts ENI based IPAM. release-note/misc This PR makes changes that have no direct user impact. labels Dec 24, 2020
@ungureanuvladvictor ungureanuvladvictor requested a review from a team as a code owner December 24, 2020 06:41
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Dec 24, 2020
@ungureanuvladvictor
Copy link
Member Author

test-me-please

Copy link
Member

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

One nit, the rest LGTM

pkg/policy/groups/aws/aws.go Outdated Show resolved Hide resolved
@ungureanuvladvictor
Copy link
Member Author

test-me-please

Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

A couple of small nits.

pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved
pkg/policy/groups/aws/aws.go Outdated Show resolved Hide resolved
pkg/policy/groups/aws/aws.go Outdated Show resolved Hide resolved
pkg/policy/groups/aws/aws.go Outdated Show resolved Hide resolved
@ungureanuvladvictor
Copy link
Member Author

ungureanuvladvictor commented Dec 28, 2020

test-me-please

updated #12511 (comment) with the failure in netnext.

@ungureanuvladvictor
Copy link
Member Author

retest-net-next

Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM. Can you put the context in the PR into the commit itself? It would be helpful to have the "why" in the commit msg

Some of the AWS API calls that we do are not doing correct pagination.
This means that we return partial output to the calling functions. Those
ones are (from the ec2 internal package):
- `describeVpcs`
- `describeSubnets`
- `describeSecurityGroups`

The ones that implement manually pagination are:
- `describeNetworkInterfaces`
- `GetInstanceTypes`

Signed-off-by: Vlad Ungureanu <vladu@palantir.com>
@ungureanuvladvictor
Copy link
Member Author

ungureanuvladvictor commented Dec 29, 2020

test-me-please

@christarazi -- updated the commit msg.

@aanm aanm merged commit a81af68 into master Dec 29, 2020
@aanm aanm deleted the vladu/aws-paginators branch December 29, 2020 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/eni Impacts ENI based IPAM. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants