Unmarshaling into an interface{} instead of structs

skyzyx commented 4 years ago

I'm trying to work with Amazon S3 Bucket Policies, which are polymorphic enough that not even the AWS SDK team has attempted to model the rules for them with Go structs. So I'm trying to decode the S3 Bucket Policy JSON into an interface{}.

Here's some rough code, extracted from my project.

type S3BucketPolicy interface{}

var bucketPolicy S3BucketPolicy

request := client.GetBucketPolicyRequest(&s3.GetBucketPolicyInput{
    Bucket: &bucketname,
})

// Errors are acceptable
response, _ := request.Send(ctx)

if response != nil && response.Policy != nil && *response.Policy != emptyString {
    if err := json.Unmarshal([]byte(*response.Policy), &bucketPolicy); err != nil {
        exitErrorf(err)
    }
}

Using encoding/json, this works just fine. However, after dropping-in github.com/segmentio/encoding/json as a replacement, I receive the following error:

Error: json: cannot unmarshal {"Version":"2012-10-17","Statement":[{"Sid":"AWSCloudTrailWrite","Effect":"Allow","Principal":{"Service":"cloudtrail.amazonaws.com"},"Action":"s3:PutObject","Resource":"arn:aws:s3:::BUCKET/AWSLogs/ACCOUNTID/*","Condition":{"StringEquals":{"s3:x-amz-acl":"bucket-owner-full-control"}}},{"Sid":"AWSLogDeliveryWrite","Effect":"Allow","Principal":{"Service":"delivery.logs.amazonaws.com"},"Action":"s3:PutObject","Resource":"arn:aws:s3:::BUCKET/AWSLogs/ACCOUNTID/*","Condition":{"StringEquals":{"s3:x-amz-acl":"bucket-owner-full-control"}}},{"Sid":"AWSLogDeliveryAclCheck","Effect":"Allow","Principal":{"Service":"delivery.logs.amazonaws.com"},"Action":"s3:GetBucketAcl","Resource":"arn:aws:s3:::BUCKET"},{"Sid":"AWSELBWrite","Effect":"Allow","Principal":{"AWS":"arn:aws:iam::ACCOUNTID:root"},"Action":"s3:PutObject","Resource":"arn:aws:s3:::BUCKET/*"},{"Sid":"AWSAclCheck","Effect":"Allow","Principal":{"Service":"cloudtrail.amazonaws.com","AWS":"arn:aws:iam::ACCOUNTID:user/logs"},"Action":"s3:GetBucketAcl","Resource":"arn:aws:s3:::BUCKET"},{"Sid":"AWSRedshiftWrite","Effect":"Allow","Principal":{"AWS":"arn:aws:iam::ACCOUNTID:user/logs"},"Action":"s3:PutObject","Resource":"arn:aws:s3:::BUCKET/*"}]} into Go value of type main.S3BucketPolicy

Here is the JSON (pretty printed), which is valid:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "AWSCloudTrailWrite",
      "Effect": "Allow",
      "Principal": {
        "Service": "cloudtrail.amazonaws.com"
      },
      "Action": "s3:PutObject",
      "Resource": "arn:aws:s3:::BUCKET/AWSLogs/ACCOUNTID/*",
      "Condition": {
        "StringEquals": {
          "s3:x-amz-acl": "bucket-owner-full-control"
        }
      }
    },
    {
      "Sid": "AWSLogDeliveryWrite",
      "Effect": "Allow",
      "Principal": {
        "Service": "delivery.logs.amazonaws.com"
      },
      "Action": "s3:PutObject",
      "Resource": "arn:aws:s3:::BUCKET/AWSLogs/ACCOUNTID/*",
      "Condition": {
        "StringEquals": {
          "s3:x-amz-acl": "bucket-owner-full-control"
        }
      }
    },
    {
      "Sid": "AWSLogDeliveryAclCheck",
      "Effect": "Allow",
      "Principal": {
        "Service": "delivery.logs.amazonaws.com"
      },
      "Action": "s3:GetBucketAcl",
      "Resource": "arn:aws:s3:::BUCKET"
    },
    {
      "Sid": "AWSELBWrite",
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::ACCOUNTID:root"
      },
      "Action": "s3:PutObject",
      "Resource": "arn:aws:s3:::BUCKET/*"
    },
    {
      "Sid": "AWSAclCheck",
      "Effect": "Allow",
      "Principal": {
        "Service": "cloudtrail.amazonaws.com",
        "AWS": "arn:aws:iam::ACCOUNTID:user/logs"
      },
      "Action": "s3:GetBucketAcl",
      "Resource": "arn:aws:s3:::BUCKET"
    },
    {
      "Sid": "AWSRedshiftWrite",
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::ACCOUNTID:user/logs"
      },
      "Action": "s3:PutObject",
      "Resource": "arn:aws:s3:::BUCKET/*"
    }
  ]
}

