Feature/network policy sql #30172

jubrad · 2024-10-23T21:44:27Z

Motivation

Adds SQL for managing network policies
Part of https://github.com/MaterializeInc/database-issues/issues/4637

Tips for reviewer

syntax discussed here #29814

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.

shepherdlybot · 2024-10-29T15:38:25Z

Mitigations

Completing required mitigations increases Resilience Coverage.

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

Risk Summary:

The pull request presents a high risk with a score of 80, driven by the predictors "Sum Bug Reports Of Files" and "Delta of Executable Lines." Historically, PRs with these predictors are 116% more likely to cause a bug than the repository baseline. Additionally, seven modified files are recent hotspots for bug fixes. The observed bug trend in the repository is increasing, indicating a need for careful review.

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
../src/parser.rs	98
../catalog/state.rs	92
../defs/statement.rs	98
../src/catalog.rs	93
../coord/ddl.rs	91
../sequencer/inner.rs	97
../statement/ddl.rs	99

ParkMyCar · 2024-10-29T21:03:50Z

@jkosh44 I can review this one since I have the context paged in, feel free to unassign yourself!

jkosh44 · 2024-10-29T21:17:21Z

@jkosh44 I can review this one since I have the context paged in, feel free to unassign yourself!

Works for me.

ParkMyCar

By and large looks good to me, just some smaller changes I'd love to see before merging.

Btw I looked into the massive diff on the Persist stats snapshot, it's because you added a new variant to the AclMode struct which changed the proptest strategy for generating Datums. So no worries about that!

doc/user/content/sql/system-catalog/mz_internal.md

src/sql-parser/src/ast/defs/statement.rs

src/sql-parser/src/parser.rs

src/adapter/src/catalog/transact.rs

ParkMyCar · 2024-10-30T20:07:39Z

src/repr/src/adt/mz_acl_item.rs

+// compute Network Policy
+const CREATE_NETWORK_POLICY_CHAR: char = 'P';


how did you select P, feels like something we should run by the sql council? Also note the comment on the other consts capitalizes the letter in the resource that we're using here, e.g. this comment should be // network Policy if we use P

yeah, well it is the P in network Policy, whether that's good enough justification IDK, but it seems as reasonable as anything I can think of.

Yeah it seems quite reasonable, we could also do n but that might confuse users with N, sticking with P sounds good to me

src/catalog/src/durable/transaction.rs

doc/user/sql-grammar/sql-grammar.bnf

ParkMyCar · 2024-10-30T20:18:36Z

test/sqllogictest/network_policy.slt

+SHOW NETWORK POLICIES
+----
+np
+(empty)


should SHOW NETWORK POLICIES include more info? maybe the rules that are in the policy?

I updated this to include rules, for better or worse, I went with a similar model to how we display replicas on clusters. The downside to this is that one could create a policy with 25 rules, leading to a rather long value.

That looks good to me!

doc/user/content/sql/system-catalog/mz_internal.md

test/testdrive/session.td

ParkMyCar

Thanks for working through this one, looks great!

### 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 MaterializeInc/database-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](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.  - [ ] 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)).  - [ ] 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]>

jubrad self-assigned this Oct 23, 2024

jubrad force-pushed the feature/network-policy-sql branch 8 times, most recently from dee5cb3 to bf15d75 Compare October 29, 2024 15:24

jubrad marked this pull request as ready for review October 29, 2024 15:36

jubrad requested review from a team as code owners October 29, 2024 15:36

jubrad requested a review from jkosh44 October 29, 2024 15:36

jubrad force-pushed the feature/network-policy-sql branch from bf15d75 to b91df2d Compare October 29, 2024 15:37

jubrad force-pushed the feature/network-policy-sql branch from b91df2d to 0596606 Compare October 29, 2024 18:00

jubrad requested a review from ParkMyCar October 29, 2024 19:44

jkosh44 removed their request for review October 29, 2024 21:17

jubrad force-pushed the feature/network-policy-sql branch from 0596606 to f9e585d Compare October 29, 2024 22:48

jubrad mentioned this pull request Oct 30, 2024

Feature/network policy predefined #30261

Merged

5 tasks

jubrad force-pushed the feature/network-policy-sql branch 2 times, most recently from 9a1519a to 427bc66 Compare October 30, 2024 16:32

ParkMyCar reviewed Oct 30, 2024

View reviewed changes

jubrad force-pushed the feature/network-policy-sql branch 3 times, most recently from c74e4e5 to 4851fc5 Compare October 31, 2024 04:57

morsapaes reviewed Oct 31, 2024

View reviewed changes

doc/user/content/sql/system-catalog/mz_internal.md Show resolved Hide resolved

morsapaes reviewed Oct 31, 2024

View reviewed changes

test/testdrive/session.td Outdated Show resolved Hide resolved

jubrad force-pushed the feature/network-policy-sql branch from 4851fc5 to d15206a Compare October 31, 2024 14:51

jubrad requested a review from ParkMyCar October 31, 2024 15:05

jubrad force-pushed the feature/network-policy-sql branch 5 times, most recently from e360c89 to 5bfe9cd Compare October 31, 2024 18:50

add network policy create/alter/drop/comment

8b132fb

jubrad force-pushed the feature/network-policy-sql branch from 5bfe9cd to 8b132fb Compare October 31, 2024 19:08

This was referenced Oct 31, 2024

Feature/network policy docs #30296

Merged

Feature/network policies lockout prevention #30303

Merged

ParkMyCar approved these changes Nov 1, 2024

View reviewed changes

jubrad merged commit 289410e into MaterializeInc:main Nov 1, 2024
83 checks passed

morsapaes mentioned this pull request Nov 1, 2024

Support network policy configuration MaterializeInc/terraform-provider-materialize#666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/network policy sql #30172

Feature/network policy sql #30172

jubrad commented Oct 23, 2024 •

edited

Loading

shepherdlybot bot commented Oct 29, 2024 •

edited

Loading

ParkMyCar commented Oct 29, 2024

jkosh44 commented Oct 29, 2024

ParkMyCar left a comment •

edited

Loading

ParkMyCar Oct 30, 2024

jubrad Oct 30, 2024

ParkMyCar Nov 1, 2024

ParkMyCar Oct 30, 2024

jubrad Oct 31, 2024

ParkMyCar Nov 1, 2024

ParkMyCar left a comment

		// compute Network Policy
		const CREATE_NETWORK_POLICY_CHAR: char = 'P';

Feature/network policy sql #30172

Feature/network policy sql #30172

Conversation

jubrad commented Oct 23, 2024 • edited Loading

Motivation

Tips for reviewer

Checklist

shepherdlybot bot commented Oct 29, 2024 • edited Loading

Mitigations

ParkMyCar commented Oct 29, 2024

jkosh44 commented Oct 29, 2024

ParkMyCar left a comment • edited Loading

Choose a reason for hiding this comment

ParkMyCar Oct 30, 2024

Choose a reason for hiding this comment

jubrad Oct 30, 2024

Choose a reason for hiding this comment

ParkMyCar Nov 1, 2024

Choose a reason for hiding this comment

ParkMyCar Oct 30, 2024

Choose a reason for hiding this comment

jubrad Oct 31, 2024

Choose a reason for hiding this comment

ParkMyCar Nov 1, 2024

Choose a reason for hiding this comment

ParkMyCar left a comment

Choose a reason for hiding this comment

jubrad commented Oct 23, 2024 •

edited

Loading

shepherdlybot bot commented Oct 29, 2024 •

edited

Loading

ParkMyCar left a comment •

edited

Loading