Skip to content

Conversation

@msvticket
Copy link
Contributor

@msvticket msvticket commented Feb 12, 2024

Issue

#3505

Description

I added support for ip target groups when service is of type ExternalName.

I have not updated any documentation, but I'm happy to do that in case you want the code change to begin with.

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the docs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 12, 2024
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 12, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @msvticket!

It looks like this is your first PR to kubernetes-sigs/aws-load-balancer-controller 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-load-balancer-controller has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 12, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @msvticket. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 12, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 12, 2024
@msvticket msvticket changed the title Adding support for creating targets from ExternalName feat: adding support for creating targets from ExternalName Mar 1, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2024
@msvticket msvticket force-pushed the externalname branch 2 times, most recently from db436ab to e265988 Compare March 16, 2024 11:49
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2024
@msvticket
Copy link
Contributor Author

Could you have a look at this @oliviassss ?

@Leen15
Copy link

Leen15 commented May 30, 2024

any update on this?

@MateuszWkDev
Copy link

I woul like to get update on this as this seems much needed functionality in order to support certain usecases

@Reyalsorik
Copy link

Any update? I would appreciate this functionality.

@nousot-cloud-guy
Copy link

Is there anything that's holding up this PR that someone could help finish? This would be really useful functionality to have.

@msvticket
Copy link
Contributor Author

Is this something you can look at @shraddhabang?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2024
@felipegouveiae
Copy link

@johngmyers @M00nF1sh please review and merge! I'm begging you!

@msvticket msvticket force-pushed the externalname branch 2 times, most recently from f553585 to bffcdc0 Compare December 11, 2024 10:43
@msvticket
Copy link
Contributor Author

I'm excited about this functionality getting built. I do think we need to be cognizant of dependents on this package and maintain a stable API surface for v2 however. Seems like some simple fix ups should get is back into that territory.

I don't mind putting some more time in this PR if it could be merged. But since this PR is ignored by the maintainers I don't see any point in spending that time now.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2025
@msvticket msvticket force-pushed the externalname branch 2 times, most recently from 0fb4be9 to 7f44c19 Compare May 7, 2025 12:07
@msvticket msvticket marked this pull request as draft May 7, 2025 13:25
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 7, 2025
@msvticket msvticket marked this pull request as ready for review May 7, 2025 13:25
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 7, 2025
@msvticket
Copy link
Contributor Author

Hi @zac-nixon!

You seem to be a newish active maintainer of aws-load-balancer-controller. Can you please review this old PR? I've just rebased it on main, built it and tested it on a staging cluster.

@deens-cyera
Copy link

Hey, any update in the pr ?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2025
@chris-codaio
Copy link

Another vote to get this reviewed & committed - just ran into this limitation this week!

}
if svc.Spec.Type == corev1.ServiceTypeExternalName && rawTargetType != string(elbv2model.TargetTypeIP) {
t.logger.Info("Target type will be ip since service is an ExternalName", "service", k8s.NamespacedName(svc))
rawTargetType = string(elbv2model.TargetTypeIP)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend throwing an error rather then overwriting this value.

Copy link
Contributor Author

@msvticket msvticket Aug 21, 2025

Choose a reason for hiding this comment

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

I think returning an error if something else than ip is set in an annotation makes sense, but still default to ip. What do you think about that?

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 have now pushed a commit with that change

@zac-nixon zac-nixon assigned zac-nixon and unassigned zac-nixon Aug 20, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: msvticket
Once this PR has been reviewed and has the lgtm label, please ask for approval from zac-nixon. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zac-nixon
Copy link
Collaborator

Hi. I'd like to update everyone on the conversation we had in Slack.

Zac Nixon
  [1:55 PM](https://kubernetes.slack.com/archives/D09ARVB64TF/p1755723355218379)
I reviewed the PR, the code your wrote makes sense but I have a concern. How does the controller know that the DNS record has been updated, so that it can update the target group? Right now, you're relying the sync time (~10 hours) to do the DNS updates. It seems that we would need some background process that is querying dns periodically and enqueuing events when the record changes.


Mårten Svantesson
  [8:51 AM](https://kubernetes.slack.com/archives/D09ARVB64TF/p1755791499599059)
Well, my thought is that this is an initial implementation that I tried to keep as simple as possible. You are right in that it probably wont suffice for everybody, but I think it is better to base improvements on feedback rather than guesses.


Zac Nixon
  [7:53 AM](https://kubernetes.slack.com/archives/D09ARVB64TF/p1755874427108159)
Yeah, I see what you're saying. I really appreciate the PR, but generally we don't accept features that aren't complete. Not having a way to timely update the LoadBalancer when the DNS entries changes seems like a big problem. Can you explain how this feature helps you out?


Mårten Svantesson
  [7:09 AM](https://kubernetes.slack.com/archives/D09ARVB64TF/p1756130961627789)
I use it to implement this solution: https://aws.amazon.com/blogs/networking-and-content-delivery/hosting-internal-https-static-websites-with-alb-s3-and-privatelink/
By using an ExternalName for s3 I don’t need a separate ALB but can use the same that is used for accessing internal application running in EKS.
Amazon Web ServicesAmazon Web Services
[Hosting Internal HTTPS Static Websites with ALB, S3, and PrivateLink | Amazon Web Services](https://aws.amazon.com/blogs/networking-and-content-delivery/hosting-internal-https-static-websites-with-alb-s3-and-privatelink/)
Amazon Simple Storage Service (Amazon S3) is a powerful platform that enables you to do various tasks. One notable feature is the ability to create a bucket with an FQDN, point an alias record to the bucket website endpoint, and immediately get up-and-running with an HTTP static website. If you want to serve HTTPS traffic […]
Dec 30th, 2022
https://aws.amazon.com/blogs/networking-and-content-delivery/hosting-internal-https-static-websites-with-alb-s3-and-privatelink/



Zac Nixon
  [8:36 AM](https://kubernetes.slack.com/archives/D09ARVB64TF/p1756136188696239)
Thanks for the link. I suspect that this works for your case because AFAIK, Private Link uses a static IP address.
[8:37](https://kubernetes.slack.com/archives/D09ARVB64TF/p1756136238044719)
Generally, we would like to supply more general solutions, this one has a pretty sharp edge where Target Group changes aren't propagated very fast.

TL;DR - There is some more work to be done here to accept this change. I don't really have the time until ~October to put more stuff on my plate.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 24, 2025
@k8s-ci-robot
Copy link
Contributor

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.

@msvticket
Copy link
Contributor Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.