From 28fae44e482822c5924cac48cb7c32c2e26b26aa Mon Sep 17 00:00:00 2001 From: Manuel Alejandro de Brito Fontes Date: Sat, 30 Apr 2022 19:20:47 -0400 Subject: [PATCH] [agent-smith] Remove egress check feature --- components/ee/agent-smith/config-schema.json | 3 - components/ee/agent-smith/example-config.json | 11 -- components/ee/agent-smith/pkg/agent/agent.go | 110 ++----------- .../ee/agent-smith/pkg/agent/agent_test.go | 153 +----------------- ...agent_check_egress_excessive_egress.golden | 13 -- .../agent_check_egress_no_infringement.golden | 13 -- ..._check_egress_very_excessive_egress.golden | 13 -- ...egress_very_excessive_egress_simple.golden | 13 -- .../agent_check_egress_zero_egress.golden | 4 - .../ee/agent-smith/pkg/config/config.go | 20 --- 10 files changed, 22 insertions(+), 331 deletions(-) delete mode 100644 components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_excessive_egress.golden delete mode 100644 components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_no_infringement.golden delete mode 100644 components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_very_excessive_egress.golden delete mode 100644 components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_very_excessive_egress_simple.golden delete mode 100644 components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_zero_egress.golden diff --git a/components/ee/agent-smith/config-schema.json b/components/ee/agent-smith/config-schema.json index 33c446d85ce90e..a8a3afe0da2096 100644 --- a/components/ee/agent-smith/config-schema.json +++ b/components/ee/agent-smith/config-schema.json @@ -35,9 +35,6 @@ "blacklists": { "$ref": "#/definitions/" }, - "egressTraffic": { - "$ref": "#/definitions/" - }, "enforcement": { "$ref": "#/definitions/" }, diff --git a/components/ee/agent-smith/example-config.json b/components/ee/agent-smith/example-config.json index 34855cbc6763a0..2a7ef4b3188706 100644 --- a/components/ee/agent-smith/example-config.json +++ b/components/ee/agent-smith/example-config.json @@ -13,16 +13,5 @@ } ] } - }, - "egressTraffic": { - "dt": "2m", - "excessive": { - "baseBudget": "300Mi", - "perDtThreshold": "100Mi" - }, - "veryExcessive": { - "baseBudget": "2Gi", - "perDtThreshold": "250Mi" - } } } diff --git a/components/ee/agent-smith/pkg/agent/agent.go b/components/ee/agent-smith/pkg/agent/agent.go index ec78f1a488cc6a..8a49e935f66060 100644 --- a/components/ee/agent-smith/pkg/agent/agent.go +++ b/components/ee/agent-smith/pkg/agent/agent.go @@ -8,25 +8,24 @@ import ( "context" "fmt" "net/url" - "os" "sort" "strings" "sync" "time" - "github.com/gitpod-io/gitpod/agent-smith/pkg/classifier" - "github.com/gitpod-io/gitpod/agent-smith/pkg/common" - "github.com/gitpod-io/gitpod/agent-smith/pkg/config" - "github.com/gitpod-io/gitpod/agent-smith/pkg/detector" - "github.com/gitpod-io/gitpod/common-go/log" - gitpod "github.com/gitpod-io/gitpod/gitpod-protocol" "github.com/prometheus/client_golang/prometheus" "golang.org/x/xerrors" - "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" "k8s.io/utils/lru" + + "github.com/gitpod-io/gitpod/agent-smith/pkg/classifier" + "github.com/gitpod-io/gitpod/agent-smith/pkg/common" + "github.com/gitpod-io/gitpod/agent-smith/pkg/config" + "github.com/gitpod-io/gitpod/agent-smith/pkg/detector" + "github.com/gitpod-io/gitpod/common-go/log" + gitpod "github.com/gitpod-io/gitpod/gitpod-protocol" ) const ( @@ -42,9 +41,8 @@ type Smith struct { Kubernetes kubernetes.Interface metrics *metrics - egressTrafficCheckHandler func(pid int) (int64, error) - timeElapsedHandler func(t time.Time) time.Duration - notifiedInfringements *lru.Cache + timeElapsedHandler func(t time.Time) time.Duration + notifiedInfringements *lru.Cache detector detector.ProcessDetector classifier classifier.ProcessClassifier @@ -112,10 +110,9 @@ func NewAgentSmith(cfg config.Config) (*Smith, error) { res := &Smith{ EnforcementRules: map[string]config.EnforcementRules{ defaultRuleset: { - config.GradeKind(config.InfringementExec, common.SeverityBarely): config.PenaltyLimitCPU, - config.GradeKind(config.InfringementExec, common.SeverityAudit): config.PenaltyStopWorkspace, - config.GradeKind(config.InfringementExec, common.SeverityVery): config.PenaltyStopWorkspaceAndBlockUser, - config.GradeKind(config.InfringementExcessiveEgress, common.SeverityVery): config.PenaltyStopWorkspace, + config.GradeKind(config.InfringementExec, common.SeverityBarely): config.PenaltyLimitCPU, + config.GradeKind(config.InfringementExec, common.SeverityAudit): config.PenaltyStopWorkspace, + config.GradeKind(config.InfringementExec, common.SeverityVery): config.PenaltyStopWorkspaceAndBlockUser, }, }, Config: cfg, @@ -125,10 +122,9 @@ func NewAgentSmith(cfg config.Config) (*Smith, error) { detector: detec, classifier: class, - notifiedInfringements: lru.New(notificationCacheSize), - metrics: m, - egressTrafficCheckHandler: getEgressTraffic, - timeElapsedHandler: time.Since, + notifiedInfringements: lru.New(notificationCacheSize), + metrics: m, + timeElapsedHandler: time.Since, } if cfg.Enforcement.Default != nil { if err := cfg.Enforcement.Default.Validate(); err != nil { @@ -240,40 +236,6 @@ func (agent *Smith) Start(ctx context.Context, callback func(InfringingWorkspace }() } - egressTicker := time.NewTicker(2 * time.Minute) - - go func() { - for { - <-egressTicker.C - wsMutex.Lock() - for pid, ws := range workspaces { - // check if the workspace is already stopped - fi, err := os.Stat(fmt.Sprintf("/proc/%d", pid)) - if err != nil { - if os.IsNotExist(err) { - log.Debugf("deleting workspace with pid %d and workspaceId %s from workspaces", pid, ws.WorkspaceID) - delete(workspaces, pid) - } else { - log.WithError(err).WithFields(log.OWI(ws.OwnerID, ws.WorkspaceID, ws.InstanceID)).Error("could not stat workspace, stale entries will continue to exist in workspaces state") - } - continue - } - infringement, err := agent.checkEgressTrafficCallback(pid, fi.ModTime()) - if err != nil { - log.WithError(err).WithFields(log.OWI(ws.OwnerID, ws.WorkspaceID, ws.InstanceID)).Error("error calling checkEgressTrafficCallback") - continue - } - if infringement != nil { - log.WithFields(log.OWI(ws.OwnerID, ws.WorkspaceID, ws.InstanceID)).Info("found egress infringing workspace") - // monitor metric here, as we are not penalizing workspaces yet - agent.metrics.egressViolations.WithLabelValues(string(infringement.Kind)).Inc() - } - - } - wsMutex.Unlock() - } - }() - defer log.Info("agent smith main loop ended") // We want to fill the classifier in a Go routine seaparete from using the classification @@ -429,45 +391,3 @@ func (agent *Smith) Collect(m chan<- prometheus.Metric) { agent.classifier.Collect(m) agent.detector.Collect(m) } - -func (agent *Smith) checkEgressTrafficCallback(pid int, pidCreationTime time.Time) (*Infringement, error) { - if agent.Config.EgressTraffic == nil { - return nil, nil - } - - podLifetime := agent.timeElapsedHandler(pidCreationTime) - resp, err := agent.egressTrafficCheckHandler(pid) - if err != nil { - return nil, err - } - - log.Debugf("total egress bytes %d", resp) - if resp <= 0 { - log.WithField("total egress bytes", resp).Warn("GetEgressTraffic returned <= 0 value") - return nil, nil - } - - type level struct { - V config.GradedInfringementKind - T *config.PerLevelEgressTraffic - } - levels := make([]level, 0, 2) - if agent.Config.EgressTraffic.VeryExcessiveLevel != nil { - levels = append(levels, level{V: config.GradeKind(config.InfringementExcessiveEgress, common.SeverityVery), T: agent.Config.EgressTraffic.VeryExcessiveLevel}) - } - if agent.Config.EgressTraffic.ExcessiveLevel != nil { - levels = append(levels, level{V: config.GradeKind(config.InfringementExcessiveEgress, common.SeverityAudit), T: agent.Config.EgressTraffic.ExcessiveLevel}) - } - - dt := int64(podLifetime / time.Duration(agent.Config.EgressTraffic.WindowDuration)) - for _, lvl := range levels { - allowance := dt*lvl.T.Threshold.Value() + lvl.T.BaseBudget.Value() - excess := resp - allowance - - if excess > 0 { - return &Infringement{Description: fmt.Sprintf("egress traffic is %.3f megabytes over limit", float64(excess)/(1024.0*1024.0)), Kind: lvl.V}, nil - } - } - - return nil, nil -} diff --git a/components/ee/agent-smith/pkg/agent/agent_test.go b/components/ee/agent-smith/pkg/agent/agent_test.go index b452c7ae6ff95a..6c284611286787 100644 --- a/components/ee/agent-smith/pkg/agent/agent_test.go +++ b/components/ee/agent-smith/pkg/agent/agent_test.go @@ -5,14 +5,8 @@ package agent import ( - "encoding/json" - "fmt" - "io/ioutil" - "path" - "reflect" "sort" "testing" - "time" "github.com/gitpod-io/gitpod/agent-smith/pkg/common" "github.com/gitpod-io/gitpod/agent-smith/pkg/config" @@ -62,20 +56,19 @@ func TestGetPenalty(t *testing.T) { func TestFindEnforcementRules(t *testing.T) { ra := config.EnforcementRules{config.GradeKind(config.InfringementExec, common.SeverityAudit): config.PenaltyLimitCPU} - rb := config.EnforcementRules{config.GradeKind(config.InfringementExcessiveEgress, common.SeverityAudit): config.PenaltyLimitCPU} tests := []struct { Desc string Rules map[string]config.EnforcementRules RemoteURL string Expectation config.EnforcementRules }{ - {"direct match", map[string]config.EnforcementRules{"foo": ra, "bar": rb}, "foo", ra}, - {"no match", map[string]config.EnforcementRules{"foo*": ra, "bar": rb}, "not found", nil}, - {"star", map[string]config.EnforcementRules{"foo*": ra, "bar": rb}, "foobar", ra}, - {"prefix match", map[string]config.EnforcementRules{"*foo": ra, "bar": rb}, "hello/foo", ra}, - {"suffix match", map[string]config.EnforcementRules{"foo*": ra, "bar": rb}, "foobar", ra}, - {"case-insensitive match", map[string]config.EnforcementRules{"foo*": ra, "bar": rb}, "Foobar", ra}, - {"submatch", map[string]config.EnforcementRules{"*foo*": ra, "bar": rb}, "hello/foo/bar", ra}, + {"direct match", map[string]config.EnforcementRules{"foo": ra}, "foo", ra}, + {"no match", map[string]config.EnforcementRules{"foo*": ra}, "not found", nil}, + {"star", map[string]config.EnforcementRules{"foo*": ra}, "foobar", ra}, + {"prefix match", map[string]config.EnforcementRules{"*foo": ra}, "hello/foo", ra}, + {"suffix match", map[string]config.EnforcementRules{"foo*": ra}, "foobar", ra}, + {"case-insensitive match", map[string]config.EnforcementRules{"foo*": ra}, "Foobar", ra}, + {"submatch", map[string]config.EnforcementRules{"*foo*": ra}, "hello/foo/bar", ra}, } for _, test := range tests { @@ -101,135 +94,3 @@ func BenchmarkFindEnforcementRules(b *testing.B) { findEnforcementRules(rules, "foobar") } } - -func TestCeckEgressTrafficCallback(t *testing.T) { - simpleTime, _ := time.Parse(time.RFC3339, "2021-07-05T15:16:17+02:00") - - type args struct { - pid int - pidCreationTime time.Time - } - tests := map[string]struct { - args args - want *Infringement - egressTrafficCheckHandler func(pid int) (int64, error) - timeElapsedHandler func(t time.Time) time.Duration - wantErr bool - }{ - "no_infringement": { - args: args{ - pid: 1234, - pidCreationTime: simpleTime, - }, - want: nil, - egressTrafficCheckHandler: func(pid int) (int64, error) { - return 2000000, nil - }, - timeElapsedHandler: func(t time.Time) time.Duration { - d, _ := time.ParseDuration("1m") - return d - }, - wantErr: false, - }, - "zero_egress": { - args: args{ - pid: 1234, - pidCreationTime: simpleTime, - }, - want: nil, - egressTrafficCheckHandler: func(pid int) (int64, error) { - return 0, nil - }, - timeElapsedHandler: func(t time.Time) time.Duration { - d, _ := time.ParseDuration("1m") - return d - }, - wantErr: false, - }, - "excessive_egress": { - args: args{ - pid: 1234, - pidCreationTime: simpleTime, - }, - want: &Infringement{ - Kind: config.GradedInfringementKind(config.InfringementExcessiveEgress), - Description: "egress traffic is 12.805 megabytes over limit", - }, - egressTrafficCheckHandler: func(pid int) (int64, error) { - return 328000000, nil - }, - timeElapsedHandler: func(t time.Time) time.Duration { - d, _ := time.ParseDuration("1m") - return d - }, - wantErr: false, - }, - "very_excessive_egress_simple": { - args: args{ - pid: 1234, - pidCreationTime: simpleTime, - }, - want: &Infringement{ - Kind: config.GradeKind(config.InfringementExcessiveEgress, common.SeverityVery), - Description: "egress traffic is 188686.863 megabytes over limit", - }, - egressTrafficCheckHandler: func(pid int) (int64, error) { - return 200000000000, nil - }, - timeElapsedHandler: func(t time.Time) time.Duration { - d, _ := time.ParseDuration("1s") - return d - }, - wantErr: false, - }, - "very_excessive_egress": { - args: args{ - pid: 1234, - pidCreationTime: simpleTime, - }, - want: &Infringement{ - Kind: config.GradeKind(config.InfringementExcessiveEgress, common.SeverityVery), - Description: "egress traffic is 188686.863 megabytes over limit", - }, - egressTrafficCheckHandler: func(pid int) (int64, error) { - return 200000000000, nil - }, - timeElapsedHandler: func(t time.Time) time.Duration { - d, _ := time.ParseDuration("1m") - return d - }, - wantErr: false, - }, - } - - for name, tt := range tests { - t.Run(name, func(t *testing.T) { - fc, err := ioutil.ReadFile(path.Join("testdata", fmt.Sprintf("agent_check_egress_%s.golden", name))) - if err != nil { - t.Errorf("cannot read config: %v", err) - return - } - var cfg config.Config - err = json.Unmarshal(fc, &cfg) - if err != nil { - t.Errorf("cannot unmarshal config: %v", err) - return - } - agent, err := NewAgentSmith(cfg) - if err != nil { - t.Errorf("cannot create test agent smith from config: %v", err) - return - } - agent.egressTrafficCheckHandler = tt.egressTrafficCheckHandler - agent.timeElapsedHandler = tt.timeElapsedHandler - got, err := agent.checkEgressTrafficCallback(tt.args.pid, tt.args.pidCreationTime) - if (err != nil) != tt.wantErr { - t.Errorf("Smith.checkEgressTrafficCallback() error = %v, wantErr %v", err, tt.wantErr) - return - } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("Smith.checkEgressTrafficCallback() = %s", cmp.Diff(got, tt.want)) - } - }) - } -} diff --git a/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_excessive_egress.golden b/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_excessive_egress.golden deleted file mode 100644 index d1c4e0b77e6004..00000000000000 --- a/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_excessive_egress.golden +++ /dev/null @@ -1,13 +0,0 @@ -{ - "egressTraffic": { - "dt": "2m", - "excessive": { - "baseBudget": "300Mi", - "perDtThreshold": "100Mi" - }, - "veryExcessive": { - "baseBudget": "2Gi", - "perDtThreshold": "250Mi" - } - } -} diff --git a/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_no_infringement.golden b/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_no_infringement.golden deleted file mode 100644 index d1c4e0b77e6004..00000000000000 --- a/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_no_infringement.golden +++ /dev/null @@ -1,13 +0,0 @@ -{ - "egressTraffic": { - "dt": "2m", - "excessive": { - "baseBudget": "300Mi", - "perDtThreshold": "100Mi" - }, - "veryExcessive": { - "baseBudget": "2Gi", - "perDtThreshold": "250Mi" - } - } -} diff --git a/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_very_excessive_egress.golden b/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_very_excessive_egress.golden deleted file mode 100644 index b4d32612430169..00000000000000 --- a/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_very_excessive_egress.golden +++ /dev/null @@ -1,13 +0,0 @@ -{ - "egressTraffic": { - "dt": "1h", - "excessive": { - "baseBudget": "300Mi", - "perDtThreshold": "100Mi" - }, - "veryExcessive": { - "baseBudget": "2Gi", - "perDtThreshold": "250Mi" - } - } -} diff --git a/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_very_excessive_egress_simple.golden b/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_very_excessive_egress_simple.golden deleted file mode 100644 index d1c4e0b77e6004..00000000000000 --- a/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_very_excessive_egress_simple.golden +++ /dev/null @@ -1,13 +0,0 @@ -{ - "egressTraffic": { - "dt": "2m", - "excessive": { - "baseBudget": "300Mi", - "perDtThreshold": "100Mi" - }, - "veryExcessive": { - "baseBudget": "2Gi", - "perDtThreshold": "250Mi" - } - } -} diff --git a/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_zero_egress.golden b/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_zero_egress.golden deleted file mode 100644 index 617daefadbcac8..00000000000000 --- a/components/ee/agent-smith/pkg/agent/testdata/agent_check_egress_zero_egress.golden +++ /dev/null @@ -1,4 +0,0 @@ -{ - "egressTraffic": { - } -} diff --git a/components/ee/agent-smith/pkg/config/config.go b/components/ee/agent-smith/pkg/config/config.go index 798e9a1daecd1b..3042ead43f16c1 100644 --- a/components/ee/agent-smith/pkg/config/config.go +++ b/components/ee/agent-smith/pkg/config/config.go @@ -12,9 +12,7 @@ import ( "github.com/gitpod-io/gitpod/agent-smith/pkg/classifier" "github.com/gitpod-io/gitpod/agent-smith/pkg/common" - "github.com/gitpod-io/gitpod/common-go/util" "golang.org/x/xerrors" - "k8s.io/apimachinery/pkg/api/resource" ) func GetConfig(cfgFile string) (*ServiceConfig, error) { @@ -96,8 +94,6 @@ type InfringementKind string const ( // InfringementExec means a user executed a blocklisted executable InfringementExec InfringementKind = "blocklisted executable" - // InfringementExcessiveEgress means a user produced too much egress traffic - InfringementExcessiveEgress InfringementKind = "excessive egress" ) // PenaltyKind describes a kind of penalty for a violating workspace @@ -141,7 +137,6 @@ func (g GradedInfringementKind) Kind() (InfringementKind, error) { wopfx := strings.TrimSpace(strings.TrimPrefix(string(g), string(g.Severity()))) validKinds := []InfringementKind{ - InfringementExcessiveEgress, InfringementExec, } for _, k := range validKinds { @@ -175,7 +170,6 @@ type Config struct { Blocklists *Blocklists `json:"blocklists,omitempty"` - EgressTraffic *EgressTraffic `json:"egressTraffic,omitempty"` Enforcement Enforcement `json:"enforcement,omitempty"` ExcessiveCPUCheck *ExcessiveCPUCheck `json:"excessiveCPUCheck,omitempty"` SlackWebhooks *SlackWebhooks `json:"slackWebhooks,omitempty"` @@ -190,20 +184,6 @@ type SlackWebhooks struct { Warning string `json:"warning,omitempty"` } -// EgressTraffic configures an upper limit of allowed egress traffic over time -type EgressTraffic struct { - WindowDuration util.Duration `json:"dt"` - - ExcessiveLevel *PerLevelEgressTraffic `json:"excessive"` - VeryExcessiveLevel *PerLevelEgressTraffic `json:"veryExcessive"` -} - -// PerLevelEgressTraffic configures the egress traffic threshold per level -type PerLevelEgressTraffic struct { - BaseBudget resource.Quantity `json:"baseBudget"` - Threshold resource.Quantity `json:"perDtThreshold"` -} - // Blocklists list s/signature blocklists for various levels of infringement type Blocklists struct { Barely *PerLevelBlocklist `json:"barely,omitempty"`