AWS SDK Go V2 Upgrade of ELB and ELBv2#1157
Conversation
|
This issue is currently awaiting triage. If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the The DetailsInstructions 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. |
|
Hi @gargipanatula. Thanks for your PR. I'm waiting for a kubernetes 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. DetailsInstructions 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. |
| input.HealthyThresholdCount = aws.Int32(mapping.HealthCheckConfig.HealthyThreshold) | ||
| input.UnhealthyThresholdCount = aws.Int32(mapping.HealthCheckConfig.UnhealthyThreshold) | ||
| } | ||
| if !strings.EqualFold(mapping.HealthCheckConfig.Protocol, elbv2.ProtocolEnumTcp) { |
There was a problem hiding this comment.
looks like this string compare change is non-trivial. do we know why we need EqualFold in v1?
There was a problem hiding this comment.
The values we're comparing here are now both enums, whereas before they were strings. AFAIK this removes the need to worry about casing, hence why EqualFold was removed.
More enums are supported by V2, so that's why I changed structs to use enums wherever possible.
| } | ||
|
|
||
| // ValidateHealthCheck replaces ELB.HealthCheck.Validate() from AWS SDK Go V1, which has been deprecated in V2 | ||
| func ValidateHealthCheck(s *elbtypes.HealthCheck) error { |
There was a problem hiding this comment.
can we link the original v1 implementation either in the code comment or here?
| p.AddHandlers(regionName, &elbClient.Handlers) | ||
|
|
||
| p.AddHandlersV2(ctx, regionName, &cfg) | ||
| var opts []func(*elb.Options) = p.cfg.GetELBEndpointOpts(regionName) |
There was a problem hiding this comment.
nit: opts := []func(*elb.Options) {... ...} instead of appending one-by-one. each append is a malloc op under the hood
There was a problem hiding this comment.
Oh I see, do you mean something like this:
var opts []func(*ec2.Options) = p.cfg.GetEC2EndpointOpts(regionName)
opts = append(opts, func(o *ec2.Options) {
o.Retryer = &customRetryer{
retry.NewStandard(),
}
o.EndpointResolverV2 = p.cfg.GetCustomEC2Resolver()
})
Couldn't figure out how spread p.cfg.GetELBEndpointOpts(regionName) into a composite literal ([]func(*elb.Options){ ... }), let me know if i'm missing something
There was a problem hiding this comment.
var opts []func(*ec2.Options) = append(
p.cfg.GetEC2EndpointOpts(regionName),
func(o *ec2.Options) {
o.Retryer = &customRetryer{
retry.NewStandard(),
}
o.EndpointResolverV2 = p.cfg.GetCustomEC2Resolver()
})
i was thinking about an oneliner like this
|
|
||
| if targetGroup != nil && (!strings.EqualFold(mapping.HealthCheckConfig.Protocol, aws.StringValue(targetGroup.HealthCheckProtocol)) || | ||
| mapping.HealthCheckConfig.Interval != aws.Int64Value(targetGroup.HealthCheckIntervalSeconds)) { | ||
| if targetGroup != nil && ((mapping.HealthCheckConfig.Protocol != targetGroup.HealthCheckProtocol) || |
There was a problem hiding this comment.
EqualFold is not case sensitive, should we still keep it?
There was a problem hiding this comment.
I took it out because the values we're comparing are now both enums of type elb.ProtocolEnum, whereas before they were strings. AFAIK we don't need to worry about casing anymore but if there is an edge case I should be aware of then I can put it back in.
There was a problem hiding this comment.
ProtocolEnum is just a string type and doesn't mandate specific values, since we are converting the string to ProtocolEnum for mapping.HealthCheckConfig.Protocol i think its possible for it value to be just http and hence would fail the check
There was a problem hiding this comment.
I see, ok in that case I'll add the EqualFold back in. Thanks for the catch!
|
/ok-to-test |
|
/lgtm |
a13aa4c to
5315185
Compare
|
/retest |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: haoranleo, yue9944882 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/approve |
* Bump k8s lib to 0.30.0-rc.0 * Support rc in tag release * Update dependencies * Bump peter-evans/create-pull-request from 6.0.2 to 6.0.3 Bumps [peter-evans/create-pull-request](https://github.com/peter-evans/create-pull-request) from 6.0.2 to 6.0.3. - [Release notes](https://github.com/peter-evans/create-pull-request/releases) - [Commits](peter-evans/create-pull-request@70a41ab...c55203c) --- updated-dependencies: - dependency-name: peter-evans/create-pull-request dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * (cleanup): Move types to their own files * (cleanup): remove unused volume handling * bumping go to 1.22.2 * (cleanup): remove instance group handling * Update dependencies * Bump peter-evans/create-pull-request from 6.0.3 to 6.0.4 Bumps [peter-evans/create-pull-request](https://github.com/peter-evans/create-pull-request) from 6.0.3 to 6.0.4. - [Release notes](https://github.com/peter-evans/create-pull-request/releases) - [Commits](peter-evans/create-pull-request@c55203c...9153d83) --- updated-dependencies: - dependency-name: peter-evans/create-pull-request dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * bump golang.org/x/net to v0.24.0 Signed-off-by: Flavian Missi <fmissi@redhat.com> * Update dependencies * Update dependencies * Use go-version-file in workflows * Bump Go to 1.23.3 * Bump Go to 1.22.3 in tests/e2e/go.mod * Ensure that addresses are added in network device index order * extract fargate into a standalone file Signed-off-by: Davanum Srinivas <davanum@gmail.com> * Update dependencies * Update dependencies * Update dependencies * allow creation of tokenreviews and subjectaccessreviews Co-Authored-By: Eric Wolak <eric.wolak@reddit.com> * docs(footer): Add trademark disclaimer * bump golang version to 1.22.4 for master branch Signed-off-by: Davanum Srinivas <davanum@gmail.com> * Update dependencies * Add kmala to the owners file * Allow user to specify Registry Pull Secret * Update dependencies * Update dependencies * add support for new aws partitions in credential provider * log useful information about the cluster and the instance Signed-off-by: Davanum Srinivas <davanum@gmail.com> * format the instance metadata details log * Update dependencies * Bump actions/setup-go from 5.0.0 to 5.0.2 Bumps [actions/setup-go](https://github.com/actions/setup-go) from 5.0.0 to 5.0.2. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@0c52d54...0a12ed9) --- updated-dependencies: - dependency-name: actions/setup-go dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * Update dependencies * Handle error while registering/deregistering target during load balancer update * Update dependencies * Bump default branch to 1.31.0 * Switch to latest master of kops Signed-off-by: Davanum Srinivas <davanum@gmail.com> * Add configuration to allow Kube Proxy health checks when using cluster wide services * Ensure removal of security group rules on deleting load balancers * Sorting LB security groups should prefer tagged security group * Make metadata logging best-effort GetInstanceIdentityDocument returns a fatal error when AWS_EC2_METADATA_DISABLED is true. Make the logging best-effort and ignore errors from GetInstanceIdentityDocument. * update golang from 1.22.5 to 1.22.7 * Add node topology labels * Refactors setting node network topology labels * Updates ecr SDK to v2 SDK * Bump actions/setup-go from 5.0.2 to 5.1.0 Bumps [actions/setup-go](https://github.com/actions/setup-go) from 5.0.2 to 5.1.0. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@0a12ed9...41dfa10) --- updated-dependencies: - dependency-name: actions/setup-go dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Migrates InstanceTopologyManager to use sdk v2 * Documents running e2e tests ad hoc * Fixes typos and formatting in development.md * Removes unused autoscaling code * Fixes EC2 SDK v2 client configuration to assume role properly * Gracefully handles getting instance topology and unhandled variants * Update to latest ko * Update ECR Regex to support new dual stack endpoints, modify ECR regex to have 5 capture groups, elimiating un-needed captures, update tests for new endpoints and parseRegionFromECRPrivateHost to support multiple region combinations * release 1.32.0-rc.2 * Version bump 1.32 * Bump actions/setup-go from 5.1.0 to 5.2.0 Bumps [actions/setup-go](https://github.com/actions/setup-go) from 5.1.0 to 5.2.0. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@41dfa10...3041bf5) --- updated-dependencies: - dependency-name: actions/setup-go dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Bump default branch to 1.32.0 * Bump peter-evans/create-pull-request from 6.0.4 to 7.0.6 Bumps [peter-evans/create-pull-request](https://github.com/peter-evans/create-pull-request) from 6.0.4 to 7.0.6. - [Release notes](https://github.com/peter-evans/create-pull-request/releases) - [Commits](peter-evans/create-pull-request@9153d83...67ccf78) --- updated-dependencies: - dependency-name: peter-evans/create-pull-request dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * Update dependencies to resolve CVE-2024-45338 * Tidy go.sum * use typed client-go constructors for rate-limit related objects Signed-off-by: Min Jin <minkimzz@amazon.com> * optimize tagging controller workqueue handling * ecr-credential-provider: Fix warning about no region in image ref * Requires node topology labels to be set for known supported instance types * Optimize node controller describe instances calls * Update instances v2 tests * Bump peter-evans/create-pull-request from 7.0.6 to 7.0.7 Bumps [peter-evans/create-pull-request](https://github.com/peter-evans/create-pull-request) from 7.0.6 to 7.0.7. - [Release notes](https://github.com/peter-evans/create-pull-request/releases) - [Commits](peter-evans/create-pull-request@67ccf78...dd2324f) --- updated-dependencies: - dependency-name: peter-evans/create-pull-request dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * Bump peter-evans/create-pull-request from 7.0.7 to 7.0.8 Bumps [peter-evans/create-pull-request](https://github.com/peter-evans/create-pull-request) from 7.0.7 to 7.0.8. - [Release notes](https://github.com/peter-evans/create-pull-request/releases) - [Commits](peter-evans/create-pull-request@dd2324f...271a8d0) --- updated-dependencies: - dependency-name: peter-evans/create-pull-request dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * feat: add tagging controller delays and work queue size metrics * clean up * Removed redundante work queue size metric and moved the measurement of tagging delay inside tagEc2Instance * added back log lines * Bump actions/setup-go from 5.2.0 to 5.4.0 Bumps [actions/setup-go](https://github.com/actions/setup-go) from 5.2.0 to 5.4.0. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@3041bf5...0aaccfd) --- updated-dependencies: - dependency-name: actions/setup-go dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * bump golang.org/x/oauth2 to v0.27.0 Signed-off-by: Min Jin <minkimzz@amazon.com> * bump golang.org/x/crypto to v0.35.0 Signed-off-by: Min Jin <minkimzz@amazon.com> * bump go runner to v2.3.1-go1.23.2-bookworm.0 Signed-off-by: Min Jin <minkimzz@amazon.com> * Enable concurrent worker syncs for tagging controller * Update default concurrency to 1 * Add validation for concurrency option * Release 1.33.0-beta.0 * 1.33 Rc release * Capturing initial tagging delay only if its the first tagging event. Renamed the metric to be more indicative of what its measuring * remove newlines * improved the function definition * added tests for isInitialTag * Batch tagging controller create/delete tags calls * Add delete tags logging * Remove enable batching flag * Add comments for exported methods * Fix linter errors * Address comments * V1.33.0 Release * upgraded to ec2 v2, buggy * add tagging files * test fixes and middleware additions * added middleware * merged upstream * cleanup debug statements * added test * put instanceid back in * added clarifying comments for v2 impls * updated v2 handler interface * Bump actions/setup-go from 5.4.0 to 5.5.0 Bumps [actions/setup-go](https://github.com/actions/setup-go) from 5.4.0 to 5.5.0. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@0aaccfd...d35c59a) --- updated-dependencies: - dependency-name: actions/setup-go dependency-version: 5.5.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * updated middleware logging * lint fixes * added waitwithcontext, fixed signingregion and endpoint override * added tests for client retry and endpoint resolution * fixed Compute() tests to actually use Compute() * debug msgs * getregion debug * formatting for aws sdk test * more lint fixes * added debugging through describeinstances trace * pass credentials into Compute() directly * removed debug msgs * Added tests for delayPresign and delayAfterRetry * updated error checking tests to be e2e * added default mode to Compute() config initialization * e2e/deps: enhance test scenarios with NLB * fix: reduce log verbosity of tagging controller * Support ServiceAccountToken in credential-provider Extend ecr-credential-provider to support fine-grained access via kubernetes ServiceAccount tokens and STS's AssumeRoleWithWebIdentity. This allows users to avoid long-lived secrets in their pods/nodes and instead use a short-lived credential generated by kubernetes in order to access private ECR images. * fix some lint findings, add name to e2e test pod * add security context to container * run as non-root user * tweak e2e test * swap container image * try using an image tag that actually exists * address review comments, switch image for e2e test * e2e: enhance test scenarios with NLB This change enhance test scenarios by: - supporting more distributions which does not allow pods to bind on privileged ports (default behavior of libjig, see issue - refact tests to allow adding more cases - introduce tests to NLB, including advanced tests to validate the node selector annotation. AWS SDK is added to satisfy this validatoin. * Adding batching to describe instance API calls * Update ELB and ELBV2 packages to AWS SDK Go V2 (kubernetes#1157) * doc: document which LB type each annotation is supported This change introduce a column to the Service Annotation documentation to make clear to users which annotation is valid for each LB type, preventing user's confusing when trying, for example, using an annotation not yet implemented for NLB. * Add kpromo reminder workflow for releases Adds a github workflow that will remind contributors to release new versions. * update go version to 1.24.4 * aws sdk go upgrade * added enable state * reset env var * update aws & awserr to go sdk v2 * refact/lb/sg: isolate sg deletion fragments to be reused Isolating security group deletion fragments from EnsureLoadBalancerDeleted to buildSecurityGroupsToDelete and deleteSecurityGroupsWithBackoff, so the envaluation criteria and backof deletion can be reused in future implementations, i.e. NLB with Security Groups. * adding yue9944882 to eks team approver Signed-off-by: Min Jin <minkimzz@amazon.com> * doc/devel: enhance docs to use in existing cluster Enhance the development documentation with steps to run the controller locally targeting existing clusters. * Update docs/development.md * fix: rule difference for nested objects The Difference method calculation are returning wrong values on service update calculating the security group rules. The core issue is caused by it is always comparing the pointer address, not the values, always generating difference between lists. Example: given s1:{a1, a2} and s2:{a2,a3,a4} s2.Difference(s1) was returning {a2, a3, a4}, instead {a3, a4} * chore(chart): update image to v1.33.0 * chore(chart): update version * e2e/loadbalancer: implement hairpin connection cases Implementing the hairpin connection test cases, and exposing an issue on NLB with internal scheme which fails when the client is trying to access a service loadbalancer which is hosted in the same node. The hairpin connection is caused by the client IP preservation attribute is set to true (default), and the service does not provide an interface to prevent the issue. The e2e is expecting to pass to prevent permanent failures in CI, but it is tracked by an issue kubernetes#1160. * refact: e2e tests documenting hooks and enhance logging/steps This change enhance the logging and ginkgo steps of the loadbalancer reachable e2e test cases. The Hooks, created to allow test case customization, is renamed and documented. Finally the configuration are encapsulated into a single structure to enhance parallel tests. * Add dd config * Add back base image --------- Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Flavian Missi <fmissi@redhat.com> Signed-off-by: Davanum Srinivas <davanum@gmail.com> Signed-off-by: Min Jin <minkimzz@amazon.com> Co-authored-by: haoranleo <haoranr@amazon.com> Co-authored-by: Kubernetes Prow Robot <20407524+k8s-ci-robot@users.noreply.github.com> Co-authored-by: Carter <mckdev@amazon.com> Co-authored-by: github-actions <actions@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Jai Devmane <jdevmane@amazon.com> Co-authored-by: Flavian Missi <fmissi@redhat.com> Co-authored-by: Ciprian Hacman <ciprian@hakman.dev> Co-authored-by: Jeremy Bopp <jeremy@bopp.net> Co-authored-by: Davanum Srinivas <davanum@gmail.com> Co-authored-by: Aastha <sendmail2aastha@gmail.com> Co-authored-by: Eric Wolak <eric.wolak@reddit.com> Co-authored-by: Maria Salcedo <mane.salcedo.gitcommit@gmail.com> Co-authored-by: Keerthan Reddy Mala <kremala@amazon.com> Co-authored-by: Aman Pasi <amanpasi.btech.cs17@iiitranchi.ac.in> Co-authored-by: Jay Deokar <jsdeokar@amazon.com> Co-authored-by: Flora Wang <floraww@amazon.com> Co-authored-by: Joel Speed <joel.speed@hotmail.co.uk> Co-authored-by: Seth Jennings <sjenning@redhat.com> Co-authored-by: vela <wwvela@amazon.com> Co-authored-by: Matt Merkes <merkes@amazon.com> Co-authored-by: Bartlomiej Dworak <bartdwo@amazon.com> Co-authored-by: leigh <lcpulzone@gmail.com> Co-authored-by: Min Jin <minkimzz@amazon.com> Co-authored-by: Shivendra Panicker <panicks@amazon.com> Co-authored-by: Shiv Bhosale <shvbsle@amazon.com> Co-authored-by: Ganesh Putta <ganiredi@amazon.com> Co-authored-by: Gargi Panatula <panatula@amazon.com> Co-authored-by: Marco Braga <mrbraga@redhat.com> Co-authored-by: Fletcher Woodruff <fwood@amazon.com> Co-authored-by: sartthak <sartthak@amazon.com> Co-authored-by: gargipanatula <44065023+gargipanatula@users.noreply.github.com> Co-authored-by: Daniel Kennedy <dannyck@amazon.com> Co-authored-by: Marco Braga <braga@mtulio.eng.br> Co-authored-by: Alberto Mardomingo <alberto.mardomingo@datadoghq.com>
Upgrading the ELB and ELBv2 packages of the AWS SDK Go V2.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The AWS SDK Go V1 is being deprecated on July 31, 2025. This PR migrates usage of the ELB and ELBV2 SDK to use V2.
Context: https://aws.amazon.com/blogs/developer/announcing-end-of-support-for-aws-sdk-for-go-v1-on-july-31-2025/
Ports over ELB and ELBV2 client creation to V2 supported middleware created in #1146.
No major changes to the functionality, but some superficial changes include the switch from int64 to int32, to match the values used by the ELB and ELBV2 packages, and the use of enums (e.g. ProtocolEnum, ActionTypeEnum) over raw strings.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Testing:
Ran unit tests (via
make test). Ran e2e tests with the following commands: