drkey: add serviceEngine and secretValueEngine #4225

JordiSubira · 2022-07-12T16:50:42Z

This PR is the 5th in a series of PRs that add support for DRKey started with #4217.

This PR contains:

The serviceEngine for the CS
The secretValueEngine for the CS
The ARC adaptative cache to keep information about the remote keys that are fetch most frequently
UT

This change is

lukedirtwalker

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @JordiSubira)

control/drkey/arc.go line 23 at r1 (raw file):

)

var _ Level1PrefetchListKeeper = (*Level1ARC)(nil)

In newer code we usually move those assertions into the test file.

control/drkey/arc.go line 26 at r1 (raw file):

// Level1ARC maintains an Adaptative Replacement Cache, storing
// the necessary metadata to prefetch Level1 keys

applies to all comments: Comments usually end with a period (.)

control/drkey/arc.go line 45 at r1 (raw file):

// in the ARC cache
func (c *Level1ARC) Update(keyPair Level1PrefetchInfo) {
	c.cache.Add(keyPair, keyPair)

if we never care about the value we could also inserts struct{}{} as value here.

control/drkey/arc.go line 48 at r1 (raw file):

}

// GetCachedASes returns the list of AS currently in cache

the comment doesn't match the method signature, please fix it.

control/drkey/arc.go line 49 at r1 (raw file):

// GetCachedASes returns the list of AS currently in cache
func (c *Level1ARC) GetLevel1InfoArray() []Level1PrefetchInfo {

I wonder if we should somehow rename this, since the struct name already contains Level1 I wonder if we really need to repeat it here. Also we don't really need the Get prefix.

control/drkey/arc_test.go line 27 at r1 (raw file):

)

func TestLevel1ARC(t *testing.T) {

I think this test would be much more readable if it would be structured as a table based test. Each test has some fill actions and some checks to be executed.

Also probably it makes more sense to split the testing of the NewLevel1ARC in a separate function TestNewLevel1ARC.

At least do the latter, the table-based is not super important and more nice to have.

control/drkey/arc_test.go line 28 at r1 (raw file):

func TestLevel1ARC(t *testing.T) {

drop the empty line.

control/drkey/arc_test.go line 40 at r1 (raw file):

	}
	cache.Update(cacheKey0)
	assert.Len(t, cache.GetLevel1InfoArray(), 1)

Here we can simply test for the exact content.

Suggestion:

	assert.Equal(t, []cs_drkey.Level1PrefetchInfo{cacheKey0}, cache.GetLevel1InfoArray())

control/drkey/arc_test.go line 65 at r1 (raw file):

	cache.Update(cacheKey0)
	assert.NotContains(t, cache.GetLevel1InfoArray(), cacheKey10)

drop empty line

control/drkey/service_engine.go line 35 at r1 (raw file):

}

// Level1PrefetchListKeeper maintains a list for those lvl1 keys

no need to safe characters in a comment.

Suggestion:

 level1

control/drkey/service_engine.go line 40 at r1 (raw file):

	//Update updates the keys in Level1Cache based on the Level1Key metadata
	Update(key Level1PrefetchInfo)
	// GetLevel1InfoArray retrieves an array whose memebers contains information regarding

Suggestion:

members

control/drkey/service_engine.go line 41 at r1 (raw file):

	Update(key Level1PrefetchInfo)
	// GetLevel1InfoArray retrieves an array whose memebers contains information regarding
	// lvl1 keys to prefetch

Suggestion:

level1 keys to prefetch.

control/drkey/service_engine.go line 52 at r1 (raw file):

// ServiceEngine maintains and provides secret values, lvl1 keys and prefetching information.
type ServiceEngine interface {

why do we need this interface? I don't really see where this would be used. for the NewService*Cleaner functions below we could have very specific interfaces inlined like this:

func NewServiceSecretCleaner(s interface{DeleteExpiredSecrets(ctx context.Context) (int, error)}) *cleaner.Cleaner {

right now it's not clear to me that/why we should have this interface here.

control/drkey/service_engine.go line 86 at r1 (raw file):

// serviceEngine keeps track of the level 1 drkey keys. It is backed by a drkey.DB
type serviceEngine struct {

how do we plan to use this if the struct isn't public? Why are some of its fields public and others not?

control/drkey/service_engine.go line 96 at r1 (raw file):

var _ ServiceEngine = (*serviceEngine)(nil)

func NewServiceEngine(localIA addr.IA,

If we make the struct public we don't really need the constructor the user can just fill the struct itself.

control/drkey/service_engine.go line 130 at r1 (raw file):

// GetLevel1Key returns the level 1 drkey from the local DB or, if not found, by asking any CS in
// the source AS of the key. It also updates the Level1Cache, if needed

Suggestion:

if needed.

control/drkey/service_engine.go line 137 at r1 (raw file):

	key, err := s.getLevel1Key(ctx, meta)
	if err == nil && ctx.Value(fromPrefetcher{}) == nil && meta.SrcIA != s.LocalIA {

who else will use this value? it's currently not very transparent maybe make a comment why we add this check.

Code quote:

fromPrefetcher{}

control/drkey/service_engine.go line 191 at r1 (raw file):

	if meta.DstIA != s.LocalIA {
		return drkey.Level1Key{},
			serrors.New("Neither srcIA nor dstIA matches localIA", "srcIA", meta.SrcIA,

Error messages don't start with capital letter, because they are logged at some point and the log message will start with the sentence.

Suggestion:

neither

control/drkey/service_engine.go line 198 at r1 (raw file):

	k, err := s.DB.GetLevel1Key(ctx, meta)
	if err == nil {
		logger.Debug("L1 key found in storage")

do we want to keep that? wouldn't that lead to quite extensive logging?

control/drkey/service_engine.go line 258 at r1 (raw file):

// DeriveASHost returns an AS-Host key based on the presented information
func (s *serviceEngine) DeriveASHost(

please structure the file so that all public methods of the struct appear before the private ones.

control/drkey/service_engine.go line 277 at r1 (raw file):

		}
	} else {
		key, err = (&generic.Deriver{}).DeriveASHost(meta.ProtoId, meta.DstHost, lvl1Key.Key)

instead of having the protoID as a deriver argument we could have it as a member in the struct then both derivers would have the same interface and the code could be a bit cleaner here:

	var deriver interface {
		DeriveHostAS(srcHost string, key drkey.Key) (drkey.Key, error)
	} = generic.Deriver{Proto: meta.ProtoId}

	if meta.ProtoId.IsPredefined() {
		deriver = specific.Deriver{}
	}
	key, err = deriver.DeriveHostAS(meta.SrcHost, lvl1Key.Key)
	if err != nil {
		return drkey.HostASKey{}, serrors.WrapStr("parsing derivation input", err)
	}

also I don't see why we need to allocate memory for the derivers those should really be objects that are created on the stack.

Same comment applies to the other derive methods.

control/drkey/service_engine.go line 343 at r1 (raw file):

	var err error

	hostASKey, err := s.DeriveHostAS(ctx, hostASMeta)

I'm not super familiar with the whole setup, but I wonder if it wouldn't make sense to have this be required to be called separately, i.e. just take hostASKey as an argument? Or why is it done this way?

Same applies for the level1 key in the methods above.

control/drkey/service_engine_test.go line 53 at r1 (raw file):

		Validity: util.SecsToTime(0).UTC(),
	}

can you add a comment what we are testing below?

control/drkey/service_engine_test.go line 79 at r1 (raw file):

	}

	_, err = store.DeriveLevel1(meta)

should we check that the created key matches the meta values?

control/drkey/service_engine_test.go line 109 at r1 (raw file):

	fetcher := mock_drkey.NewMockFetcher(mctrl)
	firstCall := fetcher.EXPECT().Level1(gomock.Any(),

couldn't you use gomock.InOrder for this?

	gomock.InOrder(
		fetcher.EXPECT().Level1(gomock.Any(), gomock.Any()).
			Return(firstLevel1Key, nil),
		fetcher.EXPECT().Level1(gomock.Any(), gomock.Any()).
			Return(secondLevel1Key, nil),
		fetcher.EXPECT().Level1(gomock.Any(), gomock.Any()).
			Return(drkey.Level1Key{}, serrors.New("error retrieving key")),
	)

JordiSubira

Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @JordiSubira and @lukedirtwalker)

control/drkey/service_engine.go line 52 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

why do we need this interface? I don't really see where this would be used. for the NewService*Cleaner functions below we could have very specific interfaces inlined like this:
func NewServiceSecretCleaner(s interface{DeleteExpiredSecrets(ctx context.Context) (int, error)}) *cleaner.Cleaner {
right now it's not clear to me that/why we should have this interface here.

We decided to have one interface that hides behinds the logic to derive/get/delete keys. It will be used in the grpc_drkey_service, the prefetcher the TaskConfig` in the CS and some UT relative to those. It is true that probably will have only the serviceEngine struct implementing this interface

control/drkey/service_engine.go line 86 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

how do we plan to use this if the struct isn't public? Why are some of its fields public and others not?

The idea was to use the constructor that hides the secretValeBackend and the PrefetchKeeper. I can make it public if you thing that's better.

control/drkey/service_engine.go line 96 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

If we make the struct public we don't really need the constructor the user can just fill the struct itself.

Indeed, I can remove it if we make it public.

control/drkey/service_engine.go line 137 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

who else will use this value? it's currently not very transparent maybe make a comment why we add this check.

The prefetcher and the serviceEngine. The idea is not updating the prefetchKeeper for those keys that are prefetch, so that we avoid having always the same keys in this list. It would only be updated when keys are requested by some host.

control/drkey/service_engine.go line 277 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

instead of having the protoID as a deriver argument we could have it as a member in the struct then both derivers would have the same interface and the code could be a bit cleaner here:
	var deriver interface {
		DeriveHostAS(srcHost string, key drkey.Key) (drkey.Key, error)
	} = generic.Deriver{Proto: meta.ProtoId}

	if meta.ProtoId.IsPredefined() {
		deriver = specific.Deriver{}
	}
	key, err = deriver.DeriveHostAS(meta.SrcHost, lvl1Key.Key)
	if err != nil {
		return drkey.HostASKey{}, serrors.WrapStr("parsing derivation input", err)
	}
also I don't see why we need to allocate memory for the derivers those should really be objects that are created on the stack.

Same comment applies to the other derive methods.

This is probably because before the derivers used an internal buffer to receive the input for the derivation. I will change the methods to receive the struct instead of the pointer.

control/drkey/service_engine.go line 343 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I'm not super familiar with the whole setup, but I wonder if it wouldn't make sense to have this be required to be called separately, i.e. just take hostASKey as an argument? Or why is it done this way?

Same applies for the level1 key in the methods above.

As I mentioned before, we decided to hide the whole derivation logic behind the ServiceEngine, e.g. the grpc_drkey_service only parses the request calls the method and generates the response.

JordiSubira

Reviewable status: 4 of 14 files reviewed, 25 unresolved discussions (waiting on @lukedirtwalker)

control/drkey/arc.go line 23 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

In newer code we usually move those assertions into the test file.

Done.

control/drkey/arc.go line 26 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

applies to all comments: Comments usually end with a period (.)

Done.

control/drkey/arc.go line 45 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

if we never care about the value we could also inserts struct{}{} as value here.

Done.

control/drkey/arc.go line 48 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

the comment doesn't match the method signature, please fix it.

Done.

control/drkey/arc.go line 49 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I wonder if we should somehow rename this, since the struct name already contains Level1 I wonder if we really need to repeat it here. Also we don't really need the Get prefix.

Done.

control/drkey/arc_test.go line 27 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think this test would be much more readable if it would be structured as a table based test. Each test has some fill actions and some checks to be executed.

Also probably it makes more sense to split the testing of the NewLevel1ARC in a separate function TestNewLevel1ARC.

At least do the latter, the table-based is not super important and more nice to have.

Done.

control/drkey/arc_test.go line 28 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

drop the empty line.

Done.

control/drkey/arc_test.go line 40 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Here we can simply test for the exact content.

Done.

control/drkey/arc_test.go line 65 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

drop empty line

Done.

control/drkey/service_engine.go line 35 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

no need to safe characters in a comment.

Done.

control/drkey/service_engine.go line 40 at r1 (raw file):

	//Update updates the keys in Level1Cache based on the Level1Key metadata
	Update(key Level1PrefetchInfo)
	// GetLevel1InfoArray retrieves an array whose memebers contains information regarding

Done.

control/drkey/service_engine.go line 41 at r1 (raw file):

	Update(key Level1PrefetchInfo)
	// GetLevel1InfoArray retrieves an array whose memebers contains information regarding
	// lvl1 keys to prefetch

Done.

control/drkey/service_engine.go line 130 at r1 (raw file):

// GetLevel1Key returns the level 1 drkey from the local DB or, if not found, by asking any CS in
// the source AS of the key. It also updates the Level1Cache, if needed

Done.

control/drkey/service_engine.go line 191 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Error messages don't start with capital letter, because they are logged at some point and the log message will start with the sentence.

Done.

control/drkey/service_engine.go line 198 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

do we want to keep that? wouldn't that lead to quite extensive logging?

Done.

control/drkey/service_engine.go line 258 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

please structure the file so that all public methods of the struct appear before the private ones.

Done.

control/drkey/service_engine.go line 277 at r1 (raw file):

Previously, JordiSubira wrote…

This is probably because before the derivers used an internal buffer to receive the input for the derivation. I will change the methods to receive the struct instead of the pointer.

Done.

control/drkey/service_engine_test.go line 53 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

can you add a comment what we are testing below?

Done.

control/drkey/service_engine_test.go line 79 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

should we check that the created key matches the meta values?

Done.

control/drkey/service_engine_test.go line 109 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

couldn't you use gomock.InOrder for this?

	gomock.InOrder(
		fetcher.EXPECT().Level1(gomock.Any(), gomock.Any()).
			Return(firstLevel1Key, nil),
		fetcher.EXPECT().Level1(gomock.Any(), gomock.Any()).
			Return(secondLevel1Key, nil),
		fetcher.EXPECT().Level1(gomock.Any(), gomock.Any()).
			Return(drkey.Level1Key{}, serrors.New("error retrieving key")),
	)

Done.

lukedirtwalker

Reviewed 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JordiSubira)

control/drkey/service_engine.go line 52 at r1 (raw file):

Previously, JordiSubira wrote…

We decided to have one interface that hides behinds the logic to derive/get/delete keys. It will be used in the grpc_drkey_service, the prefetcher the TaskConfig` in the CS and some UT relative to those. It is true that probably will have only the serviceEngine struct implementing this interface

I'm not against the interface per se. It's just like this seems like a misuse of the Go interface.
Usually interfaces should be at the place where they are used (not at the place where they are defined).
In main you instantiate the concrete type and plug them in the place where an interface is expected.

So for example to me it seems that the server will likely not need access to the DeleteExpired* methods. So I would just define on the server the interface that I need for the server. Similar for the prefetcher.

control/drkey/service_engine.go line 86 at r1 (raw file):

Previously, JordiSubira wrote…

The idea was to use the constructor that hides the secretValeBackend and the PrefetchKeeper. I can make it public if you thing that's better.

Why do we need to hide the secretValueBackend? I'm fine with either way. But I think the Struct should be public either way otherwise it's not really usable in other packages.

control/drkey/service_engine.go line 137 at r1 (raw file):

Previously, JordiSubira wrote…

The prefetcher and the serviceEngine. The idea is not updating the prefetchKeeper for those keys that are prefetch, so that we avoid having always the same keys in this list. It would only be updated when keys are requested by some host.

I see, as long at it's all local to this package it's fine.

JordiSubira

Reviewable status: 7 of 14 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)

control/drkey/service_engine.go line 52 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I'm not against the interface per se. It's just like this seems like a misuse of the Go interface.
Usually interfaces should be at the place where they are used (not at the place where they are defined).
In main you instantiate the concrete type and plug them in the place where an interface is expected.

So for example to me it seems that the server will likely not need access to the DeleteExpired* methods. So I would just define on the server the interface that I need for the server. Similar for the prefetcher.

You're right probably we're overusing this interface. I will remove it from this file and redefine a new interface in the server and the prefetcher.

control/drkey/service_engine.go line 86 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Why do we need to hide the secretValueBackend? I'm fine with either way. But I think the Struct should be public either way otherwise it's not really usable in other packages.

I made it public and removed the constructor.

lukedirtwalker

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @JordiSubira)

lukedirtwalker

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @JordiSubira)

This PR is the 5th in a series of PRs that add support for DRKey started with scionproto#4217. This PR contains: - The `ServiceEngine` for the CS - The `secretValueEngine` for the CS - The ARC adaptive cache to keep information about the remote keys that are fetch most frequently - UT

JordiSubira force-pushed the drkey_5 branch 2 times, most recently from 2eaf0b1 to b440af1 Compare July 13, 2022 12:03

JordiSubira changed the title ~~drkeu: add serviceEngine and secretValueEngine~~ drkey: add serviceEngine and secretValueEngine Jul 15, 2022

lukedirtwalker self-assigned this Jul 15, 2022

lukedirtwalker requested changes Jul 18, 2022

View reviewed changes

JordiSubira commented Jul 20, 2022

View reviewed changes

JordiSubira force-pushed the drkey_5 branch 2 times, most recently from 76fb81c to 6d88102 Compare July 20, 2022 12:13

JordiSubira commented Jul 20, 2022

View reviewed changes

lukedirtwalker requested changes Jul 21, 2022

View reviewed changes

JordiSubira commented Jul 21, 2022

View reviewed changes

lukedirtwalker approved these changes Jul 21, 2022

View reviewed changes

JordiSubira added 3 commits July 21, 2022 17:35

add serviceEngine and secretValueEngine

ffa6079

r1

8d1429f

r2

b5b4d60

lukedirtwalker force-pushed the drkey_5 branch from e1e90f8 to b5b4d60 Compare July 21, 2022 15:35

lukedirtwalker enabled auto-merge (squash) July 21, 2022 15:39

lukedirtwalker approved these changes Jul 21, 2022

View reviewed changes

lukedirtwalker merged commit b353321 into scionproto:master Jul 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

drkey: add serviceEngine and secretValueEngine #4225

drkey: add serviceEngine and secretValueEngine #4225

Uh oh!

JordiSubira commented Jul 12, 2022 •

edited by worxli

Loading

Uh oh!

lukedirtwalker left a comment

Uh oh!

JordiSubira left a comment

Uh oh!

JordiSubira left a comment

Uh oh!

lukedirtwalker left a comment

Uh oh!

JordiSubira left a comment

Uh oh!

lukedirtwalker left a comment

Uh oh!

lukedirtwalker left a comment

Uh oh!

Uh oh!

drkey: add serviceEngine and secretValueEngine #4225

drkey: add serviceEngine and secretValueEngine #4225

Uh oh!

Conversation

JordiSubira commented Jul 12, 2022 • edited by worxli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukedirtwalker left a comment

Choose a reason for hiding this comment

Uh oh!

JordiSubira left a comment

Choose a reason for hiding this comment

Uh oh!

JordiSubira left a comment

Choose a reason for hiding this comment

Uh oh!

lukedirtwalker left a comment

Choose a reason for hiding this comment

Uh oh!

JordiSubira left a comment

Choose a reason for hiding this comment

Uh oh!

lukedirtwalker left a comment

Choose a reason for hiding this comment

Uh oh!

lukedirtwalker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JordiSubira commented Jul 12, 2022 •

edited by worxli

Loading