Skip to content

Commit

Permalink
feat(resource): stop updating origin and revision through StoreResources
Browse files Browse the repository at this point in the history 
Since the new way to use resources tables is to add a resource and move association through
application_resource and unit_resource table, StoreResources doesn't need to update origin or
revision anymore. It will always upload a resource with valid origin and revision.

So this commit remove those fields from all layers in the call stack.

The only caveat is in resourcemigration handler, we check the compliance between query parameters
and actual stored resources regarding origin and revision to avoid storing the wrong data for the
wrong resource in DB (the UUID is queried from application and name in the handler and can be
subject of data race)
  • Loading branch information
@gfouillet
gfouillet committed Feb 21, 2025
1 parent 00877ca commit 2a7a59e
Show file tree
Hide file tree
Showing 11 changed files with 51 additions and 221 deletions.
23 changes: 20 additions & 3 deletions apiserver/internal/handlers/resources/resourcemigration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,21 +125,38 @@ func (h *resourcesMigrationUploadHandler) processPost(
return empty, internalerrors.Errorf("extracting resource details from request: %w", err)
}

// Check that the resource associated with the UUID has the expected Origin
// and revision, to prevent updating the blob of another revision or origin
// that may have been updated between the start of the http call and the
// retrieval of resource UUID from application name and resource name.
res, err := resourceService.GetResource(ctx, resUUID)
if err != nil {
return empty, internalerrors.Errorf("getting resource %s on application %s: %w", appName,
resourceName, err)
}
if res.Origin != details.origin || (res.Origin != charmresource.OriginUpload && res.Revision != details.revision) {
// In this case, we won't interrupt migration, because the user will
// be able to recover after the migration. However, we log an error in
// order to make it notice something get wrong and why.
h.logger.Errorf(ctx, "resource upload failed: resource %s on application %s has unexpected origin or revision: %s != %s, %d != %d",
appName, resourceName, res.Origin, details.origin, res.Revision, details.revision,
)
return res, nil
}
retrievedBy, retrievedByType := determineRetrievedBy(query)

reader, err := h.downloader.Download(r.Context(), r.Body, details.fingerprint.String(), details.size)
if err != nil {
return empty, internalerrors.Errorf("validating resource size and hash: %w", err)
}

retrievedBy, retrievedByType := determineRetrievedBy(query)
err = resourceService.StoreResource(ctx, resource.StoreResourceArgs{
ResourceUUID: resUUID,
Reader: reader,
RetrievedBy: retrievedBy,
RetrievedByType: retrievedByType,
Size: details.size,
Fingerprint: details.fingerprint,
Origin: details.origin,
Revision: details.revision,
})
if err != nil {
return empty, internalerrors.Capture(err)
Expand Down
50 changes: 31 additions & 19 deletions apiserver/internal/handlers/resources/resourcemigration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,13 @@ func (s *resourcesUploadSuite) TestServeUploadApplicationStoreResourceError(c *g
).Return("res-uuid", nil)
s.downloader.EXPECT().Download(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
s.resourceService.EXPECT().StoreResource(gomock.Any(), gomock.Any()).Return(errors.New("cannot store resource"))
s.resourceService.EXPECT().GetResource(gomock.Any(), resource.UUID("res-uuid")).Return(resource.Resource{
UUID: "res-uuid",
Resource: charmresource.Resource{
Origin: s.origin,
Revision: s.revision,
},
}, nil)

// Act
response, err := http.Post(s.srv.URL+migrateResourcesPrefix+"?"+query.Encode(), "application/octet-stream", nil)
Expand Down Expand Up @@ -192,8 +199,6 @@ func (s *resourcesUploadSuite) TestServeUploadApplicationGetResourceError(c *gc.
"app-name",
"resource-name",
).Return("res-uuid", nil)
s.downloader.EXPECT().Download(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
s.resourceService.EXPECT().StoreResource(gomock.Any(), gomock.Any()).Return(nil)
s.resourceService.EXPECT().GetResource(gomock.Any(), gomock.Any()).Return(resource.Resource{}, errors.New(
"cannot get resource"))

Expand Down Expand Up @@ -257,15 +262,17 @@ func (s *resourcesUploadSuite) TestServeUploadApplication(c *gc.C) {
ResourceUUID: "res-uuid",
Reader: http.NoBody,
RetrievedByType: resource.Application,
Origin: s.origin,
Revision: s.revision,
Fingerprint: s.fingerprint,
Size: s.size,
}).Return(nil)
s.resourceService.EXPECT().GetResource(gomock.Any(), resource.UUID("res-uuid")).Return(resource.Resource{
UUID: "res-uuid",
UUID: "res-uuid",
Resource: charmresource.Resource{
Origin: s.origin,
Revision: s.revision,
},
Timestamp: now,
}, nil)
}, nil).Times(2)

// Act
response, err := http.Post(s.srv.URL+migrateResourcesPrefix+"?"+query.Encode(), "application/octet-stream", http.NoBody)
Expand Down Expand Up @@ -299,7 +306,6 @@ func (s *resourcesUploadSuite) TestServeUploadApplicationRetrievedByUser(c *gc.C
"origin": {"upload"},
"size": {s.sizeStr},
"fingerprint": {s.fingerprintStr},
"revision": {s.revisionStr},
}
s.resourceService.EXPECT().GetResourceUUIDByApplicationAndResourceName(
gomock.Any(),
Expand All @@ -317,15 +323,17 @@ func (s *resourcesUploadSuite) TestServeUploadApplicationRetrievedByUser(c *gc.C
Reader: http.NoBody,
RetrievedByType: resource.User,
RetrievedBy: "username",
Origin: charmresource.OriginUpload,
Revision: -1,
Fingerprint: s.fingerprint,
Size: s.size,
}).Return(nil)
s.resourceService.EXPECT().GetResource(gomock.Any(), resource.UUID("res-uuid")).Return(resource.Resource{
UUID: "res-uuid",
UUID: "res-uuid",
Resource: charmresource.Resource{
Origin: charmresource.OriginUpload,
Revision: -1,
},
Timestamp: now,
}, nil)
}, nil).Times(2)

