Split `Security` into separate contexts for each domain
What does this MR do and why?
Split Security
into separate contexts for each domain
Each feature category inside the Security
namespace is a distinct
product domain and requires specific domain knowledge. Security
is a
very broad term which could cover security testing, application
security, vulnerability management, scanning, authentication /
authorization, and a plethora of other domains. To encourage separation
of concerns and avoid unsafe cross-domain usage, we should avoid lumping
so many different domains together inside the same bounded context.
MR acceptance checklist
Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
Before | After |
---|---|
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
Merge request reports
Activity
assigned to @bwill
- A deleted user
added backend label
1 Warning The master pipeline status page reported failures in If these jobs fail in your merge request with the same errors, then they are not caused by your changes.
Please check for any on-going incidents in the incident issue tracker or in the#master-broken
Slack channel.Reviewer roulette
Category Reviewer Maintainer backend @imand3r
(UTC-7, 2 hours behind author)@10io
(UTC+2, 7 hours ahead of author)~"Backend Static Code Analysis" Reviewer review is optional for ~"Backend Static Code Analysis" @jennli
(UTC-7, 2 hours behind author)config/bounded_contexts.yml
Reviewer review is optional for config/bounded_contexts.yml
@stanhu
(UTC-7, 2 hours behind author)Please check reviewer's status!
Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger added pipelinetier-1 label
- Resolved by 🤖 GitLab Bot 🤖
Proper labels assigned to this merge request. Please ignore me.
@bwill - please see the following guidance and update this merge request. 1 Error Please add typebug typefeature, or typemaintenance label to this merge request. Edited by 🤖 GitLab Bot 🤖
added maintenancerefactor label
added typemaintenance label
- Resolved by Brian Williams
- Resolved by Brian Williams
added pipeline:mr-approved label
@Quintasan
, thanks for approving this merge request.This is the first time the merge request has been approved. To ensure we don't only run predictive pipelines, and we don't break
master
, a new pipeline will be started shortly.Please wait for the pipeline to start before resolving this discussion and set auto-merge for the new pipeline. See merging a merge request for more details.
added pipelinetier-2 label and removed pipelinetier-1 label
E2E Test Result Summary
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for 43b19824 expand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Verify | 30 | 0 | 2 | 29 | 32 | ✅ | | Create | 121 | 0 | 10 | 93 | 131 | ✅ | | Package | 19 | 0 | 12 | 19 | 31 | ✅ | | Plan | 54 | 0 | 2 | 47 | 56 | ✅ | | Govern | 64 | 0 | 1 | 41 | 65 | ✅ | | Analytics | 1 | 0 | 1 | 0 | 2 | ✅ | | Data Stores | 31 | 0 | 0 | 22 | 31 | ✅ | | Manage | 0 | 0 | 1 | 0 | 1 | ➖ | | Release | 5 | 0 | 0 | 5 | 5 | ✅ | | Monitor | 8 | 0 | 0 | 7 | 8 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 333 | 0 | 29 | 263 | 362 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for 43b19824 expand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Govern | 129 | 0 | 10 | 1 | 139 | ✅ | | Create | 318 | 0 | 36 | 6 | 354 | ✅ | | Monitor | 8 | 0 | 0 | 0 | 8 | ✅ | | Verify | 8 | 0 | 2 | 0 | 10 | ✅ | | Data Stores | 22 | 0 | 0 | 0 | 22 | ✅ | | Plan | 44 | 0 | 4 | 0 | 48 | ✅ | | Package | 6 | 0 | 8 | 0 | 14 | ✅ | | Release | 2 | 0 | 0 | 0 | 2 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 537 | 0 | 60 | 7 | 597 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
@bwill From your slack comment:
I'm curious why there is a dedicated namespace for
DependencyManagement
whileVulnerabilityManagement
gets lumped underSecurity
which feels like a kitchen sink due to the number of feature categories under it.That's a good question and I can't recall the rationale for how we split those
. I do think it makes sense to split them out but I'd like to know how we were thinking about this to understand how we ended up here. Policies and Vulnerability Management seem fairly distinct although there's some feature-specific overlap like:
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/services/security/ingestion/schedule_mark_dropped_as_resolved_service.rb
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/services/security/token_revocation_service.rb
I can also see some upcoming need for unified configuration, such as exploring scan profiles for SAST, similar to those used by DAST. That to me encompasses a larger category like
scan execution
vsresult management
(the latter might split b/w vulns and dependencies as you noted)The minimal change previously was making
Security
encapsulate the global groupings but I do agree it's quite broad. We don't need it to be ambiguous but also see some overlap I feel we would lose by over-specifying contexts... I would be curious to get another member of devopssecure to chime in as well.@fcatteau you were a member of the WG too, wdyt?
Edited by Lucas CharlesI'm curious why there is a dedicated namespace for
DependencyManagement
whileVulnerabilityManagement
gets lumped underSecurity
which feels like a kitchen sink due to the number of feature categories under it.I don't know how we ended there.
The minimal change previously was making
Security
encapsulate the global groupings but I do agree it's quite broad.@theoretick I totally agree. AFAIR the working group brought that up, but it was decided that having a broad
Security
context was a good first step. At the very least it was a way to move forward with the proposal without having to define boundaries b/w vulnerability management, dependency management, types of scans, etc.@fcatteau @theoretick @bwill Just trying to understand the motivation behind it. Within
Security
we can still have dedicate subdomains likeSecurity::Dast
,Security::Vurnerabilities
,Security::DependencyManagement
, etc. If we don't know how to properly split those then we maybe don't need to do that yet and leverage the sub-domains instead.If instead we prefer to have completely separate domains, I'm ok with that too but as I said in !153581 (comment 1921000154) we could have strange cases like shared functionalities that we don't know where they should live.
A good time to split bounded contexts is when we understand well how the new bounded contexts would depend on each other. We have a similar problem in
Ci::
, as you can see it's almost double the size ofSecurity::
. We will eventually extract smaller dedicated bounded contexts (like Runners or perhaps Artifacts) but we are not clear yet where we would end up.
Kind of related: Are you by any chance interested in enforcing sub-domains? E.g. ensure that
Security::Vulnerabilities
is used instead ofSecurity::VulnerabilityManagement
or a bunch of other similar terms? If so, this may be very easy to add to.Within
Security
we can still have dedicate subdomains likeSecurity::Dast
,Security::Vurnerabilities
,Security::DependencyManagement
, etc. If we don't know how to properly split those then we maybe don't need to do that yet and leverage the sub-domains instead.@fabiopitino Thanks for bringing this up. I totally missed that the new domains were not subdomains of
Security
. I think they should be subdomains at this point.A good time to split bounded contexts is when we understand well how the new bounded contexts would depend on each other.
Yes. We might extract
DependencyManagement
at some point it's connected to policies and compliance, and not only related toSecurity
. Same thing for SCA, which covers both vulnerability scanning and license scanning.Edited by Fabien Catteau@fabiopitino At this point I'd rather have sub-domains under the existing
Security::
module.If instead we prefer to have completely separate domains, I'm ok with that too but as I said in !153581 (comment 1921000154) we could have strange cases like shared functionalities that we don't know where they should live.
@fabiopitino Isn't that true of all code? For example, where do we put
Projects
,Groups
,Users
, etc? How should this problem be addressed at a global level?Kind of related: Are you by any chance interested in enforcing sub-domains? E.g. ensure that
Security::Vulnerabilities
is used instead ofSecurity::VulnerabilityManagement
or a bunch of other similar terms? If so, this may be very easy to add to.Yes, I am interested in this because when we have separate top-level domains we are actually able to enforce the contexts that are used. When we rely on sub-domains inside
Security::
, we are right back at square one in that we do not have any enforced conventions.do you still want to split the domains now or leverage the sub-domains inside
Security::
module (existing strategy)?Here are the problems I see with using the
Security::
domain. Some of these could be fixed with sub-domain enforcement:- There is a lot code which currently is not categorized (Ex: Security Policies vs Vulnerabilities), and there is currently no mechanism of correctly categorizing them.
- Because of the wide domain, all miscellaneous security code will end up going here even if it's not related to any of the product categories in this domain.
- If we use subdomains, we're forcing everybody to write code 3+ modules deep which will become quite verbose.
/cc @fcatteau
For example, where do we put
Projects
,Groups
,Users
, etc? How should this problem be addressed at a global level?For such concepts we should use the patterns described in https://docs.gitlab.com/ee/development/software_design.html#taming-omniscient-classes and avoid adding behavior to these classes. A typical case is a feature that has group/project level settings. We tend to define those in
Projects
orProjectSetting
but they should be defined inside the related bounded context. E.g.Security::Vunerabilities::ProjectSetting
.Yes, I am interested in this because when we have separate top-level domains we are actually able to enforce the contexts that are used. When we rely on sub-domains inside
Security::
, we are right back at square one in that we do not have any enforced conventions.we can explore this avenue, but having strict sub-domains you can have less flexibility in where to put new or shared concepts. Thinking of it more, I guess it doesn't add much value and it would be better split the domains entirely. Because of the wide domain, all miscellaneous security code will end up going here even if it's not related to any of the product categories in this domain.
I feel this is a crucial aspect to discuss with others in the team in order to reach a consensus. Ultimately it's up you as domain experts.
If you feel there are clear boundaries between all these security features then we should probably go for separate top-level concerns.
I used
Security::
as initial bounded context because it was already widely used and because a lot of the security code is already deeply nested. We have a similar pattern withCi::
because we have multiple interrelated concepts and can be hard to consider in isolation.If you feel there are clear boundaries between all these security features then we should probably go for separate top-level concerns.
@fabiopitino
Vulnerabilities
is already a distinct top-level namespace with lots of code in it. I believe that moving this underSecurity::
would actually be taking a step backwards.SecurityPolicies
is also a very distinct domain which would benefit from being separated fromSecurity::
.For other code that we're not sure where to place, I think it would benefit us to pause for a moment and consider where it should go. Is it supporting Vulnerability management? Or is it a new product domain? My opinion is that separating these would cause people to ask these questions and categorize things more intentionally.
Edited by Brian Williams@bwill That's ok by me.
Compliance
is already a separate bounded context but that's probably already a distinct concept. You (teams in Secure and Govern) may find helpful having a rough representation of how dependencies among these bounded contexts would be when making a decision.A rough (probably wrong) example:
flowchart TD SecurityPolicies --> SAST --> Vulnerabilities SecurityPolicies --> DAST --> Vulnerabilities SecurityPolicies --> SoftwareCompositionAnalysis --> Vulnerabilities Compliance --> SecurityPolicies
Are there other concepts that are missing from the list? E.g. SAST is not in this MR AFAIK.
@fcatteau Can you elaborate on why you think these domains should reside under the
Security::
namespace?Are there other concepts that are missing from the list? E.g. SAST is not in this MR AFAIK.
Concepts like SAST exist but currently do not have any code inside the Rails monolith.
@fabiopitino As a short-term compromise, could we split out
Vulnerabilities
but keep theSecurity
namespace until we can resolve this discussion? Some groupthreat insights team members currently have their MRs blocked because we would like to continue to add code to the existingVulnerabilities
namespace. If this sounds reasonable, could we please merge !154618 (merged)? I think your suggestion regarding dependencies is a reasonable one but I don't have time to do the analysis for this at the moment.Edited by Brian Williams@bwill I'm merging Split `Vulnerabilities` out from `Security` (!154618 - merged)
Thanks - I didn't notice this decision was blocking other MRs. Let me know when/if this MR is ready to be merged. I have no problems either way but it sounds like we don't have a verdict yet.
Please assign it back to me when ready
- Resolved by Brian Williams
- Resolved by Fabien Catteau