Watch firewall reload in sysconfigwatcher by zenghui-shi · Pull Request #1185 · openshift/microshift

zenghui-shi · 2022-12-11T05:01:25Z

Related-Issue: https://issues.redhat.com/browse/NP-641

openshift-ci · 2022-12-11T05:02:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zshi-redhat

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [zshi-redhat]

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

zenghui-shi · 2022-12-11T13:40:31Z

resolved merge conflict.

ggiguash · 2022-12-12T11:13:32Z

This change is LGTM, but we need to wait until 4.12 branch is created.
/hold

dhellmann

For CSI, we drew a boundary so there was a clear line between the system configuration the user had to provide (volume groups and the lvmd.conf file) and the setup that MicroShift did (a default lvmd.conf if none is provided). Can we create a similar boundary for CNI so that we do not need to monitor the firewall rules, IP, or other networking changes?

dhellmann · 2022-12-12T13:43:23Z

Do we need to expose this field to users or can it be an internal value?

It will be for internal use only. Captured when microshift main process starts.

zenghui-shi · 2022-12-13T10:06:06Z

For CSI, we drew a boundary so there was a clear line between the system configuration the user had to provide (volume groups and the lvmd.conf file) and the setup that MicroShift did (a default lvmd.conf if none is provided). Can we create a similar boundary for CNI so that we do not need to monitor the firewall rules, IP, or other networking changes?

Agree on the boundary statement between microshift and plugins, I thought about adding this in CNI implementation only.
One consideration of adding this monitor logic in sysconfWatcher is that iptables could be used by other components.
For example: with hostPort in pod spec, iptables rules are used to map host ports to container ports, they will be affected by firewall reload as well. These hostPort iptables are not managed by CNI.

dhellmann · 2022-12-13T15:51:11Z

For CSI, we drew a boundary so there was a clear line between the system configuration the user had to provide (volume groups and the lvmd.conf file) and the setup that MicroShift did (a default lvmd.conf if none is provided). Can we create a similar boundary for CNI so that we do not need to monitor the firewall rules, IP, or other networking changes?

Agree on the boundary statement between microshift and plugins, I thought about adding this in CNI implementation only. One consideration of adding this monitor logic in sysconfWatcher is that iptables could be used by other components. For example: with hostPort in pod spec, iptables rules are used to map host ports to container ports, they will be affected by firewall reload as well. These hostPort iptables are not managed by CNI.

Is there a way to organize the system so that we do not need to restart MicroShift when firewall rules change at all? Can we isolate the rule sets that users manage from the ones that MicroShift manages somehow?

Related-Issue: https://issues.redhat.com/browse/NP-641

zenghui-shi · 2022-12-22T02:02:24Z

For CSI, we drew a boundary so there was a clear line between the system configuration the user had to provide (volume groups and the lvmd.conf file) and the setup that MicroShift did (a default lvmd.conf if none is provided). Can we create a similar boundary for CNI so that we do not need to monitor the firewall rules, IP, or other networking changes?

Agree on the boundary statement between microshift and plugins, I thought about adding this in CNI implementation only. One consideration of adding this monitor logic in sysconfWatcher is that iptables could be used by other components. For example: with hostPort in pod spec, iptables rules are used to map host ports to container ports, they will be affected by firewall reload as well. These hostPort iptables are not managed by CNI.

Is there a way to organize the system so that we do not need to restart MicroShift when firewall rules change at all? Can we isolate the rule sets that users manage from the ones that MicroShift manages somehow?

@dhellmann I moved the iptable detection logic to infrastructure manager service which only reloads affected microshift components (router and ovnk). This avoids microshift restart. For application workload that uses hostPort, this won't help since the iptable rules for hostPort are injected by cri-o during pod sandbox creation, these rules won't be reconciled periodically unless recreating the workload. So current patch takes care of microshift components when firewall reload happens, we probably want to document that user workload with hostPort could be impacted by user action (reload firewall). wdyt?

zenghui-shi · 2022-12-22T12:19:22Z

/retest

openshift-ci · 2022-12-22T13:07:04Z

@zshi-redhat: all tests passed!

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

zenghui-shi · 2023-01-09T13:14:32Z

/hold cancel

mangelajo · 2023-01-24T16:04:00Z

 	}

-	if err := assets.ApplyDeployments(apps, renderTemplate, renderParamsFromConfig(cfg, nil), kubeconfigPath); err != nil {
+	extraParams := assets.RenderParams{


I guess that for a decoupled CNI-plugin we may need to call the CNI plugin (know it's a CNI plugin) and call it to re-create the deployment.

We could make that part of a plugin interface i.e. have a "restart" call to the plugin which would add creation timestamps to the objects that need re-creation in the event of necessity.

Hmmmmm

This is one reason to have MicroShift run the plugin scripts, instead of relying solely on systemd. @fzdarsky

mangelajo · 2023-01-24T16:04:33Z

@dhellmann I left a comment inside about could we make something like this work with pluggable CNI.

mangelajo · 2023-01-27T16:21:12Z

/hold

I believe I found an alternative to make firewalld leave iptables alone (not flushing them)

mangelajo · 2023-01-27T16:21:36Z

/hold

dhellmann · 2023-01-31T21:50:47Z

I think since we took #1306 we no longer need this. Please reopen the PR if I misunderstood.

/close

openshift-merge-robot · 2023-01-31T21:50:58Z

PR needs rebase.

Details

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/test-infra repository.

openshift-ci · 2023-01-31T21:51:08Z

@dhellmann: Closed this PR.

Details

In response to this:

I think since we took #1306 we no longer need this. Please reopen the PR if I misunderstood.

/close

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/test-infra repository.

openshift-ci Bot requested review from benluddy and fzdarsky December 11, 2022 05:02

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2022

ggiguash reviewed Dec 11, 2022

View reviewed changes

Comment thread pkg/sysconfwatch/sysconfwatch_linux.go Outdated

ggiguash requested changes Dec 11, 2022

View reviewed changes

Comment thread pkg/sysconfwatch/sysconfwatch_linux.go Outdated

zenghui-shi force-pushed the firewall-reload-watcher branch from b65f05f to 499e54c Compare December 11, 2022 13:35

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2022

zenghui-shi force-pushed the firewall-reload-watcher branch from 499e54c to 79941fb Compare December 11, 2022 13:40

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2022

ggiguash reviewed Dec 11, 2022

View reviewed changes

Comment thread docs/howto_sysconf_watch.md Outdated

zenghui-shi force-pushed the firewall-reload-watcher branch from 79941fb to 178ad89 Compare December 12, 2022 01:15

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 12, 2022

dhellmann reviewed Dec 12, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2022

zenghui-shi added 2 commits December 21, 2022 12:00

Watch firewall reload in sysconfigwatcher

995cf83

Related-Issue: https://issues.redhat.com/browse/NP-641

Move firewall watch logic to infrastructure manager

694a167

zenghui-shi force-pushed the firewall-reload-watcher branch from 178ad89 to 694a167 Compare December 22, 2022 01:49

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2022

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2023

mangelajo reviewed Jan 24, 2023

View reviewed changes

mangelajo mentioned this pull request Jan 26, 2023

rebase 1185 #1296

Closed

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2023

openshift-ci Bot closed this Jan 31, 2023

microshift-rebase-script Bot mentioned this pull request Jan 19, 2024

**FAILURE** rebase-main-4.16.0-0.nightly-2024-01-18-153434_amd64-2024-01-18_arm64-2024-01-15 #2900

Closed

Uh oh!

Conversation

zenghui-shi commented Dec 11, 2022

Uh oh!

openshift-ci Bot commented Dec 11, 2022

Uh oh!

Uh oh!

Uh oh!

zenghui-shi commented Dec 11, 2022

Uh oh!

Uh oh!

ggiguash commented Dec 12, 2022

Uh oh!

dhellmann left a comment

Choose a reason for hiding this comment

Uh oh!

dhellmann Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

zenghui-shi Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

zenghui-shi commented Dec 13, 2022

Uh oh!

dhellmann commented Dec 13, 2022

Uh oh!

zenghui-shi commented Dec 22, 2022

Uh oh!

zenghui-shi commented Dec 22, 2022

Uh oh!

openshift-ci Bot commented Dec 22, 2022

Uh oh!

zenghui-shi commented Jan 9, 2023

Uh oh!

mangelajo Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

dhellmann Jan 25, 2023

Choose a reason for hiding this comment

Uh oh!

mangelajo commented Jan 24, 2023

Uh oh!

mangelajo commented Jan 27, 2023

Uh oh!

mangelajo commented Jan 27, 2023

Uh oh!

dhellmann commented Jan 31, 2023

Uh oh!

openshift-merge-robot commented Jan 31, 2023

Uh oh!

openshift-ci Bot commented Jan 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants