feat: configure environment security group from env manifest by paragbhingre · Pull Request #3810 · aws/copilot-cli

paragbhingre · 2022-07-25T16:44:32Z

This PR fix the issue #3719

Related to #3749

Now customers can define security group rules for EnvironmentSecurityGroup (e.g. ingress/egress) in their env manifest

Sample manifest template with security group rules is given below

network:
  vpc:
    security_group:
      ingress:
        - ip_protocol: tcp
           ports: 0-65535
           cidr: 0.0.0.0/0
      egress:
        - ip_protocol: tcp
           ports: 80
           cidr: 0.0.0.0/0

rclinard-amzn · 2022-07-25T16:49:22Z

Should this be classified as feat?

Lou1415926 · 2022-07-27T17:28:22Z

+}
+
+// IsEmpty returns true if there is no security group rule defined.
+func (cfg securityGroupConfig) IsEmpty() bool {


Does this method need to be exported?

Actually it was required before when we had not implemented the Demeter law suggested by Efe but now I think we don't need to export this I will change it to lower case.

Lou1415926 · 2022-07-27T17:55:12Z

+}
+
+type securityGroupRule struct {
+	CidrIP     string `yaml:"cidr"`


This is sort of related to my other comment in templates/environment/cf.yml. I keep debating whether in general we will need to standardize these basic fields to have a pointer, so that we can distinguish an input of zero value from an absent input in env manifest, that is for example, to tell

- to_port: 0 ip_protocol: "tcp"

from

- ip_protocol: "tcp"

If we use *int, then we would be able to; if we use int (like what you are doing now), then we won't be able to tell.

For service manifest, we need to be able to tell them apart so that environment overrides are applied correctly; but this is not a thing in environment manifest.

Up until now, it seems like for env manifest, it's not entirely necessary for us to distinguish zero-value vs. absent except for bool fields. But I don wonder though whether we should force every basic field to be a pointer field just to standardize it 💭

That is a good call out actually. In the env manifest we do not have a consistent way of implementing this particularly thing. Currently, I have changed ToPort and FromPort to be pointers so that we can figure if customer has actually set this parameters in the manifest or not? And if not then set them as 0 or 65535 whatever we decide in our other conversation here

efekarakus

Looks great! just some small feedback

efekarakus · 2022-07-27T18:16:53Z


+func convertEnvSecurityGroupCfg(mft *manifest.Environment) template.SecurityGroupConfig {
+	securityGroupConfig, isSecurityConfigSet := mft.EnvSecurityGroup()
+	if isSecurityConfigSet {


nit: we can flip the conditional to reduce nesting

Suggested change

if isSecurityConfigSet {

if !isSecurityConfigSet {

return template.SecurityGroupConfig{}

}

ingress := make(// remaining code...)

efekarakus · 2022-07-27T18:31:42Z

+	Observability       environmentObservability `yaml:"observability,omitempty,flow"`
+	HTTPConfig          EnvironmentHTTPConfig    `yaml:"http,omitempty,flow"`
+	CDNConfig           environmentCDNConfig     `yaml:"cdn,omitempty,flow"`
+	SecurityGroupConfig securityGroupConfig      `yaml:"security_group,omitempty"`


Sorry Parag this is my bad! would you mind moving back this field to under what you had originally network.vpc.?

The main reasoning behind it is that:

Consistency between the existing network.vpc.security_groups field as this sg affects the connectivity of all services in the VPC.

We will be adding also http.public.security_group so there is a nice symmetry by being two levels deep

I had a conversation with @Lou1415926 about this and we concluded that security group of the environment is not a part of VPC but it's a part of environment itself so it shouldn't go under VPC config and be on it's own in the env manifest, hence I made changes accordingly.
What are your thoughts on this?

As per internal discussion with the team we are concluding this as -

security_group should be inside network.vpc as EnvironmentSecurityGroup will get applied to all the services in the environment in the particular vpc. And it also aligns well with the http.public.security_groups that we have in the env manifest. So network.vpc's security_group is for all the services in the environment and http.public's security_groups is applied to the load balancer.

…de network.vpc

efekarakus

Yay! the port range changes look awesome!

efekarakus · 2022-08-01T16:33:02Z

+func getPortValues(cfg manifest.SecurityGroupRule) (fromPort int, toPort int, err error) {
+	if cfg.PortsConfig.Ports.IsEmpty() {
+		toPort = *cfg.PortsConfig.Port
+		fromPort = *cfg.PortsConfig.Port
+	} else {
+		fromPort, toPort, err = cfg.PortsConfig.Ports.Parse()
+		if err != nil {
+			return 0, 0, err
+		}
+	}
+	return fromPort, toPort, nil
+}


What do you think of moving this to be a method on *manifest.SecurityGroupRule instead? This way IsEmpty and Parse can become private methods.

// Ports returns the from and to ports of a security group rule. func (r manifest.SecurityGroupRule) Ports() (from, to int, err error) { if r.PortsConfig.Ports.isEmpty() { return *cfg.PortsConfig.Port, *cfg.PortsConfig.Port, nil // a single value is provided for ports. } return cfg.PortsConfig.Ports.parse() }

That's such a great idea. I like this one. I will make the change. :)
But I can only make Parse method private as IsEmpty is used in the validate_env.

Lou1415926 · 2022-08-01T22:29:38Z

+
+// PortsRangeBand represents a range of from and to port values.
+// For example, "0-65536".
+type PortsRangeBand string


qq: Do these structs have to be exported? SecurityGroupRule, PortsConfig and PortsRangeBand, etc.

Also, what do you think of reusing IntRangeBand instead of creating PortsRangeBand?

Very good question, even I was wondering if I need these structs to be exported but then I could see almost all the structs are kept public in env manifest file. But you are right I should change them if they are not exported.

Also I did not use IntRangeBand purposefully because IntRangeBand has different definition of validate than what we needed for the port configs.

I'm not sure which part of IntRangeBand's validate logic can't be reused here 🤔 I might be missing something obvious, but it seems to me that both struct's validate are just checking if they are formatted as min-max...

So the error message that validate function returns is different than what we need. Customer might get confused by the min and max wordings in the error message. Other than that I guess everything is fine. It's just that we return human readable errors for strconv.Atoi. Let me know if you still want me to use IntRangeBand

Lou1415926 · 2022-08-01T23:03:50Z

+// struct, allowing it to perform more complex unmarshaling behavior.
+// This method implements the yaml.Unmarshaler (v3) interface.
+func (cfg *PortsConfig) UnmarshalYAML(value *yaml.Node) error {
+	_, err := strconv.Atoi(value.Value) // detmines if input is integer? if err then we decode range values


qq: is this check necessary? If the input isn't an integer, value.Decode(&cfg.Port) would have returned an error anyway, right?

if customer somehow defines ports: "10" then unmarshaling fails. Also when I write an integration test, ports are considered as string and then it unmarshals ports: 10 into PortsRangeBand and the integ test fails. Hence converting it into int before unmarshal it.

What about swap the order, trying to unmarshal into int first then string?

I tried doing that as well. But the problem is that if I unmarshal into int first then it sets the value of the for example 0-65335 as 0 in the int variable and then int variable becomes non empty and wrong unmarshaling happens. So that is why I am trying to convert input into int 1st and if it is not convertible then we know for sure that it's a range.

Lou1415926

Just some nits and thoughts!

Lou1415926 · 2022-08-03T17:50:29Z

-		return *r.PortsConfig.Port, *r.PortsConfig.Port, nil // a single value is provided for ports.
+// GetPorts returns the from and to ports of a security group rule.
+func (r securityGroupRule) GetPorts() (from, to int, err error) {
+	if r.Ports.Ports == nil {


nit: r.Ports.Ports reads a little awkward 😂 :homer-disappearing what do you think of r.Ports.Range?

Hahah, I know it looks little weird.
I will change it Range.

Lou1415926 · 2022-08-03T18:27:02Z

+	if cfg.Ports != nil {
+		if err := cfg.Ports.validate(); err != nil {
+			if strings.Contains(err.Error(), "invalid range value") {
+				return fmt.Errorf("invalid ports value %s: valid port format is ${from_port}-${to_port}", *cfg.Ports)
+			}
+			return err
+		}


nit: what do you think of flipping the conditional

Suggested change

if cfg.Ports != nil {

if err := cfg.Ports.validate(); err != nil {

if strings.Contains(err.Error(), "invalid range value") {

return fmt.Errorf("invalid ports value %s: valid port format is ${from_port}-${to_port}", *cfg.Ports)

}

return err

}

if cfg.Ports == nil {

retur nil

}

// check `cfg.Ports.validate()`

}

Another nit that I'd have is that we could think of a more robust way to match the invalid range value error. Consider if one day we decide to change the error message for IntRangeBand's validate, from "invalid range value..." to something like"range %s is invalid", it's likely that we'd forget to update it here 😰 .

It is not entirely necessary for us to address the nit here. Just sharing the ⬇️ code snippets for your thoughts 💭

An alternative that can deal with this & does not involve too much modification on top of what we have here is:

// In validate.go type errInvalidRange struct { value string validFormat string } func (e *errInvalidRange) Error() string { return fmt.Sprintf("invalid range value %s: valid format is %s", e.value, e.validFormat) } // validate returns nil if IntRangeBand is configured correctly. func (r IntRangeBand) validate() error { .... // Valid minMax example: ["1-2", "1", "2"] if len(minMax) != 3 { return &errInvalidRange{ value: str, validFormat: "${min}-${max}", } } ... }

then here:

// In validate_env.go func (cfg portsConfig) validate() error { ... if err := cfg.Ports.validate(); err != nil { var targetErr *errInvalidRange if errors.As(err, &targetErr) { return &errInvalidRange{ value: aws.StringValue((*string)(cfg.Ports)), validFormat: "${from_port}-${to_port}", } } return err } return nil }

Hmm totally make sense to me. We need robust way to handle such kinds of errors and your solution looks good to me. I will make changes in this PR itself to add this robust change now only in copilot.

efekarakus

Looks great! 🚀

…oup (#3876) Related to: #3810 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

paragbhingre requested a review from a team as a code owner July 25, 2022 16:44

paragbhingre requested review from dannyrandall and removed request for a team July 25, 2022 16:44

amazon-ecs-cli-v2-pr-manager requested a review from iamhopaul123 July 25, 2022 16:44

efekarakus reviewed Jul 25, 2022

View reviewed changes

chore(manifest): add security_group to env manifest

5f382ae

paragbhingre force-pushed the security_group branch from 75f2f23 to 5f382ae Compare July 26, 2022 00:57

paragbhingre added 3 commits July 25, 2022 20:10

fix integ test

909b093

Address feedback

89e2d69

minor nits

e8dd035

Lou1415926 reviewed Jul 26, 2022

View reviewed changes

paragbhingre added 6 commits July 26, 2022 10:19

fix static check

b680761

Address feedback by Wanxian

e5dabc3

Merge branch 'mainline' into security_group

feed7c6

resolve merge cpnflict and rename method

f4b8875

nits

7337412

remove extra code

677786e

Lou1415926 reviewed Jul 27, 2022

View reviewed changes

efekarakus reviewed Jul 27, 2022

View reviewed changes

paragbhingre added 3 commits July 27, 2022 23:09

address feedback:1

fe6d70f

Address feedback to change port defaults and bring securitygroup insi…

92b4622

…de network.vpc

remove extra code

3610a1d

efekarakus reviewed Aug 1, 2022

View reviewed changes

address feedback by Efe

d2fc399

Lou1415926 reviewed Aug 1, 2022

View reviewed changes

paragbhingre added 3 commits August 2, 2022 11:18

nits

df845b5

remove extra string to int conversion from unmarshalYAML

63fad79

use IntRangeBand instead of creating new PortRangeBand

24b4525

Lou1415926 approved these changes Aug 3, 2022

View reviewed changes

Lou1415926 changed the title ~~chore(manifest): add security_group to env manifest~~ feat: add security_group to env manifest Aug 3, 2022

Lou1415926 changed the title ~~feat: add security_group to env manifest~~ feat: configure environment security group from env manifest Aug 3, 2022

paragbhingre added 3 commits August 3, 2022 12:11

minor nits and change error format

ef75b74

Merge branch 'mainline' into security_group

27c0466

fix integ test

6ac0173

efekarakus approved these changes Aug 3, 2022

View reviewed changes

remove redundant break

0937bb5

mergify Bot merged commit 105d9d8 into aws:mainline Aug 3, 2022

paragbhingre mentioned this pull request Aug 10, 2022

docs: add docs for security group rules for environment's security group #3876

Merged

paragbhingre deleted the security_group branch January 26, 2023 07:27

-	if isSecurityConfigSet {
+	if !isSecurityConfigSet {
+		return template.SecurityGroupConfig{}
+	}
+	ingress := make(// remaining code...)

Conversation

paragbhingre commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rclinard-amzn commented Jul 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paragbhingre Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efekarakus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paragbhingre Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

efekarakus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paragbhingre Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

paragbhingre commented Jul 25, 2022 •

edited

Loading

paragbhingre Jul 28, 2022 •

edited

Loading

paragbhingre Jul 28, 2022 •

edited

Loading

paragbhingre Aug 1, 2022 •

edited

Loading

paragbhingre Aug 2, 2022 •

edited

Loading