Implement routable pod CIDRs in e2e tests #363
Conversation
g-gaston
force-pushed
the
e2e-test-full-pod-networking
branch
3 times, most recently
from
c90c265 to
08caebf
Compare
test/e2e/kubernetes/node.go
Outdated
There was a problem hiding this comment.
most of these are just moved from test/e2e/kubernetes/kubernetes.go
the only thing new are the last 2 methods
| @@ -145,6 +145,20 @@ func (e *InstanceConfig) Create(ctx context.Context, ec2Client *ec2.Client, ssmC | |||
| }, nil | |||
| } | |||
|
|
|||
| // DisableSourceDestCheck disables the source/destination check for the given instance. | |||
| func DisableSourceDestCheck(ctx context.Context, ec2Client *ec2.Client, instanceID string) error { | |||
There was a problem hiding this comment.
so by default an ec2 instance blocks the packet if the src/dest is outside the vpc(?) ?
There was a problem hiding this comment.
yeah
You can enable or disable source/destination checks, which ensure that the instance is either the source or the destination of any traffic that it receives. Source/destination checks are enabled by default. You must disable source/destination checks if the instance runs services such as network address translation, routing, or firewalls.
test/e2e/peered/node.go
Outdated
| @@ -224,6 +234,12 @@ func (c *NodeCleanup) Cleanup(ctx context.Context, instance ec2.Instance) error | |||
| c.Logger.Info("Skipping EC2 Instance deletion", "instanceID", instance.ID) | |||
| return nil | |||
| } | |||
|
|
|||
| c.Logger.Info("Deleting routes for EC2 Instance", "instanceID", instance.ID, "subnetID", c.Cluster.SubnetID) | |||
| if err := ec2.DeleteRoutesForInstance(ctx, c.EC2, c.Cluster.SubnetID, instance.ID); err != nil { | |||
There was a problem hiding this comment.
is there any issue to other tests, potentially, if this fails to cleanup or just in general?
There was a problem hiding this comment.
maybe
if the route is not deleted and we try to create another route for the same cidr, it will fail. So it's possible for:
- a node to be deleted
- its route not be deleted
- then another node is created
- cilium happens to assign the same pod cidr to this new node that it had assigned to the previous one
- when the test gets to create the vpc route, it will be duplicated and fail.
The alternative is to detect the duplicated route error in the test and delete it first.
I opted not to do it because that could cause another running test to fail if it happens that the other node hasn't been deleted yet. That code could be masking a different type of issue. Moreover, the test error would be very difficult to debug.
With the current code, the error will be obvious. If this starts being a problem, we can reconsider it.
g-gaston
force-pushed
the
e2e-test-full-pod-networking
branch
from
bbcf982 to
d406714
Compare
junpengdev
force-pushed
the
e2e-test-full-pod-networking
branch
from
d406714 to
90b5514
Compare
We now create an individual route per hybrid node on the subnet where the hybrid nodes live for the pod CIDR assigned to the node using the instance as the next hop. In order for this work, it required swapping the peered VPC for a TGW. Not because the individual routes couldn't be added, but because the VPC networking would refuse to forward a packet through the peered connection with a dst IP that doesn't belong to the VPC'S CIDR blocks on the other side. This is a problem because the hybrid pod CIDR is not part of the second VPC. And it can't be if we want to be able to add the previously mentioned per-node routes.
junpengdev
force-pushed
the
e2e-test-full-pod-networking
branch
from
90b5514 to
c7466dc
Compare
Description of changes
We now create an individual route per hybrid node on the subnet where the hybrid nodes live for the pod CIDR assigned to the node using the instance as the next hop.
In order for this work, it required swapping the peered VPC for a TGW. Not because the individual routes couldn't be added, but because the VPC networking would refuse to forward a packet through the peered connection with a dst IP that doesn't belong to the VPC'S CIDR blocks on the other side. This is a problem because the hybrid pod CIDR is not part of the second VPC. And it can't be if we want to be able to add the previously mentioned per-node routes.
This enables CP to pod communication or east-west pod to pod communication.
Testing (if applicable):
Running this in my stack.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.