Skip to content

GCP: Add networkProjectID parameter to install-config. #6166

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
Merged
openshift-merge-robot merged 1 commit into from
Aug 12, 2022
Merged

GCP: Add networkProjectID parameter to install-config. #6166

openshift-merge-robot merged 1 commit into from
Aug 12, 2022

Conversation

@jstuever
Copy link
Contributor

@jstuever jstuever commented Jul 25, 2022 •
edited
Loading

This change adds the networkProjecID parameter to the GCP platform in
the install-config. When set, it also validates the project exists and
that the network and subnetworks belong to this project.

https://issues.redhat.com/browse/CORS-2036

@openshift-ci openshift-ci bot requested review from patrickdillon and rna-afk July 25, 2022 21:51
@jstuever
Copy link
Contributor Author

jstuever commented Jul 26, 2022

/retest

@jstuever
Copy link
Contributor Author

jstuever commented Jul 26, 2022

/retest

@rna-afk
Copy link
Contributor

rna-afk commented Jul 27, 2022

LGTM do we need extra unit test cases for this?

@jstuever jstuever changed the title GCP: Add networkProjectID parameter to install-config. WIP: GCP: Add networkProjectID parameter to install-config. Jul 28, 2022
@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 Jul 28, 2022
@patrickdillon
Copy link
Contributor

patrickdillon commented Aug 1, 2022 •
edited
Loading

LGTM too but holding off due to WIP label

nit: do we want static validation that hostproject != networkProject?

I do have one concern with merging this PR, unrelated to the PR itself. We do not plan on landing this feature in 4.12, but this PR will expose the fields in the install config. What is the best way to handle that (i.e. that we would have valid fields in the API for unstable features)? Potential ideas for handling this:

  1. Don't worry about it
  2. Slap a validation commit on this that will prevent users from using the feature
  3. Use a different version of the install config for unstable features
  4. Hold off on merging this until ready

@barbacbd barbacbd mentioned this pull request Aug 5, 2022
Merged
@barbacbd barbacbd mentioned this pull request Aug 8, 2022
Merged
@jstuever
GCP: Add networkProjectID parameter to install-config.
3051e7d 
This change adds the networkProjecID parameter to the GCP platform in
the install-config. When set, it also validates the project exists and
that the network and subnetworks belong to this project.
@jstuever jstuever changed the title WIP: GCP: Add networkProjectID parameter to install-config. GCP: Add networkProjectID parameter to install-config. Aug 8, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2022
@jstuever
Copy link
Contributor Author

jstuever commented Aug 8, 2022

I've added unit tests for the networkProjectID validation function. I also added wording in oc explain to indicate that this parameter is not (yet) supported. Hopefully this satisfies all of the concerns above.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 9, 2022 •
edited
Loading

@jstuever: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ibmcloud 3051e7d link false /test e2e-ibmcloud
ci/prow/e2e-libvirt 3051e7d link false /test e2e-libvirt
ci/prow/e2e-openstack 3051e7d link false /test e2e-openstack
ci/prow/e2e-gcp-shared-vpc 3051e7d link false /test e2e-gcp-shared-vpc

Full PR test history. Your PR dashboard.

Details

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.

@barbacbd barbacbd mentioned this pull request Aug 11, 2022
Merged
@rna-afk
Copy link
Contributor

rna-afk commented Aug 11, 2022

/lgtm

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

patrickdillon commented Aug 11, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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

The pull request process is described here

Details 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2022
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 11, 2022

/retest-required

Remaining retests: 2 against base HEAD 26611ff and 8 for PR HEAD 3051e7d in total

@openshift-merge-robot openshift-merge-robot merged commit 8e925f2 into openshift:master Aug 12, 2022
barbacbd added a commit to barbacbd/installer that referenced this pull request Aug 15, 2022
@barbacbd
gcp: Add validation for Network Project Data�
2f93c42 
** Added checks in types to ensure that network, computeSubnet, and controlPlaneSubnet
are set when NetworkProjectID exists.
** Added checks in installconfig to ensure that CredentialsMode is set to either Passthrough or
Manual when NetworkProjectID exists.

CORS-2048
Requires openshift#6166

Removed Duplicated networkProjectID
barbacbd added a commit to barbacbd/installer that referenced this pull request 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
@jstuever jstuever deleted the cors2036 branch January 5, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patrickdillon patrickdillon Awaiting requested review from patrickdillon

@rna-afk rna-afk Awaiting requested review from rna-afk

Assignees

@rna-afk rna-afk

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

@jstuever @rna-afk @patrickdillon @openshift-ci-robot @openshift-merge-robot