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

GCP: Remove firewall rules when providing network project id #6219

Merged
merged 2 commits into from
Aug 27, 2022
Merged

GCP: Remove firewall rules when providing network project id #6219

merged 2 commits into from
Aug 27, 2022

Conversation

barbacbd
Copy link
Contributor

** Added the Network Project ID data to gcp terraform vars
** Added checks to GCP terraform to set count to 0 for firewall rules when
the Network Project ID is available.

CORS 2039
Requires #6166

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2022
@barbacbd barbacbd changed the title WIP GCP: Remove firewall rules when providing network project id GCP: Remove firewall rules when providing network project id Aug 11, 2022
@barbacbd
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 11, 2022
@barbacbd
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 15, 2022
@barbacbd barbacbd force-pushed the skip_firewall_rules_npid branch from 806ec78 to b9769dc Compare August 17, 2022 20:26
@r4f4
Copy link
Contributor

r4f4 commented Aug 19, 2022

/test e2e-gcp

@barbacbd
Copy link
Contributor Author

/test gofmt
/test golint
/test govet
/test images
/test okd-e2e-aws
/test okd-images
/test okd-unit
/test okd-verify-codegen
/test shellcheck
/test tf-fmt
/test tf-lint

@barbacbd
Copy link
Contributor Author

/test e2e-ws
/test e2e-ibmcloud
/test e2e-libvirt
/test e2e-metal-assisted
/test e2e-metal-ipi
/test e2e-openstack
/test e2e-ovirt

@barbacbd
Copy link
Contributor Author

/test e2e-aws

@barbacbd barbacbd force-pushed the skip_firewall_rules_npid branch from c75827f to 782447c Compare August 22, 2022 15:23
@barbacbd
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 22, 2022
@barbacbd
GCP: Remove firewall rules when providing network project id
6315658 
** Added the Network Project ID data to gcp terraform vars
** Added checks to GCP terraform to set count to 0 for firewall rules when
the Network Project ID is available.

CORS 2039
Requires openshift#6166

** Added a default value for the terraform value for network_project_id
@barbacbd barbacbd force-pushed the skip_firewall_rules_npid branch from 782447c to 6315658 Compare August 22, 2022 15:55
@r4f4
Copy link
Contributor

r4f4 commented Aug 22, 2022

/test e2e-gcp

r4f4
r4f4 reviewed Aug 22, 2022
View reviewed changes
data/data/gcp/variables-gcp.tf
variable "gcp_network_project_id" {
type = string
description = "The project that the network and subnets exist in when they are not in the main ProjectID."
default = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to deploy a cluster with default = "" instead of default = null.

@barbacbd
Copy link
Contributor Author

/retest

1 similar comment
@barbacbd
Copy link
Contributor Author

/retest

@barbacbd
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2022
jstuever
jstuever requested changes Aug 24, 2022
View reviewed changes
Copy link
Contributor

@jstuever jstuever left a comment

Choose a reason for hiding this comment

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

Some minor changes as per my comments.

While making these changes, you might consider passing some sort of "disable firewall" parameter instead of relying on the "networkProjectID" directly in the Terraform templates. This will enable us to provision firewall rules in the future when networkProjectID is defined. However, I would not block this PR on that as enabling this functionality is technically out of scope.

pkg/tfvars/gcp/gcp.go Show resolved Hide resolved
pkg/asset/cluster/tfvars.go Show resolved Hide resolved
@jstuever jstuever self-requested a review August 24, 2022 19:57
@jstuever
Copy link
Contributor

/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 25, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jstuever, r4f4

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jstuever
Copy link
Contributor

/retest-required

@jstuever
Copy link
Contributor

/test govet
/test images

@jstuever
Copy link
Contributor

/test e2e-gcp

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 1c8e79b and 8 for PR HEAD 51306ad in total

@jstuever
Copy link
Contributor

jstuever commented Aug 26, 2022 •
edited
Loading

/test govet
/test images
/test tf-lint
/test unit
/test verify-vendor

@jstuever
Copy link
Contributor

/test aro-unit
/test okd-images
/test okd-unit
/test okd-verify-codegen
/test shellcheck

@jstuever
Copy link
Contributor

/test e2e-aws
/test e2e-azure
/test e2e-gcp

@jstuever
Copy link
Contributor

/test aro-unit
/test e2e-aws
/test okd-verify-codegen
/test shellcheck

@jstuever
Copy link
Contributor

/skip

@r4f4
Copy link
Contributor

r4f4 commented Aug 26, 2022

/test gofmt
/test golint
/test shellcheck
/test yaml-lint

@r4f4
Copy link
Contributor

r4f4 commented Aug 26, 2022

/test shellcheck
/test okd-unit

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 2867b6a and 7 for PR HEAD 51306ad in total

@r4f4
Copy link
Contributor

r4f4 commented Aug 26, 2022

/skip

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD 2867b6a and 6 for PR HEAD 51306ad in total

@r4f4
Copy link
Contributor

r4f4 commented Aug 26, 2022

/skip

1 similar comment
@r4f4
Copy link
Contributor

r4f4 commented Aug 26, 2022

/skip

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 82dfb36 and 5 for PR HEAD 51306ad in total

@barbacbd
Copy link
Contributor Author

/skip

@jstuever
Copy link
Contributor

jstuever commented Aug 26, 2022 •
edited
Loading

/test aro-unit
/test gofmt
/test golint
/test images
/test okd-images
/test okd-unit
/test verify-codegen
/test verify-vendor

@jstuever
Copy link
Contributor

/test shellcheck

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD 82dfb36 and 4 for PR HEAD 51306ad in total

@jstuever
Copy link
Contributor

/test gofmt
/test golint
/test images

@jstuever
Copy link
Contributor

/test okd-images
/test okd-unit
/test shellcheck

@jstuever
Copy link
Contributor

/test gofmt
/test golint

@jstuever
Copy link
Contributor

/test images

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 27, 2022

@barbacbd: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit abf77d9 into openshift:master Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r4f4 r4f4 r4f4 approved these changes

@patrickdillon patrickdillon Awaiting requested review from patrickdillon

@jstuever jstuever Awaiting requested review from jstuever

Assignees

@jstuever jstuever

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@barbacbd @r4f4 @jstuever @openshift-ci-robot @openshift-merge-robot