Is this use-case supported?

achille-roussel commented 4 years ago

This use case should be supported, tanks for the detailed bug report @skyzyx ! I'm looking into it.

achille-roussel commented 4 years ago

It looks like the issue is due to using an empty interface type other than interface{} (S3BucketPolicy in your case). If I change the destination variable to be of type interface{} it works fine.

I'll submit a fix, it shouldn't be too hard to track down 👍

achille-roussel commented 4 years ago

@skyzyx would you be able to verify if the fix we merged and released in v0.1.9 addressed the original issue you reported?

skyzyx commented 4 years ago

Sure. Give me a few.

skyzyx commented 4 years ago

I replaced my imports from encoding/json to github.com/segmentio/encoding/json.
Ran go mod tidy. The contents of go.mod include github.com/segmentio/encoding v0.1.9.
Code compiled just fine.
At runtime, when I fetch results from the S3 API, I receive the same error message.
```
Error: json: cannot unmarshal […] into Go value of type main.S3BucketPolicy
```

Do I need to change which type I unmarshal into?

achille-roussel commented 4 years ago

I just ran this program (taken from your example):

package main

import (
    "fmt"
    "log"

    "github.com/segmentio/encoding/json"
)

type S3BucketPolicy interface{}

var policyJSON = []byte(`
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "AWSCloudTrailWrite",
      "Effect": "Allow",
      "Principal": {
        "Service": "cloudtrail.amazonaws.com"
      },
      "Action": "s3:PutObject",
      "Resource": "arn:aws:s3:::BUCKET/AWSLogs/ACCOUNTID/*",
      "Condition": {
        "StringEquals": {
          "s3:x-amz-acl": "bucket-owner-full-control"
        }
      }
    },
    {
      "Sid": "AWSLogDeliveryWrite",
      "Effect": "Allow",
      "Principal": {
        "Service": "delivery.logs.amazonaws.com"
      },
      "Action": "s3:PutObject",
      "Resource": "arn:aws:s3:::BUCKET/AWSLogs/ACCOUNTID/*",
      "Condition": {
        "StringEquals": {
          "s3:x-amz-acl": "bucket-owner-full-control"
        }
      }
    },
    {
      "Sid": "AWSLogDeliveryAclCheck",
      "Effect": "Allow",
      "Principal": {
        "Service": "delivery.logs.amazonaws.com"
      },
      "Action": "s3:GetBucketAcl",
      "Resource": "arn:aws:s3:::BUCKET"
    },
    {
      "Sid": "AWSELBWrite",
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::ACCOUNTID:root"
      },
      "Action": "s3:PutObject",
      "Resource": "arn:aws:s3:::BUCKET/*"
    },
    {
      "Sid": "AWSAclCheck",
      "Effect": "Allow",
      "Principal": {
        "Service": "cloudtrail.amazonaws.com",
        "AWS": "arn:aws:iam::ACCOUNTID:user/logs"
      },
      "Action": "s3:GetBucketAcl",
      "Resource": "arn:aws:s3:::BUCKET"
    },
    {
      "Sid": "AWSRedshiftWrite",
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::ACCOUNTID:user/logs"
      },
      "Action": "s3:PutObject",
      "Resource": "arn:aws:s3:::BUCKET/*"
    }
  ]
}
`)

func main() {
    var bucketPolicy S3BucketPolicy

    if err := json.Unmarshal(policyJSON, &bucketPolicy); err != nil {
        log.Fatal(err)
    }

    fmt.Println(bucketPolicy)
}

And I get this output, no errors:

$ go run ./main.go
go: finding github.com/segmentio/encoding/json latest
go: finding github.com/segmentio/encoding v0.1.9
go: downloading github.com/segmentio/encoding v0.1.9
go: extracting github.com/segmentio/encoding v0.1.9
map[Statement:[map[Action:s3:PutObject Condition:map[StringEquals:map[s3:x-amz-acl:bucket-owner-full-control]] Effect:Allow Principal:map[Service:cloudtrail.amazonaws.com] Resource:arn:aws:s3:::BUCKET/AWSLogs/ACCOUNTID/* Sid:AWSCloudTrailWrite] map[Action:s3:PutObject Condition:map[StringEquals:map[s3:x-amz-acl:bucket-owner-full-control]] Effect:Allow Principal:map[Service:delivery.logs.amazonaws.com] Resource:arn:aws:s3:::BUCKET/AWSLogs/ACCOUNTID/* Sid:AWSLogDeliveryWrite] map[Action:s3:GetBucketAcl Effect:Allow Principal:map[Service:delivery.logs.amazonaws.com] Resource:arn:aws:s3:::BUCKET Sid:AWSLogDeliveryAclCheck] map[Action:s3:PutObject Effect:Allow Principal:map[AWS:arn:aws:iam::ACCOUNTID:root] Resource:arn:aws:s3:::BUCKET/* Sid:AWSELBWrite] map[Action:s3:GetBucketAcl Effect:Allow Principal:map[AWS:arn:aws:iam::ACCOUNTID:user/logs Service:cloudtrail.amazonaws.com] Resource:arn:aws:s3:::BUCKET Sid:AWSAclCheck] map[Action:s3:PutObject Effect:Allow Principal:map[AWS:arn:aws:iam::ACCOUNTID:user/logs] Resource:arn:aws:s3:::BUCKET/* Sid:AWSRedshiftWrite]] Version:2012-10-17]

Can you share more about the test program you use so I can try to understand why it's failing on your side?

skyzyx commented 4 years ago

Hmmm. Let me try a little refactoring to see if I missed something. Keeping the ball…

skyzyx commented 4 years ago

So, still having some trouble. I've dug through my code, and everything I have appears to match the example you posted. I'm kicking off the following function in a Go channel:

func getBucketPolicy(bucketname string, client s3.Client, c chan S3BucketPolicy) {
    defer wgS3BucketPolicy.Done()

    request := client.GetBucketPolicyRequest(&s3.GetBucketPolicyInput{
        Bucket: &bucketname,
    })

    // Errors are acceptable
    fmt.Fprint(os.Stderr, ".")
    response, _ := request.Send(ctx)

    var bucketPolicy S3BucketPolicy

    if response != nil && response.Policy != nil && *response.Policy != emptyString {
        if err := json.Unmarshal([]byte(*response.Policy), &bucketPolicy); err != nil {
            exitErrorf(err)
        }
    }

    c <- bucketPolicy
}

Replacing var bucketPolicy S3BucketPolicy with var bucketPolicy interface{} on line 12 of this function (and no other code changes) compiles and runs just fine without errors.

wgS3BucketPolicy is defined at the top-level var() block.
So is emptyString
exitErrorf() is a generic function which outputs stuff to stderr.

I can't think of anything else that's different between what I originally posted, and this function.

Changing var bucketPolicy S3BucketPolicy → var bucketPolicy interface{} appears to unblock me and give me the performance boost I'm looking for. But there appears to be something else in my code that is causing this library to not decode the way it says on the tin. :/

Anything else I can provide without giving away all my source code? :) If it's helpful, this function is being kicked-off from this function:

func getBucket(bucketname string, owner *s3.Owner, createdAt *time.Time, config *aws.Config, c chan S3Bucket) {
    defer wgS3Bucket.Done()

    defaultClient := s3.New(*config)

    region, err := s3manager.GetBucketRegionWithClient(ctx, defaultClient, bucketname)
    if err != nil {
        exitErrorf(err)
    }

    // Set the correct region for the bucket
    config.Region = region
    client := s3.New(*config)

    bucket := S3Bucket{
        CreatedAt: *createdAt,
        Name:      bucketname,
        Region:    region,
        Owner: S3Owner{
            DisplayName: *owner.DisplayName,
            ID:          *owner.ID,
        },
    }

    policyChannel := make(chan S3BucketPolicy)

    wgS3BucketPolicy.Add(1)
    go getBucketPolicy(bucketname, *client, policyChannel)

    // Wait...
    go func() {
        wgS3BucketPolicy.Wait()
        close(policyChannel)
    }()

    // Handle results
    for policy := range policyChannel {
        bucket.Policy = policy
    }

    c <- bucket
}

…which, to me, doesn't look like anything controversial.

I appreciate how engaged and responsive you've been so far. Thank you. 👍

achille-roussel commented 4 years ago

Thanks for sharing more details. I also don't see what's different in the two examples.

Are you able to confirm that you are using v0.1.9? https://github.com/golang/go/wiki/Modules#version-selection says you can run go list -m all to show the list of all dependencies used in your build
If you run the example program I shared, do you get the expected behavioir?
Which version of Go are you building your program with?

skyzyx commented 4 years ago

Haven't forgotten about this. The ball is still in my court.

segmentio / encoding

Unmarshaling into an interface{} instead of structs #26