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

Add security_mode metric tag to request metrics #14265

Closed

Conversation

KauzClay
Copy link
Contributor

@KauzClay KauzClay commented Aug 15, 2023

Fixes #

In support of: #14092
Relies on: knative/networking#842

Proposed Changes

  • activator can dynamically pick up changes in config-network
  • activator metrics handler sets security mode tag on request metrics based on the network config value from context
  • queue proxy also sets security mode metric. Security mode is set as an env var

Release Note

Activator and QueueProxy now tag certain request metrics with "security_mode" data that corresponds to the value of dataplane-trust on config-network

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 15, 2023
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers area/autoscale area/monitoring labels Aug 15, 2023
@knative-prow
Copy link

knative-prow bot commented Aug 15, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: KauzClay
Once this PR has been reviewed and has the lgtm label, please assign dsimansk for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Patch coverage: 93.54% and no project coverage change.

Comparison is base (e9425f8) 86.26% compared to head (0fc6863) 86.26%.

❗ Current head 0fc6863 differs from pull request most recent head b6e8c10. Consider uploading reports for the commit b6e8c10 to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #14265    +/-   ##
========================================
  Coverage   86.26%   86.26%            
========================================
  Files         195      199     +4     
  Lines       14702    14827   +125     
========================================
+ Hits        12683    12791   +108     
- Misses       1719     1734    +15     
- Partials      300      302     +2     
Files Changed Coverage Δ
cmd/activator/main.go 0.00% <0.00%> (ø)
pkg/queue/sharedmain/main.go 0.86% <0.00%> (ø)
pkg/activator/config/store.go 100.00% <100.00%> (ø)
pkg/activator/handler/metric_handler.go 100.00% <100.00%> (ø)
pkg/activator/handler/metrics.go 92.59% <100.00%> (ø)
pkg/queue/request_metric.go 92.56% <100.00%> (+0.18%) ⬆️
pkg/reconciler/revision/resources/queue.go 98.43% <100.00%> (+0.01%) ⬆️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KauzClay KauzClay changed the title [wip] add internal encryption metric tag to request metrics Add security_mode metric tag to request metrics Aug 15, 2023
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 15, 2023
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2023
@KauzClay KauzClay force-pushed the ck-add-internal-encryption-metric-tag branch from b8c94ae to 0fc6863 Compare August 18, 2023 14:19
@knative-prow-robot knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 18, 2023
@KauzClay KauzClay force-pushed the ck-add-internal-encryption-metric-tag branch from 0fc6863 to b6e8c10 Compare August 21, 2023 13:52
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2023
@dprotaso
Copy link
Member

Based on this comment it seems like we'd go for the request logging approach

#14092 (comment)

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2023
@knative-prow-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@KauzClay
Copy link
Contributor Author

Based on this comment it seems like we'd go for the request logging approach
#14092 (comment)

yup, I'll close this and get started on a different PR for doing it via logs

@KauzClay KauzClay closed this Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API API objects and controllers area/autoscale area/monitoring area/networking needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants