Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cloudmap: resolve hostnames before registering #139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cloudmap: resolve hostnames before registering #139

wants to merge 1 commit into from

Conversation

asimpleidea
Copy link
Member

AWS load balancers are DNS-based rather than IP-based.
This means that we need to resolve those host names before registering endpoints to the
service registry, as they only accept IPs.

This commit introduces this change and allows the operator to work fine with EKS as well.

When registering services that have a host name (or multiple ones), the operator will register
its endpoints as usual but it also introduces two new reserved metadata keys:

cnwan.io/hostname: my-hostname.load-balancer.company.com
cnwan.io/last-resolved: 26 Aug 22 08:39 UTC

The first key is self explanatory, while the second one indicates the date and time this was resolved and the IP checks out with the host name. This is because the IP for that host name might very well change and in future version the operator will make sure to change the IP address accordingly.

This addresses #137 and qualifies for v0.8.0.

@asimpleidea asimpleidea added the enhancement New feature or request label Aug 26, 2022
@asimpleidea asimpleidea requested review from ljakab and arnatal August 26, 2022 08:56
@asimpleidea asimpleidea self-assigned this Aug 26, 2022
@asimpleidea
Copy link
Member Author

Please provide feedback for cnwan.io/last-resolved as I am realizing this may not be a very fitting key or may lead to some confusion.

I am thinking of a cnwan.io/applied to indicate when the hostname-ip mapping was created/updated on the service registry. But please provide suggestions in case you have a better name!

@asimpleidea asimpleidea linked an issue Aug 26, 2022 that may be closed by this pull request
EKS: load balancer services need to be resolved prior to registering #137
Open
2 tasks
@asimpleidea asimpleidea force-pushed the feat/resolve-loadbalancer-hostnames branch from 74bf721 to 1d794a8 Compare August 29, 2022 08:00
@ljakab
Copy link
Member

ljakab commented Aug 29, 2022

I would suggest cnwan.io/ip-addresses-looked-up or similar to make it explicit that we're talking about the IPs. I haven't looked at the code yet, are you handling the cases when the FQDN resolves to multiple IPs?

A separate suggestion is to use example.com as the top-level domain instead of company.com so that we follow IETF best practices.

@asimpleidea
Copy link
Member Author

Thanks for the feedback @ljakab!

are you handling the cases when the FQDN resolves to multiple IPs?

Yes, if an FQDN resolves to N addresses, you will end up with N Endpoint objects

A separate suggestion is to use example.com as the top-level domain instead of company.com so that we follow IETF best practices.

Thanks, I will write that in the doc!

@arnatal
Copy link
Member

arnatal commented Aug 29, 2022

Sightly side question. Even though the title says cloudmap this is issue we're facing is not with cloudmap itself but rather with EKS, right? Are there any other implications of registering EKS services with any of the other Service Registries that CNWAN supports?

@asimpleidea
Copy link
Member Author

asimpleidea commented Aug 29, 2022 •
edited
Loading

Sightly side question. Even though the title says cloudmap this is issue we're facing is not with cloudmap itself but rather with EKS, right? Are there any other implications of registering EKS services with any of the other Service Registries that CNWAN supports?

Yes, you are right. The reason for this being labelled as cloudmap is because for now this only works when using Cloud Map, as the current code architecture (dated and soon-to-be deprecated) would require too many changes or a lot of duplicate code. Since this is a temporary fixing -- as the a future version will bring new APIs -- I may just copy this and put it for the others service registries as well.

With the new APIs and controllers, this algorithm will be moved in such a way that it will work with any provider, not just EKS.

@asimpleidea
cloudmap: resolve hostnames before registering (#137)
815299a 
AWS load balancers are DNS-based rather than IP-based.
This means that we need to resolve those host names before registering
endpoints to the service registry, as they only accept IPs.

This commit introduces this change and allows the operator to work fine
with EKS as well.
@asimpleidea asimpleidea force-pushed the feat/resolve-loadbalancer-hostnames branch from 1d794a8 to 815299a Compare September 1, 2022 07:26
@asimpleidea asimpleidea mentioned this pull request Mar 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljakab ljakab Awaiting requested review from ljakab

@arnatal arnatal Awaiting requested review from arnatal

At least 1 approving review is required to merge this pull request.

Assignees

@asimpleidea asimpleidea

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EKS: load balancer services need to be resolved prior to registering
3 participants
@asimpleidea @ljakab @arnatal