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.

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 private_ips attribute to aws_lb resource #2901

Closed
wants to merge 1 commit into from
Closed

Add private_ips attribute to aws_lb resource #2901

wants to merge 1 commit into from

Conversation

joshuaspence
Copy link
Contributor

Network load balancers don't have security groups, which makes it difficult to configure security group rules for the backends, which is required for health check to function correctly. According to the documentation, the way to configure security group rules for health checks involves the following steps:

  1. Open the Amazon EC2 console at https://console.aws.amazon.com/ec2/.
  2. In the navigation pane, choose "Network Interfaces".
  3. In the search field, type the name of your Network Load Balancer. There is one network interface per load balancer subnet.
  4. On the Details tab for each network interface, copy the address from Primary private IPv4 IP.

Obviously, this manual process doesn't play well with Terraform. This issue has also been raised on the AWS discussion forums.

This pull request adds a private_ips attribute to the aws_lb resource to workaround what seems to be an oversight on the part of AWS.

@joshuaspence
Copy link
Contributor Author

I'm not entirely convinced that this is a good idea, but am looking for feedback.

@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. service/elbv2 Issues and PRs that pertain to the elbv2 service. labels Jan 11, 2018
@bmathieu33
Copy link

AWS documentation actually says to do so:

If you do not want to grant access to the entire VPC CIDR, you can grant access to the private IP addresses used by the load balancer nodes. There is one IP address per load balancer subnet.

This is needed to have health check access restriction (for some people its problematic to allow the whole VPC subnet, especially if health check port and application are the same).

@@ -54,7 +54,7 @@ Terraform will autogenerate a name beginning with `tf-lb`.
* `access_logs` - (Optional) An Access Logs block. Access Logs documented below.
* `subnets` - (Optional) A list of subnet IDs to attach to the LB. Subnets
cannot be updated for Load Balancers of type `network`. Changing this value
will for load balancers of type `network` will force a recreation of the resource.
will for load balancers of type `network` will force a recreation of the resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind to fix typo here in the mean time? I think there's an extra will. It probably should be:

Changing this value for load balancers of type network will force a recreation of the resource.

@ungureanuvladvictor
Copy link
Contributor

Is there any update on this PR?

Would really love to have access to the private ips generated for the aws_lb resource.

@frittentheke
Copy link

