[PoC] Security Group Filtering #53
Conversation
|
[DoNotMerge] |
There was a problem hiding this comment.
Thank you, this is in general a very good idea to send less data over the wire. Please see my comments. I think it would be better to introduce new model and new api endpoint to get desired LRPs and that can even have less data. Let me know if this makes sense and if you have questions.
| @@ -74,6 +74,9 @@ func (h *DesiredLRPHandler) commonDesiredLRPs(logger lager.Logger, targetVersion | |||
| if len(desiredLRPs[i].CachedDependencies) == 0 { | |||
| desiredLRPs[i].CachedDependencies = nil | |||
| } | |||
| if filter.SkipEgressRules { | |||
| desiredLRPs[i].EgressRules = nil | |||
There was a problem hiding this comment.
Nullifying the Egress Rules results in Desired LRP which is partially instantiated and doesn't represent the real object. Instead I think it is better to introduce new presenter model, e.g. DesiredLRPRouteInfo, that will only have information required by route-emitter.
| @@ -65,7 +65,7 @@ func (h *DesiredLRPHandler) commonDesiredLRPs(logger lager.Logger, targetVersion | |||
|
|
|||
| err = parseRequest(logger, req, request) | |||
| if err == nil { | |||
| filter := models.DesiredLRPFilter{Domain: request.Domain, ProcessGuids: request.ProcessGuids} | |||
| filter := models.DesiredLRPFilter{Domain: request.Domain, ProcessGuids: request.ProcessGuids, SkipEgressRules: request.SkipEgressRules,} | |||
There was a problem hiding this comment.
As mentioned below, I think it is better to introduce new model and new database call, that will only pull the required fields, instead of loading all of this into memory and then nullifying it.
| ProcessGuids: filter.ProcessGuids, | ||
| Domain: filter.Domain, | ||
| ProcessGuids: filter.ProcessGuids, | ||
| SkipEgressRules: filter.SkipEgressRules, |
There was a problem hiding this comment.
I would introduce a new endpoint that only gets desired LRP routes information. Because this is not really a filter to select desired LRPs, the SkipEgressRules is a command we want to perform to present the data.
|
Hi @Keyli0Iliev, Have you had a chance to review @mariash's feedback? Is there any further work for this PR? Thanks, |
|
Hi @MarcPaquette, We already reach-out to a conclusion and the filtering is implemented as part of another PR. I shall ask @Keyli0Iliev to close this PR as it is no longer relevant. Regards, |
|
Closing PR |
Thank you for submitting a pull request to the bbs repository!
We appreciate the contribution. To help us understand the context for your pull request, please fill out this template to the best of your ability.
Please make sure to complete all of the following steps
Please provide the following information:
What is this change about?
Adding Security group filtering to DesiredLRPFilter
What problem it is trying to solve?
In cases where there are a large amount of Security groups in a single space network traffic increases considerably as more apps are added to the space
What is the impact if the change is not made?
Sub-optimal component performance and indefinite increase in network load as customers add more apps to spaces.
How should this change be described in diego-release release notes?
Added security group filtering for DesiredLRPs when enabled bbs will not send security groups in the response
Please provide any contextual information.
Route emitter RP
Issue in Diego-release Issue link
Tag your pair, your PM, and/or team!
@PlamenDoychev
Thank you!