Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/network policy predefined #30261

Merged

Conversation

jubrad
Copy link
Contributor

@jubrad jubrad commented Oct 30, 2024

Motivation

  • adds a new predefined network policy
  • removes the allow list based policy system var
    in favor of a new network_policy system var
  • swaps policy enforcement to use the default policy.

stacked on #30172
part of https://github.com/MaterializeInc/database-issues/issues/4637

Tips for reviewer

If you are reviewing this prior to #30172 going in, you should only look at the add predefined network policy and system var commit.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

Sorry, something went wrong.

@def-
Copy link
Contributor

def- commented Oct 30, 2024

I triggered a Nightly run: https://buildkite.com/materialize/nightly/builds/10207
There is a panic in Checks upgrade across four versions 1:

platform-checks-materialized-1  | thread 'coordinator' panicked at src/compute-client/src/as_of_selection.rs:392:25: failed to apply hard as-of constraint (id=u388, bounds=[[1730272190464] .. [1730272190464]], constraint=Constraint { type_: Hard, bound_type: Upper, frontier: Antichain { elements: [1730272190463] }, reason: "storage export u388 write frontier" })

Since I haven't seen it before, could be related to this PR or just an interesting new flake. Edit: The rerun was green, so probably independent of your PR. I will open an issue.

@jubrad jubrad force-pushed the feature/network-policy-predefined branch 3 times, most recently from bdd4caa to de5d7ff Compare October 31, 2024 02:18
@jubrad jubrad marked this pull request as ready for review October 31, 2024 05:09
@jubrad jubrad requested review from a team as code owners October 31, 2024 05:09
@jubrad jubrad requested a review from ParkMyCar October 31, 2024 05:09
Copy link

shepherdlybot bot commented Oct 31, 2024

Risk Score:80 / 100 Bug Hotspots:5 Resilience Coverage:66%

Mitigations

Completing required mitigations increases Resilience Coverage.

  • (Required) Code Review 🔍 Detected
  • (Required) Feature Flag
  • (Required) Integration Test 🔍 Detected
  • (Required) Observability 🔍 Detected
  • (Required) QA Review 🔍 Detected
  • (Required) Run Nightly Tests
  • Unit Test
Risk Summary:

The pull request carries a high risk score of 80, driven by predictors such as "Sum Bug Reports Of Files" and "Delta of Executable Lines." Historically, PRs with these predictors are 116% more likely to cause a bug compared to the repository baseline. Additionally, five modified files are identified as hotspots with elevated levels of recent bug fixes. The observed trend of bugs in the repository is increasing.

Note: The risk score is not based on semantic analysis but on historical predictors of bug occurrence in the repository. The attributes above were deemed the strongest predictors based on that history. Predictors and the score may change as the PR evolves in code, time, and review activity.

Bug Hotspots:
What's This?

File Percentile
../vars/definitions.rs 93
../session/vars.rs 93
../durable/upgrade.rs 94
../src/coord.rs 98
../sequencer/inner.rs 96

Sorry, something went wrong.

@jubrad jubrad force-pushed the feature/network-policy-predefined branch 2 times, most recently from 874c250 to dc2cb0b Compare November 1, 2024 03:50
Copy link
Member

@ParkMyCar ParkMyCar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the structure looks good! Just need a few changes

Comment on lines 4009 to 4092
if let Some(_policy) = values
.map(|v| self.catalog.get_network_by_name(&v[0]))
.flatten()
{
} else {
return Err(AdapterError::PlanError(plan::PlanError::VarError(
VarError::InvalidParameterValue {
name: NETWORK_POLICY.name(),
invalid_values: values.expect("Must have provided value.").to_owned(),
reason: "No network policy with such name exists!".to_string(),
},
)));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind refactoring this, it's a bit hard to grok at a glance, maybe something like:

let network_policy_exists = values
    .map(|v| self.catalog.get_network_policy_by_name(&v[0]))
    .flatten()
    .is_some();
if !network_policy_exists {
    ...
}

Also we generally allow folks to set system or session vars to an invalid value, e.g. you can set the cluster var to a non-existent cluster. I understand why we don't want to allow that here though, do you mind adding a comment calling out that this is a special case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By adding a comment do you mean to the definition or in some documentation?

lazy_value!(Vec<IpNet>; || vec![IpNet::from_str("0.0.0.0/0").expect("this is a valid IpNet")]),
"Network policy allow list that external user connections will be validated against.",
pub static NETWORK_POLICY: VarDefinition = VarDefinition::new_lazy(
"network_policy",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about naming this "fallback_network_policy" or "default_network_policy" instead of just "network_policy"? The current naming makes me think this is my current network policy, when in reality my role might have a different one applied

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of this disssions is that when we introduce the ability for users to have attached network policyes the session var will be the users network_policy and the system var will be the system network_policy, which allows us to maintain our variable inheritance model. The break is that users won't be able to change their session var value.
#29814 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, this makes sense to me! We could change this in the future if we want the session var "network_policy" to inherit from the system var "default_network_policy", but for now this LGTM

@jubrad jubrad force-pushed the feature/network-policy-predefined branch from dc2cb0b to 4962f09 Compare November 1, 2024 15:56
@jubrad jubrad requested a review from ParkMyCar November 1, 2024 15:56
@jubrad jubrad force-pushed the feature/network-policy-predefined branch from 4962f09 to 71c7ecd Compare November 1, 2024 16:00
@ParkMyCar ParkMyCar force-pushed the feature/network-policy-predefined branch from 71c7ecd to 39defe6 Compare November 8, 2024 19:28
Copy link
Member

@ParkMyCar ParkMyCar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first commit was authored by @jubrad and it looks good to me.

@jkosh44 do you mind reviewing the second commit? I made two refactors around getting and setting the "network_policy" system var

@ParkMyCar ParkMyCar requested a review from jkosh44 November 8, 2024 19:30
@ParkMyCar ParkMyCar force-pushed the feature/network-policy-predefined branch from 135a578 to 0b8e909 Compare November 11, 2024 15:23
@ParkMyCar ParkMyCar enabled auto-merge (squash) November 11, 2024 15:23
jubrad and others added 6 commits November 11, 2024 15:15
- adds a new predefined network policy
- removes the allow list based policy system var
  in favor of a new `network_policy` system var
- swaps policy enforcement to use the default policy.

move objects_v70 to objects_v71
* move computing session defaults for a role to tee us up for role default network policies
* refactor sequence_alter_system_set to handle different scenarios of changing the system default network policy
@ParkMyCar ParkMyCar force-pushed the feature/network-policy-predefined branch from a5c1406 to 4f270d3 Compare November 11, 2024 20:21
@ParkMyCar ParkMyCar merged commit c10003d into MaterializeInc:main Nov 11, 2024
80 checks passed
ParkMyCar added a commit that referenced this pull request Nov 13, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
<!--
Describe the contents of the PR briefly but completely.

If you write detailed commit messages, it is acceptable to copy/paste
them
here, or write "see commit messages for details." If there is only one
commit
in the PR, GitHub will have already added its commit message above.
-->

### Motivation
Attempt to prevent user lockout when modifying network
policies or the default `network_policy` system var by validating
the users IP against the policy changes they request.


stacked on 
- #30261
part of MaterializeInc/database-issues#4637

<!--
Which of the following best describes the motivation behind this PR?

  * This PR fixes a recognized bug.

    [Ensure issue is linked somewhere.]

  * This PR adds a known-desirable feature.

    [Ensure issue is linked somewhere.]

  * This PR fixes a previously unreported bug.

    [Describe the bug in detail, as if you were filing a bug report.]

  * This PR adds a feature that has not yet been specified.

[Write a brief specification for the feature, including justification
for its inclusion in Materialize, as if you were writing the original
     feature specification.]

   * This PR refactors existing code.

[Describe what was wrong with the existing code, if it is not obvious.]
-->

### Tips for reviewer

Only superusers can modify the `network_policy` system var and alter the
default network policy and there's currently no way to make a superuser
outside of frontegg mock making this very difficult to test.

It might be viable to add a flag or systemvar that specifies the name of
a superuser, or this will have to be run in tests that use frontegg
mock.

We could use this pr as well to turn the current user into a super user
then modify the system var
#30316


<!--
Leave some tips for your reviewer, like:

    * The diff is much smaller if viewed with whitespace hidden.
    * [Some function/module/file] deserves extra attention.
* [Some function/module/file] is pure code movement and only needs a
skim.

Delete this section if no tips.
-->

### Checklist

- [ ] This PR has adequate test coverage / QA involvement has been duly
considered. ([trigger-ci for additional test/nightly
runs](https://trigger-ci.dev.materialize.com/))
- [ ] This PR has an associated up-to-date [design
doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md),
is a design doc
([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)),
or is sufficiently small to not require a design.
  <!-- Reference the design in the description. -->
- [ ] If this PR evolves [an existing `$T ⇔ Proto$T`
mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md)
(possibly in a backwards-incompatible way), then it is tagged with a
`T-proto` label.
- [ ] If this PR will require changes to cloud orchestration or tests,
there is a companion cloud PR to account for those changes that is
tagged with the release-blocker label
([example](MaterializeInc/cloud#5021)).
<!-- Ask in #team-cloud on Slack if you need help preparing the cloud
PR. -->
- [ ] If this PR includes major [user-facing behavior
changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note),
I have pinged the relevant PM to schedule a changelog post.

---------

Co-authored-by: Parker Timmerman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants