Disable custom namespace locations

[eric-maynard]

When we create a namespace or alter its location, we must confirm that this location is within the parent location. This PR introduces introduces a check similar to the one we have for tables, where custom locations are prohibited by default. This functionality is gated behind a new behavior change flag ALLOW_NAMESPACE_CUSTOM_LOCATION. In addition to allowing us to revert to the old behavior, this flag allows some tests relying on arbitrarily-located namespaces to pass (such as those from upstream Iceberg).

Fixes: #2417

[dimas-b]

LGTM overall 👍

[dimas-b]

Feature flags constitute our SemVer "API" surface, so I believe we need to formally send a discussion email to the dev ML about this flag.

[dimas-b]

Proactively +1 from my side :)

[eric-maynard]

Is that so? That seems to create a weird incentive not to feature-flag anything.

In this particular case there's a behavior change due to the default setting of the flag so in that sense it's relevant to semver, but it's also a bugfix.

[dimas-b]

I'm +1 on this change already. So having extra discussion on the dev ML does not matter for me personally.

@flyrain WDYT?

[flyrain]

+1 for the change as well. If this is more a bug fix, I don't think we even need this flag, the only reason we need the config is that existing behavior can be broken due to the change.

[flyrain]

We could even mark it deprecated in the beginning(this PR).

[dimas-b]

@flyrain : but what's you POV about requiring dev discussion for feature flags in general?

[flyrain]

I'm supportive on having a discussion on dev mailing list. It's great to let more people know this behavior change/bug fix.

[eric-maynard]

In this case I think we want to have a flag for parity with tables -- the "bug" here IMO is that the default behavior is too lax, not that the lax behavior is accessible at all.

I'll create a quick thread to discuss the change in more detail, but in general my position is that it's not healthy for a project to require ML discussion for every new config. This creates an incentive to not protect changes with configs, and in the ideal world where all changes are config-protected this essentially necessitates an ML thread for every PR.

[eric-maynard]

Pulling this back to draft to investigate -- we may need to just block all custom namespace locations like we do for tables in order to avoid scenarios where two namespaces overlap despite being within their parent entity

[eric-maynard]

I've updated the PR to make this check more like the table one, which totally disallows custom locations by default.

[dennishuo]

AllowedLocations in the StorageConfig are an independent concept vs either of:

1. Whether to allow "custom" locations outside of the default path hierarchy that uses namespace names
2. Whether to allow "overlapping" locations

The most important one is validating that anything that can vend credentials resides under the allowedLocations.

Overlap and custom hierarchies in general are more common to disable validation for.

[eric-maynard]

This PR isn't related to credential vending, but the location that a namespace is created at.

[dennishuo]

Right, I was explaining why your PR description is misleading:

in order for our overlap checks and assumptions around credential vending to be valid

Is this PR fixing assumptions around credential vending, or just adding an option for whether to enforce "standard" locations for namespaces? In general we shouldn't need namespaces to be standard to still provide the intended guarantees about enforcement of ALLOWED_LOCATIONS.

The more important bug to fix is enforcement of ALLOWED_LOCATIONS.

[eric-maynard]

That clause is part of a sentence:

When we create a namespace or alter its location, we must confirm that this location is within the parent location ... in order for our overlap checks and assumptions around credential vending to be valid.

and is used in that sentence to articulate why we care that namespaces reside within their parent's location. Indeed, if you remove the clause from that context it could be misleading.

If there's another (more important) bugfix, let's submit that in parallel with this existing bugfix.

[sfc-gh-dhuo]

My clause was also part of a paragraph:

AllowedLocations in the StorageConfig are an independent concept vs either of:

1. Whether to allow "custom" locations outside of the default path hierarchy that uses namespace names
2. Whether to allow "overlapping" locations

The most important one is validating that anything that can vend credentials resides under the allowedLocations.

Overlap and custom hierarchies in general are more common to disable validation for.

[flyrain]

Thanks @eric-maynard for the fix. Left some comments.

[flyrain]

Minor: It'd be nice to wrap this into a method, so that we don't duplicate them in two different places.

    if (!realmConfig.getConfig(FeatureConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP)) {
      LOGGER.debug("Validating no overlap for {} with sibling tables or namespaces", namespace);
      validateNoLocationOverlap(entity, resolvedParent.getRawFullPath());
    } else {
      LOGGER.debug("Skipping location overlap validation for namespace '{}'", namespace);
    }
    if (!realmConfig.getConfig(
        FeatureConfiguration.ALLOW_NAMESPACE_CUSTOM_LOCATION, catalogEntity)) {
      LOGGER.debug("Validating that namespace {} has a location inside its parent", namespace);
      validateNamespaceLocation(entity, resolvedParent);
    }

[flyrain]

@eric-maynard, thanks for moving it to BehaviorChangeConfiguration. By doing so, it discourages any users to use it. It is still useful for test purpose. We could remove it, once we figure out related tests.

Hi @dennishuo, would you take another look?

[dimas-b]

@eric-maynard : These tests fail on main after this PR... not sure how they passed on the PR 😅 Could you take a look?

 IcebergAllowedLocationTest > testCreateTableInsideOfCatalogAllowedLocations(Path) FAILED
    java.lang.IllegalArgumentException at IcebergAllowedLocationTest.java:352

IcebergAllowedLocationTest > testCreateTableOutSideOfCatalogAllowedLocations(Path) FAILED
    java.lang.IllegalArgumentException at IcebergAllowedLocationTest.java:352

[eric-maynard]

Hey @dimas-b, you're totally right, my bad. These two PRs had a conflict. I put up a fix in #2511.

I've talked to @flyrain about this in the past, but it would be pretty cool if we could have a merge queue and force PRs to pass CI before they actually merge.

[dimas-b]

+1 to merge queue... at least we should discuss it :)