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

[karmor summary] Added Time Stamps and Count for Ingress and Egress Connections #561

Merged
merged 1 commit into from Oct 4, 2022
Merged

[karmor summary] Added Time Stamps and Count for Ingress and Egress Connections #561

merged 1 commit into from Oct 4, 2022

Conversation

yasin-cs-ko-ak
Copy link
Contributor

@yasin-cs-ko-ak yasin-cs-ko-ak commented Sep 30, 2022 •
edited

This is enhancement for karmor summary obseravability.

  • Added Count and Updated Time in these files summary.go, observability.go and kubearmor.go
  • Updated Count and Updated Time in observability.proto under message SysNwSummaryData here.

Note: We have to replace this line opb "github.com/accuknox/auto-policy-discovery/src/protobuf/v1/observability" in this file with latest updated protobuf to reflect the changes that made in discovery-engine.

@yasin-cs-ko-ak yasin-cs-ko-ak changed the title Added Time Stamps and Count for Ingress and Egress Connections [karmor summary] Added Time Stamps and Count for Ingress and Egress Connections Sep 30, 2022
@yasin-cs-ko-ak
Copy link
Contributor Author

@nyrahul @Ankurk99 @seswarrajan Kindly verify this PR.

Ankurk99
Ankurk99 suggested changes Oct 3, 2022
View reviewed changes
Copy link
Contributor

@Ankurk99 Ankurk99 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, but there seems to be a lot of noise in the PR. Can you please remove the binary files and the changes which are downgrading protoc?

src/conf/local-file.yaml Outdated Show resolved Hide resolved
src/protobuf/v1/config/config.pb.go Outdated Show resolved Hide resolved
seswarrajan
seswarrajan requested changes Oct 3, 2022
View reviewed changes
Copy link
Contributor

@seswarrajan seswarrajan left a comment

Choose a reason for hiding this comment

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

  1. Refrain from adding scripts/?_journal=OFF and scripts/accuknox.db files to the PR.
  2. Do not modify conf/local-file.yaml unless necessary.
  3. Do not add all protobuf compiled files. Add only the files with respective changes.

Rest LGTM

src/conf/local-file.yaml Outdated Show resolved Hide resolved
@yasin-cs-ko-ak
Ingress/Egress Count,UpdatedTime added
d5daa71 
Removed Unwanted Compiled Binaries
@seswarrajan
Copy link
Contributor

image

Do not add all protobuf changes to the PR.
I think the change is done only for observability. Hence only add files related to observability and ignore other files.

seswarrajan
seswarrajan approved these changes Oct 3, 2022
View reviewed changes
Copy link
Contributor

@seswarrajan seswarrajan left a comment

Choose a reason for hiding this comment

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

LGTM

@Ankurk99
Copy link
Contributor

Ankurk99 commented Oct 4, 2022

LGTM 🚀 great work @yasin-cs-ko-ak

@Ankurk99 Ankurk99 merged commit 9c120a7 into accuknox:dev Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ankurk99 Ankurk99 Ankurk99 approved these changes

@seswarrajan seswarrajan seswarrajan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yasin-cs-ko-ak @seswarrajan @Ankurk99