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

cilium: Improve user experience of policy trace with regard to port a… #15929

Merged
merged 1 commit into from May 20, 2021
Merged

cilium: Improve user experience of policy trace with regard to port a… #15929

merged 1 commit into from May 20, 2021

Conversation

Maddy007-maha
Copy link
Contributor

@Maddy007-maha Maddy007-maha commented Apr 29, 2021 •
edited

cilium: Improve user experience of policy trace related to port/protocol

Description: As part of 'cilium policy trace' command, dport was an optional paramter due to which it takes port and protocol
as 0 and ANY respectively due to which it causing undefined behaviors.
Root Cause: --dport parameter is an optional argument under 'cilium policy trace'
Fix: We made this --dport parameter as mandatory argument under 'cilium policy trace'

Fixes: #15387

Signed-off-by: Maddy007-maha mahadev.panchal@accuknox.com

--dport flag is a mandatory argument in cilium policy trace command with port and protocol

@Maddy007-maha Maddy007-maha requested a review from a team as a code owner April 29, 2021 07:16
@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 Apr 29, 2021
twpayne
twpayne suggested changes Apr 29, 2021
View reviewed changes
Copy link
Contributor

@twpayne twpayne left a comment

Choose a reason for hiding this comment

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

Please also fill in the PR template, specifically the release notes. The release notes should probably something like "Require --dport flag in cilium policy trace command".

cilium/cmd/policy_trace.go Show resolved Hide resolved
@twpayne twpayne added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 29, 2021
@Maddy007-maha Maddy007-maha force-pushed the fix_15387 branch 3 times, most recently from a2c3048 to e2a387e Compare April 29, 2021 12:26
twpayne
twpayne suggested changes Apr 30, 2021
View reviewed changes
Copy link
Contributor

@twpayne twpayne left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. The author of the commit is currently vagrant@vagrant.vm which does not match the Signed-Off-By line in the commit message. Please could you change this.

@Maddy007-maha
Copy link
Contributor Author

Thanks for the updates. The author of the commit is currently vagrant@vagrant.vm which does not match the Signed-Off-By line in the commit message. Please could you change this.

Thanks for the updates. The author of the commit is currently vagrant@vagrant.vm which does not match the Signed-Off-By line in the commit message. Please could you change this.

Yeah I have changed accordingly

-Thank you

twpayne
twpayne approved these changes Apr 30, 2021
View reviewed changes
Copy link
Contributor

@twpayne twpayne left a comment

Choose a reason for hiding this comment

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

Thank you!

@twpayne
Copy link
Contributor

twpayne commented May 3, 2021

test-me-please

@Maddy007-maha
Copy link
Contributor Author

test-me-please

Hi @twpayne,

Looks like some test suites are failed and after inspecting those failed logs => It looks like it's due to the change of command (--dport is a mandatory option now).

Do we need to rewrite the test cases according to this fix?
Please guide me on this.

-Thanks

@twpayne
Copy link
Contributor

twpayne commented May 3, 2021

Yes, the test cases need to be updated for the code change.

@Maddy007-maha Maddy007-maha requested a review from a team as a code owner May 6, 2021 09:32
@Maddy007-maha Maddy007-maha requested review from a team and nbusseneau May 6, 2021 09:32
@maintainer-s-little-helper
Copy link

Commit e2e509b6e7c006b7d6abb057ff93f6547da88a15 does not contain "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 May 6, 2021
@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 May 6, 2021
nbusseneau
nbusseneau approved these changes May 6, 2021
View reviewed changes
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

LGTM, thanks :)

nbusseneau
nbusseneau requested changes May 6, 2021
View reviewed changes
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

Actually, no... Sorry I posted on the wrong PR 😅
I think the test cases should be updated to work with the now mandatory option, not removed ;)

@Maddy007-maha
Copy link
Contributor Author

Actually, no... Sorry I posted on the wrong PR 😅

I think the test cases should be updated to work with the now mandatory option, not removed ;)

Yeah I have updated the test cases as well

Kindly review it

-Thanks

@nbusseneau
Copy link
Member

@Maddy007-maha Can you double-check that you did push the updates? In the current set of changes, I only see test removals, not updates. Am I missing something?

@Maddy007-maha
Copy link
Contributor Author

Maddy007-maha commented May 6, 2021 •
edited

@Maddy007-maha Can you double-check that you did push the updates? In the current set of changes, I only see test removals, not updates. Am I missing something?

