Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Register IGW service hostname, rather than first IPv4 address. #813

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

krarey
Copy link
Member

@krarey krarey commented Feb 8, 2021

Opening per conversation with @lkysow.

Changes proposed in this PR:

  • Remove the -resolve-hostnames flag from the Ingress Gateway's service-init container. When using a LoadBalancer service, this causes the DNS hostname to be registered in place of the first IPv4 address, bringing several benefits:
    • Consul will return a CNAME, rather than A record for .ingress.consul DNS queries. In the event there are multiple points of ingress (i.e. AWS ELB attached to multiple subnets), the traffic can be distributed in response to load balancer auto-scaling.
    • For load balancer integrations which scale outside of Kubernetes, the Consul service registration will not go stale as a result of this scaling activity.
    • Currently, in the event the service-init container executes before the load balancer resource has finished provisioning, the Consul catalog is updated with an invalid target address for the platform load balancer. Changing this to the LB hostname removes the need to perform a rolling update of the ingress gateway deployment to retrieve the correct address(es).

How I've tested this PR:

  • Deployed to an AWS EKS cluster. Used an NLB LoadBalancer k8s service to front Consul ingress gateways, and tested traffic to a destination Vault service. Feel free to ping me on Slack for full environment details.

How I expect reviewers to test this PR:

Checklist:

  • Bats tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@krarey krarey marked this pull request as ready for review February 9, 2021 00:53
@lkysow lkysow requested review from a team, ndhanushkodi and kschoche and removed request for a team and kschoche February 9, 2021 21:46
@lkysow
Copy link
Member

lkysow commented Feb 9, 2021

This looks good. The previous assumption was that using a DNS entry for address would break everything but that was an incorrect assumption. This is technically a breaking change so we'll want to add a changelog after merge. @ndhanushkodi ping me if you want some more context here/what to look at.

Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@lkysow lkysow merged commit 5f30a69 into hashicorp:master Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants