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.

Sign up for GitHub

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

Jump to bottom

Adjust inbound and outbound filter chains #28442

Merged
merged 2 commits into from
Nov 3, 2020
Merged

Adjust inbound and outbound filter chains #28442

merged 2 commits into from
Nov 3, 2020
+122 −16

Conversation

howardjohn
Copy link
Member

No description provided.

@howardjohn howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Oct 30, 2020
@howardjohn howardjohn requested review from a team as code owners October 30, 2020 21:16
@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Oct 30, 2020
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 30, 2020
@jacob-delgado
Copy link
Contributor

/retest

ramaraochavali
ramaraochavali reviewed Oct 31, 2020
View reviewed changes
Copy link
Contributor

@ramaraochavali ramaraochavali left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Few minor comments

pilot/pkg/features/pilot.go
@@ -400,4 +400,8 @@ var (
15*time.Second,
"If set, the max amount of time to delay a push by. Depends on PILOT_ENABLE_FLOW_CONTROL.",
).Get()

PilotEnableLoopBlockers = env.RegisterBoolVar("PILOT_ENABLE_LOOP_BLOCKER", true,
"If enabled, Envoy will be configured to prevent traffic directly the the inbound/outbound "+
Copy link
Contributor

Choose a reason for hiding this comment

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

"Envoy will be configured to prevent traffic directly" - This is confusing. Can we change to "Envoy will be configured to not send traffic directly to inbound/outbound ports" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about "PILOT_DENY_TRAFFIC_ON_LISTEN_PORTS" or "PILOT_DISABLE_TRAFFIC_ON_LISTEN_PORTS" ?

Copy link
Member

Choose a reason for hiding this comment

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

Seems doesn't matter, it will be removed

pilot/pkg/networking/core/v1alpha3/listener_builder.go
if features.PilotEnableLoopBlockers {
filterChains = append(filterChains, &listener.FilterChain{
Name: VirtualInboundBlackholeFilterChainName,
FilterChainMatch: &listener.FilterChainMatch{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create this once and reuse?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going to but I am concerned if an EnvoyFilter modifies it and it's shared then every push we will apply the same modification. In similar code in 1.4 we saw this where an access log filter got appended every push and ended up with millions of access log filters...

I am also concerned that's are other places in our code with this same problem

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure

@hzxuzhonghu
Copy link
Member

Not sure what it is trying to resolve, can you add some context

lambdai
lambdai approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

LGTM

New filter chain should never overlap with existing filter chains

@ramaraochavali
Copy link
Contributor

LGTM. Let me know if you want to change the flag based on this comment https://github.com/istio/istio/pull/28442/files#r515457221. I can approve if you are good with the existing name. I am fine with that as well.

@howardjohn
Copy link
Member Author

howardjohn commented Nov 3, 2020 via email

@howardjohn
Copy link
Member Author

/retest

@istio-testing istio-testing merged commit 1f422dd into istio:master Nov 3, 2020
@istio-testing
Copy link
Collaborator

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

Applying: Adjust inbound and outbound filter chains
Using index info to reconstruct a base tree...
M	pilot/pkg/features/pilot.go
M	pilot/pkg/networking/core/v1alpha3/listener.go
M	pilot/pkg/networking/core/v1alpha3/listener_builder.go
M	pilot/pkg/networking/core/v1alpha3/listener_test.go
A	pilot/pkg/networking/core/v1alpha3/sidecar_simulation_test.go
A	pilot/pkg/simulation/traffic.go
A	pilot/test/xdstest/validate.go
A	tests/integration/pilot/common/routing.go
A	tests/integration/pilot/common/traffic.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): tests/integration/pilot/common/traffic.go deleted in HEAD and modified in Adjust inbound and outbound filter chains. Version Adjust inbound and outbound filter chains of tests/integration/pilot/common/traffic.go left in tree.
CONFLICT (modify/delete): tests/integration/pilot/common/routing.go deleted in HEAD and modified in Adjust inbound and outbound filter chains. Version Adjust inbound and outbound filter chains of tests/integration/pilot/common/routing.go left in tree.
CONFLICT (modify/delete): pilot/test/xdstest/validate.go deleted in HEAD and modified in Adjust inbound and outbound filter chains. Version Adjust inbound and outbound filter chains of pilot/test/xdstest/validate.go left in tree.
CONFLICT (modify/delete): pilot/pkg/simulation/traffic.go deleted in HEAD and modified in Adjust inbound and outbound filter chains. Version Adjust inbound and outbound filter chains of pilot/pkg/simulation/traffic.go left in tree.
CONFLICT (modify/delete): pilot/pkg/networking/core/v1alpha3/sidecar_simulation_test.go deleted in HEAD and modified in Adjust inbound and outbound filter chains. Version Adjust inbound and outbound filter chains of pilot/pkg/networking/core/v1alpha3/sidecar_simulation_test.go left in tree.
Auto-merging pilot/pkg/networking/core/v1alpha3/listener_test.go
Auto-merging pilot/pkg/networking/core/v1alpha3/listener_builder.go
Auto-merging pilot/pkg/networking/core/v1alpha3/listener.go
Auto-merging pilot/pkg/features/pilot.go
CONFLICT (content): Merge conflict in pilot/pkg/features/pilot.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Adjust inbound and outbound filter chains
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 istio-testing mentioned this pull request Nov 3, 2020
Closed
@istio-testing
Copy link
Collaborator

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

@istio-testing
Copy link
Collaborator

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

Applying: Adjust inbound and outbound filter chains
Using index info to reconstruct a base tree...
M	pilot/pkg/features/pilot.go
M	pilot/pkg/networking/core/v1alpha3/listener_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/networking/core/v1alpha3/listener_test.go
Auto-merging pilot/pkg/features/pilot.go
CONFLICT (content): Merge conflict in pilot/pkg/features/pilot.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Adjust inbound and outbound filter chains
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: #28546

@istio-testing istio-testing mentioned this pull request Nov 3, 2020
Closed
@istio-testing
Copy link
Collaborator

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

Applying: Adjust inbound and outbound filter chains
Using index info to reconstruct a base tree...
M	pilot/pkg/features/pilot.go
M	pilot/pkg/networking/core/v1alpha3/listener.go
M	pilot/pkg/networking/core/v1alpha3/listener_builder.go
M	pilot/pkg/networking/core/v1alpha3/listener_test.go
A	pilot/pkg/networking/core/v1alpha3/sidecar_simulation_test.go
A	pilot/pkg/simulation/traffic.go
A	pilot/test/xdstest/validate.go
A	tests/integration/pilot/common/routing.go
A	tests/integration/pilot/common/traffic.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): tests/integration/pilot/common/traffic.go deleted in HEAD and modified in Adjust inbound and outbound filter chains. Version Adjust inbound and outbound filter chains of tests/integration/pilot/common/traffic.go left in tree.
CONFLICT (modify/delete): tests/integration/pilot/common/routing.go deleted in HEAD and modified in Adjust inbound and outbound filter chains. Version Adjust inbound and outbound filter chains of tests/integration/pilot/common/routing.go left in tree.
CONFLICT (modify/delete): pilot/test/xdstest/validate.go deleted in HEAD and modified in Adjust inbound and outbound filter chains. Version Adjust inbound and outbound filter chains of pilot/test/xdstest/validate.go left in tree.
CONFLICT (modify/delete): pilot/pkg/simulation/traffic.go deleted in HEAD and modified in Adjust inbound and outbound filter chains. Version Adjust inbound and outbound filter chains of pilot/pkg/simulation/traffic.go left in tree.
CONFLICT (modify/delete): pilot/pkg/networking/core/v1alpha3/sidecar_simulation_test.go deleted in HEAD and modified in Adjust inbound and outbound filter chains. Version Adjust inbound and outbound filter chains of pilot/pkg/networking/core/v1alpha3/sidecar_simulation_test.go left in tree.
Auto-merging pilot/pkg/networking/core/v1alpha3/listener_test.go
CONFLICT (content): Merge conflict in pilot/pkg/networking/core/v1alpha3/listener_test.go
Auto-merging pilot/pkg/networking/core/v1alpha3/listener_builder.go
Auto-merging pilot/pkg/networking/core/v1alpha3/listener.go
CONFLICT (content): Merge conflict in pilot/pkg/networking/core/v1alpha3/listener.go
Auto-merging pilot/pkg/features/pilot.go
CONFLICT (content): Merge conflict in pilot/pkg/features/pilot.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Adjust inbound and outbound filter chains
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 istio-testing mentioned this pull request Nov 3, 2020
Closed
@istio-testing
Copy link
Collaborator

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

howardjohn added a commit to howardjohn/istio that referenced this pull request Nov 3, 2020
@howardjohn
Adjust inbound and outbound filter chains (istio#28442)
6fb0e14 
* Adjust inbound and outbound filter chains

* fix test

(cherry picked from commit 1f422dd)
howardjohn added a commit to howardjohn/istio that referenced this pull request Nov 3, 2020
@howardjohn
Adjust inbound and outbound filter chains (istio#28442)
8428176 
* Adjust inbound and outbound filter chains

* fix test

(cherry picked from commit 1f422dd)
howardjohn added a commit to howardjohn/istio that referenced this pull request Nov 3, 2020
@howardjohn
Adjust inbound and outbound filter chains (istio#28442)
90c00e5 
* Adjust inbound and outbound filter chains

* fix test

(cherry picked from commit 1f422dd)
istio-testing pushed a commit that referenced this pull request Nov 4, 2020
@howardjohn
[1.6] Adjust inbound and outbound filter chains (#28442) (#28554)
76a3a53 
* Adjust inbound and outbound filter chains (#28442)

* Adjust inbound and outbound filter chains

* fix test

(cherry picked from commit 1f422dd)

* fixes
istio-testing pushed a commit that referenced this pull request Nov 5, 2020
@howardjohn
Adjust inbound and outbound filter chains (#28442) (#28551)
a5c7e5f 
* Adjust inbound and outbound filter chains

* fix test

(cherry picked from commit 1f422dd)
istio-testing pushed a commit that referenced this pull request Nov 6, 2020
@howardjohn
Adjust inbound and outbound filter chains (#28442) (#28553)
0b050f8 
* Adjust inbound and outbound filter chains

* fix test

(cherry picked from commit 1f422dd)
incfly pushed a commit to incfly/istio that referenced this pull request Apr 13, 2022
@howardjohn @yskopets
Adjust inbound and outbound filter chains (istio#28442) (istio#28553)
4e9ddb3 
* Adjust inbound and outbound filter chains

* fix test

(cherry picked from commit 1f422dd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzxuzhonghu hzxuzhonghu

@ramaraochavali ramaraochavali

@lambdai lambdai

Assignees

@lambdai lambdai

Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. release-notes-none Indicates a PR that does not require release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@howardjohn @jacob-delgado @hzxuzhonghu @ramaraochavali @istio-testing @lambdai