[Resolve #1307] Fixing Connection Manager bug #1308

jfalkenstein · 2023-02-18T16:11:40Z

This resolves a bug discovered in the ConnectionManager from #1307.

The specific situation is when the ConnectionManager.call is invoked using a stack_name and explicitly passes None for each of profile, region, and sceptre_role. The overhaul of the ConnectionManager in v4 accidentally omitted this edge case which happen to specifically affect the !stack_output_external resolver when it's run without either an AWS_DEFAULT_REGION environment variable or without an AWS_PROFILE environment variable set to a profile that defines a region.

PR Checklist

Wrote a good commit message & description [see guide below].
Commit message starts with [Resolve #issue-number].
Added/Updated unit tests.
Added/Updated integration tests (if applicable).
All unit tests (make test) are passing.
Used the same coding conventions as the rest of the project.
The new code passes pre-commit validations (pre-commit run --all-files).
The PR relates to only one subject with a clear title.
and description in grammatically correct, complete sentences.

Approver/Reviewer Checklist

Before merge squash related commits.

Other Information

Guide to writing a good commit

jfalkenstein · 2023-02-18T16:13:03Z

integration-tests/environment.py

-    config_path = os.path.join(context.sceptre_dir, "config", "9/B" + ".yaml")
-
-    with open(config_path, "r") as file:
-        file_data = file.read()
-
-    file_data = file_data.replace("{project_code}", context.project_code)
-
-    with open(config_path, "w") as file:
-        file.write(file_data)
-


This was able to be completely removed because I'm not using Jinja syntax to reference the project_code properly. We should have done that all along... in any case, this chunk of integration tests code has been a thorn in the side of anyone running integration tests locally, since it actually produced changes in the files that couldn't be committed. With this gone, we're in much better state now.

jfalkenstein · 2023-02-18T16:13:35Z

integration-tests/features/stack-output-external-resolver.feature

+  Scenario: launch a stack referencing the external output of an existing stack without explicit region or profile
+    Given stack "external-stack-output-stack-output/outputter" exists using "dependencies/independent_template.json"
+    And stack "external-stack-output/resolver-no-profile-region" does not exist
+    When the user launches stack "external-stack-output/resolver-no-profile-region"
+    Then stack "external-stack-output/resolver-no-profile-region" exists in "CREATE_COMPLETE" state


We needed this test, since this was what was effectively broken.

jfalkenstein · 2023-02-18T16:14:30Z

integration-tests/sceptre-project/config/9/B.yaml

@@ -1,5 +0,0 @@
-template:


I've renamed this StackGroup to "external-stack-output" because "9" was never a good name for a test fixture. And I've renamed this file to "resolver-with-profile-and-region" to properly describe what was being tested.

jfalkenstein · 2023-02-18T16:15:09Z

integration-tests/sceptre-project/config/external-stack-output/resolver-no-profile-region.yaml

@@ -0,0 +1,5 @@
+template:


This was the new testcase we needed, using !stack_output_external with no explicit profile/region/role.

jfalkenstein · 2023-02-18T16:17:20Z

...gration-tests/sceptre-project/config/external-stack-output/resolver-with-profile-region.yaml

+
+
+parameters:
+  DependentStackName: !stack_output_external "{{project_code}}-external-stack-output-outputter::StackName {{environment_variable.AWS_PROFILE|default('default')}}::eu-west-1"


This is what used to be 9/B.

The changes here fix issues running the integration tests locally:

We use jinja syntax to reference the project code instead of what we used to do, which was modify this template before tests were executed. Which was weird and unnecessary and produced a diff every time you ran these tests locally.

Before, it assumed you had a profile called "default" set up. But running this locally, that might not actually be the profile you're using. So now, it uses your AWS_PROFILE environment variable, if it's set; otherwise, it'll default to "default".

jfalkenstein · 2023-02-18T16:17:33Z

integration-tests/steps/stacks.py

-    sceptre_context = SceptreContext(
-        command_path=stack_name + ".yaml", project_path=context.sceptre_dir
-    )
-
-    sceptre_plan = SceptrePlan(sceptre_context)
-    status = sceptre_plan.get_status()


We never actually used these lines of code.

jfalkenstein · 2023-02-18T16:29:42Z

sceptre/connection_manager.py

+        # For historical reasons, if all three values are "None", that means we default to the
+        # Stack's configuration.
+        if (profile, region, sceptre_role) == (None, None, None):
+            profile, region, sceptre_role = self.profile, self.region, self.sceptre_role


This is the actual fix right here. When I changed the ConnectionManager, I accidentally omitted this situation coverage.

Before, the logic was:

If the profile, region, and role were all None, then...

if the stack_name was cached...

use the cached profile, region, and role by that cached stack name

otherwise, use THIS stack's profile, region, and role

Otherwise, use the explicitly specified profile, region and role

After v4 was released, the logic was:

If the stack_name was cached...

if the profile, region, and role were all None, then...

use the cached profile, region and role

otherwise, use the specified profile, region, and role, using the cached value when no value has been specified

This was a gap in our old logic, so this change was intentional and fixed some things.

otherwise, use the specified profile, region, and role, using the current stack's value when no value has been specified

The gap that I found that was causing some bugs was when the stack_name was not cached, but the profile, region, and role were passed as None. It turns out the !stack_output_external resolver would trigger this exact situation. But this wouldn't actually show up in terms of blow-ups when the user had a profile with a region or a default region specified as an environment variable.

With this fix, the logic is now:

If the stack_name was cached...

if the profile, region, and role were all None, then...

use the cached profile, region and role

otherwise, use the specified profile, region, and role, using the cached value when no value has been specified

This was a gap in our old logic, so this change was intentional and fixed some things.

otherwise...

If the profile, region, and role are all None, then...

Use the current_stack's profile, region, and role

Otherwise, use the specified profile, region, and role, using the current stack's value when no value has been specified

Great write-up!

jfalkenstein · 2023-02-18T16:47:27Z

tests/test_connection_manager.py

+    def test_call__stack_name_set_not_cached__profile_region_and_role_are_none__uses_current_stack_settings(
+        self,
+    ):
+        service = "s3"
+        command = "list_buckets"
+        instance_profile = "profile"
+        instance_role = "role"
+        stack_name = "target"
+        self.set_connection_manager_vars(instance_profile, self.region, instance_role)
+
+        expected_client = self.set_up_expected_client(
+            service, stack_name, instance_profile, self.region, instance_role
+        )
+
+        self.connection_manager.call(
+            service,
+            command,
+            stack_name=stack_name,
+            profile=None,
+            region=None,
+            sceptre_role=None,
+        )
+        expected_client.list_buckets.assert_any_call()


This test, in particular, fails without my fix and passes now.

dboitnot

Looks good.

dboitnot · 2023-02-18T17:02:28Z

sceptre/connection_manager.py

+        # For historical reasons, if all three values are "None", that means we default to the
+        # Stack's configuration.
+        if (profile, region, sceptre_role) == (None, None, None):
+            profile, region, sceptre_role = self.profile, self.region, self.sceptre_role


Great write-up!

## 4.0.2 (2023.02.20) ### Fixed - [Resolve #1307] Fixing Connection Manager bug (#1308)

jfalkenstein added 3 commits February 18, 2023 09:03

adding test for other use for external-stack-output

cb2cff8

updating integration tests to test stack-output-external

39576af

using a profile

1f79bda

jfalkenstein commented Feb 18, 2023

View reviewed changes

jfalkenstein added 2 commits February 18, 2023 10:32

updating docs

877f672

testing connection manager bug fix

8d2ce1a

jfalkenstein commented Feb 18, 2023

View reviewed changes

jfalkenstein marked this pull request as ready for review February 18, 2023 16:52

jfalkenstein requested review from dboitnot and zaro0508 February 18, 2023 16:52

dboitnot approved these changes Feb 18, 2023

View reviewed changes

jfalkenstein merged commit 4576fbe into Sceptre:master Feb 18, 2023

jfalkenstein deleted the 1307-fixing-external-stack-role branch February 18, 2023 18:22

jfalkenstein mentioned this pull request Feb 18, 2023

Releasing v4.0.2 #1310

Merged

1 task

jfalkenstein added a commit that referenced this pull request Feb 20, 2023

Releasing v4.0.2 release (#1310)

bffcf09

## 4.0.2 (2023.02.20) ### Fixed - [Resolve #1307] Fixing Connection Manager bug (#1308)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Resolve #1307] Fixing Connection Manager bug #1308

[Resolve #1307] Fixing Connection Manager bug #1308

jfalkenstein commented Feb 18, 2023 •

edited

Loading

jfalkenstein Feb 18, 2023

jfalkenstein Feb 18, 2023

jfalkenstein Feb 18, 2023

jfalkenstein Feb 18, 2023

jfalkenstein Feb 18, 2023

jfalkenstein Feb 18, 2023

jfalkenstein Feb 18, 2023

dboitnot Feb 18, 2023

jfalkenstein Feb 18, 2023

dboitnot left a comment

dboitnot Feb 18, 2023



		parameters:
		DependentStackName: !stack_output_external "{{project_code}}-external-stack-output-outputter::StackName {{environment_variable.AWS_PROFILE\|default('default')}}::eu-west-1"

[Resolve #1307] Fixing Connection Manager bug #1308

[Resolve #1307] Fixing Connection Manager bug #1308

Conversation

jfalkenstein commented Feb 18, 2023 • edited Loading

PR Checklist

Approver/Reviewer Checklist

Other Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dboitnot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfalkenstein commented Feb 18, 2023 •

edited

Loading