Skip to content

revert: [NPM] [v1.5] Remove hostUsers Configuration #3614

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

Merged
merged 8 commits into from
May 2, 2025

Conversation

rayaisaiah
Copy link
Contributor

@rayaisaiah rayaisaiah commented Apr 29, 2025

Reason for Change:
Removes the hostUsers: false setting in the NPM daemonset. Reverts from changes #2589

Also adds a public-ip and service tag to NPM scale and conformance tests to comply with SFI rules (failing with RequestDisallowedByPolicy otherwise).

Issue Fixed:
From https://kubernetes.io/docs/concepts/workloads/pods/user-namespaces/#limitations

When using a user namespace for the pod, it is disallowed to use other host namespaces. In particular, if you set hostUsers: false then you are not allowed to set any of:

hostNetwork: true
hostIPC: true
hostPID: true

Requirements:

Notes:

* removed all logs from npm dataplane (except error/warning logs)

* removed all logs from npm controller (except error/warning logs)

* restored logs that are ununused by current npm (v2)

* removed test files

* resolved comments

* keep log related to reconciling chain placement when the chain is not in the right place

* added bootup logs back

* Removed two more noisy logs

* Add loglevel config option when printing application insight logs

* Updated all non-error/warning logs to commented out and with a vap TODO

* fixed typo

* small typo fix

* updated configmap with loglevel

* updated default value

* added a default value for loglevel

* fixed typo in json

* removed comma

* changed loglevel to info in configmap

* add a short sleep in TestNetPolInBackgroundSkipAddAfterRemove

* test remove dataplane changes to see if race condition fixes

* Revert "test remove dataplane changes to see if race condition fixes"

This reverts commit 08697eb.

* test

* Revert "test"

This reverts commit 449c2af.

* test

* update dataplane to test if changes are flagged in race

* added stop channels to unit tests to avoid race condtiions

* add non noisy logs back

* increased time

* revert time change after RunPeriodicTasks

* test with 1000 seconds

* 5000 milliseconds

* tweaked the delay

* update to 1500 for defer

* increased to 1500

* increase to 2000

* removed kubernetes
@Copilot Copilot AI review requested due to automatic review settings April 29, 2025 17:55
@rayaisaiah rayaisaiah requested a review from a team as a code owner April 29, 2025 17:55
@rayaisaiah rayaisaiah requested a review from matmerr April 29, 2025 17:55
@rayaisaiah rayaisaiah added npm Related to NPM. linux labels Apr 29, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the now-unnecessary hostUsers setting from the NPM daemonset as the use of other host namespaces is disallowed when a user namespace is in use.

  • Removed hostUsers: false property from the daemonset configuration
  • Aligns the NPM configuration with Kubernetes user namespace limitations

@rayaisaiah rayaisaiah changed the title Isaiahraya/v1.5 remove host users npm revert: [NPM] Remove hostUsers Configuration Apr 29, 2025
@rayaisaiah
Copy link
Contributor Author

/azp run Azure Container Networking PR

@rayaisaiah
Copy link
Contributor Author

/azp run NPM Conformance Tests

@rayaisaiah
Copy link
Contributor Author

/azp run NPM Scale Test

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

2 similar comments
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rayaisaiah rayaisaiah enabled auto-merge April 29, 2025 17:59
huntergregory
huntergregory previously approved these changes Apr 29, 2025
@rayaisaiah rayaisaiah added this pull request to the merge queue Apr 29, 2025
@rayaisaiah rayaisaiah removed this pull request from the merge queue due to a manual request Apr 29, 2025
@rayaisaiah rayaisaiah changed the title revert: [NPM] Remove hostUsers Configuration revert: [NPM] [v1.5] Remove hostUsers Configuration Apr 30, 2025
@rayaisaiah
Copy link
Contributor Author

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rayaisaiah rayaisaiah enabled auto-merge May 1, 2025 20:40
@rayaisaiah rayaisaiah requested a review from huntergregory May 1, 2025 20:56
Copy link
Contributor

@huntergregory huntergregory left a comment

Choose a reason for hiding this comment

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

looks good to me, but did you mean to remove hostUsers from npm lite too?

@rayaisaiah
Copy link
Contributor Author

looks good to me, but did you mean to remove hostUsers from npm lite too?

npm lite isnt on release/v1.5 but i did remove it from master

@rayaisaiah rayaisaiah added this pull request to the merge queue May 2, 2025
Merged via the queue into release/v1.5 with commit f5b05e2 May 2, 2025
18 checks passed
@rayaisaiah rayaisaiah deleted the isaiahraya/v1.5RemoveHostUsersNPM branch May 2, 2025 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linux npm Related to NPM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants