Don't create secgroup for a bastion host on openstack, if it doesn't exist #4291
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
| description = "${var.cluster_name} - Bastion Server" | ||
| } | ||
|
|
||
| resource "openstack_networking_secgroup_rule_v2" "bastion" { | ||
| count = "${length(var.bastion_allowed_remote_ips)}" | ||
| count = "${var.number_of_bastions ? length(var.bastion_allowed_remote_ips) : 0}" |
There was a problem hiding this comment.
If var.number_of_bastions, isn't length(var.bastion_allowed_remote_ips) already 0 ?
There was a problem hiding this comment.
var.bastion_allowed_remote_ips is given a default value in https://github.com/kubernetes-sigs/kubespray/blob/master/contrib/terraform/openstack/variables.tf#L142, so it's length is "1" by default. I can leave the line as is, but then the default in variables file needs to be changed.
| description = "${var.cluster_name} - Bastion Server" | ||
| } | ||
|
|
||
| resource "openstack_networking_secgroup_rule_v2" "bastion" { | ||
| count = "${length(var.bastion_allowed_remote_ips)}" | ||
| count = "${var.number_of_bastions ? length(var.bastion_allowed_remote_ips) : 0}" |
|
ci check this |
|
/assign @riverzhang |
|
I have tested this in an OpenStack public cloud. It works as expected. Thank you for your contribution @trawler /lgtm |
k8s-ci-robot
added
the
lgtm
|
@Miouge1 Thank you for reviewing and testing. Can you help with getting this PR approved and merged? |
|
@trawler sorry for the wait. Can you run terraform fmt and also rebase on latest master? We added some new CI steps for validating terraform recipes. |
k8s-ci-robot
removed
the
lgtm
|
@holmsten thanks for looking into it. rebased and checked. |
|
/lgtm |
|
@holmsten: changing LGTM is restricted to assignees, and only kubernetes-sigs/kubespray repo collaborators may be assigned issues. In response to this:
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. |
|
@gix while you fix the kubespray-sigs membership, I can give this a try: /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: holmsten, Miouge1, trawler 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 |
k8s-ci-robot
added
the
approved
… if doesn't exist (kubernetes-sigs#4291)" This reverts commit 20ebb49. Fixes kubernetes-sigs#4487
even if
number_of_bastionsvar is set to 0, terraform creates a security group and rules for a non-existing bastion host.This PR sets these resources as conditionals, and uses the
join()hack from: hashicorp/terraform#11566 to set a dynamic value to the attached security groups on the instances themselves.