Fix(secgroups): ICMP Echo Request being blocked

[Kumm-Kai]

This fixes a thing that never worked before 😄

Currently ICMP echo requests are blocked by the ingress security group rules for ICMP, which looks like this: type=1:code=8 (ICMP type 1 is not used for anything in the ICMP protocol).

After this PR the ingress security group rules will look like this: type=8 (code seems to be omitted by openstack if it is 0) and allows ICMP echo requests to reach the VM.

Egress isn't a problem because the egress security group rule doesn't block anything.

[nschad]

LGTM

This is dumbest mapping ever. We can either also a.) leave it completely out possibly
OR
b.) set it to -1 this should allow all ICMP types and codes

[nightlyone]

Wasn't that setting intentional that way to mitigate icmp based attacks like ping floods? ( https://www.cloudflare.com/learning/ddos/ping-icmp-flood-ddos-attack/) tcptraceroute to the open ports of the load balancer can still be useful. I would at least make it optional, so icmp floods can be quickly mitigated. Niclas Schad ***@***.***> schrieb am Fr., 8. Dez. 2023, 16:44:

…

***@***.**** approved this pull request. — Reply to this email directly, view it on GitHub <#269 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANPNTXFHCZDKHRUJHAINJDYIMYXDAVCNFSM6AAAAABAMY35HKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZSGY3DQOBVGU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[Kumm-Kai]

Wasn't that setting intentional that way to mitigate icmp based attacks like ping floods? ( https://www.cloudflare.com/learning/ddos/ping-icmp-flood-ddos-attack/) tcptraceroute to the open ports of the load balancer can still be useful. I would at least make it optional, so icmp floods can be quickly mitigated.

I'm very certain that this wasn't intentional, as the old values were entirely wrong. Even the comment a few lines above indicates that:

yawol/internal/helper/loadbalancer.go

Line 204 in f949e16

// Ping for easy debugging

If you need to mitigate this, just add a security group that blocks ICMP.

[dergeberl]

I would also say it is not intentional but also see the point that it can be problematic.

Just create a secgroup rule is not that easy because they get reconciled. Maybe we should add it with an setting in lb.spec.options to disable it and enable it by default.

wdyt @Kumm-Kai

[rgroemmer]

rotten talked with @Kumm-Kai
/close