Add TrafficControl API #3644
Add TrafficControl API #3644
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3644 +/- ##
===========================================
- Coverage 64.33% 50.32% -14.02%
===========================================
Files 281 248 -33
Lines 39771 35806 -3965
===========================================
- Hits 25588 18019 -7569
- Misses 12196 15988 +3792
+ Partials 1987 1799 -188
Flags with carried forward coverage won't be shown. Click here to find out more.
|
pkg/apis/crd/v1alpha2/types.go
Outdated
| // return device of traffic redirecting. | ||
| // Multiple TrafficControl objects can use same device, however, the devices with same name must have consistent | ||
| // configuration. | ||
| type Device struct { |
There was a problem hiding this comment.
Device is too generic. We should at least add the TrafficControl prefix.
I still prefer TrafficControlPort, as internal and tunnel port is not a device. I did not get what the problem for child types. We can call them: TrafficControlPort/Tunnel/ERSPAN, or if we want to be generic: OVSPort/OVSTunnel/OVSERSPAN.
Or, if it looks cleaner to have a separate type for internal port, then we have OVSInternalPort/DevicePort/TunnelPort/ERSPANPort
There was a problem hiding this comment.
I wanted to use the parent type name as the suffix of the child type. But you proposal makes sense, we don't have to add it.
pkg/apis/crd/v1alpha2/types.go
Outdated
| Port *PortDevice `json:"port,omitempty"` | ||
| // VXLAN represents a VXLAN tunnel that is created on the Node. | ||
| // +optional | ||
| VXLAN *TunnelDevice `json:"vxlan,omitempty"` |
There was a problem hiding this comment.
Should we just add a type to TunnelDevice?
There was a problem hiding this comment.
ERSPAN is kind of Tunnel, if we add a type to TunnelPort, ERSPAN struct would repeat some tunnel attributes.
There was a problem hiding this comment.
I feel that is ok. Or if we follow ovs-vsctl, we can put ERSPAN parameters into TunnelPort
There was a problem hiding this comment.
After more thinking, how about:
OVSPort struct {
Name
Type // Internal, Device, GRE, GENEVE, VXLAN, ERSPAN
Tunnel // Tunnel parameters (remote IP, etc.)
ERSPAN // ERSPAN parameters
}
There was a problem hiding this comment.
If I get you correctly, it uses a type field to indicate type and have all type specific parameters in single struct. I feel ovs-vsctl organizes parameters in this way because it needs a consistent db schema for all kinds (all fields are in same level in form of key value). It seems opposite to how K8s API organizes type specific parameters.
And I feel organizing type specific parameters in the type's own struct is more clear and easier to validate and use. For users, it's clear to see which parameters a kind of port has, no need to understand the meaning of irrelevant fields. For developers, it's easier to consume the type specific fields when handling one type. Otherwise we need comments to explain which fields are needed for which types for both users and developers.
Examples of K8s APIs:
https://github.com/kubernetes/kubernetes/blob/1108bed7631f545d43530aa697175d243b99610b/pkg/apis/core/types.go#L58
https://github.com/kubernetes/kubernetes/blob/1108bed7631f545d43530aa697175d243b99610b/pkg/apis/core/types.go#L2234
There was a problem hiding this comment.
Sure. Your new version looks good to me too. I think it is just about sharing and encapsulation.
There was a problem hiding this comment.
The only thing is LocalPort looks strange, as it is just a flag and it is needed only by internal port.
d999301 to
762ab2c
Compare
pkg/apis/crd/v1alpha2/types.go
Outdated
|
|
||
| // LocalPort represents a local port that is attached to the OVS bridge. It can be an OVS internal port, a physical NIC, | ||
| // or a veth device. | ||
| type LocalPort struct { |
There was a problem hiding this comment.
This one looks strange, as it is shared by two types of ports, and just a flag to indicate which type. From OVS perspective, internal port is a separate type from port with a device attachment; they are not closer than tunnel type port? This is the major reason that I suggested to move type out to TrafficControlPort.
But if you want to go this way, how about adding a Device field to LocalPort. The port name can device name can be different, and also LocalPort is more meaningful then.
There was a problem hiding this comment.
Creating a port name different from device name doesn't seem useful, and it would be a little redundant for most cases in which port name and device name are same. And it's still mixing type specific parameters in one struct when there are already type specific sub-structs.
Another way I'm thinking is similar to what you suggested in #3644 (comment):
type TrafficControlPort struct {
// OVSInternal represents an OVS internal port that is created on the Node.
// +optional
OVSInternal *OVSInternalPort `json:"ovsInternal,omitempty"`
// Device represents a network device that is attached to the OVS bridge. It can be a physical NIC or a veth device.
// +optional
Device *DevicePort `json:"device,omitempty"`
// VXLAN represents a VXLAN tunnel that is created on the Node.
// +optional
VXLAN *TunnelPort `json:"vxlan,omitempty"`
// GENEVE represents a GENEVE tunnel that is created on the Node.
// +optional
GENEVE *TunnelPort `json:"geneve,omitempty"`
// GRE represents a GRE tunnel that is created on the Node.
// +optional
GRE *TunnelPort `json:"gre,omitempty"`
// ERSPAN represents an ERSPAN tunnel that is created on the Node.
// +optional
ERSPAN *ERSPANPort `json:"erspan,omitempty"`
}
type OVSInternalPort struct {
// The name of the OVS internal port.
Name string `json:"name"`
}
type DevicePort struct {
// The name of the device.
Name string `json:"name"`
}
// TunnelPort represents a tunnel that is created on the Node.
type TunnelPort struct {
// The remote IP of the tunnel.
RemoteIP string `json:"remoteIP"`
// The ID of the tunnel.
TunnelID int64 `json:"tunnelID,omitempty"`
}
And we don't have Name field in the parent struct. The port name of tunnel will be generated based on the tunnel's attributes so there wouldn't be conflict in theory. If tunnel's attributes are same, they will use same port, otherwise they will use different ports. For internal port and device port, if users set same name in different tc objects, it already means they want to use same port.
There was a problem hiding this comment.
I still prefer:
OVSPort struct {
Name
Type // Internal, Device, GRE, GENEVE, VXLAN, ERSPAN
Tunnel // Tunnel parameters (remote IP, etc.)
ERSPAN // ERSPAN parameters
}
It is easiest to understand. If for the API validation reason, you think should not have the Type field, your proposal is fine to me too. I think we can still add Name to TunnelPort assuming then the implementation can be simpler.
There was a problem hiding this comment.
I'm ok with moving tunnel type to TunnelPort if you prefer. Just want to show how OVSInternalPort and DevicePort will be differentiated here. The reasons why moving Name to OVSInternalPort and DevicePort are:
- Otherwise
OVSInternalPortandDevicePortwould have no attributes, which looks a little weird when setting this type as it would be
targetPort:
name: port0
ovsInternal: {}
- As you mentioned, user don't really care tunnel port name even though they exist, but user care ovsInternalPort name device name as they will need to listen on the ports.
- Avoid conflict when ports have same name but are of different types.
There was a problem hiding this comment.
Note that even if we extract TrafficControlPort to a separate API later, we cannot just use port name as the object name as they have different format requirement. Object name can be longer than port name. So the object name can only be used as a pointer from TrafficControl object to TrafficControlPort object.
So asking a port name for tunnel doesn't have the benefit I thought before.
There was a problem hiding this comment.
Again, I do not feel port name is a big deal. Even the use case you described are by users know about networking very well (otherwise they wont set up tunnel on the consumer side and consume raw packets), and why there can be many such tunnels by multiple users?
We should first say whether this can be used by tenant users in the future. If yes, and port name is mandatory but its naming scope is global, users in Namespace A can create vxlan0 pointing to their endpoints, then users in Namespace B cannot use vxlan0 as port name and they will have no idea why is it because they don't have global visibility. If this is tenant user facing future, why we don't consider multiple tunnels by multiple users?
But even you want to consider that, we can make port name optional (I feel it is not worthwhile and would make it mandatory for now, but up to you).
If the port name is optional for tunnels, then 1) is less suboptimal than 2) in my mind as API uniformity is not met anyway and there would be more docs explaining some fields must be set for some types and cannot set for other types, some fields are mandatory for some types but optional for other types, more complicated self-contained structs.
I do feel we should over-engineer to do fine-grained control to allow all users to mirror particular traffic. I never see the requirement for low level switching features. Even there is such a use case, they can request cluster admin to do that.
Guess you wanted to say "I do NOT feel". OK, without the use case, I would just set Name as mandatory and go 1).
For a separate CR, I meant CR name must be unique too. But I guess your point is CR name has no length limit and so easier to generate unique name? It is true, but still users need to understand two CRs cannot share the same parameters as they end up to be a single port on OVS.
No, I meant for users who have global visibility, they know all Names and it's their responsibility to avoid CR name conflict, just like any of other cluster scoped resources. For tenant users, they don't know CR names / Port names in other Namespaces, it doesn't make sense to require them to resolve name conflict with CR or Ports declared in other Namespaces. Again, this is only for future extensibilty if we want to provide fine-grained (namespaced) control to tenant users. If this is not a requirement, no need to discuss this further.
There was a problem hiding this comment.
I do not think "tenant" user should directly create tunnels to redirect traffic to remote (and even if that happens we should define new API for that. Definitely not the current API which is for cluster admin).
There was a problem hiding this comment.
Humm... after more thinking, I feel what you said about tunnel port name makes great sense for users only want to mirror packets to a remote tunnel and do not use/know about internal and device port at all. My problem is I always thought about all the use cases together, and so felt users would like API uniformity. To be most flexible, seems we should make tunnel port name optional to cover both users care and do not care about it. But for simplicity, I guess you like to choose only one way? Have you received the tunnel/ERSPAN only requirement?
Another my problem is I want to keep the possibility of supporting pre-created ports, so in case we need some special port configuration, we need not to change Antrea upstream. When the port is pre-created, actually Antrea does not care it is port type but just needs a port name, and the sub-type structs way might not work well?
In general, as I do not see a good way to satisfy all cases, I feel we can start from a port type and a mandatory name (even it might be strange for tunnel only users, but it should not be a big deal for them either). Later, if we really want, we can make port name optional (for internal, tunnel, ERSPAN, and for device we can also choose a separate device name field to be clearer).
There was a problem hiding this comment.
Humm... after more thinking, I feel what you said about tunnel port name makes great sense for users only want to mirror packets to a remote tunnel and do not use/know about internal and device port at all. My problem is I always thought about all the use cases together, and so felt users would like API uniformity. To be most flexible, seems we should make tunnel port name optional to cover both users care and do not care about it. But for simplicity, I guess you like to choose only one way? Have you received the tunnel/ERSPAN only requirement?
Glad to know we reach a consensus on tunnel port name. Yes, I would prefer only one way if specifying tunnel port name doesn't give much value other than API uniformity (it should be not hard for users who have OVS troubleshooting ability to figure out which port is auto-generated for their tunnels by looking for the tunnel ip and id via ovs-vsctl).
#3008 is tunnel only requirement. For traffic mirroring, I imagine mirroring to tunnel would be a more common use case, especially for users who want traffic visibility, monitoring, analyzing services via a centralized endpoint, instead of running multiple services on every Node (for listening internal/device ports) which may need another result aggregation process. Running centralized monitoring service remotely may make it easier to have something like monitoring/IDS as a service, especially when monitored Pods are running on multiple Nodes, such service's implementation only needs to start a network analyzer (can even be a Pod) and create a traffic mirroring object pointing to its IP.
Besides, from my investigation on similar features, most of them only support mirroring to remote:
Configure Port Mirroring on VMware Cloud on AWS
Configure Port Mirroring on VMware NSX-T Data Center
Cloud IDS in Google Cloud
I haven't found a similar feature in K8s ecosystem, thought Antrea might be the first one providing such capacity for end users.
Another my problem is I want to keep the possibility of supporting pre-created ports, so in case we need some special port configuration, we need not to change Antrea upstream. When the port is pre-created, actually Antrea does not care it is port type but just needs a port name, and the sub-type structs way might not work well?
I feel making port type optional may be even more hard to explain and away from API uniformity? Perhaps the use case can be covered by a type like "Opaque"? It can work in either way:
targetPort:
type: Opaque
name: xxx
---
targetPort:
opaque:
name: xxx
In general, as I do not see a good way to satisfy all cases, I feel we can start from a port type and a mandatory name (even it might be strange for tunnel only users, but it should not be a big deal for them either). Later, if we really want, we can make port name optional (for internal, tunnel, ERSPAN, and for device we can also choose a separate device name field to be clearer).
Sure, I have no problem with starting from the current version (a port type and a mandatory name). But I have one question before merging it:
Since traffic mirroring to tunnel requires almost no priviledge and can potentially work for tenant users, does it make sense to separate traffic mirroring and redirecting APIs in the begining. Then the API name could be more specific like above solutions, and it's possible to have a namespaced traffic mirroring API in the future. Traffic redirecting is unlikely to need that as it requires too many priviledges to work so only suitable for cluster admin/provider.
There was a problem hiding this comment.
You are right. If we believe mirroring to a tunnel is an important feature (but I do not believe it should be for a tenant though), then we better to create a separate API for that, then we do not have all these problems. But have you received any ask for mirroring with tunnel?
I feel the current approach is just trying to share more implementation and support tunnel with low cost. It is not a bad strategy either if the cost is really low (?). Again, my assumption is not many users will use the feature anyway.
762ab2c to
f77f879
Compare
f77f879 to
65a503f
Compare
pkg/apis/crd/v1alpha2/types.go
Outdated
| ) | ||
|
|
||
| // OVSPort represents an OVS port that can be used as the target of traffic mirroring or redirecting, and the return | ||
| // port of traffic redirecting. Multiple TrafficControl objects can use same port, however, the ports with same name |
There was a problem hiding this comment.
Do we plan to support: "Multiple TrafficControl objects can use same port" in the current version?
There was a problem hiding this comment.
I think there is no harm to support it with the statement that ports with same name must have same configuration:
If we don't allow it, it requires more code to validate the port is not duplicate with any of existing ports, or how we say it's not allowed?
If we allow it with the precondition, port sharing can work if it's configured correctly.
There was a problem hiding this comment.
We need to validate the configuration is the same, and we need to track the reference to know when to delete the port.
There was a problem hiding this comment.
But even if the names are different, configurations could be same, which is still a misconfiguration. Validating name only doesn't help much.
If it's a device port, using same device in multiple TrafficControls doesn't seem wrong or complicated.
If it's a tunnel port, user can just change the name if the validation result is just name conflict, but it still won't work if configurations are same.
For reference tracking, I think it doesn't complexity, we need to track a port's owner anyway for cleanup, expecially for restart case. We need to build an index for port and check whether a port is owned by any TrafficControl object before cleaning it up. It's just affect whether we expect len(foundObjects) == 1 or len(foundObjects) >= 1.
There was a problem hiding this comment.
I didn't plan to add a webhook for now given this is an alpha feature and we are still in discussion about whether tunnel port should have a name, and whether there should be a separate OVSPort API. Can we just assume people who try this feature will follow the document to use same configuration for same port?
I think the cleanest way to avoid port conflict is to remove tunnel port name as in theory there wouldn't be a "conflict" by doing that.
- Same tunnel configurations in different TCs mean using same tunnel
- Same device name in different TCs mean using same device
- It's practically impossible to have a conflict between tunnel port and device name as the former is auto-generated with specific prefix like (tc-vxlan-) and a hash suffix based on configuration
- Same device name and internal port name in different TCs is a misconfiguration because if one sets "eth0" as internal port but get an error because of conflict, there must be a device named "eth0", and it's not hard to figure it out and correct the configuration.
There was a problem hiding this comment.
Ok, we had long discussion on this and I think we are on the same page. How about you choose one based on complexity and your preference. If we want to remove port name for tunnel, then we should go your way of:
type TrafficControlPort struct {
OVSInternal *OVSInternalPort `json:"ovsInternal,omitempty"`
Device *DevicePort `json:"device,omitempty"`
VXLAN *TunnelPort `json:"vxlan,omitempty"`
GENEVE *TunnelPort `json:"geneve,omitempty"`
GRE *TunnelPort `json:"gre,omitempty"`
ERSPAN *ERSPANPort `json:"erspan,omitempty"`
}
// Maybe we just need a string named "InternalPort" in TrafficControlPort?
type InternalPort struct {
// The name of the OVS internal port.
Name string `json:"name"`
}
// How about NetworkDevice?
type DevicePort struct {
Name string `json:"name"`
}
// Probably do not call it TunnelPort then, but just Tunnel?
type TunnelPort struct {
RemoteIP string `json:"remoteIP"`
TunnelID int64 `json:"tunnelID,omitempty"`
}
Again, my major problem is then not straightforward to support pre-created port, but maybe we can add a filed like PrecreatedPort/ExistingPort, etc. for that when we need to support it.
|
It seems that the branch has conflicts with the main branch and needs to rebase. |
65a503f to
6c63252
Compare
|
/test-all |
6c63252 to
3407cc4
Compare
TrafficControl is a feature which allows mirroring or redirecting the
traffic Pods send or receive. It enables users to monitor and analyze
Pod traffic, and to enforce custom network protections for Pods with
fine-grained control over network traffic.
This patch adds types and CRD for TrafficControl API.
Examples:
1. Mirror Pods (web=app) ingress traffic to a VXLAN tunnel
```
apiVersion: crd.antrea.io/v1alpha2
kind: TrafficControl
metadata:
name: mirror-web-app
spec:
appliedTo:
podSelector:
matchLabels:
app: web
direction: Ingress
action: Mirror
targetPort:
vxlan:
remoteIP: 1.1.1.1
```
2. Redirect Pods (web=app) traffic in both direction to OVS internal
port firewall0 and expect the traffic to re-enter OVS via another OVS
internal port firewall1 if they are not dropped.
```
apiVersion: crd.antrea.io/v1alpha2
kind: TrafficControl
metadata:
name: redirect
spec:
appliedTo:
podSelector:
matchLabels:
role: web
direction: Ingress
action: Redirect
targetPort:
ovsInternal:
name: firewall0
returnPort:
ovsInternal:
name: firewall1
```
For antrea-io#3324
Signed-off-by: Quan Tian <qtian@vmware.com>
3407cc4 to
442ae83
Compare
|
@jianjuns In latest patch I added an optional destination port parameter for VXLAN and GENEVE tunnels since some systems use 8472 for VXLAN, and changed to use each type' own term for their tunnel IDs, for example, VNI for VXLAN, Key for GRE, and SessionID of ERSPAN. |
|
/test-all |
|
/test-networkpolicy |
TrafficControl is a feature which allows mirroring or redirecting the
traffic Pods send or receive. It enables users to monitor and analyze
Pod traffic, and to enforce custom network protections for Pods with
fine-grained control over network traffic.
This patch adds types and CRD for TrafficControl API.
Examples:
port firewall0 and expect the traffic to re-enter OVS via another OVS
internal port firewall1 if they are not dropped.
For #3324
Signed-off-by: Quan Tian qtian@vmware.com