v2 api with gRPC and gRPC gateway #408
Conversation
KeyboardNerd
force-pushed
the
grpc
branch
2 times, most recently
from
4bea71b to
e54ec16
Compare
api/api.go
Outdated
| listenAndServeWithStopper(srv, st, cfg.CertFile, cfg.KeyFile) | ||
|
|
||
| log.Info("main API stopped") | ||
| } | ||
|
|
||
| // RunHealth API |
There was a problem hiding this comment.
this isn't a particularly useful comment
api/api.go
Outdated
| v2.Run(cfg.GrpcPort, tlsConfig, cfg.PaginationKey, cfg.CertFile, cfg.KeyFile, store) | ||
| } | ||
|
|
||
| // Run V1 API |
There was a problem hiding this comment.
this isn't a particularly useful comment
api/v1/routes.go
Outdated
| @@ -24,7 +24,7 @@ import ( | |||
|
|
|||
| "github.com/julienschmidt/httprouter" | |||
| "github.com/prometheus/client_golang/prometheus" | |||
| log "github.com/sirupsen/logrus" | |||
| log "github.com/Sirupsen/logrus" | |||
There was a problem hiding this comment.
In some files this is the first import and in this one it's the last. Did you run GoImports on this? They should be ordered alphabetically.
api/v2/clairpb/clair.proto
Outdated
| import "google/api/annotations.proto"; | ||
| import "google/protobuf/empty.proto"; | ||
|
|
||
| message Vulnerability{ |
There was a problem hiding this comment.
Is this file in an idiomatic format? The non-space-separated { seems strange.
There's got to be a protobuf linter that has opinions on this stuff.
There was a problem hiding this comment.
Nope; based on the style, I shouldn't use the camel case for fields: https://developers.google.com/protocol-buffers/docs/style
There was a problem hiding this comment.
Change this so that you always have the space in between the message name and the curly bracket message Name { instead of message Name{
api/v2/clairpb/clair.proto
Outdated
|
|
||
| message VulnerabilityWithLayers{ | ||
| Vulnerability Vulnerability = 1; | ||
| repeated OrderedLayerName OrderedLayersIntroducingVulnerability = 2; |
There was a problem hiding this comment.
We can just called this LayersIntroducingVulnerabilty, it was given this verbose name because the original LayersIntroducingVulnerability was unordered and we didn't want to break backwards-compat in the v1 API.
api/v2/rpc.go
Outdated
| if r.GetLimit() <= 0 { | ||
| return nil, status.Error(codes.InvalidArgument, "Failed to provide page limit") | ||
| } | ||
| // get page token |
api/v2/rpc.go
Outdated
| } else if err != nil { | ||
| return nil, status.Error(codes.Internal, err.Error()) | ||
| } | ||
| return &google_protobuf1.Empty{}, nil |
There was a problem hiding this comment.
Is this idiomatic? I'm going to ask someone.
api/v2/server.go
Outdated
| statusStr := strconv.Itoa(lrw.StatusCode) | ||
| if lrw.StatusCode == 0 { | ||
| statusStr = "???" | ||
| } |
There was a problem hiding this comment.
We should probably stop doing this and and just assume all Prometheus requests responded with 200, but that can be handled in a separate PR.
api/v2/util.go
Outdated
| } | ||
|
|
||
| // TODO: Remove this Legacy compability code once the database is revised | ||
| // We assume every layer is the root of an ancestry for api being compatible with the current implementation of the database |
api/v2/util.go
Outdated
| layer database.Layer | ||
| err error | ||
| ) | ||
| // iteratively query the layer until we find the one layer requested |
api/token/token.go
Outdated
| "errors" | ||
| "time" | ||
|
|
||
| fernet "github.com/fernet/fernet-go" |
There was a problem hiding this comment.
Do you actually need this? I'm pretty sure without it, it still imports as fernet.
There was a problem hiding this comment.
forgot to check, it's automatically imported like that
api/token/token.go
Outdated
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package token |
There was a problem hiding this comment.
// Package token implements ...
api/v2/clairpb/clair.proto
Outdated
|
|
||
| message LayersIntroducingVulnerabilty{ | ||
| Vulnerability vulnerability = 1; | ||
| repeated OrderedLayerName ordered_layers_introducing_vulnerability = 2; |
There was a problem hiding this comment.
layers_introducing_vulnerability or maybe even just layers is sufficient because of the message's name and OrderedLayerName tells you that it's ordered
api/v2/clairpb/clair.proto
Outdated
| message Layer{ | ||
| string name = 1; | ||
| repeated string namespace_names = 2; | ||
| string parent_name = 3; |
api/v2/clairpb/clair.proto
Outdated
| import "google/api/annotations.proto"; | ||
| import "google/protobuf/empty.proto"; | ||
|
|
||
| message Vulnerability{ |
There was a problem hiding this comment.
Change this so that you always have the space in between the message name and the curly bracket message Name { instead of message Name{
api/v2/clairpb/clair.proto
Outdated
| Notification notification = 1; | ||
| } | ||
|
|
||
| service ClairAPIService{ |
There was a problem hiding this comment.
I talked to the etcd team and I think it's a nice pattern to separate Services by resource, so we can have two Services that we register to the same grpc server.
service AncestryService {
rpc GetAncestry(GetAncestryRequest) returns (GetAncestryResponse) { ... };
rpc PostAncestry(PostAncestryRequest) returns (PostAncestryResponse) { ... };
}
service NotificationService {
rpc GetNotification(GetNotificationRequest) returns (GetNotificationResponse) { ... };
rpc DeleteNotification(DeleteNotificationRequest) returns (DeleteNotificationResponse) { ... };
}Newly designed API defines Ancestry as a set of layers and shrinked the api to only the most used apis: post ancestry, get layer, get notification, delete notification Fixes quay#98
v2 api with gRPC and gRPC gateway
It's a big commit, probably the logging should be improved. ( currently it's a hack on response writer )