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

Control plane clients can now perform SVC resolution #2627

Merged
merged 3 commits into from
May 3, 2019
Merged

Control plane clients can now perform SVC resolution #2627

merged 3 commits into from
May 3, 2019

Conversation

scrye
Copy link
Contributor

@scrye scrye commented Apr 30, 2019

If backwards compatibility is desired, speakers with SVC resolution
support can be configured to fall back to using SVC addresses for data
packets.


This change is Reviewable

@scrye scrye requested a review from oncilla April 30, 2019 14:21
Copy link
Contributor

@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 18 of 18 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @scrye)


go/lib/infra/messenger/addr.go, line 36 at r1 (raw file):

}

// AddressRewriter is used to compute paths and unicast addresses for SVC

This makes it sound like AddressRewriter only computes paths for SVC destinations instead of all destinations.
How about: "AddressRewriter "


go/lib/infra/messenger/addr.go, line 59 at r1 (raw file):

}

// Rewrite takes address a and adds a path (if one does not already exist but

I do not like the a here, it is weird to read. Plus renaming the parameter could easily out-date the comment.
How about: "Rewrite takes an address and adds"


go/lib/infra/messenger/addr.go, line 118 at r1 (raw file):

}

// resolveIfSvc returns an UDP/IP address if the input address is an SVC

This should mention what happens if the destination is not an SVC address.


go/lib/infra/messenger/addr.go, line 169 at r1 (raw file):

}

func getSVC(a addr.HostAddr) addr.HostSVC {

This only introduces an additional function.
The number of if statements is not changed in resolveIfSVC
Just use ok directly.

Added bonus one less occurrence of addr.SvcNone in the code.


go/lib/infra/messenger/messenger.go, line 110 at r1 (raw file):

	// TrustStore stores and retrieves certificate chains and TRCs.
	TrustStore infra.TrustStore
	// AddressRewriter is used to compute paths and unicast addresses for SVC

this makes it sound like AddressRewriter is only used for SVC destinations.

scrye added 3 commits May 3, 2019 11:13
If backwards compatibility is desired, speakers with SVC resolution
support can be configured to fall back to using SVC addresses for data
packets.
Copy link
Contributor Author

@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, 5 unresolved discussions (waiting on @oncilla)


go/lib/infra/messenger/addr.go, line 36 at r1 (raw file):

Previously, Oncilla wrote…

This makes it sound like AddressRewriter only computes paths for SVC destinations instead of all destinations.
How about: "AddressRewriter "

Think that's a bit short 😛

Tried to reword it, lemme know what you think.


go/lib/infra/messenger/addr.go, line 59 at r1 (raw file):

Previously, Oncilla wrote…

I do not like the a here, it is weird to read. Plus renaming the parameter could easily out-date the comment.
How about: "Rewrite takes an address and adds"

Done.


go/lib/infra/messenger/addr.go, line 118 at r1 (raw file):

Previously, Oncilla wrote…

This should mention what happens if the destination is not an SVC address.

Done.


go/lib/infra/messenger/addr.go, line 169 at r1 (raw file):

Previously, Oncilla wrote…

This only introduces an additional function.
The number of if statements is not changed in resolveIfSVC
Just use ok directly.

Added bonus one less occurrence of addr.SvcNone in the code.

Done.


go/lib/infra/messenger/messenger.go, line 110 at r1 (raw file):

Previously, Oncilla wrote…

this makes it sound like AddressRewriter is only used for SVC destinations.

Done.

Copy link
Contributor

@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 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


go/lib/infra/messenger/addr.go, line 36 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Think that's a bit short 😛

Tried to reword it, lemme know what you think.

Argh, forgot to type there :0
looks good

@scrye scrye merged commit dc64e92 into scionproto:master May 3, 2019
@scrye scrye deleted the pubpr-msger-resolves-svc branch May 3, 2019 09:22
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.

2 participants