@joshuaspence Seeing that there already is the array of LoadBalancerAddresses (see: https://forums.aws.amazon.com/thread.jspa?threadID=263245) I wonder if AWS is going to return the private addresses with the ELBv2 query at some point.

@simonvanderveldt
Copy link
Contributor

Would really like this functionality because of the use case mentioned in the PRs description.
@joshuaspence have you been using this functionality since you made the PR? How's it working for you?

@frittentheke I wouldn't hold my breath, what AWS puts in their API/responses doesn't really mean a lot. For example: There's been a loadbalancers (plural) key in the ECS service definition as well since the start, hinting at support for multiple loadbalancers, but it still only takes a single loadbalancer.

@joshuaspence
Copy link
Contributor Author

I'm not actually using this functionality at the moment, but when I originally submitted the pull request I did test it out and it appeared to work as I expected.

@ctreatma
Copy link
Contributor

Does this PR just need a rebase in order to get merged? This feature would be very useful & I'm happy to do whatever I can to get it finalized.

@joshuaspence
Copy link
Contributor Author

I'm happy to rebase it, I just haven't bothered as the upstream doesn't seem interested.

@fawaf
Copy link

fawaf commented Sep 12, 2018

any plans on rebasing and merging this in? super helpful for a lot of us.

@ngamber
Copy link

ngamber commented Apr 1, 2019

A rebase and merge would be greatly appreciated. Not happy with whitelisting a whole VPC or dealing with the kludgy workarounds.

Network load balancers don't have security groups, which makes it difficult to configure security group rules for the backends, which is required for health check to function correctly. According to the [documentation](https://docs.aws.amazon.com/elasticloadbalancing/latest/network/target-group-register-targets.html), the way to configure security group rules for health checks involves the following steps:

  1. Open the Amazon EC2 console at https://console.aws.amazon.com/ec2/.
  2. In the navigation pane, choose "Network Interfaces".
  3. In the search field, type the name of your Network Load Balancer. There is one network interface per load balancer subnet.
  4. On the Details tab for each network interface, copy the address from Primary private IPv4 IP.

Obviously, this manual process doesn't play well with Terraform. This issue has also been raised on the [AWS discussion forums](https://forums.aws.amazon.com/thread.jspa?threadID=263245).

This pull request adds a `private_ips` attribute to the `aws_lb` resource to workaround what seems to be an oversight on the part of AWS.
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Apr 9, 2019
@joshuaspence
Copy link
Contributor Author

Rebased

@spa-87
Copy link

spa-87 commented Apr 19, 2019

@loivis Could your review this PR one more time, please. It's highly-awaited feature by many people (including me) :). Thanks in advance.

@oonisim
Copy link

oonisim commented Jun 22, 2019

While the feature is not available.

Using datasource aws_network_interfaces

To run separately after aws_lb has been created.

data "aws_network_interfaces" "this" {
  filter {
    name = "description"
    values = ["ELB net/${var.alb_nlb_name}/*"]
  }
  filter {
    name = "vpc-id"
    values = ["${local.vpc_chat_service_id}"]
  }
  filter {
    name = "status"
    values = ["in-use"]
  }
  filter {
    name = "attachment.status"
    values = ["attached"]
  }
}

locals {
  nlb_network_interface_ids = "${flatten(["${data.aws_network_interfaces.this.ids}"])}"
}

data "aws_network_interface" "ifs" {
  count = "${length(local.nlb_network_interface_ids)}"
  id = "${local.nlb_network_interface_ids[count.index]}"
}

locals {
  aws_nlb_network_interface_ips = "${flatten([data.aws_network_interface.ifs.*.private_ips])}"
  aws_nlb_network_interface_cidr_blocks = [ for ip in local.aws_nlb_network_interface_ips : "${ip}/32" ]
}

Using external provider

At the time when aws_lb is created.

nlb_private_ips.tf

variable "nlb_name" {
}
variable "vpc_id" {
}
variable "region" {
}

data "external" "get_nlb_ips" {
  program = ["python", "${path.module}/get_nlb_private_ips.py"]
  query = {
    aws_nlb_name  = "${var.nlb_name}"
    aws_vpc_id    = "${var.vpc_id}"
    aws_region    = "${var.region}"
  }
}

output "aws_nlb_ip" {
  value = "${jsondecode(data.external.get_nlb_ips.result.private_ips)}"
}

get_nlb_private_ips.py

import boto3
import json
import sys


def json_serial(obj):
    """JSON serializer for objects not serializable by default json code
        Args:
            obj: object to serialize into JSON
    """
    _serialize = {
        "int": lambda o: int(o),
        "float": lambda o: float(o),
        "decimal": lambda o: float(o) if o % 1 > 0 else int(o),
        "date": lambda o: o.isoformat(),
        "datetime": lambda o: o.isoformat(),
        "str": lambda o: o,
    }
    return _serialize[type(obj).__name__.lower()](obj)


def pretty_json(dict):
    """
    Pretty print Python dictionary
    Args:
        dict: Python dictionary
    Returns:
        Pretty JSON
    """
    return json.dumps(dict, indent=2, default=json_serial, sort_keys=True, )


def get_nlb_private_ips(data):
    ec2 = boto3.client('ec2', region_name=data['aws_region'])
    response = ec2.describe_network_interfaces(
        Filters=[
            {
                'Name': 'description',
                'Values': [
                    "ELB net/{AWS_NLB_NAME}/*".format(
                        AWS_NLB_NAME=data['aws_nlb_name'])
                ]
            },
            {
                'Name': 'vpc-id',
                'Values': [
                    data['aws_vpc_id']
                ]
            },
            {
                'Name': 'status',
                'Values': [
                    "in-use"
                ]
            },
            {
                'Name': 'attachment.status',
                'Values': [
                    "attached"
                ]
            }
        ]
    )

    # print(pretty_json(response))
    interfaces = response['NetworkInterfaces']

    # ifs = list(map(lamba index: interfaces[index]['PrivateIpAddresses'], xrange(len(interfaces))))
    # --------------------------------------------------------------------------------
    # Private IP addresses associated to an interface (ENI)
    # Each association has the format:
    #   {
    #     "Association": {
    #       "IpOwnerId": "693054447076",
    #       "PublicDnsName": "ec2-52-88-47-177.us-west-2.compute.amazonaws.com",
    #       "PublicIp": "52.88.47.177"
    #     },
    #     "Primary": true,
    #     "PrivateDnsName": "ip-10-5-1-205.us-west-2.compute.internal",
    #     "PrivateIpAddress": "10.5.1.205"
    #   },
    # --------------------------------------------------------------------------------
    associations = [
        association for interface in interfaces
        for association in interface['PrivateIpAddresses']
    ]
    # --------------------------------------------------------------------------------
    # Get IP from each IP association
    # --------------------------------------------------------------------------------
    private_ips = [association['PrivateIpAddress'] for association in
                   associations]

    return private_ips


def load_json():
    data = json.load(sys.stdin)
    return data


def main():
    data = load_json()
    """
    print(data['aws_region'])
    print(data['aws_vpc_id'])
    print(data['aws_nlb_name'])
    """
    ips = get_nlb_private_ips(data)
    print(json.dumps({"private_ips": json.dumps(ips)}))


if __name__ == '__main__':
    main()

@aeschright aeschright requested a review from a team June 25, 2019 18:51
@dlutsch
Copy link

dlutsch commented Jul 8, 2019

are there any plans to move forward with merging this? This is a feature many of us who use NLBs are waiting for.

@mdugdale
Copy link

is there any update on this? this feature would be very helpful

@bazmera
Copy link

bazmera commented Aug 23, 2019

Really appreciate if this is merged.It would be very helpful feature.

@h0nIg
Copy link

h0nIg commented Sep 10, 2019

@joshuaspence @loivis can you please merge this, this functionality would be very helpful

@joshuaspence
Copy link
Contributor Author

I'm not affiliated with Hashicorp in any way... I don't have permission to merge this.

@loivis
Copy link
Contributor

loivis commented Sep 10, 2019

Me neither.

@sjpalf
Copy link

sjpalf commented Nov 3, 2019

@maryelizbeth what's needed to get this PR merged (other than another rebase!)? It's been open almost 2 years now, it's a pretty trivial change, and a lot of people would find this really useful!

@wwboynton
Copy link

Kind of embarrassing how long this PR has been hanging around. @maryelizbeth any chance we can get another set of eyes on this and give @joshuaspence the okay to rebase so we can merge the PR and be done? Unless we're all missing something, this seems like a trivial change with a high positive impact for many of us.

@h0nIg
Copy link

h0nIg commented Nov 25, 2019

fixed conflicts: #11000, ready for merge

@wwboynton
Copy link

@maryelizbeth can someone from Hashicorp at least comment publicly on why this can't be merged yet?

@mixedCase
Copy link

A late happy 2 year birthday to this PR! This feature still pretty much mandatory to work with NLBs in many situations and has to be worked around.

@davidferguson-cr
Copy link

Worth tracking AWS CloudFormation feature request aws-cloudformation/cloudformation-coverage-roadmap#305 for the AWS::ElasticLoadBalancingV2::LoadBalancer-GetAtt-IpAddressList capability to see who gets there first...

@mattrayner
Copy link

Any news on this? I currently need it, or will have to whitelist the whole VPC :/

@fawaf
Copy link

fawaf commented Feb 2, 2020

+1 for merging, but conflicting files again.

@h0nIg
Copy link

h0nIg commented Feb 2, 2020

@gdavison @bflad can you please take a look? this is extremly cumbersome, since this PR will bring so much value and nobody is taking a look at it.

@h0nIg
Copy link

h0nIg commented Mar 6, 2020

fixed conflicts: #11000, ready for merge. can you please take a look? @bflad @radeksimko @Ninir @ryndaniels @stack72 @nywilken @gdavison @grubernaut @jen20 @catsby @aeschright @paddycarver @appilon @tombuildsstuff @apparentlymart @jbardin @abinashmeher999 @hendrik363

@binarymist
Copy link

Really need this..

@strowi
Copy link

strowi commented Apr 15, 2020

Also hoping for this to get merged soon...

@wwboynton
Copy link

It's one thing for a PR to drag on, it happens...but it's boggling that folks from Hashicorp refuse to even respond to explain why this is the case. Folks like @apparentlymart are usually incredibly kind and pretty upfront about explaining when a PR does not meet the standards or otherwise conflicts with the direction of the project, but this PR has been allowed to go ignored by the Terraform team for multiple years now. It's really, really disappointing behaviour...

@anthonymade
Copy link

Can somebody please comment on why this PR has not been merged and what would need to happen for this feature to be added?

@jen20
Copy link
Contributor

jen20 commented Apr 20, 2020

@h0nIg et al: sorry, I am not affiliated with HashiCorp and cannot merge things into the AWS provider!

@joshuaspence
Copy link
Contributor Author

I am also not affiliated with Hashicorp in any way and cannot merge this PR

@maryelizbeth
Copy link
Contributor

maryelizbeth commented Apr 21, 2020

Hi Y’all,

First, an apology. This PR got lost in the shuffle and we're sorry for not picking it back up or posting an update in a timely fashion.

We had been punting on this PR because it conflicts with two design principles:

  • Introduces functionality not present in the ELBv2 API
  • Introduces cross-service functionality with EC2. We avoid cross-service handling in resources, as it has the potential to introduce confusing permissions and behaviors in constrained environments.

A few months ago the ELBv2 API added the ability to configure the private IPs manually. That functionality is addressed in #11404.

We’ll work with the author of #11404 to address any changes that may be needed to merge the PR and will merge it once the work is complete.

As a result, we will close this PR in order to focus conversation on #11404.

If you feel that #11404 does not adequately address your workflow, please open a new GitHub Issue describing the gap. If you have concerns regarding the design of the upstream API, please reach out to AWS.

Once again, we apologize for letting this linger without response and will work to merge #11404 in an upcoming release.

@ghost
Copy link

ghost commented May 21, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/elbv2 Issues and PRs that pertain to the elbv2 service. size/XS Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.