Skip to content

Commit

Permalink
Merge pull request #5073 from patrickdillon/4.6-aws-unknown-region
Browse files Browse the repository at this point in the history 
Bug 1981929:  aws: allow use of unknown regions in known partitions
  • Loading branch information
@openshift-ci
openshift-ci[bot] committed Jul 30, 2021
2 parents a16b6e6 + 3f22702 commit 939aef4
Show file tree
Hide file tree
Showing 2 changed files with 176 additions and 33 deletions.
66 changes: 50 additions & 16 deletions pkg/asset/installconfig/aws/validation.go
View file
Expand Up @@ -12,6 +12,7 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation/field"

"github.com/openshift/installer/pkg/rhcos"
"github.com/openshift/installer/pkg/types"
awstypes "github.com/openshift/installer/pkg/types/aws"
)
Expand All @@ -23,6 +24,7 @@ func Validate(ctx context.Context, meta *Metadata, config *types.InstallConfig)
if config.Platform.AWS == nil {
return errors.New(field.Required(field.NewPath("platform", "aws"), "AWS validation requires an AWS platform configuration").Error())
}
allErrs = append(allErrs, validateAMI(ctx, config)...)
allErrs = append(allErrs, validatePlatform(ctx, meta, field.NewPath("platform", "aws"), config.Platform.AWS, config.Networking, config.Publish)...)

