Skip to content

Commit 8ed22cb

Browse files
gustavoavenameta-codesync[bot]
authored andcommitted
restricted_paths: introduce read_only and gate derivation on it
Summary: ## This stack Decouple restricted_paths config from the manifest_id store (T277178795). Replace `path_acls` (path -> ACL) with per-path `PathRestrictionMetadata` carrying the REPO_REGION ACL and optional permission-request group, so a future `read_only` flag can freeze manifest-id-store growth without dropping enforcement. Thrift `path_acls` is retained as a parser fallback. The configerator thrift source is the separately-published D109697491. ## This diff (behaviour change) One of the most important changes for T277178795. This will let us specify that a tent config is read-only, meaning we'll rely on it only for old manifest ids. This will be used once we migrate all the existing fbsource tents to `.slacl`, which is the long-term, scalable solution. Reviewed By: lmvasquezg Differential Revision: D109827082 fbshipit-source-id: 8c00360c8b1797f8e73025bc1e049cce6994064e
1 parent 65756f6 commit 8ed22cb

10 files changed

Lines changed: 128 additions & 11 deletions

File tree

‎eden/mononoke/derived_data/mercurial_derivation/src/mapping.rs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -353,12 +353,14 @@ async fn track_all_restricted_paths(
353353
return Ok(());
354354
}
355355

356-
// Get the configured restricted paths
356+
// Get the configured restricted paths. Skip read-only roots: derivation must
357+
// not record new manifest-id-store entries for them.
357358
let restricted_dirs: Vec<PathOrPrefix> = restricted_paths
358359
.config()
359360
.path_restriction_metadata
360-
.keys()
361-
.map(|non_root_mpath| PathOrPrefix::Path(non_root_mpath.clone().into()))
361+
.iter()
362+
.filter(|(_, metadata)| !metadata.read_only)
363+
.map(|(non_root_mpath, _)| PathOrPrefix::Path(non_root_mpath.clone().into()))
362364
.collect();
363365

364366
if restricted_dirs.is_empty() {

‎eden/mononoke/metaconfig/parser/src/convert/repo.rs‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1400,6 +1400,7 @@ fn merge_path_restriction_metadata(
14001400
PathRestrictionMetadata {
14011401
repo_region_acl,
14021402
permission_request_group,
1403+
read_only: raw.read_only.unwrap_or(false),
14031404
},
14041405
))
14051406
})
@@ -1426,6 +1427,7 @@ fn merge_path_restriction_metadata(
14261427
PathRestrictionMetadata {
14271428
repo_region_acl: legacy_acl,
14281429
permission_request_group: None,
1430+
read_only: false,
14291431
},
14301432
);
14311433
}
@@ -1656,7 +1658,8 @@ mod tests {
16561658
}
16571659

16581660
/// What it tests: a legacy `path_acls` entry with no `path_restriction_metadata`.
1659-
/// Expected: it is folded into metadata with no explicit request group.
1661+
/// Expected: it is folded into metadata with no explicit request group and
1662+
/// `read_only = false`.
16601663
#[mononoke::test]
16611664
fn test_merge_path_acls_fallback_defaults() {
16621665
let mut raw = empty_raw_restricted_paths_config();
@@ -1670,6 +1673,7 @@ mod tests {
16701673
let md = cfg.path_restriction_metadata.get(&path).expect("entry");
16711674
assert_eq!(md.repo_region_acl.to_string(), "REPO_REGION:acl1");
16721675
assert_eq!(md.permission_request_group, None);
1676+
assert!(!md.read_only, "legacy path_acls entries are not read-only");
16731677
}
16741678

16751679
/// What it tests: a malformed `repo_region_acl` string in `path_acls`.
@@ -1716,15 +1720,15 @@ mod tests {
17161720
}
17171721

17181722
/// What it tests: a `path_restriction_metadata` entry with explicit request
1719-
/// group, no legacy `path_acls`.
1723+
/// group and `read_only = true`, no legacy `path_acls`.
17201724
/// Expected: all explicit fields are preserved.
17211725
#[mononoke::test]
17221726
fn test_merge_metadata_only_explicit_fields() {
17231727
let mut raw = empty_raw_restricted_paths_config();
17241728
raw.path_restriction_metadata = Some(
17251729
[(
17261730
"foo".to_string(),
1727-
raw_metadata("REPO_REGION:acl1", Some("GROUP:reviewers"), None),
1731+
raw_metadata("REPO_REGION:acl1", Some("GROUP:reviewers"), Some(true)),
17281732
)]
17291733
.into_iter()
17301734
.collect(),
@@ -1739,6 +1743,7 @@ mod tests {
17391743
md.permission_request_group.as_ref().map(|i| i.to_string()),
17401744
Some("GROUP:reviewers".to_string())
17411745
);
1746+
assert!(md.read_only, "explicit read_only=true is honored");
17421747
}
17431748

17441749
/// What it tests: a path present in BOTH maps with the SAME repo_region_acl.
@@ -1752,7 +1757,7 @@ mod tests {
17521757
raw.path_restriction_metadata = Some(
17531758
[(
17541759
"foo".to_string(),
1755-
raw_metadata("REPO_REGION:acl1", Some("GROUP:reviewers"), None),
1760+
raw_metadata("REPO_REGION:acl1", Some("GROUP:reviewers"), Some(true)),
17561761
)]
17571762
.into_iter()
17581763
.collect(),
@@ -1767,6 +1772,7 @@ mod tests {
17671772
Some("GROUP:reviewers".to_string()),
17681773
"new field wins on matching ACL"
17691774
);
1775+
assert!(md.read_only);
17701776
}
17711777

17721778
/// What it tests: a path present in BOTH maps with DIFFERENT repo_region_acl.
@@ -1815,4 +1821,24 @@ mod tests {
18151821
"expected repo_region_acl parse error, got: {msg}"
18161822
);
18171823
}
1824+
1825+
/// What it tests: `read_only` absent on a metadata entry.
1826+
/// Expected: it defaults to false.
1827+
#[mononoke::test]
1828+
fn test_merge_read_only_absent_defaults_false() {
1829+
let mut raw = empty_raw_restricted_paths_config();
1830+
raw.path_restriction_metadata = Some(
1831+
[(
1832+
"foo".to_string(),
1833+
raw_metadata("REPO_REGION:acl1", None, None),
1834+
)]
1835+
.into_iter()
1836+
.collect(),
1837+
);
1838+
1839+
let cfg = raw.convert().unwrap();
1840+
1841+
let path = NonRootMPath::new("foo").unwrap();
1842+
assert!(!cfg.path_restriction_metadata.get(&path).unwrap().read_only);
1843+
}
18181844
}

‎eden/mononoke/metaconfig/types/src/lib.rs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2575,14 +2575,19 @@ pub struct EnforcementConditionSet {
25752575
/// Restriction metadata for a single restricted path.
25762576
///
25772577
/// Supersedes the bare path -> ACL mapping that `path_acls` used to carry: it
2578-
/// holds the REPO_REGION ACL and an optional permission-request group.
2578+
/// holds the REPO_REGION ACL, an optional permission-request group, and a
2579+
/// `read_only` flag. When `read_only` is true, derivation stops recording new
2580+
/// manifest-id-store entries for the path; enforcement on existing entries and
2581+
/// config-based authorization are unaffected.
25792582
#[derive(Debug, Clone, Eq, PartialEq)]
25802583
pub struct PathRestrictionMetadata {
25812584
/// REPO_REGION ACL protecting this path.
25822585
pub repo_region_acl: MononokeIdentity,
25832586
/// AMP group clients are redirected to when requesting access, instead of
25842587
/// exposing the REPO_REGION ACL. If `None`, defaults to `repo_region_acl`.
25852588
pub permission_request_group: Option<MononokeIdentity>,
2589+
/// When true, no new manifest-id-store entries are derived for this path.
2590+
pub read_only: bool,
25862591
}
25872592

25882593
impl PathRestrictionMetadata {

‎eden/mononoke/mononoke_api/src/test/test_restricted_paths.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1174,6 +1174,7 @@ fn build_path_restriction_metadata(
11741174
PathRestrictionMetadata {
11751175
repo_region_acl: MononokeIdentity::from_str(acl_str)?,
11761176
permission_request_group: None,
1177+
read_only: false,
11771178
},
11781179
))
11791180
})

‎eden/mononoke/mononoke_api_hg/src/tree.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -672,6 +672,7 @@ mod tests {
672672
repo_region_acl: MononokeIdentity::from_str(acl_str)
673673
.expect("Failed to parse MononokeIdentity"),
674674
permission_request_group: None,
675+
read_only: false,
675676
},
676677
)
677678
})

‎eden/mononoke/repo_attributes/restricted_paths/src/restriction_info/tests.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -542,6 +542,7 @@ fn restricted_paths_for_repo_with_mode_and_path_acls(
542542
PathRestrictionMetadata {
543543
repo_region_acl: acl.parse()?,
544544
permission_request_group: None,
545+
read_only: false,
545546
},
546547
))
547548
})

‎eden/mononoke/repo_attributes/restricted_paths/src/test_utils.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ impl RestrictedPathsConfigBuilder {
6363
PathRestrictionMetadata {
6464
repo_region_acl: MononokeIdentity::from_str(identity)?,
6565
permission_request_group: None,
66+
read_only: false,
6667
},
6768
);
6869
Ok(self)

‎eden/mononoke/repo_attributes/restricted_paths/test/utils.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1267,6 +1267,7 @@ async fn setup_test_repo(
12671267
PathRestrictionMetadata {
12681268
repo_region_acl: acl,
12691269
permission_request_group: None,
1270+
read_only: false,
12701271
},
12711272
)
12721273
})

‎eden/mononoke/repo_attributes/restricted_paths_common/src/config_based.rs‎

Lines changed: 81 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,87 @@ impl RestrictedPathsConfigBased {
9090
}
9191

9292
/// Whether derivation should record a new manifest-id-store entry for this
93-
/// path.
93+
/// path: true only when the path is a restriction root AND not read-only.
94+
/// Distinct from `is_restriction_root` (which stays read-only-agnostic) so
95+
/// that enforcement on already-recorded entries is never weakened.
9496
pub fn should_record_manifest_id_entry(&self, path: &NonRootMPath) -> bool {
95-
// TODO(T277178795): rely on `read_only` from PathRestrictionMetadata
96-
self.get_metadata_for_path(path).is_some()
97+
self.get_metadata_for_path(path)
98+
.is_some_and(|metadata| !metadata.read_only)
99+
}
100+
}
101+
102+
#[cfg(test)]
103+
mod tests {
104+
use std::str::FromStr;
105+
use std::sync::Arc;
106+
107+
use anyhow::Result;
108+
use mononoke_macros::mononoke;
109+
use mononoke_types::RepositoryId;
110+
use sql_construct::SqlConstruct;
111+
112+
use super::*;
113+
use crate::manifest_id_store::SqlRestrictedPathsManifestIdStoreBuilder;
114+
115+
fn metadata(acl: &str, read_only: bool) -> PathRestrictionMetadata {
116+
PathRestrictionMetadata {
117+
repo_region_acl: MononokeIdentity::from_str(acl).expect("valid identity"),
118+
permission_request_group: None,
119+
read_only,
120+
}
121+
}
122+
123+
fn config_based_with_metadata(
124+
entries: Vec<(&str, PathRestrictionMetadata)>,
125+
) -> Result<RestrictedPathsConfigBased> {
126+
let path_restriction_metadata = entries
127+
.into_iter()
128+
.map(|(path, md)| Ok((NonRootMPath::new(path)?, md)))
129+
.collect::<Result<_>>()?;
130+
let store = Arc::new(
131+
SqlRestrictedPathsManifestIdStoreBuilder::with_sqlite_in_memory()?
132+
.with_repo_id(RepositoryId::new(0)),
133+
);
134+
Ok(RestrictedPathsConfigBased::new(
135+
RestrictedPathsConfig {
136+
path_restriction_metadata,
137+
..Default::default()
138+
},
139+
store,
140+
None,
141+
))
142+
}
143+
144+
/// What it tests: the derivation recording gate honors `read_only`.
145+
/// Expected: a writable root records; a read-only root does not; a
146+
/// non-restriction path does not. `is_restriction_root` stays
147+
/// read-only-agnostic so enforcement on existing entries is never weakened.
148+
#[mononoke::test]
149+
fn test_should_record_manifest_id_entry_respects_read_only() -> Result<()> {
150+
let config_based = config_based_with_metadata(vec![
151+
("writable", metadata("REPO_REGION:acl1", false)),
152+
("frozen", metadata("REPO_REGION:acl2", true)),
153+
])?;
154+
155+
let writable = NonRootMPath::new("writable")?;
156+
let frozen = NonRootMPath::new("frozen")?;
157+
let other = NonRootMPath::new("other")?;
158+
159+
assert!(config_based.should_record_manifest_id_entry(&writable));
160+
assert!(config_based.is_restriction_root(&writable));
161+
162+
assert!(
163+
!config_based.should_record_manifest_id_entry(&frozen),
164+
"read_only roots must not record new manifest-id entries"
165+
);
166+
assert!(
167+
config_based.is_restriction_root(&frozen),
168+
"read_only roots are still restriction roots"
169+
);
170+
171+
assert!(!config_based.should_record_manifest_id_entry(&other));
172+
assert!(!config_based.is_restriction_root(&other));
173+
174+
Ok(())
97175
}
98176
}

‎eden/mononoke/servers/scs/scs_methods/src/methods/commit_restricted_paths.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,7 @@ mod tests {
415415
repo_region_acl: MononokeIdentity::from_str(acl_str)
416416
.context("Failed to parse MononokeIdentity from ACL string")?,
417417
permission_request_group: None,
418+
read_only: false,
418419
},
419420
))
420421
})

0 commit comments

Comments
 (0)