Add resource outputs

[nitrocode]

what

• Add outputs for aws_security_group and aws_security_group_rule resources

why

• If the aws provider ever increased the number of outputs for either of the above resources, this module would be flexible enough to automatically output those additional outputs without having to update this module again
• For example, tags_all output is missing for the aws_security_group and id is missing for aws_security_group_rule

references

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group
• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule

[nitrocode]

/test all

[osterman]

@nitrocode this module uses counts, so probably need to use join-splat pattern.

[SweetOps]

@osterman @nitrocode
First of all I wanna say that addressing to resource without defining output is hack and shouldn't be available at all... just because it can returns some sensitive data (not in this resource but anyway). Second thing is how operate with that outputs in code that's will look like "let's find the right combination of lookup/try over the output just to get ID".

For example, tags_all output is missing

isn't output of resource this is more related to aws-provider

id is missing for aws_security_group_rule

Where it can be used or for what? IMHO the rule id just useless I specially didn't add that output because no resources where it can be used.

Just for me the current naming is horrible. We've module aws-security-group which returns aws_security_group... I can suggest metadata and rules if @osterman want to merge this. And also test for new outputs are missed.

[nitrocode]

Thanks for the feedback @SweetOps !

First of all I wanna say that addressing to resource without defining output is hack and shouldn't be available at all... just because it can returns some sensitive data (not in this resource but anyway).

I see what you mean and your concerns are true. I have also seen outputted resources where it contains a sensitive value and the module fails to apply properly. Hashicorp has added a way in 0.14.x to redact sensitive block values. I haven't given it a try but it might give us the best of both worlds. If not, perhaps we can only output entire resources where there isn't any sensitive data inputted that also appears in the resource's output.

For example, tags_all output is missing

isn't output of resource this is more related to aws-provider

Perhaps that was a bad example, but the point is that if there is an additional output for the resources in this module or in other modules, we can avoid having to update every module to include some additional output of a consumed module.

id is missing for aws_security_group_rule

Where it can be used or for what? IMHO the rule id just useless I specially didn't add that output because no resources where it can be used

This is true and YAGNI seems to be in line with what you're saying. However, it's just an example of an output that someone could request. Who knows what other output is missing from which other resource that we then have to maintain through PRs, issues, etc.

Just for me the current naming is horrible. We've module aws-security-group which returns aws_security_group... I can suggest metadata and rules if @osterman want to merge this. And also test for new outputs are missed.

I agree. I couldn't really think of a good name but as Linus once said, "Less talk, more code" lol so I put in this PR to have this discussion. What name would you suggest if there are multiple resources ? What would be a more consistent/intuitive naming convention ?

metadata_aws_security_group # full resource name
metadata_security_group     # omit aws
meta_aws_security_group     # full resource name, using meta
meta_security_group         # omit aws, using meta

[SweetOps]

metadata_aws_security_group # full resource name
metadata_security_group     # omit aws
meta_aws_security_group     # full resource name, using meta
meta_security_group         # omit aws, using meta

see #1 (comment)

👇

I can suggest metadata and rules

just metadata and just rules without any additional suffixes would be best choice IMO

This is true and YAGNI seems to be in line with what you're saying. However, it's just an example of an output that someone could request. Who knows what other output is missing from which other resource that we then have to maintain through PRs, issues, etc.

I see so many words about potential needs but can't find anything related to real cases. 😜

[nitrocode]

I believe I was mistaken about the redacting block values. Instead, I spoke with apparentymart on hangops and he mentioned this.

If you are talking about root module outputs then yes, it will require you to mark the whole thing as sensitive if there is anything sensitive inside, and so better to be a bit more specific in that case. But for child modules that broad check went away in (I think) v0.15.1, so you can output whatever you want in that case

So once we move to 0.15.1 or now the latest 1.0 for our modules, it will be much easier to avoid the sensitive variable issues.

I see so many words about potential needs but can't find anything related to real cases. 😜

lol. i definitely have put in a few pull requests to add outputs to modules where i wouldn't have had to worry about it if the entire resource was outputted. it's even worse when you have to add the output to a child[1] module, then wait for that to get merged, and finally add the output to child[0] module, so you can finally make use of it in the root module.

just metadata and just rules without any additional suffixes would be best choice IMO

I'm ok with this. @osterman thoughts ?

[nitrocode]

/test all

[Nuru]

I attempted to include this in #17. The problem I ran into is that the outputs are unstable. Here is a zero-change output change:

Click to show

Plan: 0 to add, 0 to change, 0 to destroy.

Changes to Outputs:
  ~ created_sg_details = [
      ~ {
            arn                    = "arn:aws:ec2:us-east-2:126450723953:security-group/sg-02042fba4da894133"
            description            = "Managed by Terraform"
            egress                 = []
            id                     = "sg-02042fba4da894133"
          ~ ingress                = [
              + {
                  + cidr_blocks      = [
                      + "0.0.0.0/0",
                    ]
                  + description      = "Description 2"
                  + from_port        = 443
                  + ipv6_cidr_blocks = []
                  + prefix_list_ids  = []
                  + protocol         = "tcp"
                  + security_groups  = []
                  + self             = false
                  + to_port          = 445
                },
            ]
            name                   = "eg-ue2-test-sg-nuru"
            name_prefix            = ""
            owner_id               = "126450723953"
            revoke_rules_on_delete = false
            tags                   = {
                "Attributes"  = "nuru"
                "Environment" = "ue2"
                "Name"        = "eg-ue2-test-sg-nuru"
                "Namespace"   = "eg"
                "Stage"       = "test"
            }
            tags_all               = {
                "Attributes"  = "nuru"
                "Environment" = "ue2"
                "Name"        = "eg-ue2-test-sg-nuru"
                "Namespace"   = "eg"
                "Stage"       = "test"
            }
            timeouts               = null
            vpc_id                 = "vpc-03e07cf1f8daa053c"
        },
    ]
  ~ created_sg_rules   = [
      ~ {
            cidr_blocks              = [
                "0.0.0.0/0",
            ]
            description              = "Description 2"
            from_port                = 443
            id                       = "sgrule-2663518629"
          ~ ipv6_cidr_blocks         = null -> []
          ~ prefix_list_ids          = null -> []
            protocol                 = "tcp"
            security_group_id        = "sg-02042fba4da894133"
            self                     = false
            source_security_group_id = null
            to_port                  = 445
            type                     = "ingress"
        },
    ]

That is simply not acceptable.

I am closing this PR for that reason and for the reasons @SweetOps articulated. We can add specific outputs as needed.