A refactor of the queue volume constants #13146
Conversation
Fixes knative#13118 Alternative to knative#13122 Proposed Changes Enabling downstream and anyone creating an image of queue proxy to extend queue proxy This extendibility is a step to enable the runtime security proposal here Release Note Refactored queue-proxy binary to more easily support out-of-tree extension.
Fixes knative#13118 Annotation based configuration for activat8ing and configuring QP Extensions Support defaults in deployment config map
Codecov Report
@@ Coverage Diff @@
## main #13146 +/- ##
==========================================
- Coverage 86.72% 86.70% -0.03%
==========================================
Files 196 196
Lines 14463 14463
==========================================
- Hits 12543 12540 -3
- Misses 1624 1626 +2
- Partials 296 297 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
/assign davidhadas |
|
/retest |
|
/cc evankanderson
|
3bdf2e8 to
782df98
Compare
|
/reopen |
|
@davidhadas: Reopened this PR. DetailsIn response to this:
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. |
psschwei
left a comment
There was a problem hiding this comment.
One small question , otherwise LGTM
|
Changing the pr title will make it impossible to follow - other PRs were referring to this PR. I can create a new PR and close this one if this makes sense. |
|
All the links should be to the PR number (#13146 ), so it should be fine to change the title. The reason for the change is so that the git commit is accurate... prow squashes all the commits, then uses the PR title/body as the commit message, which is why those need to be up to date. |
|
As a reference: ##Proposed Changes
Note: Changes to annotations result in a new revision of the service that has the new extension configuration. Next possible steps:
Release Note Refactored queue-proxy binary to more easily support out-of-tree extension. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidhadas, psschwei The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // CertDirectory is the name of the directory path where certificates are stored. | ||
| CertDirectory = "/var/lib/knative/certs" | ||
| // CertVolumeMountPath is the name of the directory path where certificates are stored. | ||
| CertVolumeMountPath = "/var/lib/knative/certs" |
There was a problem hiding this comment.
minor nit pick: From a user's point of view, it does not matter whether a directory is mounted or part or a original directory of that file directory. Therefore I really would prefer "directory" instead of "mountPath" except when you would actually would do the mount yourself (I doubt that). I would better rename the other MountPath constants to use Directory instead (like PodInfoVolumeMountPath)
There was a problem hiding this comment.
Fine either way, as long as all mount points use the same convention.
Note however that we added a config feature to control if podInfo is mounted or not - this is also reflected in the documentation as a result - so we do expose this externally as a mounted volume.
If we open a new PR for this, I would like to also add a PodInfoAnnotationsPath (or we can call it AnnotationsPath) const in shardmain - this allows Options importing shardmain to use the shardmain const rather than importing also pkg/queue to gain access to the consts defining the annotations file path to open and read.
There was a problem hiding this comment.
"directory" works for me (it also seems more in line with the spirit of these variables, as the original comments note these are for "the name of the directory path")
Align QP volume mounting options