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

Updated summary with pod's parent information #733

Closed
wants to merge 1 commit into from

Conversation

vishnusomank
Copy link
Contributor

Fixes: #731

Related PRs --> kubearmor/kubearmor-client#299

This PR contains the following changes:

  • Updated summary information to include pod's parent_name and parent_type
  • Updated functions to retrieve daemonset details
  • Updated constants with deployment, replicaset, statefulset and daemonset values
  • Updated protobuf definitions to include parent_name and parent_type to pod information

Please refer 299#issuecomment-1561132322 for the updated output

@vishnusomank vishnusomank changed the title Updated summary information to add pods parent information Updated summary with pod's parent information May 24, 2023
@@ -36,6 +37,7 @@ message Response{
repeated CiliumSummData IngressData = 11;
repeated CiliumSummData EgressData = 12;
repeated SysNwSummaryData BindConnection = 13;
string DeployType = 14;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does DeployType signify over here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be ResourceType?

Copy link
Contributor

Choose a reason for hiding this comment

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

k8s uses the term resource owner for this.

@@ -36,6 +37,7 @@ message Response{
repeated CiliumSummData IngressData = 11;
repeated CiliumSummData EgressData = 12;
repeated SysNwSummaryData BindConnection = 13;
string DeployType = 14;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be ResourceType?

@@ -20,6 +20,7 @@ message Request{
string Type = 6;
bool Aggregate = 7;
string DeployName = 8;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name this as ResourceName?

K8sDeploymentType = "Deployment"
K8sStatefulSetsType = "StatefulSet"
K8sReplicaSetsType = "ReplicaSet"
K8sDaemonSetsType = "DaemonSet"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to add for jobs?

@vishnusomank vishnusomank force-pushed the summary-deploy branch 3 times, most recently from 87eb5ba to 67effbc Compare June 7, 2023 08:00
@vishnusomank vishnusomank requested a review from rajaSahil June 7, 2023 08:02
@vishnusomank
Copy link
Contributor Author

@rajaSahil I've updated the code based on your comment. Can you please review again?
cc: @nyrahul

// == DaemonSets == //
// ================= //

func GetDaemonSetsFromK8sClient() []types.Deployment {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create a single function and then pass the k8s objects as argument? like GetObjectSetsFromK8sClient(object), then pass daemonset/deployment etc. as a value to it. It will greatly reduce the code size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed @Ankurk99

for _, rs := range replicaSets.Items {
results = append(results, GenerateResourceList(rs.OwnerReferences,rs.Name,rs.Namespace,types.K8sReplicaSetsType,rs.Spec.Selector.MatchLabels)...)
}
case types.K8sJobsType:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to consider cronjobs? Although it creates new jobs periodically.

@@ -119,6 +119,7 @@ recommend:
operation-mode: 1 # 1: cronjob | 2: one-time-job
cron-job-time-interval: "1h0m00s" # format: XhYmZs
recommend-host-policy: true
admission-controller-policy: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

@@ -433,7 +433,7 @@ func upsertSysSummarySQL(db *sql.DB, summary types.SystemSummary, timeCount type
summary.Labels = strings.Join(sortedLabels, ",")

queryString := `cluster_name = ? and cluster_id = ? and workspace_id = ? and namespace_name = ? and namespace_id = ? and container_name = ? and container_image = ?
and podname = ? and operation = ? and labels = ? and deployment_name = ? and source = ? and destination = ?
and podname = ? and operation = ? and labels = ? and parent_name = ? and parent_type = ? and source = ? and destination = ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name table columns as resource_type and resource_name?

Comment on lines 725 to 726
" `parent_name` varchar(50) DEFAULT NULL," +
" `parent_type` varchar(50) DEFAULT NULL," +
Copy link
Contributor

Choose a reason for hiding this comment

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

resource_type and resource_name ?

db := connectSQLite(cfg, config.GetCfgObservabilityDBName())
defer db.Close()

resDeployNames := []string{}
resDeployNames := map[string]string{}

var results *sql.Rows
var err error

// Get podnames from system table
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Get podnames from system table
// Get resource type and resource names from system table.

Comment on lines +1656 to +1663
var locDeployName, locDeployType string
if err := results.Scan(
&locDeployName,
&locDeployType,
); err != nil {
return nil, err
}
resDeployNames = append(resDeployNames, locDeployName)
resDeployNames[locDeployName] = locDeployType
Copy link
Contributor

Choose a reason for hiding this comment

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

There can be possibility of having same resource names in different namespace, so when getting all the entries from table, are we handling this case?

}
}
if sysSummary.Deployment == "" {
sysSummary.Workload.Type = "Pod"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct to consider pod as a workload type?

- Updated summary information to include pod's parent_name and parent_type
- Updated functions to retrieve daemonset details
- Updated constants with deployment, replicaset, statefulset, daemonset and jobs values
- Updated protobuf definitions to include parent_name and parent_type to pod information

Signed-off-by: Vishnu Soman <[email protected]>
@seswarrajan
Copy link
Contributor

Currently putting this on hold as we are handling summary through different CLI tool

@seswarrajan seswarrajan closed this Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Summary data doesn't show information about the pod's parent except for deployment
5 participants