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

multi-network: fix eastwest gateway endpoint filtering #38762

Merged
merged 6 commits into from Jun 1, 2022

Conversation

stevenctl
Copy link
Contributor

I think this fixes/helps with #38704

Gateways care about server settings only (authn, and tlsMode labels) when doing this filtering.

We will filter out servers that we don't think accept mTLS (DISABLED or tlsMode: none). If a client somehow sent non-mTLS traffic, they would have to spoof SNI and then the server would deny them anyway.

@stevenctl stevenctl requested a review from a team as a code owner May 5, 2022 20:56
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 5, 2022
@stevenctl stevenctl changed the title multi-network: prevent eastwest gateway endpoint filtering multi-network: fix eastwest gateway endpoint filtering May 5, 2022
@nmittler
Copy link
Contributor

nmittler commented May 5, 2022

@stevenctl can you also update this test to include a Count > 1? This test passing in multi-network actually relies on Count==1, which is incredibly fragile.

Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

It needs a test

@stevenctl stevenctl requested a review from a team as a code owner May 25, 2022 18:26
@stevenctl
Copy link
Contributor Author

Adjusted a test per @nmittler comment. His PR #37914 improves coverage much more.

@stevenctl stevenctl requested review from a team as code owners May 25, 2022 18:37
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 25, 2022
@howardjohn
Copy link
Member

I think this makes sense, will let the others that commented take a look as well

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label May 27, 2022
Steven Landow added 4 commits May 31, 2022 10:16
Change-Id: I8395c8272bb6ba4b79b663194102852a739400b9
Change-Id: I4f34b1f47ddf976fa9417a63c3a4331a76f2b613
Change-Id: I3b9ca40346467c91d479f0d91c8d5c813f3898c4
Change-Id: I2cb8653f4cbd20f78c2f25c9ac7d6fa983dff2c8
@istio-testing istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR needs to be rebased before being merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 31, 2022
Steven Landow added 2 commits May 31, 2022 10:23
Change-Id: I045946df16e54fe49f7a6a5103d9cafa446474af
Change-Id: I074b4145eac7fe8d7ce967f3a149fb94be5427a6
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 31, 2022
@stevenctl
Copy link
Contributor Author

It needs a test

Nathan's PR added tests. This PR removed the need for the global DestinationRule

@hzxuzhonghu
Copy link
Member

What;'s the behavior with tls disabled before and after? IIUC, for east-west gateway the cluster will include cluster local endpoints.

@stevenctl
Copy link
Contributor Author

This makes it so TLS disabled in DR doesn't affect passthrough gateways (the endpoint will remain). That DR based filtering makes it so clients can't send plaintext that will inevitably fail since we require TLS/SNI.

If a server is plaintext, we still remove the endpoint from the gateway.

@stevenctl
Copy link
Contributor Author

will include cluster local endpoints.

Network local, non plaintext endpoints.

@hzxuzhonghu
Copy link
Member

Network local, non plaintext endpoints.

Looks good

@istio-testing
Copy link
Collaborator

In response to a cherrypick label: #38762 failed to apply on top of branch "release-1.13":

Applying: multi-network: prevent eastwest gateway endpoint filtering
Applying: rel note
Applying: skip check for 500s
Using index info to reconstruct a base tree...
A	pkg/test/framework/components/echo/check/checkers.go
Falling back to patching base and 3-way merge...
CONFLICT (add/add): Merge conflict in tests/integration/security/authorization_test.go
Auto-merging tests/integration/security/authorization_test.go
CONFLICT (modify/delete): pkg/test/framework/components/echo/check/checkers.go deleted in HEAD and modified in skip check for 500s. Version skip check for 500s of pkg/test/framework/components/echo/check/checkers.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 skip check for 500s
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

@istio-testing
Copy link
Collaborator

In response to a cherrypick label: new issue created for failed cherrypick: #39226

@istio-testing
Copy link
Collaborator

In response to a cherrypick label: #38762 failed to apply on top of branch "release-1.13":

Applying: multi-network: prevent eastwest gateway endpoint filtering
Applying: rel note
Applying: skip check for 500s
Using index info to reconstruct a base tree...
A	pkg/test/framework/components/echo/check/checkers.go
Falling back to patching base and 3-way merge...
CONFLICT (add/add): Merge conflict in tests/integration/security/authorization_test.go
Auto-merging tests/integration/security/authorization_test.go
CONFLICT (modify/delete): pkg/test/framework/components/echo/check/checkers.go deleted in HEAD and modified in skip check for 500s. Version skip check for 500s of pkg/test/framework/components/echo/check/checkers.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 skip check for 500s
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

@istio-testing
Copy link
Collaborator

In response to a cherrypick label: new issue created for failed cherrypick: #39227

@istio-testing
Copy link
Collaborator

In response to a cherrypick label: #38762 failed to apply on top of branch "release-1.14":

Applying: multi-network: prevent eastwest gateway endpoint filtering
Applying: rel note
Applying: skip check for 500s
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
CONFLICT (add/add): Merge conflict in tests/integration/security/authorization_test.go
Auto-merging tests/integration/security/authorization_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 skip check for 500s
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

@istio-testing
Copy link
Collaborator

