Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a namespace field to tuf metadata storage #1110

Closed
wants to merge 7 commits into from
Closed

Add a namespace field to tuf metadata storage #1110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
24f043b
Add a namespace field to tuf metadata storage
ecordell Feb 28, 2017
16db507
Remove namespaces table and more descriptive naming
ecordell Mar 2, 2017
615a661
Change Namespaces to Channels, many-to-many with TUFFiles
ecordell Apr 6, 2017
7aa289d
MetaUpdate has an array of Channels now, plus some minor refactoring
ecordell Mar 15, 2017
1acc425
rename namespaces -> channels migration
ecordell Mar 27, 2017
3da63b1
don't restrict delegation names
ecordell May 3, 2017
e407047
Storage cleanup
ecordell Jun 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions cmd/notary/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,15 +260,30 @@ type recordingMetaStore struct {

// GetCurrent gets the metadata from the underlying MetaStore, but also records
// that the metadata was requested
func (r *recordingMetaStore) GetCurrent(gun data.GUN, role data.RoleName) (*time.Time, []byte, error) {
r.gotten = append(r.gotten, fmt.Sprintf("%s.%s", gun.String(), role.String()))
func (r *recordingMetaStore) GetCurrent(gun data.GUN, role data.RoleName, channels ...*storage.Channel) (*time.Time, []byte, error) {
var channelBuf bytes.Buffer

if len(channels) == 0 {
channels = []*storage.Channel{&storage.Published}
}
for _, channel := range channels {
channelBuf.WriteString(channel.Name)
}
r.gotten = append(r.gotten, fmt.Sprintf("%s.%s.%s", gun.String(), role.String(), channelBuf.String()))
return r.MemStorage.GetCurrent(gun, role)
}

// GetChecksum gets the metadata from the underlying MetaStore, but also records
// that the metadata was requested
func (r *recordingMetaStore) GetChecksum(gun data.GUN, role data.RoleName, checksum string) (*time.Time, []byte, error) {
r.gotten = append(r.gotten, fmt.Sprintf("%s.%s", gun.String(), role.String()))
func (r *recordingMetaStore) GetChecksum(gun data.GUN, role data.RoleName, checksum string, channels ...*storage.Channel) (*time.Time, []byte, error) {
var channelBuf bytes.Buffer
if len(channels) == 0 {
channels = []*storage.Channel{&storage.Published}
}
for _, channel := range channels {
channelBuf.WriteString(channel.Name)
}
r.gotten = append(r.gotten, fmt.Sprintf("%s.%s.%s", gun.String(), role.String(), channelBuf.String()))
return r.MemStorage.GetChecksum(gun, role, checksum)
}

Expand Down Expand Up @@ -368,7 +383,7 @@ func TestConfigFileTLSCanBeRelativeToConfigOrAbsolute(t *testing.T) {

// validate that we actually managed to connect and attempted to download the root though
require.Len(t, m.gotten, 1)
require.Equal(t, m.gotten[0], "repo.root")
require.Equal(t, m.gotten[0], "repo.root.published")
}

// Whatever TLS config is in the config file can be overridden by the command line
Expand Down Expand Up @@ -418,7 +433,7 @@ func TestConfigFileOverridenByCmdLineFlags(t *testing.T) {

// validate that we actually managed to connect and attempted to download the root though
require.Len(t, m.gotten, 1)
require.Equal(t, m.gotten[0], "repo.root")
require.Equal(t, m.gotten[0], "repo.root.published")
}

// the config can specify trust pinning settings for TOFUs, as well as pinned Certs or CA
Expand Down
27 changes: 27 additions & 0 deletions migrations/server/mysql/0006_channels.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
ALTER TABLE `tuf_files`
DROP INDEX `gun`;

CREATE TABLE `channels` (
`id` int(11) NOT NULL AUTO_INCREMENT,
`name` VARCHAR(255) NOT NULL,
`created_at` timestamp NULL DEFAULT NULL,
`updated_at` timestamp NULL DEFAULT NULL,
`deleted_at` timestamp NULL DEFAULT NULL,
PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

INSERT INTO `channels` (id, name) VALUES (1, "published"), (2, "staged");

CREATE TABLE `channels_tuf_files` (
`channel_id` INT(11) NOT NULL,
`tuf_file_id` INT(11) NOT NULL,
FOREIGN KEY (channel_id) REFERENCES channels(`id`) ON DELETE CASCADE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, does gorm automatically add this to both the mysql and postgres table generation? (didn't see something in the model that corresponded to this directive)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, I didn't use gorm to generate the table or migration

FOREIGN KEY (tuf_file_id) REFERENCES tuf_files(`id`) ON DELETE CASCADE,
PRIMARY KEY (tuf_file_id, channel_id)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

INSERT INTO `channels_tuf_files` (channel_id, tuf_file_id) (
SELECT 1, `id` FROM `tuf_files`
);


25 changes: 25 additions & 0 deletions migrations/server/postgresql/0003_channels.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
ALTER TABLE "tuf_files" DROP CONSTRAINT "tuf_files_gun_role_version_key";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dumb SQL question: Is this constraint automatically created in 0001_initial.up.sql when we specify UNIQUE ("gun", "role", "version")? I didn't see this particular name anywhere in our repo, so was just wondering if this was something postgres specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, exactly. It's created by UNIQUE in postgres.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool thanks for explaining!


CREATE TABLE "channels" (
"id" serial PRIMARY KEY,
"name" VARCHAR(255) NOT NULL,
"created_at" timestamp NULL DEFAULT NULL,
"updated_at" timestamp NULL DEFAULT NULL,
"deleted_at" timestamp NULL DEFAULT NULL
);

INSERT INTO "channels" (id, name) VALUES (1, 'published'), (2, 'staged');

CREATE TABLE "channels_tuf_files" (
"channel_id" integer NOT NULL,
"tuf_file_id" integer NOT NULL,
FOREIGN KEY (channel_id) REFERENCES channels("id") ON DELETE CASCADE,
FOREIGN KEY (tuf_file_id) REFERENCES tuf_files("id") ON DELETE CASCADE,
PRIMARY KEY (tuf_file_id, channel_id)
);

INSERT INTO "channels_tuf_files" (channel_id, tuf_file_id) (
SELECT 1, "id" FROM "tuf_files"
);


2 changes: 1 addition & 1 deletion server/handlers/default_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ type failStore struct {
storage.MemStorage
}

func (s *failStore) GetCurrent(_ data.GUN, _ data.RoleName) (*time.Time, []byte, error) {
func (s *failStore) GetCurrent(_ data.GUN, _ data.RoleName, channels ...*storage.Channel) (*time.Time, []byte, error) {
return nil, nil, fmt.Errorf("oh no! storage has failed")
}

Expand Down
8 changes: 4 additions & 4 deletions server/handlers/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,20 @@ type getFailStore struct {

// GetCurrent returns the current metadata, or an error depending on whether
// getFailStore is configured to return an error for this role
func (f getFailStore) GetCurrent(gun data.GUN, tufRole data.RoleName) (*time.Time, []byte, error) {
func (f getFailStore) GetCurrent(gun data.GUN, tufRole data.RoleName, channels ...*storage.Channel) (*time.Time, []byte, error) {
err := f.errsToReturn[tufRole.String()]
if err == nil {
return f.MetaStore.GetCurrent(gun, tufRole)
return f.MetaStore.GetCurrent(gun, tufRole, channels...)
}
return nil, nil, err
}

// GetChecksum returns the metadata with this checksum, or an error depending on
// whether getFailStore is configured to return an error for this role
func (f getFailStore) GetChecksum(gun data.GUN, tufRole data.RoleName, checksum string) (*time.Time, []byte, error) {
func (f getFailStore) GetChecksum(gun data.GUN, tufRole data.RoleName, checksum string, channels ...*storage.Channel) (*time.Time, []byte, error) {
err := f.errsToReturn[tufRole.String()]
if err == nil {
return f.MetaStore.GetChecksum(gun, tufRole, checksum)
return f.MetaStore.GetChecksum(gun, tufRole, checksum, channels...)
}
return nil, nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions server/snapshot/snapshot_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ type FailingStore struct {
*storage.MemStorage
}

func (f FailingStore) GetCurrent(gun data.GUN, role data.RoleName) (*time.Time, []byte, error) {
func (f FailingStore) GetCurrent(gun data.GUN, role data.RoleName, channels ...*storage.Channel) (*time.Time, []byte, error) {
return nil, nil, fmt.Errorf("failing store failed")
}

Expand All @@ -72,7 +72,7 @@ type CorruptedStore struct {
*storage.MemStorage
}

func (c CorruptedStore) GetCurrent(gun data.GUN, role data.RoleName) (*time.Time, []byte, error) {
func (c CorruptedStore) GetCurrent(gun data.GUN, role data.RoleName, channels ...*storage.Channel) (*time.Time, []byte, error) {
return &time.Time{}, []byte("junk"), nil
}

Expand Down
6 changes: 3 additions & 3 deletions server/storage/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,17 @@ type MetaStore interface {
// GetCurrent returns the modification date and data part of the metadata for
// the latest version of the given GUN and role. If there is no data for
// the given GUN and role, an error is returned.
GetCurrent(gun data.GUN, tufRole data.RoleName) (created *time.Time, data []byte, err error)
GetCurrent(gun data.GUN, tufRole data.RoleName, channels ...*Channel) (created *time.Time, data []byte, err error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should GetCurrent implicitly mean published and maybe there should be a new method GetLatest that takes a channel?

Copy link
Contributor Author

@ecordell ecordell Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behavior is that GetCurrent(gun, role) uses the published channel, GetCurrent(gun, role, channel) uses the specified channel. I'm happy to add another wrapper method but that seems not dissimilar from your suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, yeah, this is fine. I had a moment of forgetfulness that a ... can also be left empty by the caller.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like in all the implementations, if multiple channels are passed, only the data in the first channel the data is found in is passed (and the channels are checked in order of preference). Should this behavior be documented as a requirement in the interface, and a general test be added (maybe in server/storage/storage_test.go) to make sure that even if there is a TUF file with the same GUN and role is in multiple channels, this function will only return the data from the first?

Similarly for GetVersion. I guess for GetChecksum it doesn't really matter, since if it's the same checksum, it doesn't matter which channel it's in because all the data will be the same, although the creation time may be different. If the creation time matters, then perhaps a similar test can be added for GetChecksum?


// GetChecksum returns the given TUF role file and creation date for the
// GUN with the provided checksum. If the given (gun, role, checksum) are
// not found, it returns storage.ErrNotFound
GetChecksum(gun data.GUN, tufRole data.RoleName, checksum string) (created *time.Time, data []byte, err error)
GetChecksum(gun data.GUN, tufRole data.RoleName, checksum string, channels ...*Channel) (created *time.Time, data []byte, err error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checksum is sufficiently specific I wonder if it's necessary to also pass a channel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same checksum can live in multiple channels when staging metadata

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah of course!


// GetVersion returns the given TUF role file and creation date for the
// GUN with the provided version. If the given (gun, role, version) are
// not found, it returns storage.ErrNotFound
GetVersion(gun data.GUN, tufRole data.RoleName, version int) (created *time.Time, data []byte, err error)
GetVersion(gun data.GUN, tufRole data.RoleName, version int, channels ...*Channel) (created *time.Time, data []byte, err error)

// Delete removes all metadata for a given GUN. It does not return an
// error if no metadata exists for the given GUN.
Expand Down
13 changes: 13 additions & 0 deletions server/storage/interface_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package storage

import (
"testing"

"github.com/stretchr/testify/require"
)

// Check that every provided implementation of a MetaStore actually conforms
func TestImplementationsConform(t *testing.T) {
impls := []MetaStore{&MemStorage{}, &SQLStorage{}, &RethinkDB{}, &TUFMetaStorage{}}
require.NotEmpty(t, impls)
}
Loading