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

Feat/boto describe default vpc #58628

Merged
merged 3 commits into from
Oct 7, 2020
Merged

Feat/boto describe default vpc #58628

merged 3 commits into from
Oct 7, 2020

Conversation

major0
Copy link
Contributor

@major0 major0 commented Oct 4, 2020 •
edited
Loading

What does this PR do?

Allow looking up the default AWS VPC which is automatically created at account creation time.

Previous Behavior

Previously the boto_vpc module only allowed looking up a VPC via the vpc_id or the vpc_name. The default VPC does not have a vpc_name and as such it is not possible to look it up.

New Behavior

If neither the vpc_id nor the vpc_name are supplied to boto_vpc.describe() then it will describe the default VPC.

The following functions were modified to make this possible:

  • _find_vpcs()
  • describe()

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@major0 major0 requested a review from a team as a code owner October 4, 2020 22:09
@major0 major0 requested review from Akm0d and removed request for a team October 4, 2020 22:09
@ghost ghost requested a review from twangboy October 4, 2020 22:09
Akm0d
Akm0d previously approved these changes Oct 4, 2020
View reviewed changes
@major0 major0 changed the title Feat/boto describe default vpc WIP: Feat/boto describe default vpc Oct 4, 2020
@major0
Copy link
Contributor Author

major0 commented Oct 4, 2020

I am curious if it wouldn't be better to simply lookup the default vpc if no vpc_id and no vpc_name are specified.

@major0 major0 force-pushed the feat/boto_describe_default_vpc branch from 8bf7549 to 07768cf Compare October 5, 2020 01:02
@major0 major0 changed the title WIP: Feat/boto describe default vpc Feat/boto describe default vpc Oct 5, 2020
@major0 major0 force-pushed the feat/boto_describe_default_vpc branch from 07768cf to 832096d Compare October 5, 2020 01:11
@major0 major0 requested a review from Akm0d October 5, 2020 01:12
@major0 major0 force-pushed the feat/boto_describe_default_vpc branch from 832096d to d306d60 Compare October 5, 2020 01:20
@major0
Copy link
Contributor Author

major0 commented Oct 5, 2020 •
edited
Loading

Reworked the PR to modify fewer functions by allowing the calling of boto_vpc.describe w/out a vpc_id or vpc_name. This required a minor rework to _find_vpcs() to support this, and replaces the check_vpc() call in describe()with _find_vpcs(). This also avoids problems with other uses of check_vpc() in which we do not want to return the default VPC if no id/name is specified.

@major0 major0 force-pushed the feat/boto_describe_default_vpc branch from d306d60 to fca025d Compare October 6, 2020 01:11
Akm0d
Akm0d previously approved these changes Oct 6, 2020
View reviewed changes
@Akm0d Akm0d added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Oct 6, 2020
@Akm0d
Copy link
Contributor

Akm0d commented Oct 6, 2020

Thanks for the PR! Once there's a test for the changes we can merge. A unit test should do the trick.

@major0 major0 force-pushed the feat/boto_describe_default_vpc branch from fca025d to 8c4c45e Compare October 7, 2020 13:33
@major0
Copy link
Contributor Author

major0 commented Oct 7, 2020 •
edited
Loading

I have updated the test case, but while I was doing so I discovered that none of the VPC tests are running.

From the code:

@skipIf(
    sys.version_info > (3, 6),
    "Disabled for 3.7+ pending https://github.com/spulec/moto/issues/1706.",
)
class BotoVpcTestCase(BotoVpcTestCaseBase, BotoVpcTestCaseMixin):

All of the following tests are skipped when running nox -e 'pytest-zeromq-3(coverage=False)' -- tests/unit/modules/test_boto*

  • tests/unit/modules/test_boto_cognitoidentity.py
  • tests/unit/modules/test_boto_elasticsearch_domain.py
  • tests/unit/modules/test_boto_elb.py
  • tests/unit/modules/test_boto_secgroup.py
  • tests/unit/modules/test_boto_vpc.py
========================= 374 passed, 186 skipped, 4 warnings in 61.19s (0:01:01) ===========================

Roughly 1/3rd of the boto tests.

@major0
Copy link
Contributor Author

major0 commented Oct 7, 2020

I suppose it is worth pointing out that a number of individual tests in tests/unit/modules/test_boto_vpc.py have likely never been operational:

SKIPPED [100] .nox/pytest-parametrized-3-crypto-none-transport-zeromq-coverage-false/lib/python3.8/site-packages/_pytest/unittest.py:126: Disabled for 3.7+ pending https://github.com/spulec/moto/issues/1706.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:1264: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:1247: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:1233: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2062: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2080: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2097: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2048: Disabled pending https://github.com/spulec/moto/issues/493
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2134: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:1972: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:1956: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2151: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2003: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2165: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:1986: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2182: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2114: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2196: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2213: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2034: Moto has not implemented this feature. Skipping for now.
SKIPPED [1] tests/unit/modules/test_boto_vpc.py:2017: Moto has not implemented this feature. Skipping for now.

@major0 major0 force-pushed the feat/boto_describe_default_vpc branch from 8c4c45e to 6d61375 Compare October 7, 2020 14:25
@major0
Copy link
Contributor Author

major0 commented Oct 7, 2020

According to the linked issue (getmoto/moto#1706), this problem should have been resolved in moto, so I have re-enabled the VPC tests and made certain they all pass on my local system.

@major0 major0 force-pushed the feat/boto_describe_default_vpc branch from 6d61375 to 5e7684f Compare October 7, 2020 14:28
@major0
Copy link
Contributor Author

major0 commented Oct 7, 2020

I have created a new PR to re-enable a large swath of boto unit tests: #58660

major0 added 2 commits October 7, 2020 08:48
@major0
feat: allow querying for the default VPC
575f5e7 
All AWS accounts are created with a default VPC in the regions to which
all instances will be launched onto if no alternate VPC is specified..
These VPC's have no name and as such can not be looked up via the
`vpc_name`. This commit makes it such that you do not have to already
know the VPC ID in order to apply basic security
changes/tweaks/enhancements to the AWS Default VPC.
@major0
fix: cleanup changes made by pre-commit scripts
1f845e2
@major0 major0 force-pushed the feat/boto_describe_default_vpc branch from 5e7684f to 1f845e2 Compare October 7, 2020 15:49
@sagetherage sagetherage requested a review from Akm0d October 7, 2020 16:38
@Akm0d Akm0d removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Oct 7, 2020
@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label Oct 7, 2020
@dwoz dwoz merged commit 886cfef into saltstack:master Oct 7, 2020
@major0 major0 deleted the feat/boto_describe_default_vpc branch October 7, 2020 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Akm0d Akm0d Akm0d approved these changes

@twangboy twangboy Awaiting requested review from twangboy

Assignees
No one assigned
Labels
Magnesium Mg release after Na prior to Al
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@major0 @Akm0d @dwoz @sagetherage