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

Make one of the source_ parameters Required in ingress google_compute_firewall #2924

Closed
rileykarson opened this issue Jan 24, 2019 · 5 comments · Fixed by GoogleCloudPlatform/magic-modules#5253, terraform-google-modules/terraform-docs-samples#15, hashicorp/terraform-provider-google-beta#3750, GoogleCloudPlatform/terraform-validator#338 or #10369
Closed

Make one of the source_ parameters Required in ingress google_compute_firewall #2924

rileykarson opened this issue Jan 24, 2019 · 5 comments · Fixed by GoogleCloudPlatform/magic-modules#5253, terraform-google-modules/terraform-docs-samples#15, hashicorp/terraform-provider-google-beta#3750, GoogleCloudPlatform/terraform-validator#338 or #10369
Assignees
@megan07
Labels
breaking-change enhancement size/s upstream-terraform
Milestone
4.0.0

Comments

@rileykarson
Copy link
Collaborator

rileykarson commented Jan 24, 2019 •
edited
Loading

There isn't a great case to not specify at least one source_ param, let's try making one of them required for a plan to succeed while using an INGRESS firewall rule.
@rileykarson rileykarson added this to the 2.0.0 milestone Jan 24, 2019
@paddycarver
Copy link
Contributor

So we originally wanted to do this because we had a test running that specified no source_* fields, which defaulted the firewall to allowing traffic from 0.0.0.0/0 which is a suboptimal default. This issue was to track the exploration of making at least one source_* field required.

Using CustomizeDiff, we are able to reject any plan that would set all source_* fields to empty. We could probably even do it to only reject plans that set all source_* fields to empty when direction is not EGRESS, which is the behaviour I believe we want. The problem is, we're not able to distinguish between computed and unset fields in CustomizeDiff, so we'd be unable to reliably tell whether values are set or not, and I'm not super eager to tell people valid plans are invalid.

We could do this at apply time, but I'm leery of apply-time validations. My thought on this is that we should accept it for 2.0.0 as a non-ideal default, and when we get the new SDK based on 0.12's capabilities, we can surface warnings. In the meantime, Sentinel users can use a policy to prevent this if they so desire. As such, I'm going to drop it from the 2.0.0 milestone. If someone feels strongly we should do the fail-at-apply validation, feel free to chime in in the comments and we can reevaluate.

@paddycarver paddycarver removed this from the 2.0.0 milestone Feb 1, 2019
@rileykarson rileykarson changed the title Investigate making source_ranges required in google_compute_firewall Make one of the source_ parameters Required in ingress google_compute_firewalls Feb 1, 2019
@rileykarson rileykarson changed the title Make one of the source_ parameters Required in ingress google_compute_firewalls Make one of the source_ parameters Required in ingress google_compute_firewall Feb 1, 2019
@paddycarver paddycarver added this to the 4.0.0 milestone Oct 7, 2019
modular-magician added a commit to modular-magician/terraform-provider-google that referenced this issue Jan 7, 2020
@modular-magician
Update google_folder import description (hashicorp#2924)
9df67fe 
Merged PR hashicorp#2924.
modular-magician added a commit that referenced this issue Jan 7, 2020
@modular-magician @chrisst
@marinkovicpetar @danawillow
Update google_folder import description (#5325)
fc38db6 
* Allow domain mapping to succeed if DNS is pending

Signed-off-by: Modular Magician <[email protected]>

* Updated google_folder.html (#4149)

* Updated google_folder.html

The page in the first example shows that you should use organization_id with value of 1234567. In the Import example, it's not clear whether organization_id is user, or folder_id is used. API call behind this import command is only accepting folder_id (can be checked when setting TF_LOG to trace and viewing the API call)

* Update website/docs/r/google_folder.html.markdown

Co-Authored-By: Dana Hoffman <[email protected]>

Co-authored-by: Dana Hoffman <[email protected]>

* add google_kms_secret_ciphertext resource, deprecate datasource (#5314)

Signed-off-by: Modular Magician <[email protected]>

Co-authored-by: Dana Hoffman <[email protected]>

* Update google_folder import description (#2924)

Merged PR #2924.

Co-authored-by: Chris Stephens <[email protected]>
Co-authored-by: Petar Marinkovic <[email protected]>
Co-authored-by: Dana Hoffman <[email protected]>
@paddycarver paddycarver removed their assignment Mar 9, 2020
@rileykarson
Copy link
Collaborator Author

rileykarson commented Oct 9, 2020 •
edited
Loading

To expand upon

Using CustomizeDiff, we are able to reject any plan that would set all source_* fields to empty. We could probably even do it to only reject plans that set all source_* fields to empty when direction is not EGRESS, which is the behaviour I believe we want. The problem is, we're not able to distinguish between computed and unset fields in CustomizeDiff, so we'd be unable to reliably tell whether values are set or not, and I'm not super eager to tell people valid plans are invalid.

I believe we could check whether any of the values are unknown, in which case we would accept the plan as if that value were set.

@megan07 megan07 self-assigned this Sep 28, 2021
@megan07 megan07 mentioned this issue Sep 29, 2021
Merged
5 tasks
This was referenced Oct 21, 2021
Merged
Merged
Closed
Merged
Merged
@JV-HCA
Copy link

JV-HCA commented Nov 5, 2021

This needs to be updated in Terraform documentation. Currently doesn't list one of these as required

@vankatamarinov
Copy link

Also, we have a bug in this implementation - issues/10494

@github-actions
Copy link

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@megan07 megan07

Labels
breaking-change enhancement size/s upstream-terraform
Projects
None yet
Milestone
4.0.0
6 participants
@paddycarver @rileykarson @megan07 @c2thorn @JV-HCA @vankatamarinov