In response to a cherrypick label: new issue created for failed cherrypick: #39228

@istio-testing
Copy link
Collaborator

In response to a cherrypick label: #38762 failed to apply on top of branch "release-1.14":

Applying: multi-network: prevent eastwest gateway endpoint filtering
Applying: rel note
Applying: skip check for 500s
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
CONFLICT (add/add): Merge conflict in tests/integration/security/authorization_test.go
Auto-merging tests/integration/security/authorization_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 skip check for 500s
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

@istio-testing
Copy link
Collaborator

In response to a cherrypick label: new issue created for failed cherrypick: #39229

stevenctl pushed a commit to stevenctl/istio that referenced this pull request Jun 3, 2022
* multi-network: prevent eastwest gateway endpoint filtering

Change-Id: I8395c8272bb6ba4b79b663194102852a739400b9

* rel note

Change-Id: I4f34b1f47ddf976fa9417a63c3a4331a76f2b613

* skip check for 500s

Change-Id: I3b9ca40346467c91d479f0d91c8d5c813f3898c4

* multiple calls per-cluster

Change-Id: I2cb8653f4cbd20f78c2f25c9ac7d6fa983dff2c8

* remove global dr workaround

Change-Id: I045946df16e54fe49f7a6a5103d9cafa446474af

* remove old tests

Change-Id: I074b4145eac7fe8d7ce967f3a149fb94be5427a6
stevenctl pushed a commit to stevenctl/istio that referenced this pull request Jun 3, 2022
* multi-network: prevent eastwest gateway endpoint filtering

Change-Id: I8395c8272bb6ba4b79b663194102852a739400b9

* rel note

Change-Id: I4f34b1f47ddf976fa9417a63c3a4331a76f2b613

* skip check for 500s

Change-Id: I3b9ca40346467c91d479f0d91c8d5c813f3898c4

* multiple calls per-cluster

Change-Id: I2cb8653f4cbd20f78c2f25c9ac7d6fa983dff2c8

* remove global dr workaround

Change-Id: I045946df16e54fe49f7a6a5103d9cafa446474af

* remove old tests

Change-Id: I074b4145eac7fe8d7ce967f3a149fb94be5427a6
istio-testing pushed a commit that referenced this pull request Jun 3, 2022
* multi-network: prevent eastwest gateway endpoint filtering

Change-Id: I8395c8272bb6ba4b79b663194102852a739400b9

* rel note

Change-Id: I4f34b1f47ddf976fa9417a63c3a4331a76f2b613

* skip check for 500s

Change-Id: I3b9ca40346467c91d479f0d91c8d5c813f3898c4

* multiple calls per-cluster

Change-Id: I2cb8653f4cbd20f78c2f25c9ac7d6fa983dff2c8

* remove global dr workaround

Change-Id: I045946df16e54fe49f7a6a5103d9cafa446474af

* remove old tests

Change-Id: I074b4145eac7fe8d7ce967f3a149fb94be5427a6
istio-testing pushed a commit that referenced this pull request Jun 3, 2022
* multi-network: prevent eastwest gateway endpoint filtering

Change-Id: I8395c8272bb6ba4b79b663194102852a739400b9

* rel note

Change-Id: I4f34b1f47ddf976fa9417a63c3a4331a76f2b613

* skip check for 500s

Change-Id: I3b9ca40346467c91d479f0d91c8d5c813f3898c4

* multiple calls per-cluster

Change-Id: I2cb8653f4cbd20f78c2f25c9ac7d6fa983dff2c8

* remove global dr workaround

Change-Id: I045946df16e54fe49f7a6a5103d9cafa446474af

* remove old tests

Change-Id: I074b4145eac7fe8d7ce967f3a149fb94be5427a6
@nmittler
Copy link
Contributor

nmittler commented Jun 7, 2022

@stevenctl this seems to have broken TestAuthz_EgressGateway, leading to the flakes related to that test in #39255.

Specifically, calls will fail when they follow the path source->eastwest->egress->target. They fail in the east-west gateway, because there are no endpoints for the SNI route for the egress-gateway.

I tried forcibly adding a DR to the istio-system namespace:

apiVersion: networking.istio.io/v1beta1
kind: DestinationRule
metadata:
  name: test-egress
  namespace: istio-system
spec:
  host: "istio-egressgateway.istio-system.svc.cluster.local"
  trafficPolicy:
    portLevelSettings:
      - port:
          number: 443
        tls:
          mode: ISTIO_MUTUAL

... but it had no effect (still no endpoints for the egress gateway SNI route).

I locally rolled back this change and it works fine.

@nmittler
Copy link
Contributor

nmittler commented Jun 7, 2022

I opened #39330

nmittler added a commit to nmittler/istio that referenced this pull request Jun 7, 2022
nmittler added a commit to nmittler/istio that referenced this pull request Jun 7, 2022
istio-testing pushed a commit that referenced this pull request Jun 7, 2022
GregHanson added a commit to GregHanson/istio that referenced this pull request Jun 9, 2022
lei-tang added a commit to lei-tang/istio that referenced this pull request Jun 9, 2022
istio-testing pushed a commit that referenced this pull request Jun 9, 2022
istio-testing pushed a commit that referenced this pull request Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants