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.

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

COS-2822: port gcp-routes to nftables #1562

Merged
merged 2 commits into from
Aug 15, 2024

Conversation

danwinship
Copy link
Contributor

@danwinship danwinship commented Aug 8, 2024

iptables is going away in RHEL10; this PR ports gcp-routes.sh from iptables to nftables.

This is just a port of openshift/machine-config-operator#4494 which updated MCO's copy of gcp-routes... the version in os is slightly simpler (because it's older and it only needs to support bootstrap and so hasn't needed the fixes that are in the MCO version). (Or, honestly, probably "it has the same problems the MCO version used to have, but our CI doesn't flag the apiserver connection failures-and-retries during bootstrap switchover like it does during upgrade, so nobody ever bothered to fix the bugs in the RHCOS version.) Anyway, see discussion in that PR.

I tried to make a jira issue for this but jira insists (to me anyway) that there is no such project as "COS" and will not let me create a new issue in it. (But if someone else wants to create one and link it to OCPSTRAT-940, go ahead.)

gcp-routes had a rule "so that existing flows (with an entry in
conntrack) continue to be balanced, even if the DNAT entry is
removed". The only way this iptables rule would actually be needed is
if (a) your masters have an iptables-based firewall (which they
shouldn't, on OCP), and (b) the firewall is so aggressive that it even
drops packets from established connections (which no firewall should
do anyway).

At any rate, even if the rule *was* necessary in some clusters, it
won't work in future nftables-only versions of RHCOS anyway, because
nftables doesn't let "accept" rules in one table override
"drop"/"reject" rules in another table; if your firewall is broken and
dropping packets that it shouldn't, you have to actually fix your
firewall rules, not hack around them somewhere else.
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 8, 2024
@openshift-ci-robot
Copy link

@danwinship: This pull request explicitly references no jira issue.

In response to this:

iptables is going away in RHEL10; this PR ports gcp-routes.sh from iptables to nftables.

This is just a port of openshift/machine-config-operator#4494 which updated MCO's copy of gcp-routes... the version in os is slightly simpler (because it's older and it only needs to support bootstrap and so hasn't needed the fixes that are in the MCO version). (Or, honestly, probably "it has the same problems the MCO version used to have, but our CI doesn't flag the apiserver connection failures-and-retries during bootstrap switchover like it does during upgrade, so nobody ever bothered to fix the bugs in the RHCOS version.) Anyway, see discussion in that PR.

I tried to make a jira issue for this but jira insists (to me anyway) that there is no such project as "COS" and will not let me create a new issue in it.

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 openshift-eng/jira-lifecycle-plugin repository.

@danwinship
Copy link
Contributor Author

/assign @jcaamano
(There is a separate copy of MCO's gcp-routes in RHCOS itself, which is only used on the bootstrap host.)

@jcaamano
Copy link

jcaamano commented Aug 8, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2024
@danwinship
Copy link
Contributor Author

/assign @travier

This goes with #1498 since CentOS 10 doesn't have iptables. (Or maybe it still does at this point, but it's going away eventually...) (This PR is needed for RHEL 10, but it works with RHEL 9 too.)

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

I've moved https://issues.redhat.com/browse/MCO-1266 to https://issues.redhat.com/browse/COS-2882. Hopefully that's what you meant. If so, you can use that for the Jira link. Also, can you add a commit message to the second commit and link to the OCPSTRAT?

Trivial LGTM for the change itself. The script lives here but we're not really the SMEs for it.

@danwinship
Copy link
Contributor Author

I've moved https://issues.redhat.com/browse/MCO-1266 to https://issues.redhat.com/browse/COS-2882. Hopefully that's what you meant.

That was my first attempt, yes, though it seems like "Clone" in jira is generally terrible anyway, because that card ended up with a bunch of MCO-tagged qe-related sub-tasks whereas presumably it should have ended up with COS ones? 🤷‍♂️

@danwinship danwinship changed the title NO-JIRA: port gcp-routes to nftables COS-2822: port gcp-routes to nftables Aug 9, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 9, 2024

@danwinship: This pull request references COS-2822 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

iptables is going away in RHEL10; this PR ports gcp-routes.sh from iptables to nftables.

This is just a port of openshift/machine-config-operator#4494 which updated MCO's copy of gcp-routes... the version in os is slightly simpler (because it's older and it only needs to support bootstrap and so hasn't needed the fixes that are in the MCO version). (Or, honestly, probably "it has the same problems the MCO version used to have, but our CI doesn't flag the apiserver connection failures-and-retries during bootstrap switchover like it does during upgrade, so nobody ever bothered to fix the bugs in the RHCOS version.) Anyway, see discussion in that PR.

I tried to make a jira issue for this but jira insists (to me anyway) that there is no such project as "COS" and will not let me create a new issue in it. (But if someone else wants to create one and link it to OCPSTRAT-940, go ahead.)

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 openshift-eng/jira-lifecycle-plugin repository.

IPTables is going away in RHEL 10 so everything needs to be done with nftables.

https://issues.redhat.com/browse/OCPSTRAT-940
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2024
@danwinship
Copy link
Contributor Author

@jlebon repushed with updated commit message

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@danwinship
Copy link
Contributor Author

@jlebon try again? bot didn't seem to notice...

@jlebon
Copy link
Member

jlebon commented Aug 14, 2024

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2024
Copy link
Contributor

openshift-ci bot commented Aug 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, jcaamano, jlebon

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2024
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 5488f67 and 2 for PR HEAD 29c6c1c in total

@jlebon
Copy link
Member

jlebon commented Aug 14, 2024

Quay.io infra flake
/retest

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3d7ec06 and 1 for PR HEAD 29c6c1c in total

Copy link
Contributor

openshift-ci bot commented Aug 15, 2024

@danwinship: all tests passed!

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 05d743e into openshift:master Aug 15, 2024
7 checks passed
@danwinship danwinship deleted the nftables branch August 15, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants