Skip to content

Fix Traffic Permissions Default Deny#19028

Merged
erichaberkorn merged 1 commit intomainfrom
fix-default-deny-traffic-permissions
Oct 4, 2023
Merged

Fix Traffic Permissions Default Deny#19028
erichaberkorn merged 1 commit intomainfrom
fix-default-deny-traffic-permissions

Conversation

@erichaberkorn
Copy link
Contributor

Description

Whenever a traffic permission exists for a given workload identity, turn on default deny. Previously, this only worked at the port level.

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@erichaberkorn erichaberkorn added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-backport labels Sep 29, 2023
@github-actions github-actions bot added the theme/envoy/xds Related to Envoy support label Sep 29, 2023
@erichaberkorn erichaberkorn force-pushed the fix-default-deny-traffic-permissions branch 4 times, most recently from 59fed97 to e414058 Compare September 29, 2023 18:10
@erichaberkorn erichaberkorn marked this pull request as ready for review September 29, 2023 18:46
@erichaberkorn erichaberkorn requested a review from a team as a code owner September 29, 2023 18:46
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks great! Just one question about skipping allow perms in default allow mode

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 what if I want to do the following in default-allow mode:

  1. create an allow-nothing traffic permission in a partition
  2. create an allow permission to a specific destination

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I misunderstood that this means default allow only if there are no traffic perms thinking that this is a global default. Maybe a comment here would be helpful

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to add docs here because this value does not necessarily behave like the global default

Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks so much for fixing it!

@vercel
Copy link

vercel bot commented Oct 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
consul ⬜️ Ignored (Inspect) Visit Preview Oct 4, 2023 1:41pm
consul-ui-staging ⬜️ Ignored (Inspect) Visit Preview Oct 4, 2023 1:41pm

@erichaberkorn erichaberkorn force-pushed the fix-default-deny-traffic-permissions branch 3 times, most recently from 2bcba2b to a83766a Compare October 3, 2023 20:21
…rn on default deny.

Previously, this was only working at the port level.
@erichaberkorn erichaberkorn force-pushed the fix-default-deny-traffic-permissions branch from a83766a to 9564a6b Compare October 4, 2023 13:41
@erichaberkorn erichaberkorn merged commit f2b7b45 into main Oct 4, 2023
@erichaberkorn erichaberkorn deleted the fix-default-deny-traffic-permissions branch October 4, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr/no-backport pr/no-changelog PR does not need a corresponding .changelog entry theme/envoy/xds Related to Envoy support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants