Skip to content

Cope with pagination in private zone discovery#99

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
Miciah:cope-with-pagination-in-private-zone-discovery
Jan 11, 2019
Merged

Cope with pagination in private zone discovery#99
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
Miciah:cope-with-pagination-in-private-zone-discovery

Conversation

@Miciah
Copy link
Contributor

@Miciah Miciah commented Jan 11, 2019

#93 changed the DNS private zone discovery to use the GetResources AWS API call. However, even though we use filters, the resources are still paginated as though no filter were applied. If the desired resource is not on the first page, then GetResources will not return it. We need to use GetResourcesPages, and we may need to look through one or more empty pages of resources till we find the desired resource.

  • pkg/dns/aws/dns.go: Use GetResourcesPages to get the private zone.

@ironcladlou @wking

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 11, 2019
Commit 8fdcf1a changed the DNS private
zone discovery to use the GetResources AWS API call.  However, even though
we use filters, the resources are still paginated as though no filter were
applied.  If the desired resource is not on the first page, then
GetResources will not return it.  We need to use GetResourcesPages, and we
may need to look through one or more empty pages of resources till we find
the desired resource.

* pkg/dns/aws/dns.go: Use GetResourcesPages to get the private zone.
@Miciah Miciah force-pushed the cope-with-pagination-in-private-zone-discovery branch from 8faf3bb to 2526a4c Compare January 11, 2019 02:57
@wking
Copy link
Member

wking commented Jan 11, 2019

I haven't had time to go through the diff in detail, but GetResourcesPages is absolutely the way to go. 👍

@Miciah
Copy link
Contributor Author

Miciah commented Jan 11, 2019

Thanks! I actually @'d you because I saw you had mentioned on an installer PR that you were following the example in this code, but now that I actually look at that PR, I see you are already using GetResourcesPages. Sorry for the noise! Nothing to see here!

@wking
Copy link
Member

wking commented Jan 11, 2019

I was riding on the "now we need the get-tagged-resources permission" coat tails ;).

@Miciah
Copy link
Contributor Author

Miciah commented Jan 11, 2019

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Jan 11, 2019

level=error msg="\t* module.vpc.aws_route_table_association.worker_routing[5]: 1 error occurred:"
level=error msg="\t* aws_route_table_association.worker_routing.5: timeout while waiting for state to become 'success' (timeout: 5m0s)"

and

could not copy stable imagestreamtag: Timeout: request did not complete within allowed duration

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Jan 11, 2019

level=error msg="\t* module.vpc.aws_route.to_nat_gw[2]: 1 error occurred:"
level=error msg="\t* aws_route.to_nat_gw.2: Error creating route: timeout while waiting for state to become 'success' (timeout: 2m0s)"
level=error
level=error
level=error msg="\t* module.vpc.aws_route.to_nat_gw[4]: 1 error occurred:"
level=error msg="\t* aws_route.to_nat_gw.4: Error creating route: timeout while waiting for state to become 'success' (timeout: 2m0s)"
level=error
level=error
level=error msg="\t* module.vpc.aws_route.to_nat_gw[3]: 1 error occurred:"
level=error msg="\t* aws_route.to_nat_gw.3: Error creating route: timeout while waiting for state to become 'success' (timeout: 2m0s)"
level=error
level=error
level=error msg="\t* module.dns.aws_route53_record.api_external: 1 error occurred:"
level=error msg="\t* aws_route53_record.api_external: [ERR]: Error building changeset: timeout while waiting for state to become 'accepted' (timeout: 5m0s)"

and

could not copy stable imagestreamtag: Timeout: request did not complete within allowed duration

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Jan 11, 2019

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Jan 11, 2019

level=error msg="\t* module.vpc.aws_route.to_nat_gw[0]: 1 error occurred:"
level=error msg="\t* aws_route.to_nat_gw.0: Error creating route: timeout while waiting for state to become 'success' (timeout: 2m0s)"
level=error
level=error
level=error msg="\t* module.vpc.aws_route.to_nat_gw[1]: 1 error occurred:"
level=error msg="\t* aws_route.to_nat_gw.1: Error creating route: timeout while waiting for state to become 'success' (timeout: 2m0s)"

/retest

})
}, f)
if innerError != nil {
err = innerError
Copy link
Contributor

@ironcladlou ironcladlou Jan 11, 2019

Choose a reason for hiding this comment

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

If both err and innerError are not nil, err is lost. I don't want to block the PR on it. How about a followup to use an aggregate (either from k8s utils if available, or https://github.com/pkg/errors?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured innerError was more important, but yeah, aggregating the error values would be best. I'll make a follow-up.

@ironcladlou
Copy link
Contributor

Thank you very much!

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants