Introduce a "host-strict" network provider as a superset of "host" #13651

obnoxxx · 2024-01-30T20:10:57Z

description of changes:
This change adds a new network provider "host-strict" to the cephCluster that behaves as a superset of the known "host" provider. in that it lets rook configure all pods for host networking, not only those that were previously affected by "host".

** ToDo:**

configure additional pods to use host networking if "host-strict" is set.

Issue resolved by this Pull Request:
Resolves #13571

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

BlaineEXE · 2024-01-31T18:42:28Z

pkg/apis/ceph.rook.io/v1/network.go

+// IsHost can be used to determine if  pods should use host networking by
+// checking whether  the cephCluster is configured to use the "host" network provider.
+// This method  also maintains compatibility with the old hostNetwork setting


Note to check space usage in doc comments. I see several double-spaces here and below. I'm not too concerned about this in draft status, but it might be good to check if the editor you're using makes it harder to see these. I have whitespace markers turned on in mine, and I find it very helpful for visualizing space and tab usage.

Suggested change

// IsHost can be used to determine if pods should use host networking by

// checking whether the cephCluster is configured to use the "host" network provider.

// This method also maintains compatibility with the old hostNetwork setting

// IsHost can be used to determine if pods should use host networking by

// checking whether the cephCluster is configured to use the "host" network provider.

// This method also maintains compatibility with the old hostNetwork setting

BlaineEXE · 2024-01-31T18:53:52Z

pkg/apis/ceph.rook.io/v1/network.go

+// IsHost can be used to determine if  pods should use host networking by
+// checking whether  the cephCluster is configured to use the "host" network provider.
+// This method  also maintains compatibility with the old hostNetwork setting


Because we are adding a new mode, it will be good for users to clarify how host and host-strict behavior differs. For host, something like, "This network mode only applies host networking to pods that require it in order to limit the number of pods exposed to the host network, improving cluster security."

And a parallel doc comment in IsHostStrict() mentioning something like, "This network mode applies host networking to all pods without caveat. This is useful for clusters with no default CNI. This mode is less secure than "host" mode. Use "host" mode instead of "host-strict" when possible."

I might also suggest adding a note like "This network mode is needed rarely and so is supported on a best-effort basis."

BlaineEXE · 2024-01-31T18:54:54Z

pkg/apis/ceph.rook.io/v1/network_test.go


 	assert.True(t, net.IsHost())
 }

+func TestNetworkCephIsHost(t *testing.T) {
+	net := NetworkSpec{provider : "host"}


If you look at the "files" view on GitHub, you can see some golang linter issues reported. The CI "action" view doesn't show these issues as well.

This particular issue is because it should be Provider (an exported go struct field) and not provider (a nonexistent, non-exported struct field).

BlaineEXE · 2024-01-31T18:58:13Z

pkg/apis/ceph.rook.io/v1/network.go

+// IsHost can be used to determine if pods should use host networking by
+// checking whether the cephCluster is configured to use the "host" network provider.
+// This method also maintains compatibility with the old hostNetwork setting


Because we are adding a new mode, it will be good for users to clarify how host and host-strict behavior differs. For host, something like, "This network mode only applies host networking to pods that require it in order to limit the number of pods exposed to the host network, improving cluster security."

And a parallel doc comment in IsHostStrict() mentioning something like, "This network mode applies host networking to all pods without caveat. This is useful for clusters with no default CNI. This mode is less secure than "host" mode. Use "host" mode instead of "host-strict" when possible."

I might also suggest adding a note like "This network mode is needed rarely and so is supported on a best-effort basis."

FWIW, when Travis and I chatted, he agreed that it isn't necessary to have e2e in CI for this since it is such a rare case. Solid local testing should be fine. Also, it isn't necessary to mention that it is supported on a best-effort basis since that is essentially how all FOSS is supported.

@BlaineEXE wrote:

Because we are adding a new mode, it will be good for users to clarify how host and host-strict behavior differs. For host, something like, "This network mode only applies host networking to pods that require it in order to limit the number of pods exposed to the host network, improving cluster security."

And a parallel doc comment in IsHostStrict() mentioning something like, "This network mode applies host networking to all pods without caveat. This is useful for clusters with no default CNI. This mode is less secure than "host" mode. Use "host" mode instead of "host-strict" when possible."

I might also suggest adding a note like "This network mode is needed rarely and so is supported on a best-effort basis."

Good points, thanks! I added corresponding sections to the PR, mostly using your words directly. I'll add Co-authored-by you when doing final squashing/polishing.

@BlaineEXE wrote:

FWIW, when Travis and I chatted, he agreed that it isn't necessary to have e2e in CI for this since it is such a rare case. Solid local testing should be fine.

Thanks. I don't complain as it makes it easier for me 😉

Also, it isn't necessary to mention that it is supported on a best-effort basis since that is essentially how all FOSS is supported.

Fair enough, but I already added that clause. Do you want me to remove it again?

BlaineEXE · 2024-01-31T18:58:30Z

pkg/apis/ceph.rook.io/v1/network_test.go

@@ -43,11 +43,32 @@ func TestNetworkCephSpecLegacy(t *testing.T) {
 }

 func TestNetworkCephIsHostLegacy(t *testing.T) {
-	net := NetworkSpec{HostNetwork: true}
+	net := NetworkSpec{ HostNetwork: true}


I'm not sure how this space got added. My golang formatter will auto remove spaces like this to keep code in line with what is expected by gofmt. You should see if your editor has a "format on save" option or something like that. Or you can run gofmt manually if you prefer.

@BlaineEXE wrote:

I'm not sure how this space got added.

I don't know either: At least I didn't intentionally modify that line.

My usual suspect for such unexpected changes is the editor. I don't like the editor to do a lot without me entering the changes myself. At least I usually try to undo formatting changes by the editor as soon as I notice them. but that is usually of course not very successful 😉

I prefer to run gofmt manually. I just did and after committing the result now the extra space is gone.

I guess this check must not be in our CI, but Go code must be formatted with gofmt and should not be changed afterwards to alter its formatting.

I think we haven't noticed it before because most people use VSCode and the Golang plugin that does this automatically. I'm surprised that our linter doesn't check for it as well.

BlaineEXE · 2024-01-31T19:00:53Z

pkg/apis/ceph.rook.io/v1/network_test.go

+
+
+
+


I'm pretty sure gofmt (the built-in tool for ensuring go source code has consistent style) will remove double empty lines in go source files.

ran gofmt and now the blank lines are gone.

obnoxxx · 2024-02-01T12:47:25Z

failure in the macos build ci check:

vet: pkg/apis/ceph.rook.io/v1/network_test.go:52:21: unknown field provider in struct literal of type NetworkSpec

I guess something did not go quite right with my modification of the types. Then again, the member provider existed before... 🤷🏼

obnoxxx · 2024-02-01T14:07:22Z

failure in the macos build ci check:
vet: pkg/apis/ceph.rook.io/v1/network_test.go:52:21: unknown field provider in struct literal of type NetworkSpec
I guess something did not go quite right with my modification of the types. Then again, the member provider existed before... 🤷🏼

Found it. I had to write Provider instead of provider ...

BlaineEXE · 2024-02-01T17:00:36Z

pkg/apis/ceph.rook.io/v1/network_test.go

+	net := NetworkSpec{Provider: "host"}
+
+	assert.True(t, net.IsHost())
+	net = NetworkSpec{Provider: "host-strict"}
+
+	assert.True(t, net.IsHost())


It's best visually to keep test setup and test check together.

Suggested change

net := NetworkSpec{Provider: "host"}

assert.True(t, net.IsHost())

net = NetworkSpec{Provider: "host-strict"}

assert.True(t, net.IsHost())

net := NetworkSpec{Provider: "host"}

assert.True(t, net.IsHost())

net = NetworkSpec{Provider: "host-strict"}

assert.True(t, net.IsHost())

BlaineEXE · 2024-02-01T17:02:06Z

pkg/apis/ceph.rook.io/v1/network_test.go

@@ -48,6 +48,24 @@ func TestNetworkCephIsHostLegacy(t *testing.T) {
 	assert.True(t, net.IsHost())
 }

+func TestNetworkCephIsHost(t *testing.T) {


These tests should also include the default provider ("") as well as the legacy boolean in their matrix to make sure that they do the right thing related to other values.

And when things start to get more complicated, it can be good to use t.Run("subtest name", ... to define subtests that clearly spell out what is being tested.

@BlaineEXE wrote:

These tests should also include the default provider ("") as well as the legacy boolean in their matrix to make sure that they do the right thing related to other values.

I added it to the pre-existing test and will check for more possible places.

And when things start to get more complicated, it can be good to use t.Run("subtest name", ... to define subtests that clearly spell out what is being tested.

I will look around the code to understand the point of this better.

pkg/apis/ceph.rook.io/v1/types.go

BlaineEXE · 2024-02-02T17:53:00Z

Bookmarking a topic for later: we should add some Validating Admission Policy to incorporate host-strict.

The existing code looks like this:

// NetworkSpec for Ceph includes backward compatibility code
// +kubebuilder:validation:XValidation:message="at least one network selector must be specified when using multus",rule="!has(self.provider) || (self.provider != 'multus' || (self.provider == 'multus' && size(self.selectors) > 0))"
type NetworkSpec struct {
	// Provider is what provides network connectivity to the cluster e.g. "host" or "multus".
	// If the Provider is updated from being empty to "host" on a running cluster, then the operator will automatically fail over all the mons to apply the "host" network settings.
	// +kubebuilder:validation:XValidation:message="network provider must be disabled (reverted to empty string) before a new provider is enabled",rule="self == '' || self == oldSelf"
	// +nullable
	// +optional
	Provider NetworkProviderType `json:"provider,omitempty"`

I think it makes sense that we will want to amend the logic to allow users to switch between host and host-strict if they want. As it currently is, they would need to switch from host to "" then to host-strict with "" as the middle step going either direction.

We should also add logic that seems to be missing where users are currently allowed to set hostNetwork: true at the same time as network.provider: "<something>" which should be a disallowed case. @subhamkrai or @sp98 might be able to correct me if I have missed the place where this logic is set though.

subhamkrai · 2024-02-05T10:44:18Z

We should also add logic that seems to be missing where users are currently allowed to set hostNetwork: true at the same time as network.provider: "<something>" which should be a disallowed case. @subhamkrai or @sp98 might be able to correct me if I have missed the place where this logic is set though.

I also don't that restriction and agree we should not allow setting network in two places.

This change is a follow-up to PR rook#13651 . It is also intended as a preparation for the introduction of a way to enforce host networking in the next round of updates to PR rook#13651 . This change improves the documentation comment and the unit testing for the `IsHost()` method of the `CephCluster.Network` spec. Signed-off-by: Michael Adam <obnox@samba.org>

…sHost() This change is a follow-up to PR rook#13651 . It is also intended as a preparation for the introduction of a way to enforce host networking in the next round of updates to PR rook#13651 . This change improves the documentation comment and the unit testing for the `IsHost()` method of the `CephCluster.Network` spec. Signed-off-by: Michael Adam <obnox@samba.org>

github-actions · 2024-04-02T20:01:49Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

github-actions · 2024-04-23T20:02:00Z

This pull request has been automatically closed due to inactivity. Please re-open if these changes are still required.

…sHost() This change is a follow-up to PR rook#13651 . It is also intended as a preparation for the introduction of a way to enforce host networking in the next round of updates to PR rook#13651 . This change improves the documentation comment and the unit testing for the `IsHost()` method of the `CephCluster.Network` spec. Signed-off-by: Michael Adam <obnox@samba.org>

…sHost() This change is a follow-up to PR rook#13651 . It is also meant to provide a continuation of the closed, stale PR rook#13878 . It is also intended as a preparation for the introduction of a way to enforce host networking in the next round of updates to PR rook#13651 . This change improves the documentation comment and the unit testing for the `IsHost()` method of the `CephCluster.Network` spec. The implementation of IsHost() is changed to validate the Network Spec and return error if the spec is invalid. Signed-off-by: Michael Adam <obnox@samba.org>

This change is a follow-up to PR rook#13651 . It is also meant to provide a continuation of the closed, stale PR rook#13878 . It is furthermore intended as a preparation for the introduction of a way to enforce host networking in the next round of updates to PR rook#13651 . This change improves the documentation, implementation, and unit testing for the `IsHost()` method of the `CephCluster.Network` spec. The implementation of IsHost() is changed to validate the Network Spec and return error if the spec is invalid. Signed-off-by: Michael Adam <obnox@samba.org> Co-authored-by: Blaine Gardner <b.blaine.gardner@gmail.com>

This change is a follow-up to PR rook#13651 . It is also meant to provide a continuation of the closed, stale PR rook#13878 . It is furthermore intended as a preparation for the introduction of a way to enforce host networking in the next round of updates to PR rook#13651 . This change improves the documentation, implementation, and unit testing for the `IsHost()` method of the `CephCluster.Network` spec. Signed-off-by: Michael Adam <obnox@samba.org> Co-authored-by: Blaine Gardner <b.blaine.gardner@gmail.com>

This change is a follow-up to PR rook#13651 . It is also meant to provide a continuation of the closed, stale PR rook#13878 . It is furthermore intended as a preparation for the introduction of a way to enforce host networking in the next round of updates to PR rook#13651 . This change improves the documentation, implementation, and unit testing for the `IsHost()` method of the `CephCluster.Network` spec. Signed-off-by: Michael Adam <obnox@samba.org> Co-authored-by: Blaine Gardner <b.blaine.gardner@gmail.com> Co-authored-by: Travis Nielsen <tnielsen@redhat.com>

This change is a follow-up to PR rook#13651 . It is also meant to provide a continuation of the closed, stale PR rook#13878 . It is furthermore intended as a preparation for the introduction of a way to enforce host networking in the next round of updates to PR rook#13651 . This change improves the documentation, implementation, and unit testing for the method of the spec. Signed-off-by: Michael Adam <obnox@samba.org> Co-authored-by: Blaine Gardner <b.blaine.gardner@gmail.com> Co-authored-by: Travis Nielsen <tnielsen@redhat.com>

obnoxxx requested a review from BlaineEXE January 30, 2024 20:10

obnoxxx marked this pull request as draft January 30, 2024 20:11

obnoxxx changed the title ~~Imtroduce "host-strict" network provider as a superset of "host".~~ Imtroduce a "host-strict" network provider as a superset of "host". Jan 30, 2024

obnoxxx force-pushed the host-strict branch from 41ae4f2 to 2acb379 Compare January 31, 2024 15:22

travisn changed the title ~~Imtroduce a "host-strict" network provider as a superset of "host".~~ Introduce a "host-strict" network provider as a superset of "host". Jan 31, 2024

obnoxxx force-pushed the host-strict branch 3 times, most recently from c0c2b5c to 5e42771 Compare January 31, 2024 17:42

BlaineEXE changed the title ~~Introduce a "host-strict" network provider as a superset of "host".~~ Introduce a "host-strict" network provider as a superset of "host" Jan 31, 2024

BlaineEXE reviewed Jan 31, 2024

View reviewed changes

obnoxxx force-pushed the host-strict branch 2 times, most recently from 6911ff0 to 4cb8a91 Compare January 31, 2024 18:55

BlaineEXE reviewed Jan 31, 2024

View reviewed changes

obnoxxx force-pushed the host-strict branch from 4cb8a91 to cb6ae45 Compare February 1, 2024 12:34

obnoxxx force-pushed the host-strict branch 2 times, most recently from 5ca4c60 to 5f90bb2 Compare February 1, 2024 13:08

obnoxxx force-pushed the host-strict branch from 5f90bb2 to 930d4e6 Compare February 1, 2024 15:42

BlaineEXE reviewed Feb 1, 2024

View reviewed changes

obnoxxx force-pushed the host-strict branch 3 times, most recently from 26898a5 to a37a263 Compare February 2, 2024 12:57

BlaineEXE reviewed Feb 2, 2024

View reviewed changes

pkg/apis/ceph.rook.io/v1/types.go Show resolved Hide resolved

obnoxxx force-pushed the host-strict branch from 127089e to 151c63a Compare February 5, 2024 15:34

obnoxxx mentioned this pull request Feb 5, 2024

disallow legacy hostNetwork setting with non-default network provider #13692

Closed

obnoxxx force-pushed the host-strict branch from 151c63a to 1da75eb Compare February 5, 2024 19:11

mergify bot mentioned this pull request Feb 5, 2024

Disallow legacy hostNetwork together with non-default provider #13693

Merged

6 tasks

github-actions bot added the stale Labeled by the stale bot label Apr 2, 2024

github-actions bot closed this Apr 23, 2024

obnoxxx mentioned this pull request May 22, 2024

network: improve the IsHost() function #14253

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a "host-strict" network provider as a superset of "host" #13651

Introduce a "host-strict" network provider as a superset of "host" #13651

obnoxxx commented Jan 30, 2024 •

edited by travisn

BlaineEXE Jan 31, 2024

obnoxxx Jan 31, 2024

BlaineEXE Jan 31, 2024

BlaineEXE Jan 31, 2024

BlaineEXE Jan 31, 2024

BlaineEXE Feb 13, 2024

obnoxxx Feb 15, 2024

obnoxxx Feb 15, 2024

BlaineEXE Jan 31, 2024

obnoxxx Feb 1, 2024

BlaineEXE Feb 1, 2024 •

edited

BlaineEXE Jan 31, 2024

obnoxxx Feb 1, 2024

obnoxxx commented Feb 1, 2024 •

edited

obnoxxx commented Feb 1, 2024

BlaineEXE Feb 1, 2024

BlaineEXE Feb 1, 2024

obnoxxx Feb 2, 2024 •

edited

BlaineEXE commented Feb 2, 2024

subhamkrai commented Feb 5, 2024 •

edited

github-actions bot commented Apr 2, 2024

github-actions bot commented Apr 23, 2024

Introduce a "host-strict" network provider as a superset of "host" #13651

Introduce a "host-strict" network provider as a superset of "host" #13651

Conversation

obnoxxx commented Jan 30, 2024 • edited by travisn

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obnoxxx commented Feb 1, 2024 • edited

obnoxxx commented Feb 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obnoxxx Feb 2, 2024 • edited

Choose a reason for hiding this comment

BlaineEXE commented Feb 2, 2024

subhamkrai commented Feb 5, 2024 • edited

github-actions bot commented Apr 2, 2024

github-actions bot commented Apr 23, 2024

obnoxxx commented Jan 30, 2024 •

edited by travisn

BlaineEXE Feb 1, 2024 •

edited

obnoxxx commented Feb 1, 2024 •

edited

obnoxxx Feb 2, 2024 •

edited

subhamkrai commented Feb 5, 2024 •

edited