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.

Sign up for GitHub

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

Jump to bottom

feat(manifest): add HTTP Health Check hidden fields #1592

Merged
merged 14 commits into from Nov 3, 2020
Merged

feat(manifest): add HTTP Health Check hidden fields #1592

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b09f875
wip
huanjani Oct 28, 2020
3647310
chore: get template building properly; start tests
huanjani Oct 29, 2020
f72c8ba
chore: finish unit tests
huanjani Oct 29, 2020
2af5287
Merge remote-tracking branch 'archer/mainline' into healthcheck
huanjani Oct 29, 2020
9017edf
fix: uc HTTP in template yaml
huanjani Oct 30, 2020
945c67c
fix: reorder fields
huanjani Oct 30, 2020
dba23a2
fix: refactor to enable ArgsOrString, change ints to duration, etc.
huanjani Nov 2, 2020
fa2bfeb
fix: remove unused method
huanjani Nov 2, 2020
3876af8
fix: remove 's' for seconds from template
huanjani Nov 2, 2020
ebcdb9c
Merge branch 'mainline' into healthcheck
huanjani Nov 2, 2020
6f0d31c
fix: default path (/) in manifest so not nil
huanjani Nov 2, 2020
ccb5a5a
chore: allow camel case and snake case for target container yaml field
huanjani Nov 3, 2020
89969dd
Merge branch 'mainline' into healthcheck
huanjani Nov 3, 2020
da4df77
Merge branch 'mainline' into healthcheck
huanjani Nov 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 1 addition & 4 deletions internal/pkg/deploy/cloudformation/stack/lb_web_svc.go
View file
Open in desktop
Expand Up @@ -115,6 +115,7 @@ func (s *LoadBalancedWebService) Template() (string, error) {
Sidecars: sidecars,
LogConfig: s.manifest.LogConfigOpts(),
Autoscaling: autoscaling,
HTTPHealthCheck: s.manifest.HTTPHealthCheckOpts(),
RulePriorityLambda: rulePriorityLambda.String(),
DesiredCountLambda: desiredCountLambda.String(),
})
Expand Down Expand Up @@ -162,10 +163,6 @@ func (s *LoadBalancedWebService) Parameters() ([]*cloudformation.Parameter, erro
ParameterKey: aws.String(LBWebServiceRulePathParamKey),
ParameterValue: s.manifest.Path,
},
{
ParameterKey: aws.String(LBWebServiceHealthCheckPathParamKey),
ParameterValue: s.manifest.HealthCheckPath,
},
{
ParameterKey: aws.String(LBWebServiceHTTPSParamKey),
ParameterValue: aws.String(strconv.FormatBool(s.httpsEnabled)),
Expand Down
23 changes: 16 additions & 7 deletions internal/pkg/deploy/cloudformation/stack/lb_web_svc_test.go
View file
Open in desktop
Expand Up @@ -95,9 +95,8 @@ func TestLoadBalancedWebService_StackName(t *testing.T) {
func TestLoadBalancedWebService_Template(t *testing.T) {
testCases := map[string]struct {
mockDependencies func(t *testing.T, ctrl *gomock.Controller, c *LoadBalancedWebService)

wantedTemplate string
wantedError error
wantedTemplate string
wantedError error
}{
"unavailable rule priority lambda template": {
mockDependencies: func(t *testing.T, ctrl *gomock.Controller, c *LoadBalancedWebService) {
Expand Down Expand Up @@ -154,6 +153,13 @@ func TestLoadBalancedWebService_Template(t *testing.T) {
m.EXPECT().Read(lbWebSvcRulePriorityGeneratorPath).Return(&template.Content{Buffer: bytes.NewBufferString("lambda")}, nil)
m.EXPECT().Read(desiredCountGeneratorPath).Return(&template.Content{Buffer: bytes.NewBufferString("something")}, nil)
m.EXPECT().ParseLoadBalancedWebService(template.WorkloadOpts{
HTTPHealthCheck: &template.HTTPHealthCheckOpts{
HealthCheckPath: aws.String("/"),
HealthyThreshold: aws.Int64(2),
UnhealthyThreshold: aws.Int64(2),
Interval: aws.Int64(10),
Timeout: aws.Int64(5),
},
RulePriorityLambda: "lambda",
DesiredCountLambda: "something",
}).Return(&template.Content{Buffer: bytes.NewBufferString("template")}, nil)
Expand All @@ -177,6 +183,13 @@ func TestLoadBalancedWebService_Template(t *testing.T) {
SecretOutputs: []string{"MySecretArn"},
PolicyOutputs: []string{"AdditionalResourcesPolicyArn"},
},
HTTPHealthCheck: &template.HTTPHealthCheckOpts{
HealthCheckPath: aws.String("/"),
HealthyThreshold: aws.Int64(2),
UnhealthyThreshold: aws.Int64(2),
Interval: aws.Int64(10),
Timeout: aws.Int64(5),
},
RulePriorityLambda: "lambda",
DesiredCountLambda: "something",
}).Return(&template.Content{Buffer: bytes.NewBufferString("template")}, nil)
Expand Down Expand Up @@ -330,10 +343,6 @@ func TestLoadBalancedWebService_Parameters(t *testing.T) {
ParameterKey: aws.String(LBWebServiceRulePathParamKey),
ParameterValue: aws.String("frontend"),
},
{
ParameterKey: aws.String(LBWebServiceHealthCheckPathParamKey),
ParameterValue: aws.String("/"),
},
{
ParameterKey: aws.String(WorkloadTaskCPUParamKey),
ParameterValue: aws.String("256"),
Expand Down
105 changes: 99 additions & 6 deletions internal/pkg/manifest/lb_web_svc.go
View file
Open in desktop
Expand Up @@ -4,18 +4,33 @@
package manifest

import (
"errors"
"path/filepath"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/copilot-cli/internal/pkg/template"
"github.com/imdario/mergo"
"gopkg.in/yaml.v3"
)

const (
lbWebSvcManifestPath = "workloads/services/lb-web/manifest.yml"
)

// Default values for HTTPHealthCheck for a load balanced web service.
const (
// LogRetentionInDays is the default log retention time in days.
LogRetentionInDays = 30
LogRetentionInDays = 30
defaultHealthCheckPath = "/"
defaultHealthyThreshold = int64(2)
defaultUnhealthyThreshold = int64(2)
defaultInterval = int64(10)
defaultTimeout = int64(5)
huanjani marked this conversation as resolved.
Show resolved Hide resolved
)

var (
errUnmarshalHealthCheckArgs = errors.New("can't unmarshal healthcheck field into string or compose-style map")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's remove words like cannot from the error message unless it's in the cli pkg (the root pkg)

When adding context to returned errors, keep the context succinct by avoiding phrases like "failed to", which state the obvious and pile up as the error percolates up through the stack:

https://github.com/uber-go/guide/blob/master/style.md#error-wrapping

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is fine to keep can't in this scenario since it is already the root error instead of wrapping the other error. Agree if we are wrapping error we want to avoid piling up like failed to get health check: can't unmarshal health check. And instead we want get health check: can't unmarshal health check. What do you all think?

Copy link
Contributor

Choose a reason for hiding this comment

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

More the opposite, cannot get health check: unmarshal health check. Where the root error that wraps others can use "cannot" to make it more human friendly but "library" pkgs shouldn't put cannot in their error messages

Copy link
Contributor

Choose a reason for hiding this comment

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

But according to https://github.com/uber-go/guide/blob/master/style.md#error-wrapping, isn't cannot get health check: unmarshal health check a bad example?

)

// LoadBalancedWebService holds the configuration to build a container image with an exposed port that receives
Expand Down Expand Up @@ -46,13 +61,60 @@ func (lc *LoadBalancedWebServiceConfig) LogConfigOpts() *template.LogConfigOpts
return lc.logConfigOpts()
}

func (lc *LoadBalancedWebServiceConfig) HTTPHealthCheckOpts() *template.HTTPHealthCheckOpts {
opts := template.HTTPHealthCheckOpts{
HealthCheckPath: aws.String(defaultHealthCheckPath),
HealthyThreshold: aws.Int64(defaultHealthyThreshold),
UnhealthyThreshold: aws.Int64(defaultUnhealthyThreshold),
Interval: aws.Int64(defaultInterval),
Timeout: aws.Int64(defaultTimeout),
}
if lc.RoutingRule.HealthCheck.HealthCheckArgs.Path != nil {
opts.HealthCheckPath = lc.RoutingRule.HealthCheck.HealthCheckArgs.Path
}
if lc.RoutingRule.HealthCheck.HealthCheckPath != nil {
opts.HealthCheckPath = lc.HealthCheck.HealthCheckPath
}
if lc.RoutingRule.HealthCheck.HealthCheckArgs.HealthyThreshold != nil {
opts.HealthyThreshold = lc.RoutingRule.HealthCheck.HealthCheckArgs.HealthyThreshold
}
if lc.RoutingRule.HealthCheck.HealthCheckArgs.UnhealthyThreshold != nil {
opts.UnhealthyThreshold = lc.RoutingRule.HealthCheck.HealthCheckArgs.UnhealthyThreshold
}
if lc.RoutingRule.HealthCheck.HealthCheckArgs.Interval != nil {
opts.Interval = aws.Int64(int64(lc.RoutingRule.HealthCheck.HealthCheckArgs.Interval.Seconds()))
}
if lc.RoutingRule.HealthCheck.HealthCheckArgs.Timeout != nil {
opts.Timeout = aws.Int64(int64(lc.RoutingRule.HealthCheck.HealthCheckArgs.Timeout.Seconds()))
}
return &opts
}

// HTTPHealthCheckArgs holds the configuration to determine if the load balanced web service is healthy.
// These options are specifiable under the "healthcheck" field.
// See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-elasticloadbalancingv2-targetgroup.html.
type HTTPHealthCheckArgs struct {
Path *string `yaml:"path"`
HealthyThreshold *int64 `yaml:"healthy_threshold"`
UnhealthyThreshold *int64 `yaml:"unhealthy_threshold"`
Timeout *time.Duration `yaml:"timeout"`
Interval *time.Duration `yaml:"interval"`
}

// HealthCheckArgsOrString is a custom type which supports unmarshaling yaml which
// can either be of type string or type HealthCheckArgs. q
type HealthCheckArgsOrString struct {
HealthCheckPath *string
HealthCheckArgs HTTPHealthCheckArgs
}

// RoutingRule holds the path to route requests to the service.
type RoutingRule struct {
Path *string `yaml:"path"`
HealthCheckPath *string `yaml:"healthcheck"`
Stickiness *bool `yaml:"stickiness"`
Path *string `yaml:"path"`
HealthCheck HealthCheckArgsOrString `yaml:"healthcheck"`
Stickiness *bool `yaml:"stickiness"`
// TargetContainer is the container load balancer routes traffic to.
TargetContainer *string `yaml:"targetContainer"`
TargetContainer *string `yaml:"target_container"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I wish we could change this :'( but we would need to support both targetContainer and target_container to maintain backwards compatibility.

Right now this will break users that have targetContainer as a field. We'd need to figure out a way of assigning two yaml fields to TargetContainer.
Maybe this works?

Suggested change
TargetContainer *string `yaml:"target_container"`
TargetContainer *string `yaml:"target_container" yaml:"targetContainer"` // "targetContainer" for backwards compatibility

If it does great, otherwise can we punt this change to a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so that ☝️ didn't work and some digging/reading makes me think there isn't a built-in way to do this, but this little hack works:
In manifest/lb_web_svc.go, I added another field to the struct for camel case.
In stack/lb_web_svc.go:

if s.manifest.TargetContainer == nil && s.manifest.TargetContainerCamelCase != nil {
		s.manifest.TargetContainer = s.manifest.TargetContainerCamelCase
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

brilliant! that sounds good to me!

Copy link
Contributor

Choose a reason for hiding this comment

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

samesies thanks for diving deep on this!

}

// LoadBalancedWebServiceProps contains properties for creating a new load balanced fargate service manifest.
Expand Down Expand Up @@ -85,7 +147,7 @@ func newDefaultLoadBalancedWebService() *LoadBalancedWebService {
LoadBalancedWebServiceConfig: LoadBalancedWebServiceConfig{
ImageConfig: ServiceImageWithPort{},
RoutingRule: RoutingRule{
HealthCheckPath: aws.String("/"),
HealthCheck: HealthCheckArgsOrString{},
huanjani marked this conversation as resolved.
Show resolved Hide resolved
},
TaskConfig: TaskConfig{
CPU: aws.Int(256),
Expand All @@ -98,6 +160,37 @@ func newDefaultLoadBalancedWebService() *LoadBalancedWebService {
}
}

func (h *HTTPHealthCheckArgs) isEmpty() bool {
if h.Path == nil && h.HealthyThreshold == nil && h.UnhealthyThreshold == nil && h.Interval == nil && h.Timeout == nil {
return true
}
return false
huanjani marked this conversation as resolved.
Show resolved Hide resolved
}

// UnmarshalYAML overrides the default YAML unmarshaling logic for the HealthCheckArgsOrString
// struct, allowing it to perform more complex unmarshaling behavior.
// This method implements the yaml.Unmarshaler (v2) interface.
func (h *HealthCheckArgsOrString) UnmarshalYAML(unmarshal func(interface{}) error) error {
if err := unmarshal(&h.HealthCheckArgs); err != nil {
switch err.(type) {
case *yaml.TypeError:
break
default:
return err
}
}

if !h.HealthCheckArgs.isEmpty() {
// Unmarshaled successfully to h.HealthCheckArgs, return.
return nil
}

if err := unmarshal(&h.HealthCheckPath); err != nil {
return errUnmarshalHealthCheckArgs
}
return nil
}

// MarshalBinary serializes the manifest object into a binary YAML document.
// Implements the encoding.BinaryMarshaler interface.
func (s *LoadBalancedWebService) MarshalBinary() ([]byte, error) {
Expand Down