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

chore: private services env compatibility checker, only enable if enabled 🤦 #4150

Closed
wants to merge 12 commits into from
2 changes: 1 addition & 1 deletion internal/pkg/deploy/cloudformation/stack/rd_web_svc.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ func (s *RequestDrivenWebService) Template() (string, error) {
Tracing: strings.ToUpper(aws.StringValue(s.manifest.Observability.Tracing)),
},
PermissionsBoundary: s.permBound,
Private: aws.BoolValue(s.manifest.Private.Basic) || s.manifest.Private.Advanced.Endpoint != nil,
Private: s.manifest.PrivateEnabled(),
AppRunnerVPCEndpoint: s.manifest.Private.Advanced.Endpoint,
Count: s.manifest.Count,
})
Expand Down
136 changes: 136 additions & 0 deletions internal/pkg/manifest/applyenv_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -790,3 +790,139 @@ func TestApplyEnv_MapToPStruct(t *testing.T) {
})
}
}

func TestDynamicWorkload_ApplyEnv(t *testing.T) {
tests := map[string]struct {
manifest string
expected func() any
}{
"update rdws http.private true -> false": {
manifest: `
type: Request-Driven Web Service

http:
private: true

environments:
test:
http:
private: false
`,
expected: func() any {
c := newDefaultRequestDrivenWebService().RequestDrivenWebServiceConfig
c.Private = BasicToUnion[*bool, VPCEndpoint](aws.Bool(false))
return c
},
},
"update rdws http.private false -> true": {
manifest: `
type: Request-Driven Web Service

http:
private: false

environments:
test:
http:
private: true
`,
expected: func() any {
c := newDefaultRequestDrivenWebService().RequestDrivenWebServiceConfig
c.Private = BasicToUnion[*bool, VPCEndpoint](aws.Bool(true))
return c
},
},
"update rdws http.private false -> VPCEndpoint": {
manifest: `
type: Request-Driven Web Service

http:
private: false

environments:
test:
http:
private:
endpoint: vpce-1234
`,
expected: func() any {
c := newDefaultRequestDrivenWebService().RequestDrivenWebServiceConfig
c.Private = AdvancedToUnion[*bool](VPCEndpoint{
Endpoint: aws.String("vpce-1234"),
})
return c
},
},
"update rdws http.private VPCEndpoint -> false": {
manifest: `
type: Request-Driven Web Service

http:
private:
endpoint: vpce-1234

environments:
test:
http:
private: false
`,
expected: func() any {
c := newDefaultRequestDrivenWebService().RequestDrivenWebServiceConfig
c.Private = BasicToUnion[*bool, VPCEndpoint](aws.Bool(false))
return c
},
},
"update rdws alias, no http.private update": {
manifest: `
type: Request-Driven Web Service

http:
private: false

environments:
test:
http:
alias: example.com
`,
expected: func() any {
c := newDefaultRequestDrivenWebService().RequestDrivenWebServiceConfig
c.Private = BasicToUnion[*bool, VPCEndpoint](aws.Bool(false))
c.Alias = aws.String("example.com")
return c
},
},
"update rdws cpu, no http.private update": {
manifest: `
type: Request-Driven Web Service

http:
private: true

environments:
test:
cpu: 2048
`,
expected: func() any {
c := newDefaultRequestDrivenWebService().RequestDrivenWebServiceConfig
c.Private = BasicToUnion[*bool, VPCEndpoint](aws.Bool(true))
c.InstanceConfig.CPU = aws.Int(2048)
return c
},
},
}

for name, tc := range tests {
t.Run(name, func(t *testing.T) {
mft, err := UnmarshalWorkload([]byte(tc.manifest))
require.NoError(t, err)

mft, err = mft.ApplyEnv("test")
require.NoError(t, err)

switch v := mft.Manifest().(type) {
case *RequestDrivenWebService:
require.Equal(t, tc.expected(), v.RequestDrivenWebServiceConfig)
}
})
}
}
8 changes: 8 additions & 0 deletions internal/pkg/manifest/rd_web_svc.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ type RequestDrivenWebServiceHttpConfig struct {
Private Union[*bool, VPCEndpoint] `yaml:"private"`
}

// PrivateEnabled returns true if r is configured for private ingress.
func (r *RequestDrivenWebServiceHttpConfig) PrivateEnabled() bool {
return aws.BoolValue(r.Private.Basic) || r.Private.Advanced.Endpoint != nil
}

// VPCEndpoint is used to configure a pre-existing VPC endpoint.
type VPCEndpoint struct {
Endpoint *string `yaml:"endpoint"`
Expand Down Expand Up @@ -182,6 +187,9 @@ func (s RequestDrivenWebService) applyEnv(envName string) (workloadManifest, err

func (s *RequestDrivenWebService) requiredEnvironmentFeatures() []string {
Copy link
Contributor

Choose a reason for hiding this comment

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

This has nothing to do with your PR but it's pretty strange to me that this method:

func (s *DynamicWorkloadManifest) RequiredEnvironmentFeatures() []string {

belongs to *DynamicWorkloadManifest 🤔 it doesn't seem relevant to a "dynamic" aspect.
I'd have expected it to be part of the individual Manifest types.

var features []string
if s.PrivateEnabled() {
dannyrandall marked this conversation as resolved.
Show resolved Hide resolved
features = append(features, template.AppRunnerPrivateServiceFeatureName)
}
features = append(features, s.Network.requiredEnvFeatures()...)
return features
}
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/manifest/svc.go
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ type HealthCheckArgsOrString struct {
}

// Path returns the default health check path if provided otherwise, returns the path from the advanced configuration.
func (hc *HealthCheckArgsOrString) Path() *string {
func (hc HealthCheckArgsOrString) Path() *string {
if hc.IsBasic() {
return aws.String(hc.Basic)
}
Expand Down
55 changes: 52 additions & 3 deletions internal/pkg/manifest/transform_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -727,16 +727,65 @@ func TestTransformer_StringOrHealthCheckArgs(t *testing.T) {
})
}

func TestTransformer_UnionWithZeroValues(t *testing.T) {
runUnionTransformerTests(t, map[string]unionTransformerTest[bool, struct{}]{
"bool true -> false, doesn't work": {
original: BasicToUnion[bool, struct{}](true),
override: BasicToUnion[bool, struct{}](false),
expected: BasicToUnion[bool, struct{}](true),
},
"bool false -> true, works": {
original: BasicToUnion[bool, struct{}](false),
override: BasicToUnion[bool, struct{}](true),
expected: BasicToUnion[bool, struct{}](true),
},
"bool false with no override": {
original: BasicToUnion[bool, struct{}](false),
expected: BasicToUnion[bool, struct{}](false),
},
"bool true with no override": {
original: BasicToUnion[bool, struct{}](true),
expected: BasicToUnion[bool, struct{}](true),
},
})

runUnionTransformerTests(t, map[string]unionTransformerTest[*bool, VPCEndpoint]{
"*bool true -> false, works": {
original: BasicToUnion[*bool, VPCEndpoint](aws.Bool(true)),
override: BasicToUnion[*bool, VPCEndpoint](aws.Bool(false)),
expected: BasicToUnion[*bool, VPCEndpoint](aws.Bool(false)),
},
"*bool false -> true, works": {
original: BasicToUnion[*bool, VPCEndpoint](aws.Bool(false)),
override: BasicToUnion[*bool, VPCEndpoint](aws.Bool(true)),
expected: BasicToUnion[*bool, VPCEndpoint](aws.Bool(true)),
},
"*bool false with no override": {
original: BasicToUnion[*bool, VPCEndpoint](aws.Bool(false)),
expected: BasicToUnion[*bool, VPCEndpoint](aws.Bool(false)),
},
"*bool true with no override": {
original: BasicToUnion[*bool, VPCEndpoint](aws.Bool(true)),
expected: BasicToUnion[*bool, VPCEndpoint](aws.Bool(true)),
},
})
}

func runUnionTransformerTests[Basic, Advanced any](t *testing.T, tests map[string]unionTransformerTest[Basic, Advanced]) {
transformers := []mergo.Transformers{
basicTransformer{},
unionTransformer{},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
// Perform default merge.
err := mergo.Merge(&tc.original, tc.override, mergo.WithOverride)
require.NoError(t, err)

// Use custom transformer.
err = mergo.Merge(&tc.original, tc.override, mergo.WithOverride, mergo.WithTransformers(unionTransformer{}))
require.NoError(t, err)
for _, tx := range transformers {
err = mergo.Merge(&tc.original, tc.override, mergo.WithOverride, mergo.WithTransformers(tx))
require.NoError(t, err)
}

require.NoError(t, err)
require.Equal(t, tc.expected, tc.original)
Expand Down
8 changes: 7 additions & 1 deletion internal/pkg/template/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ const (
)

// Available env-controller managed feature names.
//
// If you add a new feature to this list, make sure you update the
// relevant services' requiredEnvironmentFeatures() function to
// determine if the service requires the feature.
const (
ALBFeatureName = "ALBWorkloads"
EFSFeatureName = "EFSWorkloads"
Expand All @@ -38,13 +42,15 @@ var friendlyEnvFeatureName = map[string]string{
AppRunnerPrivateServiceFeatureName: "App Runner Private Services",
}

// leastVersionForFeature maps a feature to the minimum
// necessary _environment template version_ for that feature.
var leastVersionForFeature = map[string]string{
ALBFeatureName: "v1.0.0",
EFSFeatureName: "v1.3.0",
NATFeatureName: "v1.3.0",
InternalALBFeatureName: "v1.10.0",
AliasesFeatureName: "v1.4.0",
AppRunnerPrivateServiceFeatureName: "v1.23.0",
AppRunnerPrivateServiceFeatureName: "v1.13.0",
dannyrandall marked this conversation as resolved.
Show resolved Hide resolved
}

// AvailableEnvFeatures returns a list of the latest available feature, named after their corresponding parameter names.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ http:
# To match all requests you can use the "/" path.
path: '{{.RoutingRule.Path}}'
# You can specify a custom health check path. The default is "/".
# healthcheck: '{{.RoutingRule.HealthCheck.Basic}}'
# healthcheck: '{{.RoutingRule.HealthCheck.Path}}'

# Configuration for your containers and service.
image:
Expand Down