kubernetes
/
kops
mirror of https://github.com/kubernetes/kops.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Apply suggestions from code review 

Co-authored-by: Peter Rifel <rifelpet@users.noreply.github.com>
Signed-off-by: Guillaume Perrin <guillaume28.perrin@gmail.com>
This commit is contained in:
Guillaume Perrin 2022-03-28 14:35:42 +02:00
parent 19330df09f
commit db27c00fa0
5 changed files with 51 additions and 81 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
pkg/apis/kops/validation/aws.go
View File

@ -49,12 +49,12 @@ func awsValidateCluster(c *kops.Cluster) field.ErrorList {
}
for i, subnet := range c.Spec.Subnets {
f := field.NewPath("spec", "Subnets").Index(i)
f := field.NewPath("spec", "subnets").Index(i)
if subnet.AdditionalRoutes != nil {
if len(subnet.ProviderID) > 0 {
allErrs = append(allErrs, field.Invalid(f, subnet, "additional routes cannot be added if the subnet is shared"))
}
allErrs = append(allErrs, awsValidateAdditionalRoutes(f.Child("AdditionalRoutes"), subnet.AdditionalRoutes, c.Spec.NetworkCIDR)...)
allErrs = append(allErrs, awsValidateAdditionalRoutes(f.Child("additionalRoutes"), subnet.AdditionalRoutes, c.Spec.NetworkCIDR)...)
}
}
@ -363,7 +363,7 @@ func awsValidateAdditionalRoutes(fieldPath *field.Path, routes []kops.RouteSpec,
_, clusterNet, errClusterNet := net.ParseCIDR(cidr)
if errClusterNet != nil {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "NetworkCIDR"), cidr, "Invalid cluster cidr"))
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "networkCIDR"), cidr, "Invalid cluster cidr"))
} else {
for i, r := range routes {
f := fieldPath.Index(i)
@ -375,14 +375,14 @@ func awsValidateAdditionalRoutes(fieldPath *field.Path, routes []kops.RouteSpec,
!strings.HasPrefix(r.Target, "tgw-") &&
!strings.HasPrefix(r.Target, "igw-") &&
!strings.HasPrefix(r.Target, "eigw-") {
allErrs = append(allErrs, field.Invalid(f, r, "unknown target type for route"))
allErrs = append(allErrs, field.Invalid(f.Child("target"), r, "unknown target type for route"))
}
ipRoute, _, e := net.ParseCIDR(r.CIDR)
if e != nil {
allErrs = append(allErrs, field.Invalid(f, r, "invalid cidr"))
allErrs = append(allErrs, field.Invalid(f.Child("cidr"), r, "invalid cidr"))
} else if clusterNet.Contains(ipRoute) && strings.HasPrefix(r.Target, "pcx-") {
allErrs = append(allErrs, field.Forbidden(f, "target is more specific than cluster CIDR block. This route can target only an interface or an instance."))
allErrs = append(allErrs, field.Forbidden(f.Child("target"), "target is more specific than cluster CIDR block. This route can target only an interface or an instance."))
}
}
}
@ -394,7 +394,7 @@ func awsValidateAdditionalRoutes(fieldPath *field.Path, routes []kops.RouteSpec,
for i := range routes {
rCidr := routes[i].CIDR
if cidrs.Has(rCidr) {
allErrs = append(allErrs, field.Duplicate(fieldPath.Index(i).Child("CIDR"), rCidr))
allErrs = append(allErrs, field.Duplicate(fieldPath.Index(i).Child("cidr"), rCidr))
}
cidrs.Insert(rCidr)
}

12
pkg/apis/kops/validation/aws_test.go
View File

@ -714,7 +714,7 @@ func TestAWSAdditionalRoutes(t *testing.T) {
Target: "pcx-abcdef",
},
},
expected: []string{"Invalid value::spec.NetworkCIDR"},
expected: []string{"Invalid value::spec.networkCIDR"},
},
{ // bad cidr
clusterCidr: "100.64.0.0/10",
@ -724,7 +724,7 @@ func TestAWSAdditionalRoutes(t *testing.T) {
Target: "pcx-abcdef",
},
},
expected: []string{"Invalid value::spec.Subnets[0].AdditionalRoutes[0]"},
expected: []string{"Invalid value::spec.subnets[0].additionalRoutes[0].cidr"},
},
{ // bad target
clusterCidr: "100.64.0.0/10",
@ -734,7 +734,7 @@ func TestAWSAdditionalRoutes(t *testing.T) {
Target: "unknown-abcdef",
},
},
expected: []string{"Invalid value::spec.Subnets[0].AdditionalRoutes[0]"},
expected: []string{"Invalid value::spec.subnets[0].additionalRoutes[0].target"},
},
{ // target more specific
clusterCidr: "100.64.0.0/10",
@ -744,7 +744,7 @@ func TestAWSAdditionalRoutes(t *testing.T) {
Target: "pcx-abcdef",
},
},
expected: []string{"Forbidden::spec.Subnets[0].AdditionalRoutes[0]"},
expected: []string{"Forbidden::spec.subnets[0].additionalRoutes[0].target"},
},
{ // duplicates cidr
clusterCidr: "100.64.0.0/10",
@ -758,7 +758,7 @@ func TestAWSAdditionalRoutes(t *testing.T) {
Target: "tgw-abcdef",
},
},
expected: []string{"Duplicate value::spec.Subnets[0].AdditionalRoutes[1].CIDR"},
expected: []string{"Duplicate value::spec.subnets[0].additionalRoutes[1].cidr"},
},
{ // shared subnet
clusterCidr: "100.64.0.0/10",
@ -769,7 +769,7 @@ func TestAWSAdditionalRoutes(t *testing.T) {
Target: "pcx-abcdef",
},
},
expected: []string{"Invalid value::spec.Subnets[0]"},
expected: []string{"Invalid value::spec.subnets[0]"},
},
}

