NetworkChaos: Add support for ports in external targets #2932

miedzinski · 2022-02-23T12:37:58Z

Signed-off-by: Dominik Miedziński dominik.miedzinski@allegro.pl

What problem does this PR solve?

This PR adds support for declaring ports in external targets in NetworkChaos experiments.

What's changed and how it works?

Chaos-daemon now creates 3 IP sets in pods: hash:net (same as before), hash:net,port (allowing for testing ip and port pairs) and list:set that combines them. Iptables rules now reference the list:set IP set.

There are changes to PodNetworkChaos, but NetworkChaos remains unchanged and there's no need to update dashboard.

Related changes

Need to update chaos-mesh/website
Need to update Dashboard UI
Need to cheery-pick to release branches
- release-2.1
- release-2.0

Checklist

Tests

Unit test
E2E test
No code
Manual test (add steps below)

Side effects

Breaking backward compatibility

Release note

Add support for declaring ports in external targets in NetworkChaos experiments.

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

ti-chi-bot · 2022-02-23T12:38:00Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

STRRL
cwen0

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2022-02-23T12:38:07Z

Welcome @miedzinski!

It looks like this is your first PR to chaos-mesh/chaos-mesh 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to chaos-mesh/chaos-mesh. 😃

codecov · 2022-02-25T02:33:56Z

Codecov Report

Merging #2932 (48ed931) into master (96f979a) will increase coverage by 0.27%.
The diff coverage is 48.30%.

@@            Coverage Diff             @@
##           master    #2932      +/-   ##
==========================================
+ Coverage   37.92%   38.19%   +0.27%     
==========================================
  Files         105      105              
  Lines        9108     9182      +74     
==========================================
+ Hits         3454     3507      +53     
- Misses       5344     5366      +22     
+ Partials      310      309       -1

Impacted Files	Coverage Δ
controllers/podnetworkchaos/controller.go	`37.38% <0.00%> (-1.65%)`	⬇️
controllers/podnetworkchaos/ipset/ipset.go	`2.59% <0.00%> (-2.17%)`	⬇️
pkg/chaosdaemon/iptables_server.go	`66.11% <50.00%> (ø)`
pkg/chaosdaemon/ipset_server.go	`90.15% <88.57%> (+3.48%)`	⬆️
controllers/podnetworkchaos/netutils/cidr.go	`73.07% <89.28%> (+73.07%)`	⬆️
pkg/workflow/controllers/utils.go	`85.71% <0.00%> (-1.59%)`	⬇️
pkg/selector/generic/mode.go	`28.20% <0.00%> (+2.56%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02c0e1d...48ed931. Read the comment docs.

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

STRRL · 2022-03-01T05:00:30Z

Hi @miedzinski , thanks for your contribution! That's an awesome feature!

But this PR is really huge, and it would still break the API of Chaos Mesh (with podnetworkchaos). So I am afraid this PR would not be merged recently.

Currently, the design of NetworkChaos does not care about the port thing, it is designed on the Network Layer(IP Layer). If we want to filter the network traffic with ports(actually it's not difficult with iptables and current code base), we should change our design with at least Transport Layer(consider some protocol, TCP/UDP/...). It would make huge changes to the design of NetworkChaos. I think we would better draft an RFC into rfcs before we do that.

How do you think about it? @miedzinski

miedzinski · 2022-03-01T11:51:58Z

@STRRL, thanks for your response.

You're right there is a problem with backward compatibility of PodNetworkChaos, but I think it's something we can solve:

There is no need to name all IP sets in PodNetworkChaos. hash:net and hash:net,port names can be derived in daemon from list:set name.
IP set contents can remain strings and be parsed later.

This way all CRDs are unchanged. There is also a change to protobufs - if you think these shouldn't be changed too, then I can move parsing and resolving external targets to chaos daemon. That would additionally solve a TODO comment.

Regarding the RFC process, this is something I'd rather avoid if possible. Filtering traffic by ports is crucial for adoption of Chaos Mesh in my organization, but we definitely don't intend to make huge changes to design of experiments at this point. I don't think the L3 vs L4 is also an issue, because NetworkChaos already resolves hostnames and that's Application Layer.

Please let me know if you're okay with changes I suggested.

STRRL · 2022-03-02T07:00:38Z

You're right there is a problem with backward compatibility of PodNetworkChaos, but I think it's something we can solve:

There is no need to name all IP sets in PodNetworkChaos. hash:net and hash:net,port names can be derived in daemon from list:set name.

IP set contents can remain strings and be parsed later.

This way all CRDs are unchanged.

We only consider "backward compatibility" now. We could accept appending more fields with default values. But renaming/deleting fields is NOT preferred.

I have another idea based on your suggestion. We could extend v1alpha1.RawIPSet and pb.IPSet to support more type of ipset, instead of using one v1alpha1.RawIPSet to introduce the combination of ipset rules which contains hash:ip, hash:IP,port and list:net.

const (
	IPSetTypeHashIP     IPSetType = "hash:ip"
	IPSetTypeHashIPPort IPSetType = "hash:ip,port"
	IPSetTypeListNet    IPSetType = "list:net"
	// other IPSet types that we need
)

// RawIPSet represents an ipset on specific pod
type RawIPSet struct {
	// The name of set ipset
	Name string `json:"name"`

	IPSetType IPSetType `json:"ipsetType"`

	// The contents of ipset.
	// Only available when IPSetType is IPSetTypeHashIP
	Cidrs []string `json:"cidrs"`

	// The contents of ipset.
	// Only available when IPSetType is IPSetTypeHashIPPort
	CidrAndPorts []CidrAndPort `json:"cidrAndPorts"`

	// The contents of ipset.
	// Only available when IPSetType is IPSetTypeListNet
	SetNames []string `json:"setNames"`

	// The name and namespace of the source network chaos
	RawRuleSource `json:",inline"`
}

We would append three ordered RawIPset not one into PodNetworkChaos.

before:

[{
  "setName": "chaos1_set_tgt",
  "netPortName": "chaos1_netPort_tgt",
  "netName": "chaos1_net_tgt",
  "cidrs": [
    {
      "cidr": "1.1.1.1",
      "port": 53
    },
    {
      "cidr": "1.2.3.4",
      "port": 0
    }
  ]
}]

after:

[
  {
    "name": "chaos1_ipport_tgt",
    "type": "hash:ip,port",
    "cidrAndPorts": [
      {
        "cidr": "1.1.1.1",
        "port": 53
      }
    ]
  },
  {
    "name": "chaos1_ip_tgt",
    "type": "hash:ip",
    "cidrs": [
      "1.2.3.4"
    ]
  },
  {
    "name": "chaos1_set_tgt",
    "type": "list:set",
    "setName": [
      "chaos1_ipport_tgt",
      "chaos1_ip_tgt"
    ]
  }
]

What do you think about it? @miedzinski

miedzinski · 2022-03-02T07:47:38Z

That's a good solution too. I'll send a patch in the next few days.

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

miedzinski · 2022-03-04T13:33:00Z

/cc @STRRL

miedzinski · 2022-03-04T13:39:13Z

There is still one field removed from protobuf, but I believe this is not an issue, isn't it? If yes, I can remove a oneof and apply a similar layout to PodNetworkChaos.

STRRL · 2022-03-07T05:04:12Z

There is still one field removed from protobuf, but I believe this is not an issue, isn't it? If yes, I can remove a oneof and apply a similar layout to PodNetworkChaos.

It's not an issue. But I still prefer to keep the same layout to PodNetworkChaos. Please update it. ❤️

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

miedzinski · 2022-03-07T10:45:19Z

@STRRL done! Now we're only adding new fields.

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

cwen0 · 2022-03-15T03:33:15Z

controllers/chaosimpl/networkchaos/trafficcontrol/impl.go

-	dstIpset := ipset.BuildIPSet(targetPods, externalCidrs, networkchaos, string(tcType[0:2])+ipSetPostFix, m.Source)
-	impl.Log.Info("apply traffic control with filter", "sources", m.Source, "ipset", dstIpset)
+	dstSetIPSet, dstOtherIPSets := ipset.BuildIPSets(targetPods, externalCidrs, networkchaos, string(tcType[0:2])+ipSetPostFix, m.Source)
+	impl.Log.Info("apply traffic control with filter", "sources", m.Source, "ipset", dstSetIPSet)


We should print the whole ipset info at this log.

cwen0 · 2022-03-15T06:39:10Z

controllers/podnetworkchaos/ipset/ipset.go

+	netPortName := GenerateIPSetName(networkchaos, "netport_"+namePostFix)
+
+	cidrs := []string{}
+	cidrandPorts := []v1alpha1.CidrAndPort{}


s/cidrandPorts/cidrAndPorts/g

STRRL · 2022-03-17T07:44:36Z

api/v1alpha1/podnetworkchaos_types.go

+// CidrAndPort represents CIDR and port pair
+type CidrAndPort struct {
+	Cidr string `json:"cidr"`
+
+	// +kubebuilder:validation:Minimum=1
+	// +kubebuilder:validation:Maximum=65535
+	Port uint16 `json:"port"`
+}
+


We should also append fields to introduce protocol, like tcp, udp or icmp. When specify ipset rule without protocol, it would only specify with tcp.

example usage like 192.168.1.1,udp:53

This is something I'd rather avoid right now for a couple of reasons:

This PR is already huge. Filtering by protocols needs more changes to IP sets and/or iptables rules.

Adding a support for filtering by protocols is outside the scope of this PR.

Defaulting to TCP would break backward compatibility. As of now Chaos Mesh is protocol agnostic.

pkg/chaosdaemon/iptables_server.go

YangKeao · 2022-03-17T09:56:37Z

pkg/chaosdaemon/iptables_server.go

@@ -94,10 +94,10 @@ func (iptables *iptablesClient) setIptablesChain(chain *pb.Chain) error {
 	var matchPart string
 	var interfaceMatcher string
 	if chain.Direction == pb.Chain_INPUT {
-		matchPart = "src"
+		matchPart = "src,dst"


Why do we need this modification? If I'm not wrong, it means this chain applies, iff both the source and destination are in the ipset, which seems not to be what we want?

This means it must match a pair of source IP address and destination port. Please refer to man iptables-extensions - section set, --match-set parameter and man ipset - section introduction, set dimensions.

I think we need more documentation to describe it but it still makes sense:

with action: delay/loss/... (which requires tc) and direction: from, externalTargets would not works

with action: partition, direction: from, externalTagets: [10.96.0.0/16:8080], if would drop any packet from 10.96.0.0/16 to <pod-ip>:8080.

STRRL · 2022-03-17T10:58:51Z

Hi @miedzinski , could you please append an entry in section Unreleased / Added of CHANGELOG.MD?

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

miedzinski · 2022-03-17T11:04:19Z

Hi @miedzinski , could you please append an entry in section Unreleased / Added of CHANGELOG.MD?

@STRRL Done!

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

STRRL · 2022-03-18T06:39:11Z

/run-e2e-tests

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

STRRL

LGTM!

Thanks for your contribution! 🤩

cwen0 · 2022-03-29T03:08:42Z

/cc @YangKeao

cwen0

LGTM

cwen0 · 2022-04-01T07:17:47Z

/merge

ti-chi-bot · 2022-04-01T07:17:49Z

This pull request has been accepted and is ready to merge.

Commit hash: dd92f71

ti-chi-bot · 2022-04-01T07:18:03Z

@miedzinski: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

STRRL · 2022-04-01T08:24:42Z

/run-e2e-tests

Add support for ports in NetworkChaos external targets

d02e00e

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

ti-chi-bot added contribution first-time-contributor labels Feb 23, 2022

ti-chi-bot requested review from cwen0 and Hexilee February 23, 2022 12:38

ti-chi-bot added the size/XXL label Feb 23, 2022

Merge branch 'master' into network-external-targets-port

01cee4f

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

ti-chi-bot added the needs-rebase label Feb 28, 2022

miedzinski added 2 commits March 4, 2022 09:37

Merge branch 'master' into network-external-targets-port

eb33200

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

Refactor so that there are no changed fields in CRDs

6b5b2c1

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

ti-chi-bot removed the needs-rebase label Mar 4, 2022

Merge branch 'master' into network-external-targets-port

4e28aec

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

ti-chi-bot requested a review from STRRL March 4, 2022 13:33

miedzinski added 3 commits March 7, 2022 11:31

Use similar layout in protobufs to PodNetworkChaos

43312da

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

Merge branch 'master' into network-external-targets-port

1e57baf

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

Use snake case in protobufs

f8124f0

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

Merge branch 'master' into network-external-targets-port

21a4c16

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

cwen0 reviewed Mar 15, 2022

View reviewed changes

STRRL requested review from YangKeao and Andrewmatilde March 17, 2022 06:59

STRRL reviewed Mar 17, 2022

View reviewed changes

pkg/chaosdaemon/iptables_server.go Show resolved Hide resolved

pkg/chaosdaemon/iptables_server.go Show resolved Hide resolved

YangKeao reviewed Mar 17, 2022

View reviewed changes

Merge branch 'master' into network-external-targets-port

55f8a24

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

Add an entry to changelog

4a7e373

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

miedzinski force-pushed the network-external-targets-port branch from db07013 to 4a7e373 Compare March 17, 2022 11:04

Add omitempty

dd92f71

Signed-off-by: Dominik Miedziński <dominik.miedzinski@allegro.pl>

STRRL reviewed Mar 24, 2022

View reviewed changes

STRRL approved these changes Mar 24, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 label Mar 24, 2022

cwen0 requested review from YangKeao and cwen0 March 29, 2022 03:08

cwen0 approved these changes Apr 1, 2022

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels Apr 1, 2022

ti-chi-bot added the status/can-merge label Apr 1, 2022

Merge branch 'master' into network-external-targets-port

48ed931

ti-chi-bot merged commit d574350 into chaos-mesh:master Apr 1, 2022

STRRL mentioned this pull request Apr 27, 2022

Expand NetworkChaos to support src/dst ports. #2295

Open

YangKeao mentioned this pull request May 9, 2022

containerNames field in NetworkChaosSpec is documented but not implemented #3249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NetworkChaos: Add support for ports in external targets #2932

NetworkChaos: Add support for ports in external targets #2932

miedzinski commented Feb 23, 2022

ti-chi-bot commented Feb 23, 2022 •

edited

ti-chi-bot commented Feb 23, 2022

codecov bot commented Feb 25, 2022 •

edited

STRRL commented Mar 1, 2022

miedzinski commented Mar 1, 2022

STRRL commented Mar 2, 2022

miedzinski commented Mar 2, 2022

miedzinski commented Mar 4, 2022

miedzinski commented Mar 4, 2022

STRRL commented Mar 7, 2022

miedzinski commented Mar 7, 2022

cwen0 Mar 15, 2022

cwen0 Mar 15, 2022

STRRL Mar 17, 2022

STRRL Mar 17, 2022

miedzinski Mar 17, 2022

STRRL Mar 17, 2022

YangKeao Mar 17, 2022

miedzinski Mar 17, 2022

STRRL Mar 24, 2022

STRRL commented Mar 17, 2022 •

edited

miedzinski commented Mar 17, 2022

STRRL commented Mar 18, 2022

STRRL left a comment

cwen0 commented Mar 29, 2022

cwen0 left a comment

cwen0 commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

STRRL commented Apr 1, 2022

NetworkChaos: Add support for ports in external targets #2932

NetworkChaos: Add support for ports in external targets #2932

Conversation

miedzinski commented Feb 23, 2022

What problem does this PR solve?

What's changed and how it works?

Related changes

Checklist

Release note

ti-chi-bot commented Feb 23, 2022 • edited

ti-chi-bot commented Feb 23, 2022

codecov bot commented Feb 25, 2022 • edited

Codecov Report

STRRL commented Mar 1, 2022

miedzinski commented Mar 1, 2022

STRRL commented Mar 2, 2022

miedzinski commented Mar 2, 2022

miedzinski commented Mar 4, 2022

miedzinski commented Mar 4, 2022

STRRL commented Mar 7, 2022

miedzinski commented Mar 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

STRRL commented Mar 17, 2022 • edited

miedzinski commented Mar 17, 2022

STRRL commented Mar 18, 2022

STRRL left a comment

Choose a reason for hiding this comment

cwen0 commented Mar 29, 2022

cwen0 left a comment

Choose a reason for hiding this comment

cwen0 commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

STRRL commented Apr 1, 2022

ti-chi-bot commented Feb 23, 2022 •

edited

codecov bot commented Feb 25, 2022 •

edited

STRRL commented Mar 17, 2022 •

edited