// Act
_, err := http.Post(s.srv.URL+migrateResourcesPrefix+"?"+query.Encode(), "application/octet-stream", http.NoBody)
Expand Down Expand Up @@ -365,15 +373,17 @@ func (s *resourcesUploadSuite) TestServeUploadApplicationRetrievedByApplication(
Reader: http.NoBody,
RetrievedByType: resource.Application,
RetrievedBy: "app-name",
Origin: s.origin,
Revision: s.revision,
Fingerprint: s.fingerprint,
Size: s.size,
}).Return(nil)
s.resourceService.EXPECT().GetResource(gomock.Any(), resource.UUID("res-uuid")).Return(resource.Resource{
UUID: "res-uuid",
UUID: "res-uuid",
Resource: charmresource.Resource{
Origin: s.origin,
Revision: s.revision,
},
Timestamp: now,
}, nil)
}, nil).Times(2)

// Act
_, err := http.Post(s.srv.URL+migrateResourcesPrefix+"?"+query.Encode(), "application/octet-stream", http.NoBody)
Expand Down Expand Up @@ -412,15 +422,17 @@ func (s *resourcesUploadSuite) TestServeUploadApplicationRetrievedByUnit(c *gc.C
Reader: http.NoBody,
RetrievedByType: resource.Unit,
RetrievedBy: "app-name/0",
Origin: s.origin,
Revision: s.revision,
Fingerprint: s.fingerprint,
Size: s.size,
}).Return(nil)
s.resourceService.EXPECT().GetResource(gomock.Any(), resource.UUID("res-uuid")).Return(resource.Resource{
UUID: "res-uuid",
UUID: "res-uuid",
Resource: charmresource.Resource{
Origin: s.origin,
Revision: s.revision,
},
Timestamp: now,
}, nil)
}, nil).Times(2)

// Act
_, err := http.Post(s.srv.URL+migrateResourcesPrefix+"?"+query.Encode(), "application/octet-stream", http.NoBody)
Expand Down
2 changes: 0 additions & 2 deletions apiserver/internal/handlers/resources/resources.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,6 @@ func (h *ResourceHandler) upload(service ResourceService, req *http.Request, use
RetrievedByType: coreresource.User,
Size: uploaded.size,
Fingerprint: uploaded.fingerprint,
Origin: charmresource.OriginUpload,
Revision: -1,
},
)
if err != nil {
Expand Down
6 changes: 0 additions & 6 deletions apiserver/internal/handlers/resources/resources_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,6 @@ func (s *ResourcesHandlerSuite) TestPutSuccess(c *gc.C) {
RetrievedByType: coreresource.User,
Size: s.resource.Size,
Fingerprint: s.resource.Fingerprint,
Origin: charmresource.OriginUpload,
Revision: -1,
})

// Second call to GetResource gets resource details after upload.
Expand Down Expand Up @@ -347,8 +345,6 @@ func (s *ResourcesHandlerSuite) TestPutSuccessDockerResource(c *gc.C) {
RetrievedByType: coreresource.User,
Size: s.resource.Size,
Fingerprint: s.resource.Fingerprint,
Origin: charmresource.OriginUpload,
Revision: -1,
})

// Second call to GetResource gets resource details after upload.
Expand Down Expand Up @@ -433,8 +429,6 @@ func (s *ResourcesHandlerSuite) TestPutWithPending(c *gc.C) {
RetrievedByType: coreresource.User,
Size: s.resource.Size,
Fingerprint: s.resource.Fingerprint,
Origin: charmresource.OriginUpload,
Revision: -1,
})

// Second call to GetResource gets resource details after upload.
Expand Down
18 changes: 0 additions & 18 deletions domain/resource/service/resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,22 +387,6 @@ func (s *Service) storeResource(
return resourceerrors.RetrievedByTypeNotValid
}

if err := args.Origin.Validate(); err != nil {
return errors.Errorf("resource origin: %w", err)
}
if args.Origin == charmresource.OriginUpload && args.Revision != -1 {
return errors.Errorf(
"resource with origin upload must have positive -1, found %d: %w",
args.Revision, resourceerrors.ResourceRevisionNotValid,
)
}
if args.Origin == charmresource.OriginStore && args.Revision < 0 {
return errors.Errorf(
"resource with origin store must have positive revision, found %d, %w",
args.Revision, resourceerrors.ResourceRevisionNotValid,
)
}

res, err := s.st.GetResource(ctx, args.ResourceUUID)
if err != nil {
return errors.Errorf("getting resource: %w", err)
Expand Down Expand Up @@ -448,8 +432,6 @@ func (s *Service) storeResource(
IncrementCharmModifiedVersion: incrementCharmModifiedVersion,
Size: args.Size,
SHA384: args.Fingerprint.String(),
Origin: args.Origin,
Revision: args.Revision,
},
)
if err != nil {
Expand Down
Loading

0 comments on commit 2a7a59e

Please sign in to comment.