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

Feature/gh 118 aws virtual network #624

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from

Conversation

HildericSB
Copy link
Contributor

@HildericSB HildericSB commented Mar 30, 2020

Pull Request description

Description of the change

Now possible to attach a subnet to AWS instance.

What I did

  • Added VPC creation with nested subnets and SG
  • Added stand alone Subnet creation
  • Added default setup for abstract application definition.

How I did it

  • Updated the tosca aws types to match terraform specs for Subnet and VPC creation
  • Updated the tosca aws types in the A4C-yorc-provider plugins
  • Add default VPC creation that will
  1. create a security group that allow the deploying IP machine
  2. create a internet gateway (IG)
  3. configure the default VPC table to redirect by default to the IG

How to verify it

-Try deploying a ACompute linked with a ANetwork.

Applicable Issues

#Fixes #118

prov/terraform/aws/vpc.go Outdated Show resolved Hide resolved
prov/terraform/aws/vpc.go Outdated Show resolved Hide resolved
return nil
}

func (g *awsGenerator) generateSubnet(ctx context.Context, nodeParams nodeParams, instanceName string, outputs map[string]string) error {
Copy link

Choose a reason for hiding this comment

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

Method awsGenerator.generateSubnet has a Cognitive Complexity of 22 (exceeds 20 allowed). Consider refactoring.

return nil
}

func (g *awsGenerator) generateSubnet(ctx context.Context, nodeParams nodeParams, instanceName string, outputs map[string]string) error {
Copy link

Choose a reason for hiding this comment

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

Method awsGenerator.generateSubnet has 65 lines of code (exceeds 50 allowed). Consider refactoring.

"github.com/ystia/yorc/v4/prov/terraform/commons"
)

func (g *awsGenerator) generateVPC(ctx context.Context, nodeParams nodeParams, instanceName string, outputs map[string]string) error {
Copy link

Choose a reason for hiding this comment

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

Method awsGenerator.generateVPC has 52 lines of code (exceeds 50 allowed). Consider refactoring.

prov/terraform/aws/vpc.go Outdated Show resolved Hide resolved
prov/terraform/aws/vpc.go Show resolved Hide resolved
return nil
}

func (g *awsGenerator) generateSubnet(ctx context.Context, nodeParams nodeParams, instanceName string, outputs map[string]string) error {
Copy link

Choose a reason for hiding this comment

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

Method awsGenerator.generateSubnet has 54 lines of code (exceeds 50 allowed). Consider refactoring.

@HildericSB HildericSB self-assigned this Mar 31, 2020
@HildericSB HildericSB marked this pull request as ready for review March 31, 2020 15:41
Copy link
Contributor

@Loic-R Loic-R left a comment

Choose a reason for hiding this comment

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

Some typos in tosca, otherwise, looks good !
To be tested on real AWS infra to make sure that all works.

data/tosca/yorc-aws-types.yml Outdated Show resolved Hide resolved
data/tosca/yorc-aws-types.yml Outdated Show resolved Hide resolved
data/tosca/yorc-aws-types.yml Outdated Show resolved Hide resolved
@HildericSB HildericSB requested a review from Loic-R April 3, 2020 07:29
Copy link
Contributor

@stefbenoist stefbenoist left a comment

Choose a reason for hiding this comment

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

It seems you don't allow to attach an existing vpc to a compute ?

@stefbenoist
Copy link
Contributor

Can't open topology.zip

@HildericSB
Copy link
Contributor Author

llow to attach an existing

It seems you don't allow to attach an existing vpc to a compute ?

VPC can't be attached to compute instance, only subnet can.
This PR is focused on Subnet creation and attachment to an instance.

@stefbenoist
Copy link
Contributor

Ok but we should allow to attach an existing subnet from an existing VPC to a compute ?

@HildericSB
Copy link
Contributor Author

Ok but we should allow to attach an existing subnet from an existing VPC to a compute ?

Ok got it !
I'll use the subnet_id argument in tosca type and make YORC handle that.

Attachment map[string]string `json:"attachment,omitempty"`
}

// todo
Copy link

Choose a reason for hiding this comment

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

comment on exported type SecurityRule should be of the form "SecurityRule ..." (with optional leading article)

CidrBlock []string `json:"cidr_blocks,omitempty"`
}

// todo
Copy link

Choose a reason for hiding this comment

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

comment on exported type SecurityGroups should be of the form "SecurityGroups ..." (with optional leading article)

Name string `json:"name,omitempty"`
}

// todo
Copy link

Choose a reason for hiding this comment

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

comment on exported type RouteTable should be of the form "RouteTable ..." (with optional leading article)

DependsOn []string `json:"depends_on,omitempty"`
}

type DefaultRouteTable struct {
Copy link

Choose a reason for hiding this comment

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

exported type DefaultRouteTable should have comment or be unexported

DependsOn []string `json:"depends_on,omitempty"`
}

// todo
Copy link

Choose a reason for hiding this comment

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

comment on exported type InternetGateway should be of the form "InternetGateway ..." (with optional leading article)

i := 0
for _, networkReq := range networkRequirements {
networkInterface := &NetworkInterface{}
// TODO : Check if subnet and security group are defined in relationship
Copy link

Choose a reason for hiding this comment

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

TODO found

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 2 Code Smells

83.0% 83.0% Coverage
0.0% 0.0% Duplication

name = strings.Replace(strings.ToLower(name), "_", "-", -1)

// First interface will be considered the network interface of the Compute Instance
if i == 0 {
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

name = strings.Replace(strings.ToLower(name), "_", "-", -1)

// First interface will be considered the network interface of the Compute Instance
if i == 0 {
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Sep 11, 2020

Code Climate has analyzed commit c0641d8 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 2

View more on Code Climate.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 3 Code Smells

76.5% 76.5% Coverage
5.7% 5.7% Duplication

warning The version of Java (1.8.0_151) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS virtual networks support
3 participants