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

controller: reject adding ifaces to host networked pods #89

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

maiqueb
Copy link
Collaborator

@maiqueb maiqueb commented Nov 19, 2022

What this PR does / why we need it:
This PR rejects adding / removing interfaces to/from host networked pods.

The controller unit tests were refactored a bit - creating the k8s client and the dummy controller - in a JustBeforeEach block to allow each When / Context block to define the pod previously. This way, there is a lot less duplicated code.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #88

Special notes for your reviewer (optional):

The controller unit tests were refactored a bit - creating the k8s
client and the dummy controller - in a `JustBeforeEach` block to allow
each `When` / `Context` block to define the pod previously. This way,
there is a lot less duplicated code.

Signed-off-by: Miguel Duarte Barroso <[email protected]>
@maiqueb maiqueb requested a review from kmabda November 19, 2022 18:35
"rejecting to add interfaces for host networked pod: %s",
annotations.NamespacedName(newPod.GetNamespace(), newPod.GetName()),
)
pnc.Eventf(newPod, corev1.EventTypeWarning, "InterfaceAddRejected", rejectInterfaceAddEventFormat(newPod))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm assuming we want to both throw an event an log ... Maybe that's too much.

Any thoughts @kmabda ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a good idea to throw an event here to give a feedback to the user

Copy link
Collaborator

@kmabda kmabda Nov 20, 2022

Choose a reason for hiding this comment

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

One thing, since in https://github.com/k8snetworkplumbingwg/multus-dynamic-networks-controller/pull/90/files#diff-449e8d1bd46b9c978e208dde912a1d7a76d17ce553067f8d0a67a27f17338d26R152 we need to filter out hostnetwork pods as well but we can't tel before hand if an interface got added by the user or not until we process the request we could move this logic from handlePodUpdate to processNextWorkItem function.

Pro same treatment logic would be in one place i.e. we have a consistency w.r.t. filtering out unwanted pods and how we log/throw out an event to the user in case an interface got added for these pods in one place
Con a little more overhead by adding an item to the workqueue to processing it than throwing away

If you go ahead with these changes I can rebase my pull request once this one get merged.

@maiqueb maiqueb merged commit a09c185 into main Nov 21, 2022
@maiqueb maiqueb deleted the reject-updates-host-networked-pods branch November 21, 2022 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ignore host networked pods
2 participants