Skip to content
This repository has been archived by the owner on Feb 5, 2020. It is now read-only.

openstack: Fix non-self-hosted etcd security groups #1052

Merged
merged 1 commit into from
Jun 13, 2017

Conversation

coreypobrien
Copy link
Contributor

Security groups for controller nodes were broken when experimental was false
because they attempted to reference the self-hosted etcd secrurity group
which doesn't exist in that configuration.

This change consolidates all security groups into their own openstack module
to ensure that shared groups are only created once per plan. Both the self-
hosted and non-self-hosted etcd security groups need to be created so that
references to them resolve even if they ultimately aren't used due to
the conditional logic from tectonic_experimental.

@coreosbot
Copy link

Can one of the admins verify this patch?

1 similar comment
@coreosbot
Copy link

Can one of the admins verify this patch?

@s-urbaniak
Copy link
Contributor

ok to test

s-urbaniak
s-urbaniak previously approved these changes Jun 12, 2017
Copy link
Contributor

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

LGTM once green 👍

@s-urbaniak
Copy link
Contributor

@coreypobrien CI unfortunately complains with:

misformatted files (run 'terraform fmt .' to fix): platforms/openstack/nova/main.tf

@s-urbaniak
Copy link
Contributor

and there is a conflict because I just merged another OpenStack PR ;-)

Security groups for controller nodes were broken when experimental was false
because they attempted to reference the self-hosted etcd secrurity group
which doesn't exist in that configuration.
This change consolidates all security groups into their own openstack module
to ensure that shared groups are only created once per plan. Both the self-
hosted and non-self-hosted etcd security groups need to be created so that
references to them resolve even if they ultimately aren't used due to
the conditional logic from tectonic_experimental.
@s-urbaniak
Copy link
Contributor

CI failure seems unrelated:

Error applying plan:

1 error(s) occurred:

* module.etcd.aws_instance.etcd_node[0]: 1 error(s) occurred:

* aws_instance.etcd_node.0: InvalidInstanceID.NotFound: The instance ID 'i-0a8ac7efc18e0ca58' does not exist
	status code: 400, request id: 687476cd-bd34-43b9-a764-f835bf264e6a

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.
make: *** [apply] Error 1

I will restart it.

@s-urbaniak
Copy link
Contributor

retest this please

@s-urbaniak s-urbaniak mentioned this pull request Jun 12, 2017
6 tasks
@Quentin-M
Copy link
Contributor

ok to test

@coreosbot
Copy link

Can one of the admins verify this patch?

@lblackstone
Copy link
Contributor

@s-urbaniak ✅ :)

Copy link
Contributor

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

This LGTM. I guess we could rename the secgroups module as net or the like and move all network related resources there, iff we decide to remove the nova flavor in #1062. This would align it with i.e. AWS where the aws/vpc module includes all network related resources including security groups.

Thanks!

@s-urbaniak s-urbaniak merged commit 37387ba into coreos:master Jun 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants