Skip to content

Commit

Permalink
Incorporate review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Jeroen van Erp <jeroen@hierynomus.com>
  • Loading branch information
@hierynomus
hierynomus committed Jan 19, 2022
1 parent 55ffb84 commit aced1cb
Show file tree
Hide file tree
Showing 9 changed files with 82 additions and 65 deletions.
4 changes: 2 additions & 2 deletions docs/cluster_spec.md
View file
Expand Up @@ -257,7 +257,7 @@ spec:
- 12.34.56.78/32
```

Instead of listing all CIDRs, it is possible to specify a pre-existing [AWS Prefix List](https://docs.aws.amazon.com/vpc/latest/userguide/managed-prefix-lists.html) ID
Instead of listing all CIDRs, it is possible to specify a pre-existing [AWS Prefix List](https://docs.aws.amazon.com/vpc/latest/userguide/managed-prefix-lists.html) ID.

## kubernetesApiAccess

Expand All @@ -271,7 +271,7 @@ spec:
- 12.34.56.78/32
```

Instead of listing all CIDRs, it is possible to specify a pre-existing [AWS Prefix List](https://docs.aws.amazon.com/vpc/latest/userguide/managed-prefix-lists.html) ID
Instead of listing all CIDRs, it is possible to specify a pre-existing [AWS Prefix List](https://docs.aws.amazon.com/vpc/latest/userguide/managed-prefix-lists.html) ID.

## cluster.spec Subnet Keys

Expand Down
12 changes: 9 additions & 3 deletions pkg/apis/kops/validation/validation.go
View file
Expand Up @@ -85,21 +85,27 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie

// SSHAccess
for i, cidr := range spec.SSHAccess {
if !strings.HasPrefix(cidr, "pl-") {
if strings.HasPrefix(cidr, "pl-") && kops.CloudProviderID(spec.CloudProvider) != kops.CloudProviderAWS {
allErrs = append(allErrs, field.Invalid(fieldPath.Child("sshAccess").Index(i), cidr, "Prefix List ID only supported for AWS"))
} else {
allErrs = append(allErrs, validateCIDR(cidr, fieldPath.Child("sshAccess").Index(i))...)
}
}

// KubernetesAPIAccess
for i, cidr := range spec.KubernetesAPIAccess {
if !strings.HasPrefix(cidr, "pl-") {
if strings.HasPrefix(cidr, "pl-") && kops.CloudProviderID(spec.CloudProvider) != kops.CloudProviderAWS {
allErrs = append(allErrs, field.Invalid(fieldPath.Child("kubernetesAPIAccess").Index(i), cidr, "Prefix List ID only supported for AWS"))
} else {
allErrs = append(allErrs, validateCIDR(cidr, fieldPath.Child("kubernetesAPIAccess").Index(i))...)
}
}

// NodePortAccess
for i, cidr := range spec.NodePortAccess {
if !strings.HasPrefix(cidr, "pl-") {
if strings.HasPrefix(cidr, "pl-") && kops.CloudProviderID(spec.CloudProvider) != kops.CloudProviderAWS {
allErrs = append(allErrs, field.Invalid(fieldPath.Child("nodePortAccess").Index(i), cidr, "Prefix List ID only supported for AWS"))
} else {
allErrs = append(allErrs, validateCIDR(cidr, fieldPath.Child("nodePortAccess").Index(i))...)
}
}
Expand Down
1 change: 0 additions & 1 deletion pkg/model/awsmodel/BUILD.bazel
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

63 changes: 53 additions & 10 deletions pkg/model/awsmodel/api_loadbalancer.go
View file
Expand Up @@ -19,17 +19,27 @@ package awsmodel
import (
"fmt"
"sort"
"strings"
"time"

"github.com/aws/aws-sdk-go/service/ec2"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/klog/v2"
"k8s.io/kops/pkg/apis/kops"
"k8s.io/kops/pkg/dns"
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kops/upup/pkg/fi/cloudup/awstasks"
"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
"k8s.io/kops/upup/pkg/fi/utils"
)

type PrefixListAddressFamily string

const (
IPv4AddressFamily PrefixListAddressFamily = "IPv4"
IPv6AddressFamily PrefixListAddressFamily = "IPv6"
)

// LoadBalancerDefaultIdleTimeout is the default idle time for the ELB
const LoadBalancerDefaultIdleTimeout = 5 * time.Minute

Expand All @@ -39,6 +49,7 @@ type APILoadBalancerBuilder struct {

Lifecycle fi.Lifecycle
SecurityLifecycle fi.Lifecycle
Cloud awsup.AWSCloud
}

var _ fi.ModelBuilder = &APILoadBalancerBuilder{}
Expand Down Expand Up @@ -368,7 +379,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
SecurityGroup: lbSG,
ToPort: fi.Int64(443),
}
LimitIngress(t, cidr)
t.SetCidrOrPrefix(cidr)
AddDirectionalGroupRule(c, t)
}

Expand Down Expand Up @@ -414,31 +425,45 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
SecurityGroup: masterGroup.Task,
ToPort: fi.Int64(443),
}
LimitIngress(t, cidr)
t.SetCidrOrPrefix(cidr)
AddDirectionalGroupRule(c, t)
}

icmpIpv6 := false
if strings.HasPrefix(cidr, "pl-") {
b, err := b.IsIPv6PrefixList(cidr)
if err != nil {
return err
}
icmpIpv6 = b
} else {
icmpIpv6 = utils.IsIPv6CIDR(cidr)
}

// Allow ICMP traffic required for PMTU discovery
if utils.IsIPv6CIDR(cidr) {
c.AddTask(&awstasks.SecurityGroupRule{
if icmpIpv6 {
t := &awstasks.SecurityGroupRule{
Name: fi.String("icmpv6-pmtu-api-elb-" + cidr),
Lifecycle: b.SecurityLifecycle,
IPv6CIDR: fi.String(cidr),
FromPort: fi.Int64(-1),
Protocol: fi.String("icmpv6"),
SecurityGroup: masterGroup.Task,
ToPort: fi.Int64(-1),
})
} else if utils.IsIPv4CIDR(cidr) {
c.AddTask(&awstasks.SecurityGroupRule{
}
t.SetCidrOrPrefix(cidr)
c.AddTask(t)
} else {
t := &awstasks.SecurityGroupRule{
Name: fi.String("icmp-pmtu-api-elb-" + cidr),
Lifecycle: b.SecurityLifecycle,
CIDR: fi.String(cidr),
FromPort: fi.Int64(3),
Protocol: fi.String("icmp"),
SecurityGroup: masterGroup.Task,
ToPort: fi.Int64(4),
})
}
t.SetCidrOrPrefix(cidr)
c.AddTask(t)
}

if b.Cluster.Spec.API != nil && b.Cluster.Spec.API.LoadBalancer != nil && b.Cluster.Spec.API.LoadBalancer.SSLCertificate != "" {
Expand All @@ -451,7 +476,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
SecurityGroup: masterGroup.Task,
ToPort: fi.Int64(8443),
}
LimitIngress(t, cidr)
t.SetCidrOrPrefix(cidr)
c.AddTask(t)
}
}
Expand Down Expand Up @@ -524,6 +549,24 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
return nil
}

func (b *APILoadBalancerBuilder) IsIPv6PrefixList(prefixListID string) (bool, error) {
out, err := b.Cloud.EC2().DescribeManagedPrefixLists(&ec2.DescribeManagedPrefixListsInput{PrefixListIds: []*string{fi.String(prefixListID)}})
if err != nil {
return false, fmt.Errorf("error describing prefix list %s: %w", prefixListID, err)
}

if len(out.PrefixLists) != 1 {
return false, fmt.Errorf("unexpected number of prefix lists: %d", len(out.PrefixLists))
}

prefixList := out.PrefixLists[0]
if prefixList.AddressFamily != nil {
return PrefixListAddressFamily(fi.StringValue(prefixList.AddressFamily)) == IPv6AddressFamily, nil
} else {
return false, fmt.Errorf("unknown prefix list address family for prefix list: %s", prefixListID)
}
}

type scoredSubnet struct {
score int
subnet *kops.ClusterSubnetSpec
Expand Down
2 changes: 1 addition & 1 deletion pkg/model/awsmodel/bastion.go
View file
Expand Up @@ -218,7 +218,7 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error {
FromPort: fi.Int64(22),
ToPort: fi.Int64(22),
}
LimitIngress(t, sshAccess)
t.SetCidrOrPrefix(sshAccess)
AddDirectionalGroupRule(c, t)
}