if config.ControlPlane != nil && config.ControlPlane.Platform.AWS != nil {
Expand All @@ -40,10 +42,6 @@ func Validate(ctx context.Context, meta *Metadata, config *types.InstallConfig)
func validatePlatform(ctx context.Context, meta *Metadata, fldPath *field.Path, platform *awstypes.Platform, networking *types.Networking, publish types.PublishingStrategy) field.ErrorList {
allErrs := field.ErrorList{}

if !isAWSSDKRegion(platform.Region) && platform.AMIID == "" {
allErrs = append(allErrs, field.Required(fldPath.Child("amiID"), "AMI must be provided"))
}

if len(platform.Subnets) > 0 {
allErrs = append(allErrs, validateSubnets(ctx, meta, fldPath.Child("subnets"), platform.Subnets, networking, publish)...)
}
Expand All @@ -56,6 +54,53 @@ func validatePlatform(ctx context.Context, meta *Metadata, fldPath *field.Path,
return allErrs
}

func validateAMI(ctx context.Context, config *types.InstallConfig) field.ErrorList {
// accept AMI from the rhcos stream metadata
if sets.NewString(rhcos.AMIRegions...).Has(config.Platform.AWS.Region) {
return nil
}

// accept AMI specified at the platform level
if config.Platform.AWS.AMIID != "" {
return nil
}

// accept AMI specified for the default machine platform
if config.Platform.AWS.DefaultMachinePlatform != nil {
if config.Platform.AWS.DefaultMachinePlatform.AMIID != "" {
return nil
}
}

// accept AMIs specified specifically for each machine pool
controlPlaneHasAMISpecified := false
if config.ControlPlane != nil && config.ControlPlane.Platform.AWS != nil {
controlPlaneHasAMISpecified = config.ControlPlane.Platform.AWS.AMIID != ""
}
computesHaveAMISpecified := true
for _, c := range config.Compute {
if c.Replicas != nil && *c.Replicas == 0 {
continue
}
if c.Platform.AWS == nil || c.Platform.AWS.AMIID == "" {
computesHaveAMISpecified = false
}
}
if controlPlaneHasAMISpecified && computesHaveAMISpecified {
return nil
}

// accept AMI that can be copied from us-east-1 if the region is in the standard AWS partition
if partition, partitionFound := endpoints.PartitionForRegion(endpoints.DefaultPartitions(), config.Platform.AWS.Region); partitionFound {
if partition.ID() == endpoints.AwsPartitionID {
return nil
}
}

// fail validation since we do not have an AMI to use
return field.ErrorList{field.Required(field.NewPath("platform", "aws", "amiID"), "AMI must be provided")}
}

func validateSubnets(ctx context.Context, meta *Metadata, fldPath *field.Path, subnets []string, networking *types.Networking, publish types.PublishingStrategy) field.ErrorList {
allErrs := field.ErrorList{}
privateSubnets, err := meta.PrivateSubnets(ctx)
Expand Down Expand Up @@ -177,7 +222,7 @@ func validateDuplicateSubnetZones(fldPath *field.Path, subnets map[string]Subnet
}

func validateServiceEndpoints(fldPath *field.Path, region string, services []awstypes.ServiceEndpoint) error {
if isAWSSDKRegion(region) {
if _, partitionFound := endpoints.PartitionForRegion(endpoints.DefaultPartitions(), region); partitionFound {
return nil
}

Expand All @@ -192,17 +237,6 @@ func validateServiceEndpoints(fldPath *field.Path, region string, services []aws
return utilerrors.NewAggregate(errs)
}

func isAWSSDKRegion(region string) bool {
for _, partition := range endpoints.DefaultPartitions() {
for _, partitionRegion := range partition.Regions() {
if region == partitionRegion.ID() {
return true
}
}
}
return false
}

var requiredServices = []string{
"ec2",
"elasticloadbalancing",
Expand Down
143 changes: 126 additions & 17 deletions pkg/asset/installconfig/aws/validation_test.go
View file
Expand Up @@ -5,6 +5,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"k8s.io/utils/pointer"

"github.com/openshift/installer/pkg/ipnet"
"github.com/openshift/installer/pkg/types"
Expand Down Expand Up @@ -37,13 +38,15 @@ func validInstallConfig() *types.InstallConfig {
},
},
ControlPlane: &types.MachinePool{
Replicas: pointer.Int64Ptr(3),
Platform: types.MachinePoolPlatform{
AWS: &aws.MachinePool{
Zones: []string{"a", "b", "c"},
},
},
},
Compute: []types.MachinePool{{
Replicas: pointer.Int64Ptr(3),
Platform: types.MachinePoolPlatform{
AWS: &aws.MachinePool{
Zones: []string{"a", "b", "c"},
Expand Down Expand Up @@ -123,7 +126,7 @@ func TestValidate(t *testing.T) {
availZones []string
privateSubnets map[string]Subnet
publicSubnets map[string]Subnet
exptectErr string
expectErr string
}{{
name: "valid no byo",
installConfig: func() *types.InstallConfig {
Expand Down Expand Up @@ -181,7 +184,7 @@ func TestValidate(t *testing.T) {
}(),
availZones: validAvailZones(),
publicSubnets: validPublicSubnets(),
exptectErr: `^\[platform\.aws\.subnets: Invalid value: \[\]string{\"valid-public-subnet-a\", \"valid-public-subnet-b\", \"valid-public-subnet-c\"}: No private subnets found, controlPlane\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\"}: No subnets provided for zones \[a b c\], compute\[0\]\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\"}: No subnets provided for zones \[a b c\]\]$`,
expectErr: `^\[platform\.aws\.subnets: Invalid value: \[\]string{\"valid-public-subnet-a\", \"valid-public-subnet-b\", \"valid-public-subnet-c\"}: No private subnets found, controlPlane\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\"}: No subnets provided for zones \[a b c\], compute\[0\]\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\"}: No subnets provided for zones \[a b c\]\]$`,
}, {
name: "invalid no public subnets",
installConfig: func() *types.InstallConfig {
Expand All @@ -195,7 +198,7 @@ func TestValidate(t *testing.T) {
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
exptectErr: `^platform\.aws\.subnets: Invalid value: \[\]string{\"valid-private-subnet-a\", \"valid-private-subnet-b\", \"valid-private-subnet-c\"}: No public subnet provided for zones \[a b c\]$`,
expectErr: `^platform\.aws\.subnets: Invalid value: \[\]string{\"valid-private-subnet-a\", \"valid-private-subnet-b\", \"valid-private-subnet-c\"}: No public subnet provided for zones \[a b c\]$`,
}, {
name: "invalid cidr does not belong to machine CIDR",
installConfig: func() *types.InstallConfig {
Expand All @@ -212,7 +215,7 @@ func TestValidate(t *testing.T) {
}
return s
}(),
exptectErr: `^platform\.aws\.subnets\[6\]: Invalid value: \"invalid-cidr-subnet\": subnet's CIDR range start 192.168.126.0 is outside of the specified machine networks$`,
expectErr: `^platform\.aws\.subnets\[6\]: Invalid value: \"invalid-cidr-subnet\": subnet's CIDR range start 192.168.126.0 is outside of the specified machine networks$`,
}, {
name: "invalid cidr does not belong to machine CIDR",
installConfig: func() *types.InstallConfig {
Expand All @@ -235,7 +238,7 @@ func TestValidate(t *testing.T) {
}
return s
}(),
exptectErr: `^\[platform\.aws\.subnets\[6\]: Invalid value: \"invalid-private-cidr-subnet\": subnet's CIDR range start 192.168.126.0 is outside of the specified machine networks, platform\.aws\.subnets\[7\]: Invalid value: \"invalid-public-cidr-subnet\": subnet's CIDR range start 192.168.127.0 is outside of the specified machine networks\]$`,
expectErr: `^\[platform\.aws\.subnets\[6\]: Invalid value: \"invalid-private-cidr-subnet\": subnet's CIDR range start 192.168.126.0 is outside of the specified machine networks, platform\.aws\.subnets\[7\]: Invalid value: \"invalid-public-cidr-subnet\": subnet's CIDR range start 192.168.127.0 is outside of the specified machine networks\]$`,
}, {
name: "invalid missing public subnet in a zone",
installConfig: func() *types.InstallConfig {
Expand All @@ -253,7 +256,7 @@ func TestValidate(t *testing.T) {
return s
}(),
publicSubnets: validPublicSubnets(),
exptectErr: `^platform\.aws\.subnets: Invalid value: \[\]string{\"valid-private-subnet-a\", \"valid-private-subnet-b\", \"valid-private-subnet-c\", \"valid-public-subnet-a\", \"valid-public-subnet-b\", \"valid-public-subnet-c\", \"no-matching-public-private-zone\"}: No public subnet provided for zones \[f\]$`,
expectErr: `^platform\.aws\.subnets: Invalid value: \[\]string{\"valid-private-subnet-a\", \"valid-private-subnet-b\", \"valid-private-subnet-c\", \"valid-public-subnet-a\", \"valid-public-subnet-b\", \"valid-public-subnet-c\", \"no-matching-public-private-zone\"}: No public subnet provided for zones \[f\]$`,
}, {
name: "invalid multiple private in same zone",
installConfig: func() *types.InstallConfig {
Expand All @@ -271,7 +274,7 @@ func TestValidate(t *testing.T) {
return s
}(),
publicSubnets: validPublicSubnets(),
exptectErr: `^platform\.aws\.subnets\[6\]: Invalid value: \"valid-private-zone-c-2\": private subnet valid-private-subnet-c is also in zone c$`,
expectErr: `^platform\.aws\.subnets\[6\]: Invalid value: \"valid-private-zone-c-2\": private subnet valid-private-subnet-c is also in zone c$`,
}, {
name: "invalid multiple public in same zone",
installConfig: func() *types.InstallConfig {
Expand All @@ -289,7 +292,7 @@ func TestValidate(t *testing.T) {
}
return s
}(),
exptectErr: `^platform\.aws\.subnets\[6\]: Invalid value: \"valid-public-zone-c-2\": public subnet valid-public-subnet-c is also in zone c$`,
expectErr: `^platform\.aws\.subnets\[6\]: Invalid value: \"valid-public-zone-c-2\": public subnet valid-public-subnet-c is also in zone c$`,
}, {
name: "invalid no subnet for control plane zones",
installConfig: func() *types.InstallConfig {
Expand All @@ -300,7 +303,7 @@ func TestValidate(t *testing.T) {
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
exptectErr: `^controlPlane\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\", \"d\"}: No subnets provided for zones \[d\]$`,
expectErr: `^controlPlane\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\", \"d\"}: No subnets provided for zones \[d\]$`,
}, {
name: "invalid no subnet for control plane zones",
installConfig: func() *types.InstallConfig {
Expand All @@ -311,7 +314,7 @@ func TestValidate(t *testing.T) {
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
exptectErr: `^controlPlane\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\", \"d\", \"e\"}: No subnets provided for zones \[d e\]$`,
expectErr: `^controlPlane\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\", \"d\", \"e\"}: No subnets provided for zones \[d e\]$`,
}, {
name: "invalid no subnet for compute[0] zones",
installConfig: func() *types.InstallConfig {
Expand All @@ -322,7 +325,7 @@ func TestValidate(t *testing.T) {
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
exptectErr: `^compute\[0\]\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\", \"d\"}: No subnets provided for zones \[d\]$`,
expectErr: `^compute\[0\]\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\", \"d\"}: No subnets provided for zones \[d\]$`,
}, {
name: "invalid no subnet for compute zone",
installConfig: func() *types.InstallConfig {
Expand All @@ -340,7 +343,7 @@ func TestValidate(t *testing.T) {
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
exptectErr: `^\[compute\[0\]\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\", \"d\"}: No subnets provided for zones \[d\], compute\[1\]\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"e\"}: No subnets provided for zones \[e\]\]$`,
expectErr: `^\[compute\[0\]\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\", \"d\"}: No subnets provided for zones \[d\], compute\[1\]\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"e\"}: No subnets provided for zones \[e\]\]$`,
}, {
name: "custom region invalid service endpoints none provided",
installConfig: func() *types.InstallConfig {
Expand All @@ -352,7 +355,7 @@ func TestValidate(t *testing.T) {
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
exptectErr: `^platform\.aws\.serviceEndpoints: Invalid value: (.|\n)*: \[failed to find endpoint for service "ec2": (.|\n)*, failed to find endpoint for service "elasticloadbalancing": (.|\n)*, failed to find endpoint for service "iam": (.|\n)*, failed to find endpoint for service "route53": (.|\n)*, failed to find endpoint for service "s3": (.|\n)*, failed to find endpoint for service "sts": (.|\n)*, failed to find endpoint for service "tagging": (.|\n)*\]$`,
expectErr: `^platform\.aws\.serviceEndpoints: Invalid value: (.|\n)*: \[failed to find endpoint for service "ec2": (.|\n)*, failed to find endpoint for service "elasticloadbalancing": (.|\n)*, failed to find endpoint for service "iam": (.|\n)*, failed to find endpoint for service "route53": (.|\n)*, failed to find endpoint for service "s3": (.|\n)*, failed to find endpoint for service "sts": (.|\n)*, failed to find endpoint for service "tagging": (.|\n)*\]$`,
}, {
name: "custom region invalid service endpoints some provided",
installConfig: func() *types.InstallConfig {
Expand All @@ -365,7 +368,7 @@ func TestValidate(t *testing.T) {
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
exptectErr: `^platform\.aws\.serviceEndpoints: Invalid value: (.|\n)*: \[failed to find endpoint for service "elasticloadbalancing": (.|\n)*, failed to find endpoint for service "route53": (.|\n)*, failed to find endpoint for service "sts": (.|\n)*, failed to find endpoint for service "tagging": (.|\n)*$`,
expectErr: `^platform\.aws\.serviceEndpoints: Invalid value: (.|\n)*: \[failed to find endpoint for service "elasticloadbalancing": (.|\n)*, failed to find endpoint for service "route53": (.|\n)*, failed to find endpoint for service "sts": (.|\n)*, failed to find endpoint for service "tagging": (.|\n)*$`,
}, {
name: "custom region valid service endpoints",
installConfig: func() *types.InstallConfig {
Expand All @@ -378,6 +381,110 @@ func TestValidate(t *testing.T) {
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
}, {
name: "AMI omitted for new region in standard partition",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-newregion-1"
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
}, {
name: "accept platform-level AMI",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-gov-east-1"
c.Platform.AWS.AMIID = "custom-ami"
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
}, {
name: "accept AMI from default machine platform",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-gov-east-1"
c.Platform.AWS.DefaultMachinePlatform = &aws.MachinePool{AMIID: "custom-ami"}
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
}, {
name: "accept AMIs specified for each machine pool",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-gov-east-1"
c.ControlPlane.Platform.AWS.AMIID = "custom-ami"
c.Compute[0].Platform.AWS.AMIID = "custom-ami"
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
}, {
name: "AMI not provided for control plane",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-gov-east-1"
c.Compute[0].Platform.AWS.AMIID = "custom-ami"
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
expectErr: `^platform\.aws\.amiID: Required value: AMI must be provided$`,
}, {
name: "AMI not provided for compute",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-gov-east-1"
c.ControlPlane.Platform.AWS.AMIID = "custom-ami"
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
expectErr: `^platform\.aws\.amiID: Required value: AMI must be provided$`,
}, {
name: "machine platform not provided for compute",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-gov-east-1"
c.ControlPlane.Platform.AWS.AMIID = "custom-ami"
c.Compute[0].Platform.AWS = nil
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
expectErr: `^platform\.aws\.amiID: Required value: AMI must be provided$`,
}, {
name: "AMI omitted for compute with no replicas",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-gov-east-1"
c.ControlPlane.Platform.AWS.AMIID = "custom-ami"
c.Compute[0].Replicas = pointer.Int64Ptr(0)
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
}, {
name: "AMI not provided for US gov region",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-gov-east-1"
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
expectErr: `^platform\.aws\.amiID: Required value: AMI must be provided$`,
}, {
name: "AMI not provided for unknown region",
installConfig: func() *types.InstallConfig {
Expand All @@ -389,7 +496,7 @@ func TestValidate(t *testing.T) {
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
exptectErr: `^platform\.aws\.amiID: Required value: AMI must be provided$`,
expectErr: `^platform\.aws\.amiID: Required value: AMI must be provided$`,
}}

for _, test := range tests {
Expand All @@ -400,10 +507,12 @@ func TestValidate(t *testing.T) {
publicSubnets: test.publicSubnets,
}
err := Validate(context.TODO(), meta, test.installConfig)
if test.exptectErr == "" {
if test.expectErr == "" {
assert.NoError(t, err)
} else {
assert.Regexp(t, test.exptectErr, err.Error())
if assert.Error(t, err) {
assert.Regexp(t, test.expectErr, err.Error())
}
}
})
}
Expand Down

0 comments on commit 939aef4

Please sign in to comment.