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

Add infrastructure ConfigValidator for checking VPC IDs #395

Merged
merged 2 commits into from
Aug 30, 2021

Conversation

stoyanr
Copy link
Contributor

@stoyanr stoyanr commented Aug 26, 2021

How to categorize this PR?

/area ops-productivity
/area quality
/kind bug
/platform aws

What this PR does / why we need it:

  • Adds an implementation of the infrastructure.ConfigValidator for checking if a VPC ID exists, has correct VPC attribute values, and have an internet gateway attached. See Add infrastructure config validator gardener#4547.
  • Vendors g/g v1.30.0-dev (in a separate commit).
  • Adds unit tests for the infrastructure.ConfigValidator implementation (in a separate commit).

Which issue(s) this PR fixes:

Special notes for your reviewer:

Release note:

NONE

@stoyanr stoyanr requested review from a team as code owners August 26, 2021 12:10
@gardener-robot gardener-robot added needs/review Needs review area/ops-productivity Operator productivity related (how to improve operations) area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/bug Bug platform/aws Amazon web services platform/infrastructure size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Aug 26, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 26, 2021
@stoyanr
Copy link
Contributor Author

stoyanr commented Aug 26, 2021

/invite @ialidzhikov @rfranzke @kon-angelo

@stoyanr stoyanr changed the title Add infra config validator Add infrastructure ConfigValidator for checking VPC IDs Aug 26, 2021
@stoyanr
Copy link
Contributor Author

stoyanr commented Aug 26, 2021

/squash

@stoyanr stoyanr marked this pull request as draft August 26, 2021 13:18
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 26, 2021
@stoyanr
Copy link
Contributor Author

stoyanr commented Aug 27, 2021

/test

@testmachinery
Copy link

testmachinery bot commented Aug 27, 2021

Testrun: e2e-pvr8q
Workflow: e2e-pvr8q-wf
Phase: Failed

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| dnsrecord-test      | dnsrecord-test      | Succeeded | 7m37s    |
| infrastructure-test | infrastructure-test | Failed    | 15m19s   |
| bastion-test        | bastion-test        | Succeeded | 5m0s     |
+---------------------+---------------------+-----------+----------+

@stoyanr
Copy link
Contributor Author

stoyanr commented Aug 27, 2021

/test

@testmachinery
Copy link

testmachinery bot commented Aug 27, 2021

Testrun: e2e-44xs2
Workflow: e2e-44xs2-wf
Phase: Failed

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| infrastructure-test | infrastructure-test | Failed    | 15m52s   |
| bastion-test        | bastion-test        | Succeeded | 5m17s    |
| dnsrecord-test      | dnsrecord-test      | Succeeded | 10m19s   |
+---------------------+---------------------+-----------+----------+

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 27, 2021
@stoyanr
Copy link
Contributor Author

stoyanr commented Aug 27, 2021

/test

@testmachinery
Copy link

testmachinery bot commented Aug 27, 2021

Testrun: e2e-jt7bc
Workflow: e2e-jt7bc-wf
Phase: Succeeded

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| infrastructure-test | infrastructure-test | Succeeded | 15m16s   |
| bastion-test        | bastion-test        | Succeeded | 5m24s    |
| dnsrecord-test      | dnsrecord-test      | Succeeded | 6m13s    |
+---------------------+---------------------+-----------+----------+

@stoyanr stoyanr marked this pull request as ready for review August 27, 2021 12:32
@gardener-robot
Copy link

@rfranzke, @kon-angelo You have pull request review open invite, please check

@ialidzhikov
Copy link
Member

/hold
as there is already ongoing release validation

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Aug 30, 2021
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm

func ignoreNotFound(err error) error {
if err == nil {
return nil
func IsNotFoundError(err error) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: doc string missing

@gardener-robot gardener-robot added needs/lgtm Needs approval for merging reviewed/lgtm Has approval for merging and removed needs/lgtm Needs approval for merging needs/review Needs review needs/second-opinion Needs second review by someone else labels Aug 30, 2021
@stoyanr stoyanr merged commit 7514078 into gardener:master Aug 30, 2021
@stoyanr stoyanr deleted the add-infra-config-validator branch August 30, 2021 15:45
ialidzhikov added a commit to ialidzhikov/gardener-extension-provider-aws that referenced this pull request Aug 31, 2021
stoyanr pushed a commit that referenced this pull request Aug 31, 2021
@ialidzhikov
Copy link
Member

Let's reopen this PR or have a new one and have it in after the release of v1.28.0 (as we did a revert with 8f00f94).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ops-productivity Operator productivity related (how to improve operations) area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/bug Bug needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/aws Amazon web services platform/infrastructure reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies reviewed/lgtm Has approval for merging size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants