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.

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

CORS-2280: IBMCloud: Add TF support for private DNS #6282

Merged

Conversation

cjschaef
Copy link
Member

Add support for creating, configuring, and destroying Terraform
resources for private (Internal) IPI clusters on IBM Cloud, using
DNS Services, rather than CIS (public/External).

Partial: https://issues.redhat.com/browse/CORS-2255

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 30, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@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 30, 2022
@cjschaef cjschaef marked this pull request as ready for review August 30, 2022 20:56
@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 30, 2022
@openshift-ci openshift-ci bot requested review from barbacbd and rvanderp3 August 30, 2022 20:57
Add support for creating, configuring, and destroying Terraform
resources for private (Internal) IPI clusters on IBM Cloud, using
DNS Services, rather than CIS (public/External).

Partial: https://issues.redhat.com/browse/CORS-2255
@cjschaef cjschaef force-pushed the ibmcloud_private_terraform branch from 0570110 to 9d7a9bc Compare August 30, 2022 21:18
@cjschaef
Copy link
Member Author

e2e-ibmcloud failed due to known issue of e2e-ibmcloud-ipi-ibmcloud-gather-resources, which is unrelated to installer code. So that should be acceptable, given it's a set of IBM Cloud CLI commands for debugging purposes.

@jeffnowicki
Copy link
Contributor

jeffnowicki commented Aug 31, 2022

/retitle CORS-2280: IBMCloud: Add TF support for private DNS

@openshift-ci openshift-ci bot changed the title CORS-2255: IBMCloud: Add TF support for private DNS CORS-2280: IBMCloud: Add TF support for private DNS Aug 31, 2022
@cjschaef
Copy link
Member Author

/retest

@rvanderp3
Copy link
Contributor

/lgtm

@rvanderp3
Copy link
Contributor

