BS: Add revoker for IFs with missing keepalive #2625

lukedirtwalker · 2019-04-30T09:32:56Z

Adds a Revoker task that issues revocations for interfaces that have timed
and renews evocations for already revoked interfaces periodically.

Fixes #2566

This change is

oncilla

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @lukedirtwalker)

a discussion (no related file):
Please add a description what the code change does.

go/beacon_srv/internal/ifstate/revoker.go, line 51 at r1 (raw file):

var _ periodic.Task = (*Revoker)(nil)

// Revoker revokes interfaces for which we didn't receive an ifid keepalive message for too long.

"Revoker issues revocations for interfaces that have timed out. Revocations for already revoked interfaces are renewed periodically."

go/beacon_srv/internal/ifstate/revoker.go, line 59 at r1 (raw file):

}

func NewRevoker(intfs *Interfaces, msger infra.Messenger,

comment

go/beacon_srv/internal/ifstate/revoker.go, line 71 at r1 (raw file):

}

func (r *Revoker) Run(ctx context.Context) {

comment

go/beacon_srv/internal/ifstate/revoker.go, line 81 at r1 (raw file):

	for ifid, intf := range r.intfs.All() {
		if intf.Expire() {
			var existingRevInfo *path_mgmt.RevInfo

the logic here is a bit too complex.
All we care about whether we should reissue the revocation or not.

var validRev bool
if srev := intf.Revocation(); srev != nil {
  if rev, err := srev.RevInfo(); err == nil && rev.RelativeTTL(time.Now()) >= r.cfg.RegOverlap {
    validRev = true
  }
} else {
  log.Info("[Revoker] Interface went down", "ifid", ifid)
}
if !validRev {
  ifids = append(ifids, ifid)
}

go/beacon_srv/internal/ifstate/revoker.go, line 93 at r1 (raw file):

				log.Info("[Revoker] interface went down", "ifid", ifid)
			}
			if sendRevocation := existingRevInfo == nil ||

sendRevocation is not required, just have the expression itself here

go/beacon_srv/internal/ifstate/revoker.go, line 100 at r1 (raw file):

	}
	if len(revIfIds) > 0 {
		revs := r.createRevocations(revIfIds)

For the IfStateReq handler we will need to create the IfStateInfos from the Interfaces object anyway.
I think it would be easier to do the following:

iterate over all ifids that require revocation and issue the revocation
call r.intfs.IfStateInfo() that returns the IfStateInfo with all revocations.
pass that to the push methods.

This also prevents the weird delete statements below.

go/beacon_srv/internal/ifstate/revoker.go, line 108 at r1 (raw file):

}

func (r *Revoker) createRevocations(

The logic is wrong here. This only returns the freshly issued revocations, not existing ones.
On the BR this would delete existing revocations, possibly causing black holes.

go/beacon_srv/internal/ifstate/revoker.go, line 177 at r1 (raw file):

			NextHop: br.CtrlAddrs.OverlayAddr(topo.Overlay),
		}
		if err := r.msger.SendIfStateInfos(ctx, msg, a, messenger.NextId()); err != nil {

slow speakers will do head-of-line blocking.
This might very well be the case, since a BR might be down (causing the revocation in the first place)

go/beacon_srv/internal/ifstate/revoker_test.go, line 57 at r1 (raw file):

func TestMain(m *testing.M) {
	itopo.Init("", proto.ServiceType_unset, itopo.Callbacks{})

Add log suppression.

go/beacon_srv/internal/ifstate/revoker_test.go, line 71 at r1 (raw file):

		ctx, cancelF := context.WithTimeout(context.Background(), timeout)
		defer cancelF()
		mctrl := gomock.NewController(t)

our style is to have gomock directly after the Convey.

ctx is only used a couple of lines down anyway.

go/beacon_srv/internal/ifstate/revoker_test.go, line 109 at r1 (raw file):

		revoker.Run(ctx)
		Convey("Check interface state", func() {
			for ifid, intf := range intfs.All() {

for ifid, intf := range intfs.All() {
	if ifid == 101 {
		continue
	}
	SoMsg(fmt.Sprintf("Intf %d should be active", ifid),
		intf.State(), ShouldEqual, Active)
}
SoMsg("Intf 101 should be revoked", intf.State(), ShouldEqual, Revoked)

go/beacon_srv/internal/ifstate/revoker_test.go, line 119 at r1 (raw file):

			}
		})
		checkSentMessages(t, verifier)

just pass revVerifier(pub)

go/beacon_srv/internal/ifstate/revoker_test.go, line 149 at r1 (raw file):

		revoker.Run(ctx)
		// gomock tests that no calls to the messenger are made.
		Convey("Check interface state didn't change", func() {

ditto

go/beacon_srv/internal/ifstate/revoker_test.go, line 151 at r1 (raw file):

		Convey("Check interface state didn't change", func() {
			for ifid, intf := range intfs.All() {
				if ifid == 101 {

this should also check that the revocation is the same as before

go/beacon_srv/internal/ifstate/revoker_test.go, line 191 at r1 (raw file):

		revoker.Run(ctx)
		// gomock tests that no calls to the messenger are made.
		Convey("Check interface state didn't change", func() {

ditto

go/beacon_srv/internal/ifstate/revoker_test.go, line 195 at r1 (raw file):

				if ifid == 101 {
					SoMsg(fmt.Sprintf("Intf %d should be revoked", ifid),
						intf.State(), ShouldEqual, Revoked)

check the revocation differs

go/beacon_srv/internal/ifstate/revoker_test.go, line 313 at r1 (raw file):

}

func brCount() int {

this is overkill

lukedirtwalker

Reviewable status: 2 of 4 files reviewed, 18 unresolved discussions (waiting on @oncilla)

a discussion (no related file):

Previously, Oncilla wrote…

Please add a description what the code change does.

Done.

go/beacon_srv/internal/ifstate/revoker.go, line 51 at r1 (raw file):

Previously, Oncilla wrote…

"Revoker issues revocations for interfaces that have timed out. Revocations for already revoked interfaces are renewed periodically."

Done.

go/beacon_srv/internal/ifstate/revoker.go, line 59 at r1 (raw file):

Previously, Oncilla wrote…

comment

Done.

go/beacon_srv/internal/ifstate/revoker.go, line 71 at r1 (raw file):

Previously, Oncilla wrote…

comment

Done.

go/beacon_srv/internal/ifstate/revoker.go, line 81 at r1 (raw file):

Previously, Oncilla wrote…

the logic here is a bit too complex.
All we care about whether we should reissue the revocation or not.

var validRev bool
if srev := intf.Revocation(); srev != nil {
  if rev, err := srev.RevInfo(); err == nil && rev.RelativeTTL(time.Now()) >= r.cfg.RegOverlap {
    validRev = true
  }
} else {
  log.Info("[Revoker] Interface went down", "ifid", ifid)
}
if !validRev {
  ifids = append(ifids, ifid)
}

ok simplified it a bit more by adding a method hasValidRevocation

go/beacon_srv/internal/ifstate/revoker.go, line 93 at r1 (raw file):

Previously, Oncilla wrote…

sendRevocation is not required, just have the expression itself here

Done.

go/beacon_srv/internal/ifstate/revoker.go, line 100 at r1 (raw file):

Previously, Oncilla wrote…

For the IfStateReq handler we will need to create the IfStateInfos from the Interfaces object anyway.
I think it would be easier to do the following:

iterate over all ifids that require revocation and issue the revocation

call r.intfs.IfStateInfo() that returns the IfStateInfo with all revocations.

pass that to the push methods.

This also prevents the weird delete statements below.

We only send the revoked interfaces and not all. I got rid of the weird deletion though and simplified the code a bit. I will consider code sharing with IfStateReq handler once I implement it.

go/beacon_srv/internal/ifstate/revoker.go, line 108 at r1 (raw file):

Previously, Oncilla wrote…

The logic is wrong here. This only returns the freshly issued revocations, not existing ones.
On the BR this would delete existing revocations, possibly causing black holes.

No it is not, we only send the newly revoked interfaces the BR should not make any assumption about the rest of the interfaces.

Though we might consider to always send everything if it simplifies things once we have the ifstatereq handler.

go/beacon_srv/internal/ifstate/revoker.go, line 177 at r1 (raw file):

Previously, Oncilla wrote…

slow speakers will do head-of-line blocking.
This might very well be the case, since a BR might be down (causing the revocation in the first place)

Done.

go/beacon_srv/internal/ifstate/revoker_test.go, line 57 at r1 (raw file):

Previously, Oncilla wrote…

Add log suppression.

Done.

go/beacon_srv/internal/ifstate/revoker_test.go, line 71 at r1 (raw file):

Previously, Oncilla wrote…

our style is to have gomock directly after the Convey.

ctx is only used a couple of lines down anyway.

Done.

go/beacon_srv/internal/ifstate/revoker_test.go, line 109 at r1 (raw file):

Previously, Oncilla wrote…

for ifid, intf := range intfs.All() {
	if ifid == 101 {
		continue
	}
	SoMsg(fmt.Sprintf("Intf %d should be active", ifid),
		intf.State(), ShouldEqual, Active)
}
SoMsg("Intf 101 should be revoked", intf.State(), ShouldEqual, Revoked)

I added a helper method for this.

go/beacon_srv/internal/ifstate/revoker_test.go, line 119 at r1 (raw file):

Previously, Oncilla wrote…

just pass revVerifier(pub)

Done.

go/beacon_srv/internal/ifstate/revoker_test.go, line 149 at r1 (raw file):

Previously, Oncilla wrote…

ditto

Done.

go/beacon_srv/internal/ifstate/revoker_test.go, line 151 at r1 (raw file):

Previously, Oncilla wrote…

this should also check that the revocation is the same as before

Done.

go/beacon_srv/internal/ifstate/revoker_test.go, line 191 at r1 (raw file):

Previously, Oncilla wrote…

ditto

Done.

go/beacon_srv/internal/ifstate/revoker_test.go, line 195 at r1 (raw file):

Previously, Oncilla wrote…

check the revocation differs

Done.

go/beacon_srv/internal/ifstate/revoker_test.go, line 313 at r1 (raw file):

Previously, Oncilla wrote…

this is overkill

Done.

oncilla

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lukedirtwalker)

go/beacon_srv/internal/ifstate/revoker.go, line 59 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Done.

lol, but okay.

go/beacon_srv/internal/ifstate/revoker.go, line 15 at r2 (raw file):

// limitations under the License.

package ifstate

Package level doc would also be nice.

go/beacon_srv/internal/ifstate/revoker.go, line 81 at r3 (raw file):

	for ifid, intf := range r.intfs.All() {
		if intf.Expire() && !r.hasValidRevocation(intf) {
			log.Info("[Revoker] interface went down", "ifid", ifid)

This periodically announces the interfaces being down. And not on first occurrence.

go/beacon_srv/internal/ifstate/revoker.go, line 90 at r3 (raw file):

				revs[ifid] = srev
			} else {
				log.Error("Failed to revoke!", "err", err)

Add ifid info and [Revoker]

go/beacon_srv/internal/ifstate/revoker.go, line 114 at r3 (raw file):

	revInfo := &path_mgmt.RevInfo{
		IfID:         ifid,
		RawIsdas:     itopo.Get().ISD_AS.IAInt(),

We should limit the number of places we use itopo since it is global (horrible but necessary) state.
ia might be as well be passed in the constructor.
LinkType is available through r.intfs.Get(ifid).TopoInfo().LinkType

go/beacon_srv/internal/ifstate/revoker.go, line 144 at r3 (raw file):

		})
	}
	for brId, br := range topo.BR {

Info could easily come from r.intfs.All()

go/beacon_srv/internal/ifstate/revoker.go, line 152 at r3 (raw file):

	revs map[common.IFIDType]*path_mgmt.SignedRevInfo) {

	topo := itopo.Get()

This shares code with registrar.localServer.
Thinking about it, it would be nice to have something like:

type Selector interface {
    AnySvc(svc proto.ServiceType) (*snet.Addr, error)
}

That can be passed in through the constructor.
The implementation would essentially do what is here. But the revoker does not need to care about that.

We could reduce the usage of itopo almost everywhere in beacon_srv/internal with this.

go/beacon_srv/internal/ifstate/revoker.go, line 175 at r3 (raw file):

}

func (r *Revoker) sendToBr(ctx context.Context, brId string, br topology.BRInfo,

method order is odd.

go/beacon_srv/internal/ifstate/revoker_test.go, line 71 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Done.

I was more thinking of moving ctx declaration to where it is used.

Fixes scionproto#2566

lukedirtwalker

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

go/beacon_srv/internal/ifstate/revoker.go, line 15 at r2 (raw file):

Previously, Oncilla wrote…

Package level doc would also be nice.

Done.

go/beacon_srv/internal/ifstate/revoker.go, line 81 at r3 (raw file):

Previously, Oncilla wrote…

This periodically announces the interfaces being down. And not on first occurrence.

Done.

go/beacon_srv/internal/ifstate/revoker.go, line 90 at r3 (raw file):

Previously, Oncilla wrote…

Add ifid info and [Revoker]

Done.

go/beacon_srv/internal/ifstate/revoker.go, line 114 at r3 (raw file):

Previously, Oncilla wrote…

We should limit the number of places we use itopo since it is global (horrible but necessary) state.
ia might be as well be passed in the constructor.
LinkType is available through r.intfs.Get(ifid).TopoInfo().LinkType

I introduced TopoProvider field/interface

go/beacon_srv/internal/ifstate/revoker.go, line 144 at r3 (raw file):

Previously, Oncilla wrote…

Info could easily come from r.intfs.All()

Hm but then we would resend revocations that might already have been there. Like this we only send the new information.

go/beacon_srv/internal/ifstate/revoker.go, line 152 at r3 (raw file):

Previously, Oncilla wrote…

This shares code with registrar.localServer.
Thinking about it, it would be nice to have something like:
type Selector interface {
    AnySvc(svc proto.ServiceType) (*snet.Addr, error)
}
That can be passed in through the constructor.
The implementation would essentially do what is here. But the revoker does not need to care about that.

We could reduce the usage of itopo almost everywhere in beacon_srv/internal with this.

I added a TopoProvider that already helps with the itopo removal.
I prefer to tackle this in a separate PR since it touches more code, create #2629 to track it.

go/beacon_srv/internal/ifstate/revoker.go, line 175 at r3 (raw file):

Previously, Oncilla wrote…

method order is odd.

Done.

lukedirtwalker

Reviewable status: 1 of 7 files reviewed, 8 unresolved discussions (waiting on @oncilla)

go/beacon_srv/internal/ifstate/revoker_test.go, line 71 at r1 (raw file):

Previously, Oncilla wrote…

I was more thinking of moving ctx declaration to where it is used.

Done.

oncilla

Reviewed 5 of 6 files at r4.
Reviewable status: 6 of 7 files reviewed, all discussions resolved (waiting on @oncilla)

go/beacon_srv/internal/ifstate/revoker.go, line 144 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Hm but then we would resend revocations that might already have been there. Like this we only send the new information.

No, I meant which BR's to send to.
But with the new interface, this is fine.

oncilla

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

lukedirtwalker requested a review from oncilla April 30, 2019 09:32

oncilla requested changes Apr 30, 2019

View reviewed changes

lukedirtwalker commented Apr 30, 2019

View reviewed changes

lukedirtwalker force-pushed the pubBSRevoker branch from fad50a5 to 7f066d9 Compare May 2, 2019 06:02

scrye added this to the Q2S2 milestone May 2, 2019

oncilla requested changes May 2, 2019

View reviewed changes

lukedirtwalker added 4 commits May 2, 2019 14:51

BS: Add revoker for IFs with missing keepalive

86e2206

Fixes scionproto#2566

fix lint

c4dd90a

r1

4b84207

r3

ae24a6c

lukedirtwalker force-pushed the pubBSRevoker branch from 7f066d9 to ae24a6c Compare May 2, 2019 12:52

lukedirtwalker commented May 2, 2019

View reviewed changes

oncilla reviewed May 2, 2019

View reviewed changes

oncilla approved these changes May 2, 2019

View reviewed changes

lukedirtwalker merged commit 2ed1277 into scionproto:master May 2, 2019

lukedirtwalker deleted the pubBSRevoker branch May 2, 2019 14:45

oncilla mentioned this pull request May 16, 2019

BS: send RevInfos to PS, not IFStateInfos. #1549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BS: Add revoker for IFs with missing keepalive #2625

BS: Add revoker for IFs with missing keepalive #2625

lukedirtwalker commented Apr 30, 2019 •

edited

Loading

oncilla left a comment

lukedirtwalker left a comment

oncilla left a comment

lukedirtwalker left a comment

lukedirtwalker left a comment

oncilla left a comment

oncilla left a comment

BS: Add revoker for IFs with missing keepalive #2625

BS: Add revoker for IFs with missing keepalive #2625

Conversation

lukedirtwalker commented Apr 30, 2019 • edited Loading

oncilla left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

lukedirtwalker commented Apr 30, 2019 •

edited

Loading