Skip to content

Commit f9dfdff

Browse files
rchinchaclaude
rchincha
and
claude
authored
fix(storage): check table existence before CreateTable in dynamodb cache (#4260)
* fix(storage): check table existence before CreateTable in dynamodb cache The storage-cache DynamoDB driver's NewTable always called CreateTable on startup and only tolerated the error when it happened to contain "Table already exists" (a ResourceInUseException, returned when the caller has CreateTable permission and the table exists). When the caller instead lacks dynamodb:CreateTable entirely -- even with full read/write/describe access to an existing table -- IAM denies the API call before DynamoDB ever checks the table's existence, so the driver got an AccessDeniedException it didn't recognize and zot failed to start. Add tableExists (via DescribeTable, which the caller does have permission for) and only call CreateTable when the table is genuinely missing, mirroring the pattern already used by the metaDB DynamoDB driver in pkg/meta/dynamodb/dynamodb.go. Use errors.As against the typed SDK exceptions instead of matching on err.Error() substrings. Added an internal regression test that intercepts only the CreateTable call against a real (localstack) DynamoDB endpoint to simulate the IAM policy from the report, proving it fails on the old code and passes with this fix. Fixes #4259 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(storage): trim comment wording in dynamodb cache per review Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * test(storage): align mocked DynamoDB error shape with existing stubs Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(storage): clarify required IAM permissions in tableExists comment Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(storage): confirm table exists before treating ResourceInUseException as benign ResourceInUseException from CreateTable can also occur while a table is being deleted, not just when a concurrent instance created it. Re-check via DescribeTable so a mid-deletion race doesn't get silently ignored. Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(storage): fall back to CreateTable when DescribeTable is denied NewTable previously failed outright if DescribeTable was denied, even when the caller had dynamodb:CreateTable permission but not dynamodb:DescribeTable. Treat AccessDeniedException from DescribeTable as "existence unknown" and fall back to CreateTable, still tolerating ResourceInUseException. When confirming a benign ResourceInUseException race, also tolerate AccessDeniedException on the confirming DescribeTable call, since a CreateTable-only caller can't perform that check either. Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * test(storage): cover NewTable fallback when DescribeTable is denied Adds a deny-DescribeTable transport mirroring the existing deny-CreateTable one, and exercises both the "table doesn't exist yet" and "table already exists" fallback paths, locking in the fix from 6ff6589. Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(storage): avoid shadowing err in NewTable's CreateTable call Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * test(storage): verify CreateTable actually ran in DescribeTable-denied test Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> --------- Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 25097ff commit f9dfdff

2 files changed

Lines changed: 268 additions & 21 deletions

File tree

‎pkg/storage/cache/dynamodb.go‎

Lines changed: 81 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,15 @@ package cache
22

33
import (
44
"context"
5+
"errors"
56
"slices"
6-
"strings"
77

88
"github.com/aws/aws-sdk-go-v2/aws"
99
"github.com/aws/aws-sdk-go-v2/config"
1010
"github.com/aws/aws-sdk-go-v2/feature/dynamodb/attributevalue"
1111
"github.com/aws/aws-sdk-go-v2/service/dynamodb"
1212
"github.com/aws/aws-sdk-go-v2/service/dynamodb/types"
13+
"github.com/aws/smithy-go"
1314
godigest "github.com/opencontainers/go-digest"
1415

1516
zerr "zotregistry.dev/zot/v2/errors"
@@ -32,29 +33,88 @@ type Blob struct {
3233
OriginalBlobPath string `dynamodbav:"OriginalBlobPath,string"`
3334
}
3435

36+
// tableExists checks for the table via DescribeTable rather than attempting CreateTable
37+
// and inspecting the error. This allows callers that have dynamodb:DescribeTable (and read/write item
38+
// permissions) on an existing table, but not dynamodb:CreateTable, to work.
39+
func (d *DynamoDBDriver) tableExists(tableName string) (bool, error) {
40+
_, err := d.client.DescribeTable(context.TODO(), &dynamodb.DescribeTableInput{
41+
TableName: aws.String(tableName),
42+
})
43+
if err == nil {
44+
return true, nil
45+
}
46+
47+
var notFoundErr *types.ResourceNotFoundException
48+
if errors.As(err, &notFoundErr) {
49+
return false, nil
50+
}
51+
52+
return false, err
53+
}
54+
55+
// isAccessDenied reports whether err is an AWS AccessDeniedException. This isn't a
56+
// modeled DynamoDB exception type, so it's matched via the generic smithy API error.
57+
func isAccessDenied(err error) bool {
58+
var apiErr smithy.APIError
59+
60+
return errors.As(err, &apiErr) && apiErr.ErrorCode() == "AccessDeniedException"
61+
}
62+
3563
func (d *DynamoDBDriver) NewTable(tableName string) error {
36-
//nolint:mnd
37-
_, err := d.client.CreateTable(context.TODO(), &dynamodb.CreateTableInput{
38-
TableName: &tableName,
39-
AttributeDefinitions: []types.AttributeDefinition{
40-
{
41-
AttributeName: aws.String("Digest"),
42-
AttributeType: types.ScalarAttributeTypeS,
64+
exists, err := d.tableExists(tableName)
65+
if err != nil {
66+
// DescribeTable may be denied for callers that only have dynamodb:CreateTable (and
67+
// not dynamodb:DescribeTable); fall back to attempting CreateTable directly below.
68+
if !isAccessDenied(err) {
69+
return err
70+
}
71+
72+
exists = false
73+
}
74+
75+
if !exists {
76+
//nolint:mnd
77+
_, err = d.client.CreateTable(context.TODO(), &dynamodb.CreateTableInput{
78+
TableName: &tableName,
79+
AttributeDefinitions: []types.AttributeDefinition{
80+
{
81+
AttributeName: aws.String("Digest"),
82+
AttributeType: types.ScalarAttributeTypeS,
83+
},
4384
},
44-
},
45-
KeySchema: []types.KeySchemaElement{
46-
{
47-
AttributeName: aws.String("Digest"),
48-
KeyType: types.KeyTypeHash,
85+
KeySchema: []types.KeySchemaElement{
86+
{
87+
AttributeName: aws.String("Digest"),
88+
KeyType: types.KeyTypeHash,
89+
},
4990
},
50-
},
51-
ProvisionedThroughput: &types.ProvisionedThroughput{
52-
ReadCapacityUnits: aws.Int64(10),
53-
WriteCapacityUnits: aws.Int64(5),
54-
},
55-
})
56-
if err != nil && !strings.Contains(err.Error(), "Table already exists") {
57-
return err
91+
ProvisionedThroughput: &types.ProvisionedThroughput{
92+
ReadCapacityUnits: aws.Int64(10),
93+
WriteCapacityUnits: aws.Int64(5),
94+
},
95+
})
96+
97+
// tableExists and CreateTable aren't atomic, so still tolerate a benign race where
98+
// another zot instance created the table between the check above and this call.
99+
// ResourceInUseException can also occur while a table is being deleted, so confirm
100+
// the table actually exists before treating the error as benign. If the caller isn't
101+
// permitted to call DescribeTable either (dynamodb:CreateTable-only IAM policies),
102+
// there's no way to confirm further, so fall back to treating it as benign.
103+
var inUseErr *types.ResourceInUseException
104+
if err != nil {
105+
if !errors.As(err, &inUseErr) {
106+
return err
107+
}
108+
109+
confirmedExists, existsErr := d.tableExists(tableName)
110+
111+
switch {
112+
case existsErr != nil && !isAccessDenied(existsErr):
113+
return existsErr
114+
case existsErr == nil && !confirmedExists:
115+
return err
116+
}
117+
}
58118
}
59119

60120
d.tableName = tableName

‎pkg/storage/cache/dynamodb_internal_test.go‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
package cache
2+
3+
import (
4+
"bytes"
5+
"context"
6+
"io"
7+
"net/http"
8+
"os"
9+
"testing"
10+
11+
"github.com/aws/aws-sdk-go-v2/aws"
12+
awsconfig "github.com/aws/aws-sdk-go-v2/config"
13+
"github.com/aws/aws-sdk-go-v2/service/dynamodb"
14+
. "github.com/smartystreets/goconvey/convey"
15+
16+
"zotregistry.dev/zot/v2/pkg/log"
17+
tskip "zotregistry.dev/zot/v2/pkg/test/skip"
18+
)
19+
20+
// denyCreateTableTransport simulates an IAM policy that allows DescribeTable/GetItem/etc.
21+
// but denies dynamodb:CreateTable, by intercepting only CreateTable calls and returning
22+
// the same AccessDeniedException shape AWS returns; every other action is forwarded to
23+
// the real (localstack) endpoint.
24+
type denyCreateTableTransport struct {
25+
base http.RoundTripper
26+
}
27+
28+
func (t *denyCreateTableTransport) RoundTrip(req *http.Request) (*http.Response, error) {
29+
if req.Header.Get("X-Amz-Target") == "DynamoDB_20120810.CreateTable" {
30+
body := `{"__type":"com.amazon.coral.service#AccessDeniedException",` +
31+
`"message":"User: arn:aws:iam::123456789012:user/zot is not authorized to perform: ` +
32+
`dynamodb:CreateTable on resource: arn:aws:dynamodb:us-east-2:123456789012:table/BlobTable"}`
33+
34+
return &http.Response{
35+
StatusCode: http.StatusBadRequest,
36+
Status: "400 Bad Request",
37+
Header: http.Header{
38+
"Content-Type": []string{"application/x-amz-json-1.0"},
39+
"X-Amzn-Errortype": []string{"AccessDeniedException"},
40+
},
41+
Body: io.NopCloser(bytes.NewBufferString(body)),
42+
Request: req,
43+
}, nil
44+
}
45+
46+
return t.base.RoundTrip(req)
47+
}
48+
49+
func TestNewTableWithoutCreateTablePermission(t *testing.T) {
50+
tskip.SkipDynamo(t)
51+
52+
Convey("NewTable succeeds against a pre-existing table without dynamodb:CreateTable permission", t, func() {
53+
endpoint := os.Getenv("DYNAMODBMOCK_ENDPOINT")
54+
55+
httpClient := &http.Client{Transport: &denyCreateTableTransport{base: http.DefaultTransport}}
56+
57+
cfg, err := awsconfig.LoadDefaultConfig(context.Background(),
58+
awsconfig.WithRegion("us-east-2"),
59+
awsconfig.WithHTTPClient(httpClient),
60+
)
61+
So(err, ShouldBeNil)
62+
63+
client := dynamodb.NewFromConfig(cfg, func(o *dynamodb.Options) {
64+
o.BaseEndpoint = aws.String(endpoint)
65+
})
66+
67+
const tableName = "Issue4259PreExistingTable"
68+
69+
// create the table up front using a plain client (unaffected by the interceptor,
70+
// since it's not a CreateTable call through the deny-wrapped client)
71+
plainCfg, err := awsconfig.LoadDefaultConfig(context.Background(), awsconfig.WithRegion("us-east-2"))
72+
So(err, ShouldBeNil)
73+
74+
plainClient := dynamodb.NewFromConfig(plainCfg, func(o *dynamodb.Options) {
75+
o.BaseEndpoint = aws.String(endpoint)
76+
})
77+
78+
driver := &DynamoDBDriver{client: plainClient, log: log.NewTestLogger()}
79+
So(driver.NewTable(tableName), ShouldBeNil)
80+
81+
// now exercise NewTable again through the client that denies CreateTable: since the
82+
// table already exists, DescribeTable should short-circuit before CreateTable is ever called
83+
deniedDriver := &DynamoDBDriver{client: client, log: log.NewTestLogger()}
84+
So(deniedDriver.NewTable(tableName), ShouldBeNil)
85+
So(deniedDriver.tableName, ShouldEqual, tableName)
86+
})
87+
}
88+
89+
// denyDescribeTableTransport simulates an IAM policy that allows CreateTable/GetItem/etc.
90+
// but denies dynamodb:DescribeTable, by intercepting only DescribeTable calls and returning
91+
// an AccessDeniedException; every other action is forwarded to the real (localstack) endpoint.
92+
type denyDescribeTableTransport struct {
93+
base http.RoundTripper
94+
}
95+
96+
func (t *denyDescribeTableTransport) RoundTrip(req *http.Request) (*http.Response, error) {
97+
if req.Header.Get("X-Amz-Target") == "DynamoDB_20120810.DescribeTable" {
98+
body := `{"__type":"com.amazon.coral.service#AccessDeniedException",` +
99+
`"message":"User: arn:aws:iam::123456789012:user/zot is not authorized to perform: ` +
100+
`dynamodb:DescribeTable on resource: arn:aws:dynamodb:us-east-2:123456789012:table/BlobTable"}`
101+
102+
return &http.Response{
103+
StatusCode: http.StatusBadRequest,
104+
Status: "400 Bad Request",
105+
Header: http.Header{
106+
"Content-Type": []string{"application/x-amz-json-1.0"},
107+
"X-Amzn-Errortype": []string{"AccessDeniedException"},
108+
},
109+
Body: io.NopCloser(bytes.NewBufferString(body)),
110+
Request: req,
111+
}, nil
112+
}
113+
114+
return t.base.RoundTrip(req)
115+
}
116+
117+
func newDenyDescribeTableDriver(t *testing.T) *DynamoDBDriver {
118+
t.Helper()
119+
120+
endpoint := os.Getenv("DYNAMODBMOCK_ENDPOINT")
121+
122+
httpClient := &http.Client{Transport: &denyDescribeTableTransport{base: http.DefaultTransport}}
123+
124+
cfg, err := awsconfig.LoadDefaultConfig(context.Background(),
125+
awsconfig.WithRegion("us-east-2"),
126+
awsconfig.WithHTTPClient(httpClient),
127+
)
128+
So(err, ShouldBeNil)
129+
130+
client := dynamodb.NewFromConfig(cfg, func(o *dynamodb.Options) {
131+
o.BaseEndpoint = aws.String(endpoint)
132+
})
133+
134+
return &DynamoDBDriver{client: client, log: log.NewTestLogger()}
135+
}
136+
137+
func TestNewTableWithoutDescribeTablePermission(t *testing.T) {
138+
tskip.SkipDynamo(t)
139+
140+
Convey("NewTable falls back to CreateTable when dynamodb:DescribeTable is denied", t, func() {
141+
Convey("table does not exist yet", func() {
142+
const tableName = "Issue4259NewTableNoDescribe"
143+
144+
deniedDriver := newDenyDescribeTableDriver(t)
145+
So(deniedDriver.NewTable(tableName), ShouldBeNil)
146+
So(deniedDriver.tableName, ShouldEqual, tableName)
147+
148+
// confirm CreateTable actually ran (rather than NewTable returning nil
149+
// without ever creating the table) via an unwrapped client
150+
endpoint := os.Getenv("DYNAMODBMOCK_ENDPOINT")
151+
152+
plainCfg, err := awsconfig.LoadDefaultConfig(context.Background(), awsconfig.WithRegion("us-east-2"))
153+
So(err, ShouldBeNil)
154+
155+
plainClient := dynamodb.NewFromConfig(plainCfg, func(o *dynamodb.Options) {
156+
o.BaseEndpoint = aws.String(endpoint)
157+
})
158+
159+
_, err = plainClient.DescribeTable(context.Background(), &dynamodb.DescribeTableInput{
160+
TableName: aws.String(tableName),
161+
})
162+
So(err, ShouldBeNil)
163+
})
164+
165+
Convey("table already exists", func() {
166+
const tableName = "Issue4259ExistingTableNoDescribe"
167+
168+
endpoint := os.Getenv("DYNAMODBMOCK_ENDPOINT")
169+
170+
plainCfg, err := awsconfig.LoadDefaultConfig(context.Background(), awsconfig.WithRegion("us-east-2"))
171+
So(err, ShouldBeNil)
172+
173+
plainClient := dynamodb.NewFromConfig(plainCfg, func(o *dynamodb.Options) {
174+
o.BaseEndpoint = aws.String(endpoint)
175+
})
176+
177+
driver := &DynamoDBDriver{client: plainClient, log: log.NewTestLogger()}
178+
So(driver.NewTable(tableName), ShouldBeNil)
179+
180+
// CreateTable will fail with ResourceInUseException; since confirming via DescribeTable
181+
// is also denied, that should still be tolerated as benign.
182+
deniedDriver := newDenyDescribeTableDriver(t)
183+
So(deniedDriver.NewTable(tableName), ShouldBeNil)
184+
So(deniedDriver.tableName, ShouldEqual, tableName)
185+
})
186+
})
187+
}

0 commit comments

Comments
 (0)