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.

Sign up for GitHub

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

Jump to bottom

agent: Remove enable-remote-node-identity flag #31228

Merged
merged 4 commits into from Mar 26, 2024
Merged

agent: Remove enable-remote-node-identity flag #31228

merged 4 commits into from Mar 26, 2024

Conversation

doniacld
Copy link
Contributor

@doniacld doniacld commented Mar 7, 2024 •
edited

Remove the flag enable-remote-node-identity after being deprecated in v1.15.
Update the daemon, manager and associated tests.

Remove helm option `enable-remote-node-identity` after being deprecated in v1.15.

@doniacld doniacld requested review from a team as code owners March 7, 2024 19:31
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 7, 2024
@gandro gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. sig/agent Cilium agent related. labels Mar 11, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 11, 2024
gandro
gandro requested changes Mar 11, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment •
edited

Choose a reason for hiding this comment

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

Thanks! Overall this is a good cleanup, but there are some issues with the git history. In particular, we want each commit itself to build and ideally also pass CI. In order to achieve this, I think you need to reorder some of the commits.

daemon/cmd/daemon_main.go Show resolved Hide resolved
contrib/vagrant/start.sh Show resolved Hide resolved
install/kubernetes/cilium/templates/cilium-configmap.yaml Show resolved Hide resolved
Documentation/cmdref/cilium-agent.md Outdated Show resolved Hide resolved
pkg/node/manager/manager.go Show resolved Hide resolved
pkg/node/manager/manager.go Show resolved Hide resolved
pkg/node/manager/manager.go Show resolved Hide resolved
@gandro
Copy link
Member

gandro commented Mar 11, 2024 •
edited

I also wonder if we should call this out in the upgrade docs. I do hope we no longer have users running with enable-remote-node-identity: false, but if we do they will have to expect drops during upgrade, something we should explicitly call out.

@doniacld doniacld force-pushed the pr/doniacld/remove-enable-remote-node-identity branch from 16692bc to edc9799 Compare March 12, 2024 10:26
gandro
gandro requested changes Mar 12, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks, looking much better already. Still a few minor things that should be addressed

install/kubernetes/cilium/values.yaml.tmpl Show resolved Hide resolved
pkg/node/manager/manager.go Outdated Show resolved Hide resolved
pkg/option/config.go Outdated Show resolved Hide resolved
@maintainer-s-little-helper
Copy link

Commit 1fc8196 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 12, 2024
@doniacld doniacld force-pushed the pr/doniacld/remove-enable-remote-node-identity branch from 1fc8196 to a2c613d Compare March 12, 2024 13:50
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 12, 2024
@gandro gandro added this pull request to the merge queue Mar 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 21, 2024
@doniacld doniacld force-pushed the pr/doniacld/remove-enable-remote-node-identity branch from ba87019 to 88e7a15 Compare March 21, 2024 14:50
@maintainer-s-little-helper
Copy link

Commits a692f40, 835eca3, b3899a6, 02d43b5, f18377c, e903b61, 9ad5027 do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 21, 2024
@doniacld doniacld force-pushed the pr/doniacld/remove-enable-remote-node-identity branch from 88e7a15 to c9919ff Compare March 21, 2024 15:14
@maintainer-s-little-helper
Copy link

Commits a692f40, 835eca3, b3899a6, 02d43b5, f18377c, e903b61, 9ad5027 do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@doniacld doniacld force-pushed the pr/doniacld/remove-enable-remote-node-identity branch from c9919ff to f2c4d21 Compare March 21, 2024 15:24
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 21, 2024
@gandro
Copy link
Member

gandro commented Mar 21, 2024

/test

@gandro gandro enabled auto-merge March 21, 2024 15:52
danehans
danehans approved these changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@danehans danehans left a comment

Choose a reason for hiding this comment

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

@doniacld this PR needs a rebase. Otherwise, LGTM.

@gandro
Copy link
Member

gandro commented Mar 25, 2024

Needs another rebase due to conflicts

@gandro gandro added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 25, 2024
@doniacld
contrib/vagrant: Remove enableRemoteNodeIdentity agent flag
d703061 
The enable-remote-node-identity agent flag was marked as deprecated for
1.15 in commit cf472ef ("daemon: Deprecate
EnableRemoteNodeIdentity"). This commit removes the option from the deployment tool.

Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@doniacld
helm,documentation: Remove enableRemoteNodeIdentity agent flag
4c4daf9 
The enable-remote-node-identity agent flag was marked as deprecated for 1.15 in
commit cf472ef ("daemon: Deprecate EnableRemoteNodeIdentity"). This commit
removes the option in the helm values and updates the documentation.

Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@doniacld
manager: Remove legacy node ipbehaviour for remote node identity
a5ee4aa 
This commit wipes the code from the legacy remote node ip behaviour. It also
removes the `TestRemoteNodeIdentities` test case in `manager_test` and
completes `TestIPCache`.

Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@doniacld
daemon,test: remove enableRemoteNodeIdentity flag
2032c00 
The enable-remote-node-identity agent flag was marked as deprecated for
1.15 in commit cf472ef ("daemon: Deprecate
EnableRemoteNodeIdentity").

Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@doniacld doniacld force-pushed the pr/doniacld/remove-enable-remote-node-identity branch from f2c4d21 to 2032c00 Compare March 26, 2024 08:14
@gandro gandro removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 26, 2024
@gandro
Copy link
Member

gandro commented Mar 26, 2024

/test

@gandro gandro added this pull request to the merge queue Mar 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 26, 2024
@gandro gandro added this pull request to the merge queue Mar 26, 2024
@gandro gandro removed this pull request from the merge queue due to a manual request Mar 26, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 26, 2024
@gandro gandro added this pull request to the merge queue Mar 26, 2024
Merged via the queue into cilium:main with commit 2f82995 Mar 26, 2024
62 checks passed
@julianwiedmann julianwiedmann mentioned this pull request May 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derailed derailed derailed approved these changes

@gandro gandro gandro approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@tklauser tklauser tklauser approved these changes

@danehans danehans danehans approved these changes

@lambdanis lambdanis lambdanis approved these changes

@ysksuzuki ysksuzuki ysksuzuki approved these changes

@aditighag aditighag aditighag approved these changes

@viktor-kurchenko viktor-kurchenko viktor-kurchenko approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/agent Cilium agent related.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@doniacld @gandro @tklauser @derailed @nathanjsweet @danehans @lambdanis @ysksuzuki @aditighag @viktor-kurchenko