diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c7bb4ea..4c122d61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,22 @@ # Changelog +## Unreleased + +### Changed + +- S3 object sync keeps a count and a total size per bucket instead of every object, + so its memory no longer grows with the number of objects in the account. It used + to hold them all until the end of the sync, plus a copy of the bucket per object, + and an account with a few busy buckets took a 512 MiB container to its limit in + seconds. `inspect -i bucket_sizer` reads the totals, and the sync line counts the + objects from them. (OpenSystemsLab/awless-ro#4) + +### Added + +- `aws.storage.s3object.keep` (off by default) keeps every object in the local graph + as before, for `list s3objects --local`. Without it, `list s3objects --local` says + where the sizes are and how to list the objects. + ## v0.6.1 ### Fixed diff --git a/README.md b/README.md index 4ddfa743..d1b87120 100644 --- a/README.md +++ b/README.md @@ -511,6 +511,13 @@ and skipped; the rest still land. Two resource types are off by default because they cost an API call per parent — one per bucket, one per hosted zone. The sync says so rather than reporting zero of them. +With S3 object sync on, the sync keeps the number and total size of the objects in +each bucket, which is what `inspect -i bucket_sizer` reads, and not the objects +themselves, so memory stays flat however much the buckets hold. To keep every object +too, for `list s3objects --local` on small buckets, set +`aws.storage.s3object.keep true`. `list s3objects` without `--local` asks AWS +directly either way. + You rarely need to run it by hand. `show` and `inspect` refresh the copy on their own for the services they touch, and changing region with `switch` syncs the new one once. The first run does not sync. `--no-sync` skips that for one command, and diff --git a/aws/fetch/config.go b/aws/fetch/config.go index d014b255..082b3d0b 100644 --- a/aws/fetch/config.go +++ b/aws/fetch/config.go @@ -66,3 +66,10 @@ func (c *Config) getBoolDefaultTrue(key string) bool { return true } + +// getBool is false unless the key is set to true: for the settings that cost +// something to turn on. +func (c *Config) getBool(key string) bool { + b, _ := c.Extra[key].(bool) + return b +} diff --git a/aws/fetch/manual_fetchers.go b/aws/fetch/manual_fetchers.go index 87716bd3..d0a68084 100644 --- a/aws/fetch/manual_fetchers.go +++ b/aws/fetch/manual_fetchers.go @@ -676,8 +676,13 @@ func addManualStorageFetchFuncs(conf *Config, funcs map[string]fetch.Func) { } }() + // A sync keeps per-bucket totals only, unless asked for the objects too. + // `list s3objects` fetches by type and prints the objects themselves. + fetchingType, _ := fetch.IsFetchingByType(ctx) + keepObjects := fetchingType == "s3object" || conf.getBool("aws.storage.s3object.keep") + err := forEachBucketParallel(ctx, cache, conf.APIs.S3, func(b s3types.Bucket) error { - return fetchObjectsForBucket(ctx, conf.APIs.S3, b, resourcesC) + return fetchObjectsForBucket(ctx, conf.APIs.S3, b, keepObjects, resourcesC) }) close(resourcesC) diff --git a/aws/fetch/s3_helpers.go b/aws/fetch/s3_helpers.go index 4e4d7833..89e5fd88 100644 --- a/aws/fetch/s3_helpers.go +++ b/aws/fetch/s3_helpers.go @@ -10,6 +10,7 @@ import ( s3types "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/theazz/awless-ro/aws/conv" + "github.com/theazz/awless-ro/cloud/properties" "github.com/theazz/awless-ro/cloud/rdf" "github.com/theazz/awless-ro/fetch" "github.com/theazz/awless-ro/graph" @@ -33,7 +34,25 @@ func forEachBucketParallel(ctx context.Context, cache fetch.Cache, api S3API, f }) } -func fetchObjectsForBucket(ctx context.Context, api S3API, bucket s3types.Bucket, resourcesC chan<- *graph.Resource) error { +// fetchObjectsForBucket pages through a bucket's objects and sends the bucket with +// two totals on it, ObjectCount and Size in bytes, which is all bucket_sizer reads. +// +// It used to send every object as a resource, plus a fresh copy of the bucket per +// object, and a sync held all of them until it wrote the graph: memory grew with the +// number of objects in the account, and a few busy buckets were enough to take a +// 512 MiB container to its limit. The objects themselves are only sent when +// keepObjects is set, for `list s3objects` and whoever turns on +// aws.storage.s3object.keep. +// +// A listing that fails part way sends no totals, since a partial count would read as +// the bucket's size. +func fetchObjectsForBucket(ctx context.Context, api S3API, bucket s3types.Bucket, keepObjects bool, resourcesC chan<- *graph.Resource) error { + parent, err := awsconv.InitResource(bucket) + if err != nil { + return err + } + + var count, size int paginator := s3.NewListObjectsV2Paginator(api, &s3.ListObjectsV2Input{Bucket: bucket.Name}) for paginator.HasMorePages() { page, err := paginator.NextPage(ctx) @@ -41,21 +60,24 @@ func fetchObjectsForBucket(ctx context.Context, api S3API, bucket s3types.Bucket return err } for _, output := range page.Contents { - res, err := awsconv.NewResource(output) - if err != nil { - return err + count++ + size += int(aws.ToInt64(output.Size)) + if !keepObjects { + continue } - res.SetProperty("Bucket", aws.ToString(bucket.Name)) - resourcesC <- res - - parent, err := awsconv.InitResource(bucket) + res, err := awsconv.NewResource(output) if err != nil { return err } + res.SetProperty(properties.Bucket, aws.ToString(bucket.Name)) res.AddRelation(rdf.ChildrenOfRel, parent) - resourcesC <- parent + resourcesC <- res } } + + parent.SetProperty(properties.ObjectCount, count) + parent.SetProperty(properties.Size, size) + resourcesC <- parent return nil } diff --git a/aws/fetch/s3_objects_test.go b/aws/fetch/s3_objects_test.go new file mode 100644 index 00000000..03fb0a6a --- /dev/null +++ b/aws/fetch/s3_objects_test.go @@ -0,0 +1,202 @@ +package awsfetch + +import ( + "context" + "errors" + "fmt" + "strconv" + "testing" + + awssdk "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/s3" + s3types "github.com/aws/aws-sdk-go-v2/service/s3/types" + + "github.com/theazz/awless-ro/cloud" + "github.com/theazz/awless-ro/cloud/properties" + "github.com/theazz/awless-ro/fetch" + "github.com/theazz/awless-ro/graph" +) + +// pagedS3 lists each bucket's objects a page at a time, as S3 does past 1000 keys, +// and can fail on a given page of a given bucket. +type pagedS3 struct { + stubS3 + pageSize int + failBucket string + failPage int +} + +func (p *pagedS3) ListObjectsV2(_ context.Context, in *s3.ListObjectsV2Input, _ ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) { + name := awssdk.ToString(in.Bucket) + page := tokenIndex(awssdk.ToString(in.ContinuationToken)) + if name == p.failBucket && page == p.failPage { + return nil, errors.New("listing failed") + } + all := p.objects[name] + start, end := page*p.pageSize, (page+1)*p.pageSize + if end > len(all) { + end = len(all) + } + out := &s3.ListObjectsV2Output{Contents: all[start:end], IsTruncated: awssdk.Bool(end < len(all))} + if end < len(all) { + out.NextContinuationToken = awssdk.String(strconv.Itoa(page + 1)) + } + return out, nil +} + +func objectsOfSize(n int, size int64) []s3types.Object { + objects := make([]s3types.Object, n) + for i := range objects { + objects[i] = s3types.Object{Key: awssdk.String(fmt.Sprintf("key-%d", i)), Size: awssdk.Int64(size)} + } + return objects +} + +func newPagedS3() *pagedS3 { + return &pagedS3{ + pageSize: 1000, + stubS3: stubS3{ + buckets: map[string][]s3types.Bucket{"eu-west-1": {{Name: awssdk.String("logs")}, {Name: awssdk.String("empty")}}}, + objects: map[string][]s3types.Object{"logs": objectsOfSize(2500, 3)}, + grants: map[string][]s3types.Grant{"logs": {{Permission: "READ", Grantee: &s3types.Grantee{ID: awssdk.String("owner"), Type: "CanonicalUser"}}}}, + }, + failPage: -1, + } +} + +func fetchObjects(t *testing.T, api S3API, extra map[string]interface{}, ctx context.Context) []*graph.Resource { + t.Helper() + conf := NewConfig(&AWSAPI{S3: api}) + conf.Extra["aws.storage.s3object.sync"] = true + for k, v := range extra { + conf.Extra[k] = v + } + funcs := BuildStorageFetchFuncs(conf) + resources, _, err := funcs["s3object"](context.WithValue(ctx, "region", "eu-west-1"), fetch.NewFetcher(funcs)) + if err != nil { + t.Fatal(err) + } + return resources +} + +func byType(resources []*graph.Resource, typ string) []*graph.Resource { + var out []*graph.Resource + for _, r := range resources { + if r.Type() == typ { + out = append(out, r) + } + } + return out +} + +// Object sync used to keep every object of every bucket in memory until the end of +// the sync, and an account with a few busy buckets took a 512 MiB container to its +// limit in seconds. bucket_sizer, the only reader, needs two numbers per bucket. +func TestObjectSyncKeepsPerBucketTotalsNotObjects(t *testing.T) { + resources := fetchObjects(t, newPagedS3(), nil, context.Background()) + + if objects := byType(resources, cloud.S3Object); len(objects) != 0 { + t.Errorf("got %d object resources, want none: memory would grow with the object count", len(objects)) + } + + buckets := byType(resources, cloud.Bucket) + totals := map[string][2]interface{}{} + for _, b := range buckets { + if _, seen := totals[b.Id()]; seen { + t.Errorf("bucket %s was sent more than once", b.Id()) + } + totals[b.Id()] = [2]interface{}{b.Properties()[properties.ObjectCount], b.Properties()[properties.Size]} + } + want := map[string][2]interface{}{ + "logs": {2500, 7500}, + "empty": {0, 0}, + } + if fmt.Sprint(totals) != fmt.Sprint(want) { + t.Errorf("got totals %v, want %v", totals, want) + } +} + +// The totals go into the graph on the bucket the bucket fetcher builds, not on a +// second node. +func TestObjectTotalsLandOnTheSyncedBucket(t *testing.T) { + conf := NewConfig(&AWSAPI{S3: newPagedS3()}) + conf.Extra["aws.storage.s3object.sync"] = true + ctx := context.WithValue(context.Background(), "region", "eu-west-1") + + g, err := fetch.NewFetcher(BuildStorageFetchFuncs(conf)).Fetch(ctx) + if err != nil { + t.Fatal(err) + } + logs, err := g.GetResource(cloud.Bucket, "logs") + if err != nil { + t.Fatal(err) + } + if got := logs.Properties()[properties.ObjectCount]; got != 2500 { + t.Errorf("object count %v, want 2500", got) + } + if got := logs.Properties()[properties.Size]; got != 7500 { + t.Errorf("size %v, want 7500", got) + } + if _, ok := logs.Properties()[properties.Grants]; !ok { + t.Error("the bucket lost what the bucket fetcher put on it") + } +} + +// Keeping the objects themselves stays possible, for `list s3objects --local` on small +// buckets. Each one still points at its bucket, which is built once. +func TestObjectSyncKeepsObjectsWhenAskedTo(t *testing.T) { + resources := fetchObjects(t, newPagedS3(), map[string]interface{}{"aws.storage.s3object.keep": true}, context.Background()) + + objects := byType(resources, cloud.S3Object) + if len(objects) != 2500 { + t.Fatalf("got %d objects, want 2500", len(objects)) + } + if got := objects[0].Properties()[properties.Bucket]; got != "logs" { + t.Errorf("object names bucket %v, want logs", got) + } + if n := len(byType(resources, cloud.Bucket)); n != 2 { + t.Errorf("got %d bucket resources, want one per bucket", n) + } +} + +// `list s3objects` without --local asks AWS for the objects and prints them, so they +// have to come back whatever the sync setting says. +func TestLiveObjectListingReturnsObjects(t *testing.T) { + conf := NewConfig(&AWSAPI{S3: newPagedS3()}) + ctx := context.WithValue(context.Background(), "region", "eu-west-1") + ctx = context.WithValue(ctx, "force", true) + + g, err := fetch.NewFetcher(BuildStorageFetchFuncs(conf)).FetchByType(ctx, cloud.S3Object) + if err != nil { + t.Fatal(err) + } + objects, err := g.GetAllResources(cloud.S3Object) + if err != nil { + t.Fatal(err) + } + if len(objects) != 2500 { + t.Errorf("got %d objects, want 2500", len(objects)) + } +} + +// A listing that fails half way gives no totals for that bucket: a partial count +// would read as the bucket's size. +func TestFailedListingGivesNoPartialTotals(t *testing.T) { + api := newPagedS3() + api.failBucket, api.failPage = "logs", 1 + + conf := NewConfig(&AWSAPI{S3: api}) + conf.Extra["aws.storage.s3object.sync"] = true + funcs := BuildStorageFetchFuncs(conf) + ctx := context.WithValue(context.Background(), "region", "eu-west-1") + + resources, _, err := funcs["s3object"](ctx, fetch.NewFetcher(funcs)) + if err == nil { + t.Fatal("the listing error was swallowed") + } + for _, b := range byType(resources, cloud.Bucket) { + if n, ok := b.Properties()[properties.ObjectCount]; ok && b.Id() == "logs" { + t.Errorf("logs got a count of %v from a listing that failed", n) + } + } +} diff --git a/aws/services/services_test.go b/aws/services/services_test.go index 1cb7f5ad..b456b9a0 100644 --- a/aws/services/services_test.go +++ b/aws/services/services_test.go @@ -814,36 +814,48 @@ func TestBuildStorageRdfGraph(t *testing.T) { }, } - mocks3 := &mockS3{manualS3Mock: manualS3Mock{buckets: buckets, objects: objects, grants: bucketsACL}} - StorageService = mocks3 - storage := Storage{ - S3API: mocks3, - region: "eu-west-1", - fetcher: fetch.NewFetcher(awsfetch.BuildStorageFetchFuncs(awsfetch.NewConfig(&awsfetch.AWSAPI{S3: mocks3}))), - } + // A sync keeps the number and size of each bucket's objects on the bucket, and the + // objects themselves only when aws.storage.s3object.keep is set. + for _, keep := range []bool{false, true} { + t.Run(fmt.Sprintf("keep=%t", keep), func(t *testing.T) { + mocks3 := &mockS3{manualS3Mock: manualS3Mock{buckets: buckets, objects: objects, grants: bucketsACL}} + StorageService = mocks3 + conf := awsfetch.NewConfig(&awsfetch.AWSAPI{S3: mocks3}) + conf.Extra["aws.storage.s3object.keep"] = keep + storage := Storage{ + S3API: mocks3, + region: "eu-west-1", + fetcher: fetch.NewFetcher(awsfetch.BuildStorageFetchFuncs(conf)), + } - g, err := storage.Fetch(context.Background()) - if err != nil { - t.Fatal(err) - } - resources, err := g.Find(cloud.NewQuery("region", "bucket")) - if err != nil { - t.Fatal(err) - } + g, err := storage.Fetch(context.Background()) + if err != nil { + t.Fatal(err) + } + resources, err := g.Find(cloud.NewQuery("region", "bucket")) + if err != nil { + t.Fatal(err) + } - expected := map[string]cloud.Resource{ - "eu-west-1": resourcetest.Region("eu-west-1").Build(), - "bucket_eu_1": resourcetest.Bucket("bucket_eu_1").Prop(p.Grants, []*graph.Grant{{Grantee: graph.Grantee{GranteeID: "usr_2"}, Permission: "Write"}}).Build(), - "bucket_eu_2": resourcetest.Bucket("bucket_eu_2").Prop(p.Grants, []*graph.Grant{{Grantee: graph.Grantee{GranteeID: "usr_1"}, Permission: "Write"}}).Build(), - } - expectedChildren := map[string][]string{ - "eu-west-1": {"bucket_eu_1", "bucket_eu_2"}, - "bucket_eu_1": {"obj_4"}, - "bucket_eu_2": {"obj_5", "obj_6"}, - } - expectedAppliedOn := map[string][]string{} + expected := map[string]cloud.Resource{ + "eu-west-1": resourcetest.Region("eu-west-1").Build(), + "bucket_eu_1": resourcetest.Bucket("bucket_eu_1").Prop(p.Grants, []*graph.Grant{{Grantee: graph.Grantee{GranteeID: "usr_2"}, Permission: "Write"}}). + Prop(p.ObjectCount, 1).Prop(p.Size, 0).Build(), + "bucket_eu_2": resourcetest.Bucket("bucket_eu_2").Prop(p.Grants, []*graph.Grant{{Grantee: graph.Grantee{GranteeID: "usr_1"}, Permission: "Write"}}). + Prop(p.ObjectCount, 2).Prop(p.Size, 0).Build(), + } + expectedChildren := map[string][]string{ + "eu-west-1": {"bucket_eu_1", "bucket_eu_2"}, + } + if keep { + expectedChildren["bucket_eu_1"] = []string{"obj_4"} + expectedChildren["bucket_eu_2"] = []string{"obj_5", "obj_6"} + } + expectedAppliedOn := map[string][]string{} - compareResources(t, g, resources, expected, expectedChildren, expectedAppliedOn) + compareResources(t, g, resources, expected, expectedChildren, expectedAppliedOn) + }) + } } func TestBuildDnsRdfGraph(t *testing.T) { diff --git a/cloud/properties/gen_properties.go b/cloud/properties/gen_properties.go index d7f539db..8f77ccf5 100644 --- a/cloud/properties/gen_properties.go +++ b/cloud/properties/gen_properties.go @@ -147,6 +147,7 @@ const ( NewInstancesProtected = "NewInstancesProtected" Notifications = "Notifications" OKActions = "OKActions" + ObjectCount = "ObjectCount" OptionGroups = "OptionGroups" Origins = "Origins" OutboundRules = "OutboundRules" diff --git a/cloud/rdf/gen_rdf.go b/cloud/rdf/gen_rdf.go index ab6a1189..e19c1257 100644 --- a/cloud/rdf/gen_rdf.go +++ b/cloud/rdf/gen_rdf.go @@ -149,6 +149,7 @@ const ( NewInstancesProtected = "cloud:newInstancesProtected" Notifications = "cloud:notifications" OKActions = "cloud:okActions" + ObjectCount = "cloud:objectCount" OptionGroups = "cloud:optionGroups" Origins = "cloud:origins" OutboundRules = "net:outboundRules" @@ -364,6 +365,7 @@ func init() { properties.NewInstancesProtected: NewInstancesProtected, properties.Notifications: Notifications, properties.OKActions: OKActions, + properties.ObjectCount: ObjectCount, properties.OptionGroups: OptionGroups, properties.Origins: Origins, properties.OutboundRules: OutboundRules, @@ -579,6 +581,7 @@ var Properties = RDFProperties{ NewInstancesProtected: {ID: NewInstancesProtected, RdfType: "rdf:Property", RdfsLabel: "NewInstancesProtected", RdfsDefinedBy: "rdfs:Literal", RdfsDataType: "xsd:boolean"}, Notifications: {ID: Notifications, RdfType: "rdf:Property", RdfsLabel: "Notifications", RdfsDefinedBy: "rdfs:list", RdfsDataType: "rdfs:Class"}, OKActions: {ID: OKActions, RdfType: "rdf:Property", RdfsLabel: "OKActions", RdfsDefinedBy: "rdfs:list", RdfsDataType: "xsd:string"}, + ObjectCount: {ID: ObjectCount, RdfType: "rdf:Property", RdfsLabel: "ObjectCount", RdfsDefinedBy: "rdfs:Literal", RdfsDataType: "xsd:int"}, OptionGroups: {ID: OptionGroups, RdfType: "rdf:Property", RdfsLabel: "OptionGroups", RdfsDefinedBy: "rdfs:list", RdfsDataType: "xsd:string"}, Origins: {ID: Origins, RdfType: "rdf:Property", RdfsLabel: "Origins", RdfsDefinedBy: "rdfs:list", RdfsDataType: "cloud-owl:DistributionOrigin"}, OutboundRules: {ID: OutboundRules, RdfType: "rdf:Property", RdfsLabel: "OutboundRules", RdfsDefinedBy: "rdfs:list", RdfsDataType: "net-owl:FirewallRule"}, diff --git a/commands/json_test.go b/commands/json_test.go index c6b0d270..038ef8d7 100644 --- a/commands/json_test.go +++ b/commands/json_test.go @@ -100,6 +100,27 @@ func TestSyncJSONGolden(t *testing.T) { } } +// A sync keeps per-bucket totals rather than the objects, so counting object +// resources would print "0 s3object" for buckets holding thousands. +func TestSyncCountsObjectsFromBucketTotals(t *testing.T) { + withConfig(t, map[string]interface{}{"aws.storage.s3object.sync": true}, map[string]interface{}{}) + + g := graph.NewGraph() + if err := g.AddResource( + resourcetest.Bucket("logs").Prop(p.ObjectCount, 2500).Prop(p.Size, 7500).Build(), + resourcetest.Bucket("empty").Prop(p.ObjectCount, 0).Prop(p.Size, 0).Build(), + ); err != nil { + t.Fatal(err) + } + stats := syncStats("storage", g) + if got := stats.Resources["s3object"]; got != 2500 { + t.Errorf("got %d s3objects, want 2500", got) + } + if got, want := stats.String(), "storage: 2 buckets, 2500 s3objects"; got != want { + t.Errorf("got %q, want %q", got, want) + } +} + func TestCheckFormat(t *testing.T) { prev := formatGlobalFlag t.Cleanup(func() { formatGlobalFlag = prev }) diff --git a/commands/list.go b/commands/list.go index 7ab44472..73f65b55 100644 --- a/commands/list.go +++ b/commands/list.go @@ -118,6 +118,9 @@ var listSpecificResourceCmd = func(resType string) *cobra.Command { var err error g, err = sync.LoadLocalGraphForService(srvName, config.GetAWSProfile(), config.GetAWSRegion()) exitOn(err) + if resType == cloud.S3Object { + noteNoObjectsKept(g, srvName) + } } else { exitOn(fmt.Errorf("cannot find service for resource type %s", resType)) } @@ -183,6 +186,41 @@ func warnIfNotSynced(service string) { default: return } + printNote(msg) +} + +// noteNoObjectsKept says why `list s3objects --local` is empty when the buckets are +// not: object sync is off, or on and keeping a count and a size per bucket rather +// than the objects. Nothing is said when objects are there to list, or when the +// service was never synced, which warnIfNotSynced has said already. +func noteNoObjectsKept(g cloud.GraphAPI, service string) { + if objects, err := g.Find(cloud.NewQuery(cloud.S3Object)); err != nil || len(objects) > 0 { + return + } + if !sync.ServiceSyncedFor(service, config.GetAWSProfile(), config.GetAWSRegion()) { + return + } + isOn := func(key string) bool { + v, _ := config.Get(key) + on, _ := v.(bool) + return on + } + switch { + case !isOn("aws.storage.s3object.sync"): + printNote("S3 objects are not synced: aws.storage.s3object.sync is off. Drop --local to list them " + + "from AWS, or turn object sync on with `awless-ro config set aws.storage.s3object.sync true` " + + "and `awless-ro config set aws.storage.s3object.keep true`.") + case !isOn("aws.storage.s3object.keep"): + printNote("a sync keeps the number and total size of the objects in each bucket, not the objects: " + + "see `awless-ro inspect -i bucket_sizer`. Drop --local to list the objects from AWS, or keep " + + "them from the next sync on with `awless-ro config set aws.storage.s3object.keep true`.") + } +} + +// printNote is logged, except in JSON, where it is a plain stderr line like the +// inspectors' notes, so that a caller who keeps the logs out of the result still +// gets it. +func printNote(msg string) { if jsonOutput() { fmt.Fprintln(os.Stderr, msg) return diff --git a/commands/sync.go b/commands/sync.go index 271eda0a..0ee98a9c 100644 --- a/commands/sync.go +++ b/commands/sync.go @@ -30,6 +30,7 @@ import ( "github.com/spf13/cobra" "github.com/theazz/awless-ro/aws/services" "github.com/theazz/awless-ro/cloud" + "github.com/theazz/awless-ro/cloud/properties" "github.com/theazz/awless-ro/config" "github.com/theazz/awless-ro/graph" "github.com/theazz/awless-ro/logger" @@ -244,6 +245,13 @@ func syncStats(serviceName string, g cloud.GraphAPI) syncedService { continue } + // A sync keeps a count per bucket rather than the objects, unless told to keep + // them, so the objects in the graph would count none of them. + if rt == cloud.S3Object { + stats.Resources[rt] = objectsCountedOnBuckets(g) + continue + } + res, err := g.Find(cloud.NewQuery(rt)) if err != nil { continue @@ -253,6 +261,19 @@ func syncStats(serviceName string, g cloud.GraphAPI) syncedService { return stats } +func objectsCountedOnBuckets(g cloud.GraphAPI) int { + buckets, err := g.Find(cloud.NewQuery(cloud.Bucket)) + if err != nil { + return 0 + } + var n int + for _, b := range buckets { + count, _ := b.Properties()[properties.ObjectCount].(int) + n += count + } + return n +} + // syncDisabledFor reports whether a resource type was skipped by configuration, and // under which key, so that the message can name the setting to change. // diff --git a/commands/testdata/config.golden b/commands/testdata/config.golden index 35f79e2a..bb00448f 100644 --- a/commands/testdata/config.golden +++ b/commands/testdata/config.golden @@ -12,6 +12,7 @@ "aws.notification.sync": null, "aws.profile": "default", "aws.region": "eu-west-1", + "aws.storage.s3object.keep": null, "aws.storage.s3object.sync": false, "aws.storage.sync": null, "region": "us-east-1" diff --git a/config/config.go b/config/config.go index e73e033d..fd8cb918 100644 --- a/config/config.go +++ b/config/config.go @@ -45,6 +45,10 @@ var configDefinitions = map[string]*Definition{ // and why, since the cost is what makes it off: a call per bucket, a call per // hosted zone, a call per metric. "aws.storage.s3object.sync": {help: "Sync S3 objects: off by default, one API call per bucket", defaultValue: "false", parseParamFn: parseBool}, + // What the object sync keeps by default is a count and a size per bucket, which is + // all bucket_sizer reads. Keeping every object makes memory grow with the number + // of objects in the account. + "aws.storage.s3object.keep": {help: "Keep each S3 object in the local graph, not just per-bucket totals: off by default, memory grows with the object count", defaultValue: "false", parseParamFn: parseBool}, "aws.dns.sync": {help: "Enable/disable sync of DNS service (when empty: true)", defaultValue: "true", parseParamFn: parseBool}, "aws.dns.record.sync": {help: "Sync DNS records: off by default, one API call per hosted zone", defaultValue: "false", parseParamFn: parseBool}, "aws.notification.sync": {help: "Enable/disable sync of SNS service (when empty: true)", defaultValue: "true", parseParamFn: parseBool}, diff --git a/gen/aws/properties_definitions.go b/gen/aws/properties_definitions.go index 85db774a..f6bae820 100644 --- a/gen/aws/properties_definitions.go +++ b/gen/aws/properties_definitions.go @@ -144,6 +144,7 @@ var PropertiesDefinitions = []property{ {AwlessLabel: "NewInstancesProtected", RDFLabel: fmt.Sprintf("%s:newInstancesProtected", rdf.CloudNS), RDFType: rdf.RdfProperty, RdfsDefinedBy: rdf.RdfsLiteral, RdfsDataType: rdf.XsdBoolean}, {AwlessLabel: "Notifications", RDFLabel: fmt.Sprintf("%s:notifications", rdf.CloudNS), RDFType: rdf.RdfProperty, RdfsDefinedBy: rdf.RdfsList, RdfsDataType: rdf.RdfsClass}, {AwlessLabel: "OKActions", RDFLabel: fmt.Sprintf("%s:okActions", rdf.CloudNS), RDFType: rdf.RdfProperty, RdfsDefinedBy: rdf.RdfsList, RdfsDataType: rdf.XsdString}, + {AwlessLabel: "ObjectCount", RDFLabel: fmt.Sprintf("%s:objectCount", rdf.CloudNS), RDFType: rdf.RdfProperty, RdfsDefinedBy: rdf.RdfsLiteral, RdfsDataType: rdf.XsdInt}, {AwlessLabel: "OptionGroups", RDFLabel: fmt.Sprintf("%s:optionGroups", rdf.CloudNS), RDFType: rdf.RdfProperty, RdfsDefinedBy: rdf.RdfsList, RdfsDataType: rdf.XsdString}, {AwlessLabel: "Origins", RDFLabel: fmt.Sprintf("%s:origins", rdf.CloudNS), RDFType: rdf.RdfProperty, RdfsDefinedBy: rdf.RdfsList, RdfsDataType: rdf.DistributionOrigin}, {AwlessLabel: "OutboundRules", RDFLabel: fmt.Sprintf("%s:outboundRules", rdf.NetNS), RDFType: rdf.RdfProperty, RdfsDefinedBy: rdf.RdfsList, RdfsDataType: rdf.NetFirewallRule}, diff --git a/inspect/inspectors/bucket_sizer.go b/inspect/inspectors/bucket_sizer.go index 7076c8d6..404db51d 100644 --- a/inspect/inspectors/bucket_sizer.go +++ b/inspect/inspectors/bucket_sizer.go @@ -30,13 +30,13 @@ type BucketSizer struct { total int buckets map[string]*bucket - // objectsInGraph separates "every bucket is empty" from "no objects were ever - // fetched", which are the same zero in the total and very different answers. - objectsInGraph int + // unsized counts the buckets with no count on them, whose listing failed or + // never ran. Reported, so that the total is not read as the account's. + unsized int - // unattributed counts objects carrying no bucket name. Reported rather than - // dropped, so the total can be reconciled with the rows. - unattributed int + // objectsInGraph is only looked at when no bucket was sized: objects with no + // totals are a graph synced by an earlier version, with object sync already on. + objectsInGraph int } type bucket struct { @@ -47,35 +47,38 @@ func (*BucketSizer) Name() string { return "bucket_sizer" } +// Inspect reads the totals a sync keeps on each bucket: the number of objects and +// their size in bytes. It used to add up the objects themselves, which meant keeping +// every object of the account in the graph. A bucket with no count was never sized +// and gets no row rather than a zero. func (i *BucketSizer) Inspect(g cloud.GraphAPI) error { i.buckets = make(map[string]*bucket) - objects, err := g.Find(cloud.NewQuery(cloud.S3Object)) + buckets, err := g.Find(cloud.NewQuery(cloud.Bucket)) if err != nil { return err } - i.objectsInGraph = len(objects) - - for _, obj := range objects { - // Read rather than asserted. These were unchecked type assertions, so an - // object whose Size did not arrive as an int, or that carried no Bucket, took - // the process down — and an inspector exists to report on whatever the - // account happens to contain. - name, ok := obj.Properties()[properties.Bucket].(string) - if !ok || name == "" { - i.unattributed++ + + for _, b := range buckets { + // Read rather than asserted: an inspector reports on whatever the graph + // happens to hold. + count, ok := b.Properties()[properties.ObjectCount].(int) + if !ok { + i.unsized++ continue } - size, _ := obj.Properties()[properties.Size].(int) + size, _ := b.Properties()[properties.Size].(int) i.total = i.total + size - b := i.buckets[name] - if b == nil { - b = new(bucket) - i.buckets[name] = b + i.buckets[b.Id()] = &bucket{objects: count, size: size} + } + + if len(i.buckets) == 0 { + objects, err := g.Find(cloud.NewQuery(cloud.S3Object)) + if err != nil { + return err } - b.size = b.size + size - b.objects = b.objects + 1 + i.objectsInGraph = len(objects) } return nil @@ -92,30 +95,35 @@ type BucketSize struct { // not repeated as a row of its own. func (i *BucketSizer) Rows(notes io.Writer) interface{} { rows := []BucketSize{} - if i.objectsInGraph == 0 { + if len(i.buckets) == 0 { i.printNothingFetched(notes) return rows } for _, name := range i.sortedNames() { rows = append(rows, BucketSize{Bucket: name, Objects: i.buckets[name].objects, Bytes: i.buckets[name].size}) } - i.printUnattributed(notes) + i.printUnsized(notes) return rows } -// printNothingFetched says why there is nothing to size. No objects at all is the -// state a fresh install is in, because object sync is off by default: one API call +// printNothingFetched says why there is nothing to size. No bucket sized at all is +// the state a fresh install is in, because object sync is off by default: one API call // per bucket is too much to spend unasked. Printing a table whose only row read // "0.000000 Gb" answered a question about the account that had not been looked into. func (i *BucketSizer) printNothingFetched(w io.Writer) { - fmt.Fprintln(w, "No S3 objects in the local graph, so there is nothing to size.") + if i.objectsInGraph > 0 { + fmt.Fprintln(w, "The local graph holds S3 objects but no bucket sizes: it was synced by an earlier version. Sync it again with:") + fmt.Fprintln(w, "\tawless-ro sync --storage") + return + } + fmt.Fprintln(w, "No bucket in the local graph has been sized, so there is nothing to report.") fmt.Fprintln(w, "Object sync is off by default because it costs one API call per bucket. Turn it on with:") fmt.Fprintln(w, "\tawless-ro config set aws.storage.s3object.sync true") } -func (i *BucketSizer) printUnattributed(w io.Writer) { - if i.unattributed > 0 { - fmt.Fprintf(w, "\n%d object(s) named no bucket and are not counted above.\n", i.unattributed) +func (i *BucketSizer) printUnsized(w io.Writer) { + if i.unsized > 0 { + fmt.Fprintf(w, "\n%d bucket(s) have no size, because their listing failed or never ran; the total leaves them out.\n", i.unsized) } } @@ -132,7 +140,7 @@ func (i *BucketSizer) sortedNames() []string { } func (i *BucketSizer) Print(w io.Writer) { - if i.objectsInGraph == 0 { + if len(i.buckets) == 0 { i.printNothingFetched(w) return } @@ -151,5 +159,5 @@ func (i *BucketSizer) Print(w io.Writer) { tabw.Flush() - i.printUnattributed(w) + i.printUnsized(w) } diff --git a/inspect/inspectors/bucket_sizer_test.go b/inspect/inspectors/bucket_sizer_test.go index aa9b54b5..a53afff4 100644 --- a/inspect/inspectors/bucket_sizer_test.go +++ b/inspect/inspectors/bucket_sizer_test.go @@ -33,12 +33,31 @@ func TestBucketSizerSaysNothingWasFetchedRatherThanZero(t *testing.T) { } } -func TestBucketSizerTotalsBySizeAndBucket(t *testing.T) { +// Buckets synced with object sync off carry no totals: that is "never looked", not +// "empty". +func TestBucketSizerSaysNothingWasFetchedForBucketsWithoutTotals(t *testing.T) { + g := graph.NewGraph() + g.AddResource(resourcetest.Bucket("logs").Build()) + + var out bytes.Buffer + sizer := &BucketSizer{} + if err := sizer.Inspect(g); err != nil { + t.Fatal(err) + } + sizer.Print(&out) + if got := out.String(); !strings.Contains(got, "aws.storage.s3object.sync") || strings.Contains(got, "TOTAL") { + t.Errorf("a bucket that was never sized is reported as sized:\n%s", got) + } +} + +// The sizes come from the totals a sync keeps on each bucket. Objects kept in the +// graph as well (aws.storage.s3object.keep) are not counted a second time. +func TestBucketSizerReadsTheTotalsOnEachBucket(t *testing.T) { g := graph.NewGraph() g.AddResource( - object("obj_1", "logs", 1_500_000_000), - object("obj_2", "logs", 500_000_000), - object("obj_3", "backups", 3_000_000_000), + sized("logs", 2, 2_000_000_000), + sized("backups", 1, 3_000_000_000), + resourcetest.S3Object("obj_1").Prop(properties.Bucket, "logs").Prop(properties.Size, 1_500_000_000).Build(), ) var out bytes.Buffer @@ -69,9 +88,9 @@ func TestBucketSizerTotalsBySizeAndBucket(t *testing.T) { func TestBucketSizerOutputIsOrdered(t *testing.T) { g := graph.NewGraph() g.AddResource( - object("a", "zebra", 1), - object("b", "alpha", 1), - object("c", "middle", 1), + sized("zebra", 1, 1), + sized("alpha", 1, 1), + sized("middle", 1, 1), ) var first string @@ -98,20 +117,18 @@ func TestBucketSizerOutputIsOrdered(t *testing.T) { } } -// The property reads used to be unchecked type assertions, so an object with no -// Bucket, or a Size that did not arrive as an int, took the process down. An inspector -// reports on whatever the account happens to contain. +// The property reads are checked, not asserted: an inspector reports on whatever the +// graph happens to hold, and an unchecked assertion took the process down. func TestBucketSizerSurvivesOddProperties(t *testing.T) { g := graph.NewGraph() g.AddResource( - object("good", "logs", 1_000_000_000), - // No bucket: counted apart rather than attributed or crashed on. - resourcetest.S3Object("orphan").Prop(properties.Size, 42).Build(), - // A size that is not an int. - resourcetest.S3Object("odd").Prop(properties.Bucket, "logs"). - Prop(properties.Size, "not a number").Build(), - // No size at all. - resourcetest.S3Object("sizeless").Prop(properties.Bucket, "logs").Build(), + sized("logs", 3, 1_000_000_000), + // A size that is not an int: the objects count, the size does not. + resourcetest.Bucket("odd").Prop(properties.ObjectCount, 4).Prop(properties.Size, "not a number").Build(), + // A count that is not an int: not sized. + resourcetest.Bucket("odder").Prop(properties.ObjectCount, "many").Build(), + // No totals at all: never sized. + resourcetest.Bucket("unsized").Build(), ) var out bytes.Buffer @@ -120,27 +137,67 @@ func TestBucketSizerSurvivesOddProperties(t *testing.T) { t.Fatal(err) } sizer.Print(&out) - got := out.String() - // The one good object still counts, and the sizeless ones count as objects - // without adding to the size. - if want := "1.000000 Gb"; !strings.Contains(got, want) { - t.Errorf("output is missing %q:\n%s", want, got) + if sizer.total != 1_000_000_000 { + t.Errorf("total is %d bytes, want 1000000000:\n%s", sizer.total, out.String()) } - if n := sizer.buckets["logs"].objects; n != 3 { - t.Errorf("logs holds %d objects, want 3", n) + if odd := sizer.buckets["odd"]; odd == nil || odd.objects != 4 { + t.Errorf("odd should hold 4 objects, got %+v", odd) } - if sizer.unattributed != 1 { - t.Errorf("%d objects counted as naming no bucket, want 1", sizer.unattributed) + for _, name := range []string{"odder", "unsized"} { + if _, ok := sizer.buckets[name]; ok { + t.Errorf("%s has no count, yet has a row", name) + } } - if !strings.Contains(got, "named no bucket") { - t.Errorf("the object naming no bucket is not reported:\n%s", got) +} + +// A bucket whose listing failed, or never ran because an earlier one failed, has no +// count. Left out silently, the total would read as the account's. +func TestBucketSizerSaysWhichBucketsWereNotSized(t *testing.T) { + g := graph.NewGraph() + g.AddResource( + sized("logs", 2, 2_000_000_000), + resourcetest.Bucket("denied").Build(), + ) + + sizer := &BucketSizer{} + if err := sizer.Inspect(g); err != nil { + t.Fatal(err) + } + var out, notes bytes.Buffer + sizer.Print(&out) + sizer.Rows(¬es) + + for name, got := range map[string]string{"text": out.String(), "notes": notes.String()} { + if !strings.Contains(got, "1 bucket(s) have no size") { + t.Errorf("%s: the unsized bucket is not reported:\n%s", name, got) + } + } +} + +// A graph synced by an earlier version holds the objects and no totals. Object sync is +// on there, so "turn it on" would be the wrong advice. +func TestBucketSizerOnAGraphFromAnEarlierVersion(t *testing.T) { + g := graph.NewGraph() + g.AddResource( + resourcetest.Bucket("logs").Build(), + resourcetest.S3Object("obj_1").Prop(properties.Bucket, "logs").Prop(properties.Size, 1).Build(), + ) + + sizer := &BucketSizer{} + if err := sizer.Inspect(g); err != nil { + t.Fatal(err) + } + var out bytes.Buffer + sizer.Print(&out) + if got := out.String(); !strings.Contains(got, "sync --storage") || strings.Contains(got, "config set aws.storage.s3object.sync") { + t.Errorf("want advice to sync again, got:\n%s", got) } } -func object(id, bucket string, size int) *graph.Resource { - return resourcetest.S3Object(id). - Prop(properties.Bucket, bucket). +func sized(name string, objects, size int) *graph.Resource { + return resourcetest.Bucket(name). + Prop(properties.ObjectCount, objects). Prop(properties.Size, size). Build() } diff --git a/inspect/inspectors/json_test.go b/inspect/inspectors/json_test.go index 536e752e..8b7241c1 100644 --- a/inspect/inspectors/json_test.go +++ b/inspect/inspectors/json_test.go @@ -27,10 +27,9 @@ func rowsJSON(t *testing.T, rows interface{}) []byte { func TestBucketSizerRowsGolden(t *testing.T) { g := graph.NewGraph() g.AddResource( - object("a", "zebra", 1_500_000_000), - object("b", "alpha", 2), - object("c", "alpha", 3), - resourcetest.S3Object("orphan").Prop(properties.Size, 42).Build(), + sized("zebra", 1, 1_500_000_000), + sized("alpha", 2, 5), + resourcetest.Bucket("unsized").Build(), ) sizer := &BucketSizer{} if err := sizer.Inspect(g); err != nil { @@ -38,9 +37,6 @@ func TestBucketSizerRowsGolden(t *testing.T) { } var notes bytes.Buffer golden.Assert(t, "bucket_sizer", rowsJSON(t, sizer.Rows(¬es))) - if !strings.Contains(notes.String(), "named no bucket") { - t.Errorf("the object naming no bucket is not reported on the side:\n%s", notes.String()) - } } // Nothing fetched is an empty array, with the reason on the side rather than in the diff --git a/main_test.go b/main_test.go index b600ad08..e878ac62 100644 --- a/main_test.go +++ b/main_test.go @@ -539,6 +539,68 @@ func TestLocalListServiceNotSyncedInRegion(t *testing.T) { } } +// A sync keeps per-bucket totals, not the objects, so `list s3objects --local` has +// none to show even when the buckets hold thousands. It says why and where to look +// instead, rather than answering an empty list. +func TestLocalListObjectsSaysWhyItIsEmpty(t *testing.T) { + bin := build(t) + bucket := " \"logs\" .\n .\n" + home := graphHome(t, map[string]string{"eu-west-1/storage": bucket}) + + listObjects := func() (string, string) { + t.Helper() + stdout, stderr, exit := runLocal(t, bin, home, "list", "s3objects", "--local", "--format", "json") + if exit != 0 { + t.Errorf("exit = %d, want 0", exit) + } + if strings.TrimSpace(stdout) != "[]" { + t.Errorf("stdout = %q, want []", stdout) + } + return stdout, stderr + } + + // Object sync off, the default: that is the setting to name. + _, stderr := listObjects() + if !strings.Contains(stderr, "aws.storage.s3object.sync") { + t.Errorf("object sync is off, and the note does not say so:\n%s", stderr) + } + + // On, without keep: only totals were kept. + if _, _, exit := runLocal(t, bin, home, "config", "set", "aws.storage.s3object.sync", "true"); exit != 0 { + t.Fatalf("config set failed") + } + _, stderr = listObjects() + for _, want := range []string{"bucket_sizer", "aws.storage.s3object.keep"} { + if !strings.Contains(stderr, want) { + t.Errorf("the note does not mention %s:\n%s", want, stderr) + } + } + + // Buckets list without it. + _, stderr, _ = runLocal(t, bin, home, "list", "buckets", "--local", "--format", "json") + if strings.Contains(stderr, "aws.storage.s3object") { + t.Errorf("listing buckets printed the objects note: %s", stderr) + } +} + +// Objects in the graph, from keep or an earlier version: they list, with no note +// explaining an empty answer. +func TestLocalListObjectsKeptHasNoNote(t *testing.T) { + bin := build(t) + home := graphHome(t, map[string]string{ + "eu-west-1/storage": " \"logs\" .\n .\n" + + " \"obj-1\" .\n .\n", + }) + + stdout, stderr, _ := runLocal(t, bin, home, "list", "s3objects", "--local", "--format", "json") + if !strings.Contains(stdout, "obj-1") { + t.Errorf("the object does not list: %s", stdout) + } + if strings.Contains(stderr, "aws.storage.s3object") { + t.Errorf("a note explains an empty list that is not empty: %s", stderr) + } +} + // Part 2 end to end: a 9 MiB document literal — far past the old scanner ceiling — loads // and lists. `list` does not render the document value itself, so this is the end-to-end // proof that the decoder now reads an arbitrarily large single line.