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

test: Remove special case for host identity when remote-node identity is disabled #16450

Conversation

romanspb80
Copy link
Contributor

The test-case "Installing fromEntities host and world policy" with disabled remote-node identity is no longer needed due to lack of any network endpoints outside of the cluster.

Fixes: #15491

Signed-off-by: Roman Ptitcyn romanspb@yahoo.com

@romanspb80 romanspb80 requested a review from a team as a code owner June 6, 2021 15:57
@romanspb80 romanspb80 requested a review from a team June 6, 2021 15:57
@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 Jun 6, 2021
@romanspb80 romanspb80 force-pushed the Remove_special_test_case_for_host_identity_when_remote-node_identity_is_disabled branch from 75a6c0b to 565a812 Compare June 6, 2021 20:39
@pchaigno pchaigno added area/CI Continuous Integration testing issue or flake release-note/ci This PR makes changes to the CI. labels Jun 7, 2021
@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 Jun 7, 2021
@pchaigno pchaigno added the sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. label Jun 7, 2021
@pchaigno
Copy link
Member

pchaigno commented Jun 7, 2021

test-me-please

@romanspb80
Copy link
Contributor Author

I think the tests are stuck

@pchaigno pchaigno closed this Jun 9, 2021
@pchaigno pchaigno reopened this Jun 9, 2021
@christarazi
Copy link
Member

test-1.20-4.19

@christarazi
Copy link
Member

test-1.21-4.9

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 14, 2021
@joestringer
Copy link
Member

joestringer commented Jun 14, 2021

Was there a discussion about this anywhere? I don't see a detailed analysis either in the issue #15491 or here in this PR.

The test-case "Installing fromEntities host and world policy" with disabled remote-node identity is no longer needed due to lack of any network endpoints outside of the cluster.

From what I can tell, this test was about policy handling for remote nodes in the cluster, where the "world" entities policy is expected to allow traffic directly from remote nodes in native mode + remote-node-identity=disabled.

Can you explain further what you mean by it is no longer needed? Do we no longer want to make such a guarantee? Or you think that this behaviour is already covered by another test in the testsuite? Or something else?

@joestringer joestringer removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 14, 2021
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Please provide a more concrete rationale before we merge this (see comment above).

@joestringer
Copy link
Member

From out-of-band discussion with @pchaigno it sounds like basically this test code being removed is either (1) not run in regular test environments or (2) not effective in evaluating whether the remote-node identity option is performing correctly, so the extra test code is unnecessary.

@romanspb80 would you mind updating the commit message to include the above detail as well? If for whatever reason we find out in future that this test code was actually useful or necessary, it could be helpful to be able to read back through the git commit logs to find these details out.

With that I think this should be OK to merge.

@romanspb80 romanspb80 force-pushed the Remove_special_test_case_for_host_identity_when_remote-node_identity_is_disabled branch from 565a812 to 8390ff1 Compare June 15, 2021 21:06
@romanspb80
Copy link
Contributor Author

@romanspb80 would you mind updating the commit message to include the above detail as well? If for whatever reason we find out in future that this test code was actually useful or necessary, it could be helpful to be able to read back through the git commit logs to find these details out.

With that I think this should be OK to merge.

OK. Done.
Thanks

This test code being removed has no effect on the CI currently, so it
either doesn't run in regular test environments or is otherwise not
effective in evaluating whether the remote-node identity option is
performing correctly, so the extra test code is unnecessary.

Fixes: cilium#15491

Signed-off-by: Roman Ptitcyn <romanspb@yahoo.com>
@joestringer joestringer force-pushed the Remove_special_test_case_for_host_identity_when_remote-node_identity_is_disabled branch from 8390ff1 to f28f543 Compare June 15, 2021 21:22
@joestringer
Copy link
Member

👍 Only change was to update the commit message, good to merge.

@romanspb80 just to let you know in future, if you configure your email / name in git using the following commands, then you can use git commit -s to sign off your commits, so then you can get the standard formatting of the signoff line without needing to manually type out your name / email each time:

git config --global user.name "FIRST_NAME LAST_NAME"
git config --global user.email "MY_NAME@example.com"

@joestringer joestringer merged commit 67d83c0 into cilium:master Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake release-note/ci This PR makes changes to the CI. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test: Reconsider special case for host identity when remote-node identity is disabled
5 participants