Expand Down
10 changes: 5 additions & 5 deletions pkg/model/awsmodel/external_access.go
View file
Expand Up @@ -70,7 +70,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error {
FromPort: fi.Int64(22),
ToPort: fi.Int64(22),
}
LimitIngress(t, sshAccess)
t.SetCidrOrPrefix(sshAccess)
AddDirectionalGroupRule(c, t)
}

Expand All @@ -84,7 +84,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error {
FromPort: fi.Int64(22),
ToPort: fi.Int64(22),
}
LimitIngress(t, sshAccess)
t.SetCidrOrPrefix(sshAccess)
AddDirectionalGroupRule(c, t)
}
}
Expand All @@ -107,7 +107,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error {
FromPort: fi.Int64(int64(nodePortRange.Base)),
ToPort: fi.Int64(int64(nodePortRange.Base + nodePortRange.Size - 1)),
}
LimitIngress(t, nodePortAccess)
t.SetCidrOrPrefix(nodePortAccess)
c.AddTask(t)
}
{
Expand All @@ -119,7 +119,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error {
FromPort: fi.Int64(int64(nodePortRange.Base)),
ToPort: fi.Int64(int64(nodePortRange.Base + nodePortRange.Size - 1)),
}
LimitIngress(t, nodePortAccess)
t.SetCidrOrPrefix(nodePortAccess)
c.AddTask(t)
}
}
Expand All @@ -142,7 +142,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error {
FromPort: fi.Int64(443),
ToPort: fi.Int64(443),
}
LimitIngress(t, apiAccess)
t.SetCidrOrPrefix(apiAccess)
AddDirectionalGroupRule(c, t)
}
}
Expand Down
42 changes: 0 additions & 42 deletions pkg/model/awsmodel/utils.go
View file

This file was deleted.

2 changes: 1 addition & 1 deletion upup/pkg/fi/cloudup/apply_cluster.go
View file
Expand Up @@ -536,7 +536,7 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error {
}

l.Builders = append(l.Builders,
&awsmodel.APILoadBalancerBuilder{AWSModelContext: awsModelContext, Lifecycle: clusterLifecycle, SecurityLifecycle: securityLifecycle},
&awsmodel.APILoadBalancerBuilder{AWSModelContext: awsModelContext, Lifecycle: clusterLifecycle, SecurityLifecycle: securityLifecycle, Cloud: cloud.(awsup.AWSCloud)},
&awsmodel.BastionModelBuilder{AWSModelContext: awsModelContext, Lifecycle: clusterLifecycle, SecurityLifecycle: securityLifecycle},
&awsmodel.DNSModelBuilder{AWSModelContext: awsModelContext, Lifecycle: clusterLifecycle},
&awsmodel.ExternalAccessModelBuilder{AWSModelContext: awsModelContext, Lifecycle: securityLifecycle},
Expand Down
11 changes: 11 additions & 0 deletions upup/pkg/fi/cloudup/awstasks/securitygrouprule.go
View file
Expand Up @@ -29,6 +29,7 @@ import (
"k8s.io/kops/upup/pkg/fi/cloudup/cloudformation"
"k8s.io/kops/upup/pkg/fi/cloudup/terraform"
"k8s.io/kops/upup/pkg/fi/cloudup/terraformWriter"
"k8s.io/kops/upup/pkg/fi/utils"
)

// +kops:fitask
Expand Down Expand Up @@ -139,6 +140,16 @@ func (e *SecurityGroupRule) Find(c *fi.Context) (*SecurityGroupRule, error) {
return nil, nil
}

func (e *SecurityGroupRule) SetCidrOrPrefix(cidr string) {
if strings.HasPrefix(cidr, "pl-") {
e.PrefixList = &cidr
} else if utils.IsIPv6CIDR(cidr) {
e.IPv6CIDR = &cidr
} else {
e.CIDR = &cidr
}
}

func (e *SecurityGroupRule) matches(rule *ec2.SecurityGroupRule) bool {
matchFromPort := int64(-1)
if e.FromPort != nil {
Expand Down

0 comments on commit aced1cb

Please sign in to comment.