search

cloudposse / terraform-aws-security-group

Terraform module to provision an AWS Security Group
https://cloudposse.com/accelerate
Apache License 2.0
36 stars 35 forks source link

Add resource outputs #16

Closed nitrocode closed 3 years ago

nitrocode commented 3 years ago

what

why

references

nitrocode commented 3 years ago

/test all

osterman commented 3 years ago

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

nitrocode commented 3 years ago

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 commented 3 years ago 
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 https://github.com/cloudposse/terraform-aws-security-group/pull/1#discussion_r567571670

:point_down:

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. :stuck_out_tongue_winking_eye:

nitrocode commented 3 years ago

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. :stuck_out_tongue_winking_eye:

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 commented 3 years ago

/test all

Nuru commented 3 years ago

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 ```hcl 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.