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

Allow configuring docker machine egress rules #351

Conversation

mhulscher
Copy link
Contributor

@mhulscher mhulscher commented Jul 28, 2021

Description

I would like to be able to configure the egress rules of the docker-machine security group. This PR introduces a new variable called docker_machine_egress_rules to configure these egress rules. It defaults to allow all egress traffic to maintain backwards compatibility.

Migrations required

YES, for a smooth upgrade the security group for docker machine needs to be tainted

terraform taint module.runner.aws_security_group.docker_machine

Verification

Ran a terraform plan in the runner-docker example.

Documentation

Ran pre-commit with latest version of each tool; this also re-generates larges portions of the README.

@mhulscher mhulscher changed the title Conditionally create docker machine security group Allow configuring docker machine egress rules Jul 28, 2021
@npalm
Copy link
Collaborator

npalm commented Jul 28, 2021

Please can you check the failing CI job, the modules is still on version 0.13.6. Assume you have used another terraform version. We use tfenv ot set the used terraform version. Examples are verified with multiple terraform versions

@mhulscher
Copy link
Contributor Author

mhulscher commented Jul 29, 2021

Hi @npalm ,

It looks like terraform fmt's opinion about what is correct formatting has changed since terraform 0.13.6. When I run terraform fmt against my current PR, latest terraform recommends that I remove the whitespace between the character ! and the next term in a negation. However, terraform 0.13.6 recommends the exact opposite.

~/.bins/terraform-0.13.6 fmt -recursive -check=true -diff             
main.tf
--- old/main.tf
+++ new/main.tf
@@ -38,7 +38,7 @@
 }
 
 locals {
-  enable_asg_recreation = var.enable_forced_updates != null ? !var.enable_forced_updates : var.enable_asg_recreation
+  enable_asg_recreation = var.enable_forced_updates != null ? ! var.enable_forced_updates : var.enable_asg_recreation
 
   template_user_data = templatefile("${path.module}/template/user-data.tpl",
     {
@@ -125,7 +125,7 @@
       runners_root_size                 = var.runners_root_size
       runners_iam_instance_profile_name = var.runners_iam_instance_profile_name
       runners_use_private_address_only  = var.runners_use_private_address
-      runners_use_private_address       = !var.runners_use_private_address
+      runners_use_private_address       = ! var.runners_use_private_address
       runners_request_spot_instance     = var.runners_request_spot_instance
       runners_environment_vars          = jsonencode(var.runners_environment_vars)
       runners_pre_build_script          = var.runners_pre_build_script

If I update main.tf to please 0.13.6, newer versions will start complaining. Do you have any suggestions what to do with this?

@npalm
Copy link
Collaborator

npalm commented Jul 29, 2021

Please update to please the current TF version for the module, I will upgrade sources and formating asap to a newer version

@mhulscher
Copy link
Contributor Author

@npalm done!

Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

Made a small suggestion for a change. Furthermore there is a small migration required. I ran a upgrade on the default example, which fails. After tainting the security group an upgrade was smooth.

terraform taint module.runner.aws_security_group.docker_machine

Please can you update the PR description with a small not for the required migration

variables.tf Outdated Show resolved Hide resolved
Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

Updated description, and made a smaal change in the default description of the egress rule.

@npalm npalm merged commit 845e018 into cattle-ops:develop Jul 31, 2021
npalm added a commit that referenced this pull request Jul 31, 2021
semantic-releaser bot pushed a commit that referenced this pull request Jul 31, 2021
## [4.28.0](4.27.0...4.28.0) (2021-07-31)

### Features

* Allow configuring docker machine egress rules, see PR [#351](#351) for upgrade instructions ([f41ce19](f41ce19))
* support for settings Sentry DSN ([#352](#352)) ([5dbe1f7](5dbe1f7))
@mhulscher mhulscher deleted the conditionally-create-docker-machine-security-group branch August 3, 2021 09:03
semantic-releaser bot pushed a commit that referenced this pull request Aug 28, 2021
## [4.29.0](4.28.0...4.29.0) (2021-08-28)

### Features

* Allow configuring docker machine egress rules, see PR [#351](#351) for upgrade instructions ([845e018](845e018))
* Parametrize runner instance launch configuration metadata options ([#348](#348)) ([a4406dc](a4406dc))
* replace launch configuration with launch template ([#337](#337)) ([b805fb6](b805fb6))
* support for settings Sentry DSN ([#352](#352)) ([2a07466](2a07466))

### Bug Fixes

* Use better ressources names ([#356](#356)) ([817e040](817e040))
@semantic-releaser
Copy link
Contributor

🎉 This PR is included in version 4.29.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

npalm pushed a commit that referenced this pull request Oct 4, 2021
## [4.29.0](4.28.0...4.29.0) (2021-08-28)

### Features

* Allow configuring docker machine egress rules, see PR [#351](#351) for upgrade instructions ([845e018](845e018))
* Parametrize runner instance launch configuration metadata options ([#348](#348)) ([a4406dc](a4406dc))
* replace launch configuration with launch template ([#337](#337)) ([b805fb6](b805fb6))
* support for settings Sentry DSN ([#352](#352)) ([2a07466](2a07466))

### Bug Fixes

* Use better ressources names ([#356](#356)) ([817e040](817e040))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants