MON-3749: enable request headers flags for metrics server #2293
Conversation
slashpai
commented
Mar 28, 2024
slashpai
commented
- I added CHANGELOG entry for this change.
- No user facing changes, so no entry in CHANGELOG was needed.
Signed-off-by: Jayapriya Pai <[email protected]>
|
@slashpai: This pull request references MON-3749 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
approved
|
will update adding |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Small nit, can we squash (unless squash and merge is enabled in CMO repo) these commits before merging this? |
ya sure I wanted to verify if updated changes works needed to keep separate commit for easy update. First commit is already verified working. I can squash commit once I test the change locally :) |
Signed-off-by: Jayapriya Pai <[email protected]>
|
/skip |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielmellado, slashpai The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest-required |
|
@slashpai: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
There was a problem hiding this comment.
If you agree with my comments JP, I'd be happy to address them myself as the PR is already merged.
Hopefully metrics-server would learn to do this itself: kubernetes-sigs/metrics-server#1432
| "tls.crt": []byte(tlsCA), | ||
| "tls.key": []byte(tlsKey), |
There was a problem hiding this comment.
I don't see where we mount/set these two.
There was a problem hiding this comment.
cluster-monitoring-operator/assets/metrics-server/deployment.yaml
Lines 85 to 86 in 900ffda
| return fmt.Errorf("failed to create metrics-server secret: %w", err) | ||
| } | ||
|
|
||
| err = t.deleteOldMetricsServerSecrets(secret.Labels["monitoring.openshift.io/hash"]) |
There was a problem hiding this comment.
Not related to the PR: Couldn't this be replaced by a hash on the Deployment itself as we do for the certs?
So we have only one way of "reloading the Deployment if the secret changes".
(I'd create a ticket for that)
(cc @simonpasquier in case you know that this logic is needed for other things than reloading)
There was a problem hiding this comment.
I wasn't sure either why this was there for prometheus-adapter as well. Happy to move to similar logic we have for other secrets if this is not needed
There was a problem hiding this comment.
+1 on doing that since we already use the other logic within MS.
Signed-off-by: Jayapriya Pai <[email protected]>