2
pkg/model/awsmodel/context.go
View File

@ -80,7 +80,7 @@ func (b *AWSModelContext) LinkToUtilitySubnetInZone(zoneName string) (*awstasks.
return b.LinkToSubnet(matches[0]), nil
}
func (b *AWSModelContext) LinkToPrivateSubnetInZone(zoneName string) ([]*awstasks.Subnet, error) {
func (b *AWSModelContext) LinkToPrivateSubnetsInZone(zoneName string) ([]*awstasks.Subnet, error) {
var matches []*kops.ClusterSubnetSpec
for i := range b.Cluster.Spec.Subnets {
s := &b.Cluster.Spec.Subnets[i]

56
pkg/model/awsmodel/network.go
View File

@ -569,7 +569,7 @@ func (b *NetworkModelBuilder) Build(c *fi.ModelBuilderContext) error {
})
}
subnets, err := b.LinkToPrivateSubnetInZone(zone)
subnets, err := b.LinkToPrivateSubnetsInZone(zone)
if err != nil {
return err
}
@ -638,14 +638,14 @@ func (b *NetworkModelBuilder) Build(c *fi.ModelBuilderContext) error {
func addAdditionalRoutes(routes []kops.RouteSpec, sbName string, rt *awstasks.RouteTable, lf fi.Lifecycle, c *fi.ModelBuilderContext) error {
for _, r := range routes {
t := &awstasks.Route{
Name: fi.String("public-" + sbName + "." + r.CIDR),
Lifecycle: lf,
CIDR: fi.String(r.CIDR),
RouteTable: rt,
}
if strings.HasPrefix(r.Target, "pcx-") {
t := &awstasks.Route{
Name: fi.String("public-" + sbName + "." + r.CIDR),
Lifecycle: lf,
CIDR: fi.String(r.CIDR),
RouteTable: rt,
VPCPeeringConnection: fi.String(r.Target),
}
t.VPCPeeringConnectionID = fi.String(r.Target)
c.AddTask(t)
} else if strings.HasPrefix(r.Target, "i-") {
inst := &awstasks.Instance{
@ -658,13 +658,7 @@ func addAdditionalRoutes(routes []kops.RouteSpec, sbName string, rt *awstasks.Ro
if err != nil {
return err
}
t := &awstasks.Route{
Name: fi.String("public-" + sbName + "." + r.CIDR),
Lifecycle: lf,
CIDR: fi.String(r.CIDR),
RouteTable: rt,
Instance: inst,
}
t.Instance = inst
c.AddTask(t)
} else if strings.HasPrefix(r.Target, "nat-") {
nat := &awstasks.NatGateway{
@ -677,22 +671,10 @@ func addAdditionalRoutes(routes []kops.RouteSpec, sbName string, rt *awstasks.Ro
if err != nil {
return err
}
t := &awstasks.Route{
Name: fi.String("public-" + sbName + "." + r.CIDR),
Lifecycle: lf,
CIDR: fi.String(r.CIDR),
RouteTable: rt,
NatGateway: nat,
}
t.NatGateway = nat
c.AddTask(t)
} else if strings.HasPrefix(r.Target, "tgw-") {
t := &awstasks.Route{
Name: fi.String("public-" + sbName + "." + r.CIDR),
Lifecycle: lf,
CIDR: fi.String(r.CIDR),
RouteTable: rt,
TransitGatewayID: fi.String(r.Target),
}
t.TransitGatewayID = fi.String(r.Target)
c.AddTask(t)
} else if strings.HasPrefix(r.Target, "igw-") {
internetGW := &awstasks.InternetGateway{
@ -705,13 +687,7 @@ func addAdditionalRoutes(routes []kops.RouteSpec, sbName string, rt *awstasks.Ro
if err != nil {
return err
}
t := &awstasks.Route{
Name: fi.String("public-" + sbName + "." + r.CIDR),
Lifecycle: lf,
CIDR: fi.String(r.CIDR),
RouteTable: rt,
InternetGateway: internetGW,
}
t.InternetGateway = internetGW
c.AddTask(t)
} else if strings.HasPrefix(r.Target, "eigw-") {
eigw := &awstasks.EgressOnlyInternetGateway{
@ -724,13 +700,7 @@ func addAdditionalRoutes(routes []kops.RouteSpec, sbName string, rt *awstasks.Ro
if err != nil {
return err
}
t := &awstasks.Route{
Name: fi.String("public-" + sbName + "." + r.CIDR),
Lifecycle: lf,
CIDR: fi.String(r.CIDR),
RouteTable: rt,
EgressOnlyInternetGateway: eigw,
}
t.EgressOnlyInternetGateway = eigw
c.AddTask(t)
}
}

48
upup/pkg/fi/cloudup/awstasks/route.go
View File

@ -45,7 +45,7 @@ type Route struct {
InternetGateway *InternetGateway
NatGateway *NatGateway
TransitGatewayID *string
VPCPeeringConnection *string
VPCPeeringConnectionID *string
}
func (e *Route) Find(c *fi.Context) (*Route, error) {
@ -102,7 +102,7 @@ func (e *Route) Find(c *fi.Context) (*Route, error) {
actual.TransitGatewayID = r.TransitGatewayId
}
if r.VpcPeeringConnectionId != nil {
actual.VPCPeeringConnection = r.VpcPeeringConnectionId
actual.VPCPeeringConnectionID = r.VpcPeeringConnectionId
}
if aws.StringValue(r.State) == "blackhole" {
@ -159,7 +159,7 @@ func (s *Route) CheckChanges(a, e, changes *Route) error {
if e.TransitGatewayID != nil {
targetCount++
}
if e.VPCPeeringConnection != nil {
if e.VPCPeeringConnectionID != nil {
targetCount++
}
if targetCount == 0 {
@ -196,7 +196,7 @@ func (_ *Route) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Route) error {
klog.Fatal("both CIDR and IPv6CIDR were unexpectedly nil")
}
if e.EgressOnlyInternetGateway == nil && e.InternetGateway == nil && e.NatGateway == nil && e.TransitGatewayID == nil && e.VPCPeeringConnection == nil {
if e.EgressOnlyInternetGateway == nil && e.InternetGateway == nil && e.NatGateway == nil && e.TransitGatewayID == nil && e.VPCPeeringConnectionID == nil {
return fmt.Errorf("missing target for route")
} else if e.EgressOnlyInternetGateway != nil {
request.EgressOnlyInternetGatewayId = checkNotNil(e.EgressOnlyInternetGateway.ID)
@ -206,8 +206,8 @@ func (_ *Route) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Route) error {
request.NatGatewayId = checkNotNil(e.NatGateway.ID)
} else if e.TransitGatewayID != nil {
request.TransitGatewayId = e.TransitGatewayID
} else if e.VPCPeeringConnection != nil {
request.VpcPeeringConnectionId = e.VPCPeeringConnection
} else if e.VPCPeeringConnectionID != nil {
request.VpcPeeringConnectionId = e.VPCPeeringConnectionID
}
if e.Instance != nil {
@ -242,7 +242,7 @@ func (_ *Route) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Route) error {
klog.Fatal("both CIDR and IPv6CIDR were unexpectedly nil")
}
if e.InternetGateway == nil && e.NatGateway == nil && e.TransitGatewayID == nil && e.VPCPeeringConnection == nil {
if e.InternetGateway == nil && e.NatGateway == nil && e.TransitGatewayID == nil && e.VPCPeeringConnectionID == nil {
return fmt.Errorf("missing target for route")
} else if e.InternetGateway != nil {
request.GatewayId = checkNotNil(e.InternetGateway.ID)
@ -250,8 +250,8 @@ func (_ *Route) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Route) error {
request.NatGatewayId = checkNotNil(e.NatGateway.ID)
} else if e.TransitGatewayID != nil {
request.TransitGatewayId = e.TransitGatewayID
} else if e.VPCPeeringConnection != nil {
request.VpcPeeringConnectionId = e.VPCPeeringConnection
} else if e.VPCPeeringConnectionID != nil {
request.VpcPeeringConnectionId = e.VPCPeeringConnectionID
}
if e.Instance != nil {
@ -290,7 +290,7 @@ type terraformRoute struct {
NATGatewayID *terraformWriter.Literal `cty:"nat_gateway_id"`
TransitGatewayID *string `cty:"transit_gateway_id"`
InstanceID *terraformWriter.Literal `cty:"instance_id"`
VPCPeeringConnection *string `cty:"vpc_peering_connection_id"`
VPCPeeringConnectionID *string `cty:"vpc_peering_connection_id"`
}
func (_ *Route) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *Route) error {
@ -300,7 +300,7 @@ func (_ *Route) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *Rou
IPv6CIDR: e.IPv6CIDR,
}
if e.EgressOnlyInternetGateway == nil && e.InternetGateway == nil && e.NatGateway == nil && e.TransitGatewayID == nil && e.VPCPeeringConnection == nil {
if e.EgressOnlyInternetGateway == nil && e.InternetGateway == nil && e.NatGateway == nil && e.TransitGatewayID == nil && e.VPCPeeringConnectionID == nil {
return fmt.Errorf("missing target for route")
} else if e.EgressOnlyInternetGateway != nil {
tf.EgressOnlyInternetGatewayID = e.EgressOnlyInternetGateway.TerraformLink()
@ -310,8 +310,8 @@ func (_ *Route) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *Rou
tf.NATGatewayID = e.NatGateway.TerraformLink()
} else if e.TransitGatewayID != nil {
tf.TransitGatewayID = e.TransitGatewayID
} else if e.VPCPeeringConnection != nil {
tf.VPCPeeringConnection = e.VPCPeeringConnection
} else if e.VPCPeeringConnectionID != nil {
tf.VPCPeeringConnectionID = e.VPCPeeringConnectionID
}
if e.Instance != nil {
@ -325,14 +325,14 @@ func (_ *Route) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *Rou
}
type cloudformationRoute struct {
RouteTableID *cloudformation.Literal `json:"RouteTableId"`
CIDR *string `json:"DestinationCidrBlock,omitempty"`
IPv6CIDR *string `json:"DestinationIpv6CidrBlock,omitempty"`
InternetGatewayID *cloudformation.Literal `json:"GatewayId,omitempty"`
NATGatewayID *cloudformation.Literal `json:"NatGatewayId,omitempty"`
TransitGatewayID *string `json:"TransitGatewayId,omitempty"`
InstanceID *cloudformation.Literal `json:"InstanceId,omitempty"`
VPCPeeringConnection *string `json:"VpcPeeringConnectionId,omitempty"`
RouteTableID *cloudformation.Literal `json:"RouteTableId"`
CIDR *string `json:"DestinationCidrBlock,omitempty"`
IPv6CIDR *string `json:"DestinationIpv6CidrBlock,omitempty"`
InternetGatewayID *cloudformation.Literal `json:"GatewayId,omitempty"`
NATGatewayID *cloudformation.Literal `json:"NatGatewayId,omitempty"`
TransitGatewayID *string `json:"TransitGatewayId,omitempty"`
InstanceID *cloudformation.Literal `json:"InstanceId,omitempty"`
VPCPeeringConnectionID *string `json:"VpcPeeringConnectionId,omitempty"`
}
func (_ *Route) RenderCloudformation(t *cloudformation.CloudformationTarget, a, e, changes *Route) error {
@ -342,7 +342,7 @@ func (_ *Route) RenderCloudformation(t *cloudformation.CloudformationTarget, a,
IPv6CIDR: e.IPv6CIDR,
}
if e.InternetGateway == nil && e.NatGateway == nil && e.TransitGatewayID == nil && e.VPCPeeringConnection == nil {
if e.InternetGateway == nil && e.NatGateway == nil && e.TransitGatewayID == nil && e.VPCPeeringConnectionID == nil {
return fmt.Errorf("missing target for route")
} else if e.InternetGateway != nil {
tf.InternetGatewayID = e.InternetGateway.CloudformationLink()
@ -350,8 +350,8 @@ func (_ *Route) RenderCloudformation(t *cloudformation.CloudformationTarget, a,
tf.NATGatewayID = e.NatGateway.CloudformationLink()
} else if e.TransitGatewayID != nil {
tf.TransitGatewayID = e.TransitGatewayID
} else if e.VPCPeeringConnection != nil {
tf.VPCPeeringConnection = e.VPCPeeringConnection
} else if e.VPCPeeringConnectionID != nil {
tf.VPCPeeringConnectionID = e.VPCPeeringConnectionID
}
if e.Instance != nil {