Add support for clusters running in private VPC #627
Conversation
aaroniscode
force-pushed
the
enable_private_vpc
branch
3 times, most recently
from
5e8d935
to
c733a75
Compare
aaroniscode
force-pushed
the
enable_private_vpc
branch
2 times, most recently
from
81ba681
to
78f4936
Compare
There was a problem hiding this comment.
Overall looking good. Added suggestions for unit tests, but we can also talk more over Slack if you need more details.
While this will unblock the "private VPC" use case, I am concerned about the performance implications for this approach, since these O(1) operations are now O(N) in terms of number of remote API calls. I suppose when performance becomes an issue we can address that too - there are other places in the codebase we'll also likely have to revisit from a performance standpoint.
aaroniscode
force-pushed
the
enable_private_vpc
branch
from
97e1520
to
0119392
Compare
|
@erikfuller I think the PR has addresses all your feedback and I appreciate the test guidance. Also rebased as there was a conflict. |
There was a problem hiding this comment.
Thanks, @aaroniscode, tests are looking good. Just a typo and a couple small adjustments and we'll be good to merge.
pkg/aws/services/tagging_test.go
Outdated
| "Key2": aws.String("Value2"), | ||
| "Key3": aws.String("Value3"), | ||
| }, | ||
| testName: "match subnet of tags from source's multiple tags", |
aaroniscode
force-pushed
the
enable_private_vpc
branch
from
c0cb66f
to
5886139
Compare
NOTE: This is a draft PR for review by the team. If the design of the feature is approved, I will add any required tests.
What type of PR is this?
feature
Which issue does this PR fix:
no open Issue
What does this PR do / Why do we need it:
Add support for clusters running in private VPC. Today the controller won't function in a fully private VPC as there is no PrivateLink support for the ResourceTagging API
If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:
Create a private VPC (one that cannot access the Internet) and try to the Getting Started Guide
Testing done on this change:
ran the controller locally and tested the Getting Started Guide
Automation added to e2e:
none at this time
Will this PR introduce any new dependencies?:
No
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No breakage
Does this PR introduce any user-facing change?:
Yes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.