If you see the test cases, we have 2 categories as follows

  1. with --dport argument test cases
  2. without --dport argument test cases

As we made --dport as a mandatory argument, so we removed the rest of the void (unused --dport option) test cases.
And we are already taking care of --dport argument test cases so no need for any changes

-Thanks

@Maddy007-maha Maddy007-maha requested a review from twpayne May 6, 2021 16:50
nbusseneau
nbusseneau requested changes May 6, 2021
View reviewed changes
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

@Maddy007-maha I re-read #15387 in more details, and actually what we want is not to require --dport, but rather than when --dport is used, always require both ports and protocol to be provided.

@Maddy007-maha
Copy link
Contributor Author

@Maddy007-maha I re-read #15387 in more details, and actually what we want is not to require --dport, but rather than when --dport is used, always require both ports and protocol to be provided.

Hi @nbusseneau,
If we refer to this comment #15387 (comment)
which clearly explains the port and protocol are required as an argument.

If we do skip the dport option it takes wildcard {0/ANY} values which wont match with any policies and as a result it shows invalid results.

For Eg please refer #15387 (comment)
As it gives invalid results with wild card values, we have made changes for the dport option as a mandatory argument.

@nbusseneau
Copy link
Member

nbusseneau commented May 7, 2021 •
edited

Hi @nbusseneau,
If we refer to this comment #15387 (comment)
which clearly explains the port and protocol are required as an argument.

If we do skip the dport option it takes wildcard {0/ANY} values which wont match with any policies and as a result it shows invalid results.

For Eg please refer #15387 (comment)
As it gives invalid results with wild card values, we have made changes for the dport option as a mandatory argument.

@Maddy007-maha Yes, I did see Thomas' comment, and while I agree on first read it might look like an argument for having --dport as required, on a second read and with the whole issue context, it is not: I double confirm that what we want is that when --dport is used, we should require both port and protocol to be provided. But --dport itself should stay optional.

@nbusseneau
Copy link
Member

@Maddy007-maha Following clarification on Slack, and contrary to my previous comments, you are indeed right and we do want to mark --dport as required. Apologies for the confusion 👐🏻

Can you please re-review my comment here about how we can move forward to fix the tests?

@nbusseneau
Copy link
Member

nbusseneau commented May 10, 2021 •
edited

Thanks for the modifications, just triggered the CI tests :)

nbusseneau
nbusseneau approved these changes May 12, 2021
View reviewed changes
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

Thanks, all tests have passed, I think this should be OK from a CI perspective.

@Maddy007-maha
cilium: Improve user experience of policy trace related to port/protocol
546b0b0 
Description: As part of 'cilium policy trace' command, dport was an optional paramter due to which it takes port and protocol
as 0 and ANY respectively due to which it causing undefined behaviors.
Root Cause: --dport parameter is an optional argument under 'cilium policy trace'
Fix: We made this --dport parameter as mandatory argument under 'cilium policy trace'

Fixes: cilium#15387

Signed-off-by: Maddy007-maha <mahadev.panchal@accuknox.com>

```
--dport flag is a mandatory argument in cilium policy trace command with port and protocol
```
nbusseneau
nbusseneau approved these changes May 17, 2021
View reviewed changes
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

Seems like you rebased and re-requested a review, right? I see no changes, so still LGTM.

@nbusseneau
Copy link
Member

nbusseneau commented May 17, 2021 •
edited

I've re-triggered CI to get checks green in order to merge.

@nbusseneau
Copy link
Member

nbusseneau commented May 18, 2021 •
edited

1.16 on netnext failed on Suite-k8s-1.16.K8sDatapathConfig Encapsulation Check vxlan connectivity with per-endpoint routes with:

/home/jenkins/workspace/Cilium-PR-K8s-1.16-net-next/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:518
Connectivity test between nodes failed
Expected
    <bool>: false
to be true
/home/jenkins/workspace/Cilium-PR-K8s-1.16-net-next/src/github.com/cilium/cilium/test/k8sT/DatapathConfiguration.go:257

I think this is a flake that hasn't been filed yet.
I am retriggering the job just in case, but am 90% sure.

EDIT: Logged as #16203.

@nbusseneau nbusseneau added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 19, 2021
@twpayne twpayne merged commit 0767f14 into cilium:master May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twpayne twpayne twpayne approved these changes

@nbusseneau nbusseneau nbusseneau 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve user experience of policy trace with regard to port and protocol
3 participants
@Maddy007-maha @twpayne @nbusseneau