Initial filtering logic / fv logic #9920
Conversation
marvin-tigera
added
release-note-required
Brian-McM
added
docs-not-required
Brian-McM
force-pushed
the
bm-add-filtering-to-whisker-backend
branch
7 times, most recently
from
90b1669 to
cd74b32
Compare
Brian-McM
force-pushed
the
bm-add-filtering-to-whisker-backend
branch
4 times, most recently
from
9959a07 to
1a12222
Compare
| } | ||
|
|
||
| // TODO maybe we can push getting tls files to the common http utilities package? | ||
| if cfg.TlsKeyPath != "" && cfg.TlsCertPath != "" { |
There was a problem hiding this comment.
TLSKeyPath and TLSCertPath?
whisker-backend/pkg/apis/v1/flows.go
Outdated
| type PolicyKind string | ||
|
|
||
| const ( | ||
| PolicyKindUnspecified PolicyKind = "" |
There was a problem hiding this comment.
For my own understanding, could you explain what the benefit of redefining all of these is?
And given there is a benefit, why not keep the values the same as the underlying goldmane API?
There was a problem hiding this comment.
The main reason is that I'm hesitant to import grpc into this API definition. This API doesn't depend on grpc so I'd rather not import it. I'm open to discussing this more, but I do have a strong preference for not bleeding the goldmane API definitions here even if it is tedious to redefine.
The policy kinds are also enums and I think we want to accept strings from the API?
There was a problem hiding this comment.
Is there a real reason that importing gRPC / protobuf is going to have negative engineering consequences here?
There's a real cost with duplicating a copy of this API when as far as I can tell, we could be using the same values.
The policy kinds are also enums and I think we want to accept strings from the API?
Yeah, but see my other comments - the enums have String() methods that return their string representation, and the proto package provides an easy way to map back and forth between the two.
| } | ||
| } | ||
|
|
||
| func protoToPolicyKind(kind proto.PolicyKind) whiskerv1.PolicyKind { |
There was a problem hiding this comment.
I think that all of this code could go away if we just kept the string values consistent with the Goldmane API.
The conversion would look like this:
func protoToPolicyKind(kind proto.PolicyKind) whiskerv1.PolicyKind {
return whiskerv1.PolicyKind(kind.String())
}func policyKindToProto(kind whiskerv1.PolicyKind) proto.PolicyKind {
if v, ok := proto.PolicyKind_value[kind]; ok {
return v
}
return proto.PolicyKindUnspecified
}There was a problem hiding this comment.
Same goes for all of the enum fields
There was a problem hiding this comment.
ref in autogenerated code:
calico/goldmane/proto/api.pb.go
Lines 203 to 228 in 80f5804
There was a problem hiding this comment.
I'm on the fence here, it might become more simple in one regard but in another it's much more fragile.
If any ordering changes then it doesn't match up anymore, same goes for spelling. This is sort of the problem with the lack of go enums, we can case any string as these types.
So I'd rather be explicit here to ensure we have some sort of proper matchup. I was even toying with the idea of panicking if they don't match up since somebody must have changed something for it not to match up.
There was a problem hiding this comment.
If any ordering changes then it doesn't match up anymore, same goes for spelling
Ordering of what changes? Spelling of what changes? I'm afraid I don't follow what scenario you're thinking of where the existing approach is more capable than what I suggested.
This code is inextricably linked to the protobuf API that is backing it, regardless of how we implement this function. But implementing it the way I have suggested makes keeping these two in sync easier and less error prone.
There was a problem hiding this comment.
I updated it to use the proto constants.
| logrus.WithError(err).Fatal("Failed to create server.") | ||
| } | ||
|
|
||
| // TODO Should we require that this is TLS? It will be in the same pod as nginx. |
There was a problem hiding this comment.
If it's in the same pod, then we don't need TLS. Can remove this TODO
whisker-backend/cmd/app/app.go
Outdated
| func Run(ctx context.Context, cfg *config.Config) { | ||
| logrus.WithField("cfg", cfg.String()).Info("Applying configuration...") | ||
|
|
||
| // TODO not sure if we're going to require TLS communication since these will be part of the same pod, at least |
There was a problem hiding this comment.
We will need TLS here - plan is to have Goldmane in a separate deployment no?
There was a problem hiding this comment.
oooff I was hoping that me moving this from the main file to app wouldn't require any changes (these are all follow up things I need to do that weren't introduced here).
Any change you can just pretend this file wasn't committed, since it's really just the main file :)?
| BytesOut int64 `json:"bytes_out"` | ||
| StartTime time.Time `json:"start_time"` | ||
| EndTime time.Time `json:"end_time"` | ||
| Action Action `json:"action"` |
There was a problem hiding this comment.
I do wonder if we're just making things harder for ourselves by not using a consistent, 1:1 representation of a Flow here.
I know the UI already expects this, but perhaps in a future PR before release we can fix this up to more closely resemble the representation of a Flow we use throughout the rest of OSS Calico? e.g., with flow.Key.Action instead of flow.Action?
There was a problem hiding this comment.
I'd have to think about this, you may be right, but this is intended to be an API to best serve the UI and it's needs.
So if it makes more sense for the UI to have a this nested under Key then I'm for it.
There was a problem hiding this comment.
To close the loop on this, I'm down to follow up with changing this. The structure is already simple enough, might as well roll with it.
Brian-McM
force-pushed
the
bm-add-filtering-to-whisker-backend
branch
4 times, most recently
from
f4b484c to
38fc04b
Compare
lib/std/chanutil/chan.go
Outdated
| "time" | ||
| ) | ||
|
|
||
| var ErrChannelClosed = errors.New("context canceled") |
There was a problem hiding this comment.
I don't think we should be defining our own errors that mask over well-known messages from the standard library.
This will be super confusing if we see "context canceled" and it didn't originate from the actual Golang "context" package . Same for "deadline exceeded"
I think if we want to introduce our own errors, we should be using distinct types to allow error checking more easily. Much like k8s does:
type errChannelClosed struct {}
func (e errChannelClosed) Error() string {
return "channel closed"
}
func IsChannelClosed(e error) bool {
if _, ok := e.(errChannelClosed); ok {
return true
}
return false
}There was a problem hiding this comment.
This is a typo, this should be "channel closed" not "context conceled".
I'm not opposed to using types for our own errors and I was a die hard fan of this previously, but defining errors like this that don't change is pretty engrained in the standard library and they've extended support of the errors package to check for this exact type of error. I've come to accept that this is idomatic go to define errors like this and it's not going away.
There was a problem hiding this comment.
Yeah, I don't feel compelled to follow this pattern just because it's in the standard library.
I'm fine to leave this as is, but wanted to make a philosophical point here. The stdlib does plenty of other non-idiomatic things - it's not the final authority on what Good Golang is.
We're writing our own library because of deficiencies in the standard library. We should get to choose what that library looks like.
| @@ -0,0 +1,3 @@ | |||
| module github.com/projectcalico/calico/lib/std | |||
There was a problem hiding this comment.
Yeah, we want to check there are no other lines added to this file basically.
whisker-backend/cmd/app/app.go
Outdated
| func Run(ctx context.Context, cfg *config.Config) { | ||
| logrus.WithField("cfg", cfg.String()).Info("Applying configuration...") | ||
|
|
||
| // TODO not sure if we're going to require TLS communication since these will be part of the same pod, at least |
whisker-backend/pkg/apis/v1/flows.go
Outdated
| type PolicyKind string | ||
|
|
||
| const ( | ||
| PolicyKindUnspecified PolicyKind = "" |
There was a problem hiding this comment.
Is there a real reason that importing gRPC / protobuf is going to have negative engineering consequences here?
There's a real cost with duplicating a copy of this API when as far as I can tell, we could be using the same values.
The policy kinds are also enums and I think we want to accept strings from the API?
Yeah, but see my other comments - the enums have String() methods that return their string representation, and the proto package provides an easy way to map back and forth between the two.
| } | ||
| } | ||
|
|
||
| func protoToPolicyKind(kind proto.PolicyKind) whiskerv1.PolicyKind { |
There was a problem hiding this comment.
If any ordering changes then it doesn't match up anymore, same goes for spelling
Ordering of what changes? Spelling of what changes? I'm afraid I don't follow what scenario you're thinking of where the existing approach is more capable than what I suggested.
This code is inextricably linked to the protobuf API that is backing it, regardless of how we implement this function. But implementing it the way I have suggested makes keeping these two in sync easier and less error prone.
| Err error | ||
| } | ||
|
|
||
| func newSSEScanner[E any](t *testing.T, r io.Reader) <-chan ObjWithErr[*E] { |
There was a problem hiding this comment.
What's an SSE?
Can we avoid acronyms (especially without function comments explaining them)
There was a problem hiding this comment.
SSE is "Server Side Event", and I've found that it is a very standard acronym for that (just type SSE in google and it's the second result).
I can add a comment for that though.
There was a problem hiding this comment.
Yeah, it might be standard in some contexts but it certainly isn't in this codebase. I would not be surprised if this was the first instance of that acronym in projectcalico/calico. Let's use that to judge how common it is.
This adds the ability to filter flows that are streamed and listed from the whisker backend.
…essage for chanutil
Brian-McM
force-pushed
the
bm-add-filtering-to-whisker-backend
branch
from
38fc04b to
3485f67
Compare
Description
Add filtering logic to whisker backend
This adds the ability to filter flows that are streamed and listed from the whisker backend.
Related issues/PRs
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.