Resolve comments

api/constants/constants.go

@@ -321,11 +321,3 @@ const (
 	// allowed AWS role ARNs.
 	TraitAWSRoleARNs = "aws_role_arns"
 )
-
-const (
-	// AWSInstanceStateName represents the state of the AWS EC2
-	// instance - (pending | running | shutting-down | terminated | stopping | stopped )
-	// https://docs.aws.amazon.com/cli/latest/reference/ec2/describe-instances.html
-	// Used for filtering instances for automatic EC2 discovery
-	AWSInstanceStateName = "instance-state-name"
-)

lib/config/configuration.go

@@ -1060,9 +1060,9 @@ func applySSHConfig(fc *FileConfig, cfg *service.Config) (err error) {
 	for _, matcher := range fc.SSH.AWSMatchers {
 		cfg.SSH.AWSMatchers = append(cfg.SSH.AWSMatchers,
 			services.AWSMatcher{
-				Types:   matcher.Types,
-				Regions: matcher.Regions,
-				Tags:    matcher.Tags,
+				Types:   matcher.Matcher.Types,
+				Regions: matcher.Matcher.Regions,
+				Tags:    matcher.Matcher.Tags,
 				Params: services.InstallerParams{
 					JoinMethod: matcher.InstallParams.JoinParams.Method,
 					JoinToken:  matcher.InstallParams.JoinParams.TokenName,

lib/config/fileconf.go

@@ -437,7 +437,7 @@ func (conf *FileConfig) CheckAndSetDefaults() error {
 		}
 	}
 
-	matchers := make([]AWSMatcher, 0, len(conf.SSH.AWSMatchers))
+	matchers := make([]AWSEC2Matcher, 0, len(conf.SSH.AWSMatchers))
 
 	for _, matcher := range conf.SSH.AWSMatchers {
 		if matcher.InstallParams == nil {
@@ -1027,7 +1027,7 @@ type SSH struct {
 	DisableCreateHostUser bool `yaml:"disable_create_host_user,omitempty"`
 
 	// AWSMatchers are used to match EC2 instances
-	AWSMatchers []AWSMatcher `yaml:"aws,omitempty"`
+	AWSMatchers []AWSEC2Matcher `yaml:"aws,omitempty"`
 }
 
 // AllowTCPForwarding checks whether the config file allows TCP forwarding or not.
@@ -1214,6 +1214,12 @@ type AWSMatcher struct {
 	Regions []string `yaml:"regions,omitempty"`
 	// Tags are AWS tags to match.
 	Tags map[string]apiutils.Strings `yaml:"tags,omitempty"`
+}
+
+// AWSEC2Matcher matches EC2 instances
+type AWSEC2Matcher struct {
+	// Matcher is used to match EC2 instances based on tags
+	Matcher AWSMatcher `yaml:",inline"`
 	// InstallParams sets the join method when installing on
 	// discovered EC2 nodes
 	InstallParams *InstallParams `yaml:"install,omitempty"`

lib/srv/server/cloudwatcher.go

@@ -23,7 +23,6 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
-	"github.com/gravitational/teleport/api/constants"
 	"github.com/gravitational/teleport/api/types"
 	"github.com/gravitational/teleport/lib/cloud"
 	"github.com/gravitational/teleport/lib/services"
@@ -32,6 +31,14 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
+const (
+	// AWSInstanceStateName represents the state of the AWS EC2
+	// instance - (pending | running | shutting-down | terminated | stopping | stopped )
+	// https://docs.aws.amazon.com/cli/latest/reference/ec2/describe-instances.html
+	// Used for filtering instances for automatic EC2 discovery
+	AWSInstanceStateName = "instance-state-name"
+)
+
 // EC2Instances contains information required to send SSM commands to EC2 instances
 type EC2Instances struct {
 	// Region is the AWS region where the instances are located.
@@ -64,17 +71,19 @@ func (w *Watcher) Run() {
 	defer ticker.Stop()
 	for {
 		for _, fetcher := range w.fetchers {
-			inst, err := fetcher.GetEC2Instances(w.ctx)
+			instancesColl, err := fetcher.GetEC2Instances(w.ctx)
 			if err != nil {
 				if trace.IsNotFound(err) {
 					continue
 				}
 				log.WithError(err).Error("Failed to fetch EC2 instances")
 				continue
 			}
-			select {
-			case w.InstancesC <- *inst:
-			case <-w.ctx.Done():
+			for _, inst := range instancesColl {
+				select {
+				case w.InstancesC <- inst:
+				case <-w.ctx.Done():
+				}
 			}
 		}
 		select {
@@ -99,7 +108,7 @@ func NewCloudWatcher(ctx context.Context, matchers []services.AWSMatcher, client
 		ctx:           cancelCtx,
 		cancel:        cancelFn,
 		fetchInterval: time.Minute,
-		InstancesC:    make(chan EC2Instances),
+		InstancesC:    make(chan EC2Instances, 2),
 	}
 	for _, matcher := range matchers {
 		for _, region := range matcher.Regions {
@@ -137,8 +146,8 @@ type ec2InstanceFetcher struct {
 }
 
 func newEC2InstanceFetcher(cfg ec2FetcherConfig) *ec2InstanceFetcher {
-	tagFilters := []*ec2.Filter{&ec2.Filter{
-		Name:   aws.String(constants.AWSInstanceStateName),
+	tagFilters := []*ec2.Filter{{
+		Name:   aws.String(AWSInstanceStateName),
 		Values: aws.StringSlice([]string{ec2.InstanceStateNameRunning}),
 	}}
 
@@ -161,18 +170,20 @@ func newEC2InstanceFetcher(cfg ec2FetcherConfig) *ec2InstanceFetcher {
 }
 
 // GetEC2Instances fetches all EC2 instances matching configured filters.
-func (f *ec2InstanceFetcher) GetEC2Instances(ctx context.Context) (*EC2Instances, error) {
-	var instances []*ec2.Instance
-	var accountID string
+func (f *ec2InstanceFetcher) GetEC2Instances(ctx context.Context) ([]EC2Instances, error) {
+	var instances []EC2Instances
 	err := f.EC2.DescribeInstancesPagesWithContext(ctx, &ec2.DescribeInstancesInput{
 		Filters: f.Filters,
 	},
 		func(dio *ec2.DescribeInstancesOutput, b bool) bool {
 			for _, res := range dio.Reservations {
-				if accountID == "" {
-					accountID = aws.StringValue(res.OwnerId)
-				}
-				instances = append(instances, res.Instances...)
+				instances = append(instances, EC2Instances{
+					AccountID:  aws.StringValue(res.OwnerId),
+					Region:     f.Region,
+					Document:   f.Document,
+					Instances:  res.Instances,
+					Parameters: f.Parameters,
+				})
 			}
 			return true
 		})
@@ -185,11 +196,5 @@ func (f *ec2InstanceFetcher) GetEC2Instances(ctx context.Context) (*EC2Instances
 		return nil, trace.NotFound("no ec2 instances found")
 	}
 
-	return &EC2Instances{
-		AccountID:  accountID,
-		Region:     f.Region,
-		Document:   f.Document,
-		Instances:  instances,
-		Parameters: f.Parameters,
-	}, nil
+	return instances, nil
 }

lib/srv/server/cloudwatcher_test.go

@@ -20,7 +20,7 @@ import (
 	"context"
 	"testing"
 
-	"github.com/aws/aws-sdk-go-v2/aws"
+	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/request"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
@@ -45,10 +45,42 @@ type mockEC2Client struct {
 	output *ec2.DescribeInstancesOutput
 }
 
+func instanceMatches(inst *ec2.Instance, filters []*ec2.Filter) bool {
+	allMatched := true
+	for _, filter := range filters {
+		name := aws.StringValue(filter.Name)
+		val := aws.StringValue(filter.Values[0])
+		if name == AWSInstanceStateName && aws.StringValue(inst.State.Name) != ec2.InstanceStateNameRunning {
+			return false
+		}
+		for _, tag := range inst.Tags {
+			if aws.StringValue(tag.Key) != name[4:] {
+				continue
+			}
+			allMatched = allMatched && aws.StringValue(tag.Value) != val
+		}
+	}
+
+	return !allMatched
+}
+
 func (m *mockEC2Client) DescribeInstancesPagesWithContext(
 	ctx context.Context, input *ec2.DescribeInstancesInput,
 	f func(dio *ec2.DescribeInstancesOutput, b bool) bool, opts ...request.Option) error {
-	f(m.output, true)
+	output := &ec2.DescribeInstancesOutput{}
+	for _, res := range m.output.Reservations {
+		var instances []*ec2.Instance
+		for _, inst := range res.Instances {
+			if instanceMatches(inst, input.Filters) {
+				instances = append(instances, inst)
+			}
+		}
+		output.Reservations = append(output.Reservations, &ec2.Reservation{
+			Instances: instances,
+		})
+	}
+
+	f(output, true)
 	return nil
 }
 
@@ -61,11 +93,13 @@ func TestEC2Watcher(t *testing.T) {
 			Types:   []string{"EC2"},
 			Regions: []string{"us-west-2"},
 			Tags:    map[string]utils.Strings{"teleport": {"yes"}},
+			SSM:     &services.AWSSSM{},
 		},
 		{
 			Types:   []string{"EC2"},
 			Regions: []string{"us-west-2"},
 			Tags:    map[string]utils.Strings{"env": {"dev"}},
+			SSM:     &services.AWSSSM{},
 		},
 	}
 	ctx := context.Background()
@@ -130,12 +164,14 @@ func TestEC2Watcher(t *testing.T) {
 
 	result := <-watcher.InstancesC
 	require.Equal(t, EC2Instances{
-		Region:    "us-west-2",
-		Instances: []*ec2.Instance{&present},
+		Region:     "us-west-2",
+		Instances:  []*ec2.Instance{&present},
+		Parameters: map[string]string{"token": ""},
 	}, result)
 	result = <-watcher.InstancesC
 	require.Equal(t, EC2Instances{
-		Region:    "us-west-2",
-		Instances: []*ec2.Instance{&presentOther},
+		Region:     "us-west-2",
+		Instances:  []*ec2.Instance{&presentOther},
+		Parameters: map[string]string{"token": ""},
 	}, result)
 }