TrustStore: Add chain request handler

[oncilla]

Add certificate chain request handler.

fixes #3485

---

This change is 

[karampok]

Reviewed 1 of 4 files at r1.
Reviewable status: 0 of 3 files reviewed, 8 unresolved discussions (waiting on @karampok and @oncilla)

---

go/lib/infra/modules/trust/v2/handlers.go, line 36 at r1 (raw file):

	provider CryptoProvider
}

I will only comment the Handler() or the NewHandler function that we might need later

---

go/lib/infra/modules/trust/v2/handlers.go, line 42 at r1 (raw file):

		return infra.MetricsErrInternal
	}
	logger := log.FromCtx(h.request.Context())

maybe worth to have
ctx=h.request.Contect(), imo it will make the code more readable

---

go/lib/infra/modules/trust/v2/handlers.go, line 45 at r1 (raw file):

	chainReq, ok := h.request.Message.(*cert_mgmt.ChainReq)
	if !ok {
		logger.Error("[TrustStore:chainReqHandler] wrong message type, expected cert_mgmt.ChainReq",

For smaller lines, I would have the "[Trustore:chaineReqHandler]" in a const variable and all the logs one line
(and also allow me to copy stuff and correctly send up log tags)

---

go/lib/infra/modules/trust/v2/handlers.go, line 61 at r1 (raw file):

		TrustStoreOpts: infra.TrustStoreOpts{
			LocalOnly: chainReq.CacheOnly,
		},

Why two levels of nesting, can we have one?

---

go/lib/infra/modules/trust/v2/handlers.go, line 63 at r1 (raw file):

		},
		AllowInactiveTRC: true,
	}

Before we go to provider, ideally there should be a validate step, can we/makes sense to have a validation step? (in the future)

---

go/lib/infra/modules/trust/v2/handlers.go, line 64 at r1 (raw file):

		AllowInactiveTRC: true,
	}
	raw, err := h.provider.GetRawChain(h.request.Context(), chainReq.IA(), chainReq.Version,

You have chain.IA() and peer.IA how are connected these two? or can be irrelevant.
Why the provider needs the peer?

You have chainReq.IA/Version maybe you can just pass the chainReq as argument (maybe reduce the TrustStore config)

As I see the code a function definition as follow seems enough
`func GetRawChain(context, chainReq, AllowInactiveTRC bool)

---

go/lib/infra/modules/trust/v2/handlers_test.go, line 140 at r1 (raw file):

		},
	}
	for name, test := range tests {

Do you see we should enable the parallel?

---

go/lib/infra/modules/trust/v2/handlers_test.go, line 143 at r1 (raw file):

		t.Run(name, func(t *testing.T) {
			ctrl := gomock.NewController(t)
			defer ctrl.Finish()

rw := mock_infra.NewMockResponseWriter(ctrl) 
 p := mock_v2.NewMockCryptoProvider(ctrl)  

handler:=&trust. ChainReqHandler{ //exported through test_export
		request:  test.request(rw),  // a bit more intuitive the arg to be of rw interface if possible and you save lines per case
		provider: test.provider(p),
	}
 result := handler.Handle() 

In this unittest I expect only to test only the Handle(), there should a different unit-test function for the NewChainReqHandler

[oncilla]

Reviewable status: 0 of 3 files reviewed, 8 unresolved discussions (waiting on @karampok and @oncilla)

---

go/lib/infra/modules/trust/v2/handlers.go, line 36 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

I will only comment the Handler() or the NewHandler function that we might need later

There will be a factory method on the trust store to do that.

---

go/lib/infra/modules/trust/v2/handlers.go, line 42 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

maybe worth to have
ctx=h.request.Contect(), imo it will make the code more readable

Done.

---

go/lib/infra/modules/trust/v2/handlers.go, line 45 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

For smaller lines, I would have the "[Trustore:chaineReqHandler]" in a const variable and all the logs one line
(and also allow me to copy stuff and correctly send up log tags)

I tried having:

const tagChainReq = "[TrustStore:chainReqHandler] "
...
logger.Error(tagChainReq+"Unable to service request, ...")

and

func (h *chainReqHandler) tag() string {
    return "[TrustStore:chainReqHandler] "
}
...
logger.Error(h.tag()+"Unable to service request, ...")

but both of them look horrible.
what did you have in mind?

---

go/lib/infra/modules/trust/v2/handlers.go, line 61 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Why two levels of nesting, can we have one?

TrustStoreOpts are general options regarding the trust store.
ChainOpts is specific to chain requests.

---

go/lib/infra/modules/trust/v2/handlers.go, line 63 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Before we go to provider, ideally there should be a validate step, can we/makes sense to have a validation step? (in the future)

There is not much that can be invalid here. IA and version, nothing else is provided.

---

go/lib/infra/modules/trust/v2/handlers.go, line 64 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

You have chain.IA() and peer.IA how are connected these two? or can be irrelevant.
Why the provider needs the peer?

You have chainReq.IA/Version maybe you can just pass the chainReq as argument (maybe reduce the TrustStore config)

As I see the code a function definition as follow seems enough
`func GetRawChain(context, chainReq, AllowInactiveTRC bool)

chainReq.IA() and peer.IA are completely unrelated.
chainReq.IA() tells us what certificate chain is requested.
peer.IA tells us who requested the certificate chain.

Conceptually, chainReq is a network request. I don't think it fits well to use it as arguments to the GetRawChain function, because it is not required to stem from the network.

---

go/lib/infra/modules/trust/v2/handlers_test.go, line 140 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Do you see we should enable the parallel?

I cannot really parse the sentence.

Are you asking whether we should enable parallel testing?
If so, done.

---

go/lib/infra/modules/trust/v2/handlers_test.go, line 143 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

rw := mock_infra.NewMockResponseWriter(ctrl) 
 p := mock_v2.NewMockCryptoProvider(ctrl)  

handler:=&trust. ChainReqHandler{ //exported through test_export
		request:  test.request(rw),  // a bit more intuitive the arg to be of rw interface if possible and you save lines per case
		provider: test.provider(p),
	}
 result := handler.Handle() 

In this unittest I expect only to test only the Handle(), there should a different unit-test function for the NewChainReqHandler

The NewChainReqHandler is only there for testing (it is part of export_test.go).
It is not an exported constructor.

[karampok]

Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

go/lib/infra/modules/trust/v2/handlers.go, line 36 at r1 (raw file):

Previously, Oncilla wrote…

There will be a factory method on the trust store to do that.

Do you have a reason why you comment the unexported struct and not the exported method?
Would not make sense to have the New function as part of that PR?

---

go/lib/infra/modules/trust/v2/handlers.go, line 45 at r1 (raw file):

Previously, Oncilla wrote…

I tried having:

const tagChainReq = "[TrustStore:chainReqHandler] "
...
logger.Error(tagChainReq+"Unable to service request, ...")

and

func (h *chainReqHandler) tag() string {
    return "[TrustStore:chainReqHandler] "
}
...
logger.Error(h.tag()+"Unable to service request, ...")

but both of them look horrible.
what did you have in mind?

const logtag= ""
logger.Error (tag, "custom message", ..) 

looks good enough to me.

Ideally it would be

logger:=logger.WithCtx(ctx).WithTag(logtag)

but not sure that the log framework supports it

---

go/lib/infra/modules/trust/v2/handlers.go, line 61 at r1 (raw file):

Previously, Oncilla wrote…

TrustStoreOpts are general options regarding the trust store.
ChainOpts is specific to chain requests.

LocalOnly:chainReq.CacheOnly is what @lukedirtwalker asked in slack that we can remove?

---

go/lib/infra/modules/trust/v2/handlers.go, line 64 at r1 (raw file):

Previously, Oncilla wrote…

chainReq.IA() and peer.IA are completely unrelated.
chainReq.IA() tells us what certificate chain is requested.
peer.IA tells us who requested the certificate chain.

Conceptually, chainReq is a network request. I don't think it fits well to use it as arguments to the GetRawChain function, because it is not required to stem from the network.

Imo more than three arguments in the function, we should have struct as an argument:
You did not answer

1. Do we need the peer in the implementation of the provider?

I would like to see the interface function to be simpler but if you think it can not be, then okay for me

---

go/lib/infra/modules/trust/v2/handlers.go, line 83 at r2 (raw file):

		return infra.MetricsErrTrustStore(err)
	}
	chainMessage := &cert_mgmt.Chain{

maybe rename to reply

---

go/lib/infra/modules/trust/v2/handlers_test.go, line 140 at r1 (raw file):

Previously, Oncilla wrote…

I cannot really parse the sentence.

Are you asking whether we should enable parallel testing?
If so, done.

That's exactly what I mean.

---

go/lib/infra/modules/trust/v2/handlers_test.go, line 143 at r1 (raw file):

Previously, Oncilla wrote…

The NewChainReqHandler is only there for testing (it is part of export_test.go).
It is not an exported constructor.

That is clear, what is the reason not to use the struct directly?

[oncilla]

Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @karampok, @lukedirtwalker, and @oncilla)

---

go/lib/infra/modules/trust/v2/handlers.go, line 36 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Do you have a reason why you comment the unexported struct and not the exported method?
Would not make sense to have the New function as part of that PR?

Those comments are there to help people that read the code to understand what the struct's purpose is.
It's not there for godoc purposes.

I don't see the need to comment the Handle method, as it is pretty self explanatory.

As the "constructor" will be a factory method on the store, and that does not exist yet, we cannot do so.

---

go/lib/infra/modules/trust/v2/handlers.go, line 45 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

const logtag= ""
logger.Error (tag, "custom message", ..) 

looks good enough to me.

Ideally it would be

logger:=logger.WithCtx(ctx).WithTag(logtag)

but not sure that the log framework supports it

Our log framework does not support either of these options, unfortunately.

---

go/lib/infra/modules/trust/v2/handlers.go, line 61 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

LocalOnly:chainReq.CacheOnly is what @lukedirtwalker asked in slack that we can remove?

yes, but let me purge CacheOnly in a seperate PR, as it will touch more than this handler.

---

go/lib/infra/modules/trust/v2/handlers.go, line 64 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Imo more than three arguments in the function, we should have struct as an argument:
You did not answer

1. Do we need the peer in the implementation of the provider?

I would like to see the interface function to be simpler but if you think it can not be, then okay for me

what we would do is:

package trc

type ID struct {
    ISD addr.ISD
    Version scrypto.Version
}
----
package cert

type ID struct {
    IA addr.IA
    Version scrypto.Version
}
----
type CryptoProvider interface {
	GetTRC(ctx context.Context, id trc.ID, opts infra.TRCOpts) (*trc.TRC, error)
	GetRawTRC(ctx context.Context, id trc.ID, opts infra.TRCOpts, opts infra.TRCOpts) ([]byte, error)
	GetRawChain(ctx context.Context, id chain.ID, opts infra.ChainOpts) ([]byte, error)
}

and put client net.Addr into the infra.TrustStoreOpts

Sounds good?

But again, in a separate PR.

---

go/lib/infra/modules/trust/v2/handlers.go, line 83 at r2 (raw file):

Previously, karampok (Konstantinos) wrote…

maybe rename to reply

Done.

---

go/lib/infra/modules/trust/v2/handlers_test.go, line 143 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

That is clear, what is the reason not to use the struct directly?

The type has private fields, I don't know of any other way to initialize them through a constructor, (or reflection)

[karampok]

Reviewed 2 of 4 files at r2, 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

---

go/lib/infra/modules/trust/v2/handlers.go, line 45 at r1 (raw file):

Previously, Oncilla wrote…

Our log framework does not support either of these options, unfortunately.

I still would go for logger.Error(tagChainReq+"Unable to service request, ...")
but no strong opinion, resolved.

---

go/lib/infra/modules/trust/v2/handlers.go, line 64 at r1 (raw file):

Previously, Oncilla wrote…

what we would do is:

package trc

type ID struct {
    ISD addr.ISD
    Version scrypto.Version
}
----
package cert

type ID struct {
    IA addr.IA
    Version scrypto.Version
}
----
type CryptoProvider interface {
	GetTRC(ctx context.Context, id trc.ID, opts infra.TRCOpts) (*trc.TRC, error)
	GetRawTRC(ctx context.Context, id trc.ID, opts infra.TRCOpts, opts infra.TRCOpts) ([]byte, error)
	GetRawChain(ctx context.Context, id chain.ID, opts infra.ChainOpts) ([]byte, error)
}

and put client net.Addr into the infra.TrustStoreOpts

Sounds good?

But again, in a separate PR.

This looks wrong but it is a typo

GetRawTRC(ctx context.Context, id trc.ID, opts infra.TRCOpts, opts infra.TRCOpts) ([]byte, error)

Yes that sounds good to me!

(if we add code, just to remove it afterwards, I am not really sure why we add it and not just block on the other PR)

---

go/lib/infra/modules/trust/v2/handlers_test.go, line 143 at r1 (raw file):

Previously, Oncilla wrote…

The type has private fields, I don't know of any other way to initialize them through a constructor, (or reflection)

I see, I think i had to have private struct, public fields, what do you think?
But resolved