Skip to content

Add traffic permissions integration tests.#19008

Merged
erichaberkorn merged 2 commits intomainfrom
tp-tests
Oct 6, 2023
Merged

Add traffic permissions integration tests.#19008
erichaberkorn merged 2 commits intomainfrom
tp-tests

Conversation

@erichaberkorn
Copy link
Contributor

@erichaberkorn erichaberkorn commented Sep 26, 2023

Description

Add integration tests for TCP traffic permissions.

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 26, 2023
@erichaberkorn erichaberkorn force-pushed the tp-tests branch 8 times, most recently from 5f591b7 to eb17a1e Compare September 28, 2023 13:33
Comment on lines 138 to 145
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't particularly like this, but it's hard to do much better without making a breaking change that impacts tons and tons of tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ishustava It feels weird to me that configuring a traffic permission with default allow only swaps to default deny for a single port rather than all port. Should we change this behavior to swap all ports to default deny?

@erichaberkorn erichaberkorn marked this pull request as ready for review September 28, 2023 17:10
@erichaberkorn erichaberkorn requested a review from a team as a code owner September 28, 2023 17:10
@vercel
Copy link

vercel bot commented Oct 4, 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 4:02pm
consul-ui-staging ⬜️ Ignored (Inspect) Oct 4, 2023 4:02pm

// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

package tproxy
Copy link
Member

Choose a reason for hiding this comment

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

Should this be trafficpermissions?

Copy link
Member

@jmurret jmurret left a comment

Choose a reason for hiding this comment

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

👍 LGTM. Can you add the comment in code that I mentioned? Once it is merged, I will reference it in the ticket that I create.

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a note for something like:

On Mac M1s when TProxy is enabled, consul-dataplane that are spawned from this image (only used in consul-container integration tests) will terminate with the below error. It is related to tproxy-startup.sh calling iptables SDK which then calls the underly iptables. We are investigating how this works on M1s with consul-envoy images which do not have this problem. For the time being tproxy tests on Mac M1s will fail locally but pass in CI.

Error setting up traffic redirection rules: failed to run command: /sbin/iptables -t nat -N CONSUL_PROXY_INBOUND, err: exit status 1, output: iptables: Failed to initialize nft: Protocol not supported

@erichaberkorn erichaberkorn force-pushed the tp-tests branch 2 times, most recently from acaa084 to 53cf850 Compare October 6, 2023 14:58
@erichaberkorn erichaberkorn merged commit ad3aab1 into main Oct 6, 2023
@erichaberkorn erichaberkorn deleted the tp-tests branch October 6, 2023 16:06
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants