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.

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

TrustStore: Add chain push handler #3501

Merged
merged 7 commits into from
Dec 10, 2019

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Dec 9, 2019

Add certificate chain push handler to the trust store.
Currently, unit tests are missing. Edited by @scrye: added unit tests.

fixes #3486


This change is Reviewable

@scrye scrye force-pushed the pub-trust-handler-chain-push branch from 83701a3 to 7cd6086 Compare December 10, 2019 07:56
@scrye scrye self-assigned this Dec 10, 2019
oncilla and others added 3 commits December 10, 2019 14:24
Add certificate chain push handler to the trust store.
Currently, unit tests are missing.

fixes scionproto#3486
@scrye scrye force-pushed the pub-trust-handler-chain-push branch from 5fee5ed to edccd3d Compare December 10, 2019 13:47
Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 4 of 4 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @scrye)


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

// HandlerTimeout is the lifetime of the handlers.
const HandlerTimeout = 3 * time.Second

from @lukedirtwalker review. Drop this. The default handler timeout should be enough.


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

func (h *chainPushHandler) Handle() *infra.HandlerResult {
	if h.request == nil {

log this as an error?


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

	rw, ok := infra.ResponseWriterFromContext(h.request.Context())
	if !ok {
		logger.Warn("[TrustStore:chainPushHandler] Unable to service request, no Messenger found")

s/Messenger/ResponseWriter

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @oncilla)


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

Previously, Oncilla wrote…

log this as an error?

Log to package then? There's no context available to extract the proper logger yet.

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @scrye)


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

Previously, scrye (Sergiu Costea) wrote…

Log to package then? There's no context available to extract the proper logger yet.

IMO, yes.

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

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


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

Previously, Oncilla wrote…

from @lukedirtwalker review. Drop this. The default handler timeout should be enough.

Done.


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

Previously, Oncilla wrote…

IMO, yes.

Done.

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r3.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @oncilla)

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)


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

Previously, Oncilla wrote…

s/Messenger/ResponseWriter

Done.

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion


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

		return infra.MetricsErrInternal
	}
	subCtx, cancelF := context.WithTimeout(h.request.Context(), messenger.DefaultHandlerTimeout)

you don't need a subctx

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)


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

Previously, Oncilla wrote…

you don't need a subctx

Done.

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@scrye scrye merged commit 50b21fa into scionproto:master Dec 10, 2019
@oncilla oncilla deleted the pub-trust-handler-chain-push branch December 11, 2019 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TrustStore: Chain push handler
2 participants