/assign @patrickdillon

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2022
@@ -67,7 +67,7 @@ resource "ibm_is_security_group" "bootstrap" {
resource "ibm_is_security_group_rule" "bootstrap_ssh_inbound" {
group = ibm_is_security_group.bootstrap.id
direction = "inbound"
remote = local.public_endpoints ? "0.0.0.0/0" : var.control_plane_security_group_id_list.0.id
remote = local.public_endpoints ? "0.0.0.0/0" : var.control_plane_security_group_id_list[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any other instance of this variable changed. Is this missing id or did the type change?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually a bug fix, for a bug that was never noticed until we started testing Internal, which hasn't been supported until this and the previous CORS-2280 PR for Golang changes.

For instance, using this PR with a revert of the line above, Terraform fails since the control_plane_security_group_id_list is a list of Id's (strings) already, not SecurityGroup data sources.

# go/src/github.com/openshift/installer/bin/openshift-install version
go/src/github.com/openshift/installer/bin/openshift-install unreleased-master-6395-g75c6ab06c034cfc841b672b936a6a750930606d7
built from commit 75c6ab06c034cfc841b672b936a6a750930606d7
release image registry.ci.openshift.org/origin/release:4.12
release architecture amd64

# git -C go/src/github.com/openshift/installer/ diff cjschaef/ibmcloud_private_terraform..75c6ab06c034cfc841b672b936a6a750930606d7
diff --git a/data/data/ibmcloud/bootstrap/main.tf b/data/data/ibmcloud/bootstrap/main.tf
index 4471b3325..2f0aaea6a 100644
--- a/data/data/ibmcloud/bootstrap/main.tf
+++ b/data/data/ibmcloud/bootstrap/main.tf
@@ -67,7 +67,7 @@ resource "ibm_is_security_group" "bootstrap" {
 resource "ibm_is_security_group_rule" "bootstrap_ssh_inbound" {
   group     = ibm_is_security_group.bootstrap.id
   direction = "inbound"
-  remote    = local.public_endpoints ? "0.0.0.0/0" : var.control_plane_security_group_id_list[0]
+  remote    = local.public_endpoints ? "0.0.0.0/0" : var.control_plane_security_group_id_list.0.id
   tcp {
     port_min = 22
     port_max = 22


# go/src/github.com/openshift/installer/bin/openshift-install create cluster --dir clusters/private-sg-id-list-1/
WARNING Found override for release image. Please be warned, this is not advised 
INFO Consuming Worker Machines from target directory 
INFO Consuming OpenShift Install (Manifests) from target directory 
INFO Consuming Master Machines from target directory 
INFO Consuming Common Manifests from target directory 
INFO Consuming Openshift Manifests from target directory 
INFO Obtaining RHCOS image file from 'https://rhcos.mirror.openshift.com/art/storage/releases/rhcos-4.12/412.86.202208101039-0/x86_64/rhcos-412.86.202208101039-0-ibmcloud.x86_64.qcow2.gz?sha256=09b599849b945bdd405b18765225160f50e07ca205fe9787f70f188c8a96f293' 
INFO The file was found in cache: /root/.cache/openshift-installer/image_cache/rhcos-412.86.202208101039-0-ibmcloud.x86_64.qcow2. Reusing... 
INFO Creating infrastructure resources...         
ERROR                                              
ERROR Error: Unsupported attribute                 
ERROR                                              
ERROR   on main.tf line 70, in resource "ibm_is_security_group_rule" "bootstrap_ssh_inbound": 
ERROR   70:   remote    = local.public_endpoints ? "0.0.0.0/0" : var.control_plane_security_group_id_list.0.id 
ERROR     ├────────────────                        
ERROR     │ var.control_plane_security_group_id_list[0] is "r018-a45f4ff4-b452-44a7-bc12-72bc0e424644" 
ERROR                                              
ERROR Can't access attributes on a primitive-typed value (string). 
ERROR failed to fetch Cluster: failed to generate asset "Cluster": failure applying terraform for "bootstrap" stage: failed to create cluster: failed to apply Terraform: exit status 1 
ERROR                                              
ERROR Error: Unsupported attribute                 
ERROR                                              
ERROR   on main.tf line 70, in resource "ibm_is_security_group_rule" "bootstrap_ssh_inbound": 
ERROR   70:   remote    = local.public_endpoints ? "0.0.0.0/0" : var.control_plane_security_group_id_list.0.id 
ERROR     ├────────────────                        
ERROR     │ var.control_plane_security_group_id_list[0] is "r018-a45f4ff4-b452-44a7-bc12-72bc0e424644" 
ERROR                                              
ERROR Can't access attributes on a primitive-typed value (string).

vpc_crn = module.vpc.vpc_crn
base_domain = var.base_domain
cluster_domain = var.cluster_domain
is_external = local.public_endpoints
Copy link
Contributor

Choose a reason for hiding this comment

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

consider making is_external more descriptive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I'll see about following up this PR with some more descriptive

@@ -54,11 +54,29 @@ module "cis" {
cis_id = var.ibmcloud_cis_crn
base_domain = var.base_domain
cluster_domain = var.cluster_domain
is_external = local.public_endpoints
Copy link
Contributor

Choose a reason for hiding this comment

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

consider passing this in as a boolean to the ibm-variables where the logic is moved to tfvars in the installer.

Copy link
Member Author

Choose a reason for hiding this comment

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

The boolean is based on PublishStrategy currently, and just passed around after that.
If it makes sense to add boolean version of the PublishStrategy (basically) to tfvars that gets used in place of that Internel/External string, I can do that in a follow up PR.

@patrickdillon
Copy link
Contributor

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 6, 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

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 Sep 6, 2022
@jeffnowicki
Copy link
Contributor

/retest

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD b8714c6 and 2 for PR HEAD 9d7a9bc in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 02354a2 and 1 for PR HEAD 9d7a9bc in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 7, 2022

@cjschaef: 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-libvirt 9d7a9bc link false /test e2e-libvirt
ci/prow/e2e-ibmcloud 9d7a9bc link false /test e2e-ibmcloud

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 d10d64c into openshift:master Sep 7, 2022
@cjschaef cjschaef deleted the ibmcloud_private_terraform branch September 7, 2022 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants