-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add support for VPC Endpoint Services #2636
base: main
Are you sure you want to change the base?
Conversation
Hi @hintofbasil. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, just a couple of questions
@@ -12,3 +14,25 @@ func ChunkStrings(targets []string, chunkSize int) [][]string { | |||
} | |||
return chunks | |||
} | |||
|
|||
func DiffStringSlice(first, second []string) ([]*string, []*string, []*string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment here to explain what this function does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one.
} | ||
|
||
func (m *defaultEndpointServiceManager) ReconcileTags(ctx context.Context, resID string, desiredTags map[string]string, opts ...ReconcileTagsOption) error { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this missing the implementation, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was templated by rifelpet but I ended up using the tagging manager implementation instead of duplicating the logic. Removed it.
if tt.wantErr { | ||
assert.Error(t, err) | ||
} else { | ||
assert.Equal(t, tt.want, got) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not adding a wantErr
to every test case and just assert.Equal(t, tt.want, got)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The errors returned are generally wrapped errors. Wrapped errors include a stack trace so can't be compared an error generated in the test.
If there is a nice way to tests for equality with wrapped errors I'd be very happy to change it.
@hintofbasil, thanks for your changes. We are currently finalizing the v2.4.2 patch release, I will review your changes after the patch release. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2636 +/- ##
==========================================
+ Coverage 55.05% 55.53% +0.48%
==========================================
Files 148 157 +9
Lines 8532 9002 +470
==========================================
+ Hits 4697 4999 +302
- Misses 3505 3662 +157
- Partials 330 341 +11
☔ View full report in Codecov by Sentry. |
Hi @kishorj, thanks for agreeing to take a look. A few people in Skyscanner have agreed to review it in the mean time. Matteo has already put one review in. Hopefully this makes things easier for you. We are planning on deploying this internally in the next few weeks. We understand it likely won't be reviewed by then - but would it be possible to get the e2e tests ran against this? I would make us feel much more confident in running a fork and setting these up ourselves seems unrealistic. |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only got through 11 of the files, but a few nits and some style questions.
docs/guide/service/annotations.md
Outdated
@@ -47,6 +47,10 @@ | |||
| [service.beta.kubernetes.io/aws-load-balancer-target-node-labels](#target-node-labels) | stringMap | | | | |||
| [service.beta.kubernetes.io/aws-load-balancer-attributes](#load-balancer-attributes) | stringMap | | | | |||
| [service.beta.kubernetes.io/aws-load-balancer-manage-backend-security-group-rules](#manage-backend-sg-rules) | boolean | true | | | |||
| [service.beta.kubernetes.io/aws-load-balancer-endpoint-service-enabled](#endpoint-service-enable)| boolean | false | | | |||
| [service.beta.kubernetes.io/aws-load-balancer-endpoint-service-acceptance-required](#endpoint-service-acceptance)| boolean | | | | |||
| [service.beta.kubernetes.io/aws-load-balancer-endpoint-service-allowed-principals](#endpoint-allowed-principles)| stringList | | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
| [service.beta.kubernetes.io/aws-load-balancer-endpoint-service-allowed-principals](#endpoint-allowed-principles)| stringList | | | | |
| [service.beta.kubernetes.io/aws-load-balancer-endpoint-service-allowed-principals](#endpoint-allowed-principals)| stringList | | | |
docs/guide/service/annotations.md
Outdated
|
||
- <a name="endpoint-service-acceptance">`service.beta.kubernetes.io/aws-load-balancer-endpoint-service-acceptance-required`</a> specifies whether requests to attach an Endpoint to the Endpoint Service require manual acceptance. | ||
|
||
- <a name="endpoint-allowed-principles">`service.beta.kubernetes.io/aws-load-balancer-endpoint-service-allowed-principals`</a> is a list of principles from which an Endpoint can be attached to this Endpoint Service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matching nit:
- <a name="endpoint-allowed-principles">`service.beta.kubernetes.io/aws-load-balancer-endpoint-service-allowed-principals`</a> is a list of principles from which an Endpoint can be attached to this Endpoint Service. | |
- <a name="endpoint-allowed-principals">`service.beta.kubernetes.io/aws-load-balancer-endpoint-service-allowed-principals`</a> is a list of principles from which an Endpoint can be attached to this Endpoint Service. |
docs/guide/service/annotations.md
Outdated
|
||
- <a name="endpoint-allowed-principles">`service.beta.kubernetes.io/aws-load-balancer-endpoint-service-allowed-principals`</a> is a list of principles from which an Endpoint can be attached to this Endpoint Service. | ||
|
||
- <a name="endpoint-private-dns">`service.beta.kubernetes.io/aws-load-balancer-endpoint-service-private-dns-name`</a> is the private DNS name given to the Endpoint Service. This will need to be verifies through a valid DNS record. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- <a name="endpoint-private-dns">`service.beta.kubernetes.io/aws-load-balancer-endpoint-service-private-dns-name`</a> is the private DNS name given to the Endpoint Service. This will need to be verifies through a valid DNS record. | |
- <a name="endpoint-private-dns">`service.beta.kubernetes.io/aws-load-balancer-endpoint-service-private-dns-name`</a> is the private DNS name given to the Endpoint Service. This will need to be verified through a valid DNS record. |
ReconcilePermissions(ctx context.Context, permissions *ec2model.VPCEndpointServicePermissions) error | ||
} | ||
|
||
// NewdefaultEndpointServiceManager constructs new defaultEndpointServiceManager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
// NewdefaultEndpointServiceManager constructs new defaultEndpointServiceManager. | |
// NewDefaultEndpointServiceManager constructs new defaultEndpointServiceManager. |
externalManagedTags []string | ||
} | ||
|
||
func (m *defaultEndpointServiceManager) Create(ctx context.Context, resSG *ec2model.VPCEndpointService) (ec2model.VPCEndpointServiceStatus, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems there's still a few references to SG
rather than ES
throughout this function.
func (m *defaultEndpointServiceManager) Create(ctx context.Context, resSG *ec2model.VPCEndpointService) (ec2model.VPCEndpointServiceStatus, error) { | |
func (m *defaultEndpointServiceManager) Create(ctx context.Context, resES *ec2model.VPCEndpointService) (ec2model.VPCEndpointServiceStatus, error) { |
|
||
func (m *defaultEndpointServiceManager) Update(ctx context.Context, resES *ec2model.VPCEndpointService, sdkES networking.VPCEndpointServiceInfo) (ec2model.VPCEndpointServiceStatus, error) { | ||
|
||
m.logger.Info("Updating", "resES", resES, "sdkES", sdkES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to log this if no changes are being made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In pkg/deploy/ec2/endpoint_service_synthesizer.go
we calculate the endpoint services that need created, deleted and updated and only call the relevant function. If no endpoint services need updated then this function is never called. It feels safe to me.
} | ||
|
||
func (m *defaultEndpointServiceManager) ReconcilePermissions(ctx context.Context, permissions *ec2model.VPCEndpointServicePermissions) error { | ||
m.logger.Info("Reconciling Permissions") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern through the rest of the codebase is lower case logging. Do we want to deviate from that here?
m.logger.Info("Build priciples", | ||
"AddPrinciples", addPrinciples, | ||
"RemovePrinciples", removePrinciples, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m.logger.Info("Build priciples", | |
"AddPrinciples", addPrinciples, | |
"RemovePrinciples", removePrinciples, | |
) | |
m.logger.Info("build principals", | |
"addPrinciples", addPrincipals, | |
"removePrinciples", removePrincipals, | |
) |
docs/guide/ingress/annotations.md
Outdated
@@ -57,6 +57,10 @@ You can add annotations to kubernetes Ingress and Service objects to customize t | |||
|[alb.ingress.kubernetes.io/actions.${action-name}](#actions)|json|N/A|Ingress|N/A| | |||
|[alb.ingress.kubernetes.io/conditions.${conditions-name}](#conditions)|json|N/A|Ingress|N/A| | |||
|[alb.ingress.kubernetes.io/target-node-labels](#target-node-labels)|stringMap|N/A|Ingress,Service|N/A| | |||
|[alb.ingress.kubernetes.io/aws-load-balancer-endpoint-service-enabled](#endpoint-service-enable)|boolean|false| | |||
|[alb.ingress.kubernetes.io/aws-load-balancer-endpoint-service-acceptance-required](#endpoint-service-acceptance)|boolean|| | |||
|[alb.ingress.kubernetes.io/aws-load-balancer-endpoint-service-allowed-principals](#endpoint-allowed-principles)|stringList|| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
|[alb.ingress.kubernetes.io/aws-load-balancer-endpoint-service-allowed-principals](#endpoint-allowed-principles)|stringList|| | |
|[alb.ingress.kubernetes.io/aws-load-balancer-endpoint-service-allowed-principals](#endpoint-allowed-principals)|stringList|| |
|
||
serviceId, err := permissions.Spec.ServiceId.Resolve(ctx) | ||
if err != nil { | ||
return errors.Wrap(err, "Failed to resolve VPCEndpointServicePermissions serviceID") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as with logging casing.
e0ca744
to
488bf6a
Compare
488bf6a
to
af0618d
Compare
This has been rebased and the formatting changes above implemented. |
I'm unable to replicate the e2e test failure. I've added the following resources to a cluster running this branch of the controller.
This brings up a load balancer which successfully provisions. I've then deleted the Ingress and the load balancer successfully deletes. Then deleted the namespace which also successfully deletes. From what I can see this follows the exact logic of the test. As such I'm going to trigger a rerun to see if it was a flake. |
/retest |
I didn't see it while reading the code, but I'm wondering whether there are more IAM actions that we'll need to allow in the IAM policy used by this controller. |
You were correct, after adding these the tests are passing. Thanks! |
This is a continuation of their work. Rifelpet provided a rough outline for the code; function definitions and structs mainly. This adds the actual implementation to make the feature work. It sticks pretty closely to Rifelpet's original design - and even includes their commits showing how it was built up from there. |
Thank you for the detailed explanation. I was going to say earlier this morning that configuring the private DNS name on the VPC endpoint service side seemed backwards to me, thinking that it would be consumers creating interface VPC endpoints that would decide on the name, but then I went back and read the AWS documentation on the feature, and see that your design matches that feature's interface: the service provider chooses a DNS name. I still find this so confusing. That's not your fault, though. |
sgTags := m.trackingProvider.ResourceTags(resES.Stack(), resES, resES.Spec.Tags) | ||
sdkTags := convertTagsToSDKTags(sgTags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgTags := m.trackingProvider.ResourceTags(resES.Stack(), resES, resES.Spec.Tags) | |
sdkTags := convertTagsToSDKTags(sgTags) | |
esTags := m.trackingProvider.ResourceTags(resES.Stack(), resES, resES.Spec.Tags) | |
sdkTags := convertTagsToSDKTags(esTags) |
for _, sdkSG := range unmatchedSDKESs { | ||
if err := s.esManager.Delete(ctx, sdkSG); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for _, sdkSG := range unmatchedSDKESs { | |
if err := s.esManager.Delete(ctx, sdkSG); err != nil { | |
for _, sdkES := range unmatchedSDKESs { | |
if err := s.esManager.Delete(ctx, sdkES); err != nil { |
sdk networking.VPCEndpointServiceInfo | ||
} | ||
|
||
func matchResAndSDKEndpointServices(resSGs []*ec2model.VPCEndpointService, sdkSGs []networking.VPCEndpointServiceInfo, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func matchResAndSDKEndpointServices(resSGs []*ec2model.VPCEndpointService, sdkSGs []networking.VPCEndpointServiceInfo, | |
func matchResAndSDKEndpointServices(resESs []*ec2model.VPCEndpointService, sdkESs []networking.VPCEndpointServiceInfo, |
sdk networking.VPCEndpointServiceInfo | ||
} | ||
|
||
func matchResAndSDKEndpointServices(resSGs []*ec2model.VPCEndpointService, sdkSGs []networking.VPCEndpointServiceInfo, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More references to 'sg' in this function that should be updated to 'es'
pkg/deploy/ec2/tagging_manager.go
Outdated
for sgID, esInfo := range esInfoByIDForTagFilter { | ||
esInfoByID[sgID] = esInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for sgID, esInfo := range esInfoByIDForTagFilter { | |
esInfoByID[sgID] = esInfo | |
for esID, esInfo := range esInfoByIDForTagFilter { | |
esInfoByID[esID] = esInfo |
Thanks @rifelpet, seems quite a few of these typos escaped me. |
just wondering if it's ready to be merged? would like to have this feature in the default branch. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hintofbasil The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Any update on this? |
The code is up to date on my end. Just waiting for a review from kishorj and/or M00nf1sh. I presume it being added to the v2.7.0 roadmap means they are aware it still needs review. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
/assign @oliviassss |
Thanks for your contribution, and sorry for the delay. I will discuss and review this PR internally with our approvers to gather opinions. |
Sorry for the delay, we discussed this feature internally, it's a major feature that needs a minor release, and also we need to do a security review with it. So we aim at shipping it with v2.8.0 |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
Hey @oliviassss AWS has released 2.8.1 of the LB controller and this PR still doesn't seem to have been merged. I don't see support for managing VPC private endpoints natively in the controller's release notes. Any idea what the blocker for this is? We'd love an update, as we have several major architectural changes that depend on this at Salesforce. |
/assign @M00nF1sh |
…lancer-controller into endpoint-service
Any update on this? |
It would be so awesome if this one got merged. We're considering implementing an alternative solution, but doing it through the load balancer controller would be so much cleaner. |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@M00nF1sh and update on this? |
We already have 2.10.0 version of load-balancer-controller and this PR is not part of it. We are trying to design a solution using multiple VPC endpoint services and having native support is helpful. Can you please share the plan on when this PR will make it into a release? |
Hello @oliviassss @kishorj @M00nF1sh ! |
Issue
#1859
Description
This change adds support for creating, deleting and updating VPC endpoint
services along with their permissions. It supports the three suggested
configuration options - allowed priciples, acceptance required, and private DNS
name.
An enable annotation was also added so support enabling/disabling the feature
on a per load balancer basis. The proposed design only supported
enabling/disabling this at the controller level.
Outputting the created VPC Endpoint Service parameters is deliberatly not
supported as there are still open questions as to the best approach to be used
and this change is already far too big and adding more to it does not seem
sensible. This feature can be added in a follow-up change.
This change has been manually tested on a Kubernetes service.
I apologise for the large PR. I couldn't determine suitable breakpoints to split
this change up into smaller changes. I can do so if you can suggest the break
points. I'm also fairly new to Golang so am very happy for any feedback as I
know the code quality isn't amazing here.
Checklist
README.md
, or thedocs
directory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