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.

Sign up for GitHub

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

Jump to bottom

ec2_asg: enable integration tests, change instance type to a type allowed by policy #815

Merged
merged 7 commits into from
Feb 15, 2022
Merged

ec2_asg: enable integration tests, change instance type to a type allowed by policy #815

merged 7 commits into from
Feb 15, 2022

Conversation

mandar242
Copy link
Contributor

@mandar242 mandar242 commented Nov 30, 2021 •
edited by goneri
Loading

Depends-On: ansible/ansible-zuul-jobs#1247

SUMMARY

Enable ec2_asg integration tests as fix for resolving ec2_asg failing due to policies missing is merged.
Also change the instance type used in couple of assertions to a type which is allowed by the policy to fix failure.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ec2_asg

@mandar242 mandar242 changed the title ec2_asg: enable integration tests, change instance type to a type allowed by policy [WIP] ec2_asg: enable integration tests, change instance type to a type allowed by policy Nov 30, 2021
@jillr
Copy link
Collaborator

jillr commented Nov 30, 2021

recheck

1 similar comment
@goneri
Copy link
Member

goneri commented Nov 30, 2021

recheck

@jillr
Copy link
Collaborator

jillr commented Nov 30, 2021

@mandar242 Ah, there's a bug in the tests that only happens when the resource_prefix (which is based on the test system hostname) has dashes at the end.

    - name: load balancer name has to be less than 32 characters
      # the 8 digit identifier at the end of resource_prefix helps determine during which test something
      # was created
      set_fact:
        load_balancer_name: "{{ item }}-lb"
      loop: "{{ resource_prefix | regex_findall('.{8}$') }}"

This regex takes whatever the last 8 characters are, without consideration that LoadBalancer name cannot begin or end with a dash.

The resource_prefix in CI looks like: ansible-test-54272280-ip-172-16-141-188 so we end up with a load_balancer_name of -141-188 which gives the ValidationError. We didn't see this when we tested locally because both of our laptops have hostnames without dashes in them. ansible-test now has another variable we can use to solve exactly this kind of problem with AWS character limits. Could you please remove this set_fact statement and instead make the tests use '{{ tiny_prefix }}-lb' for the LB name?

@mandar242 mandar242 force-pushed the ec2_asg_integration_fix branch from 8cfba75 to e2876cb Compare November 30, 2021 21:47
@mandar242 mandar242 requested a review from jillr December 8, 2021 17:37
jillr
jillr reviewed Dec 17, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

Spent some time on the failures for the mixed-instances test but I haven't got it sorted out yet. I suspect it might have something to do with the way AWSServiceRoleForAutoScaling is being used to access the launch template. Documenting what I've tried here:
Changing the compute policy to allow the AllowAsgInstancesInstanceType Sid to access resource '*' gets me to a new, more descriptive error that An error occurred (AccessDenied) when calling the UpdateAutoScalingGroup operation: You are not authorized to use launch template: lt-0a2196568c24cb25d",. I'm not finding a whole lot of documentation for PassRole for ASGs but I took a couple tries at allowing AWSServiceRoleForAutoScaling to be passed to ec2 with no success. I've reverted the CI staging policies back to main in the interim.

@tremble Do you have any experience working with ASG policies and launch templates?

tests/integration/targets/ec2_asg/tasks/main.yml Outdated Show resolved Hide resolved
@mandar242 mandar242 changed the title [WIP] ec2_asg: enable integration tests, change instance type to a type allowed by policy ec2_asg: enable integration tests, change instance type to a type allowed by policy Jan 17, 2022
@softwarefactory-project-zuul

This comment was marked as outdated.

Sign in to view
@mandar242
Copy link
Contributor Author

recheck

@softwarefactory-project-zuul

This comment was marked as outdated.

Sign in to view
@markuman markuman mentioned this pull request Feb 1, 2022
Closed
1 task
@mandar242
Copy link
Contributor Author

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (third-party-check pipeline).

@ansibullbot
Copy link

cc @garethr @markuman @s-hertel @tremble
click here for bot help

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review has_issue integration tests/integration plugins plugin (any type) tests tests labels Feb 2, 2022
@softwarefactory-project-zuul

This comment was marked as outdated.

Sign in to view
@mandar242 mandar242 mentioned this pull request Feb 10, 2022
Merged
@jillr jillr mentioned this pull request Feb 10, 2022
Merged
@mandar242
Copy link
Contributor Author

recheck

@softwarefactory-project-zuul

This comment was marked as outdated.

Sign in to view
@mandar242
Copy link
Contributor Author

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@mandar242 mandar242 force-pushed the ec2_asg_integration_fix branch from 2016171 to 53f7251 Compare February 15, 2022 08:29
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@jillr jillr added the mergeit Merge the PR (SoftwareFactory) label Feb 15, 2022
@jillr
Copy link
Collaborator

jillr commented Feb 15, 2022

thanks @mandar242! good work on this one!

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit f58525b into ansible-collections:main Feb 15, 2022
@mandar242 mandar242 deleted the ec2_asg_integration_fix branch February 26, 2022 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jillr jillr jillr approved these changes

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug community_review has_issue integration tests/integration mergeit Merge the PR (SoftwareFactory) plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mandar242 @jillr @goneri @ansibullbot