-
Notifications
You must be signed in to change notification settings - Fork 191
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
Bug 1809195: Send CVO metrics over https #358
Conversation
@jottofar: This pull request references Bugzilla bug 1809195, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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 kubernetes/test-infra repository. |
a1c1ff4
to
8eab894
Compare
/retest |
/test e2e-aws-upgrade |
/test images |
f148673
to
24c6cfc
Compare
After some wandering in the wilderness above, I think I'm starting to wrap my head around this. Take a look at wking/cluster-version-operator@c8cb5cf, pick up whatever seems useful, and then move on to figure out the Go behind the new X.509 options. |
78ea74f
to
1adcf60
Compare
/test e2e-aws |
CVO metrics are currently being sent insecurely over http. Change metrics configuration to use https and TLS encryption. CVO will continue to use the same metrics port and will continue to support http so a connection mux is setup on the existing port.
Using library-go@v0.0.0-20200303185131-81598fff9efa version to minimize changes, e.g. client-go kubernetes rebase to v1.18.0-beta.2.
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jottofar, wking 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
1 similar comment
/retest |
/test e2e-aws |
@jottofar: All pull requests linked via external trackers have merged: openshift/cluster-version-operator#358. Bugzilla bug 1809195 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
During modifications for openshift#358 a new first line of spec was added and therefore next line should not contain '-'.
We began serving metrics over HTTPS with 6132bc3 (Bug 1809195: Send CVO metrics over https, 2020-05-07, openshift#358), which also requested monitoring to scrape us over HTTPS. Now that that is all in place in 4.6, we no longer need to serve over HTTP in 4.7 and later. This commit pivots us to always serving over HTTPS. Because we are no longer serving HTTP, move to requiring --serving-cert-file and --serving-key-file when --listen is non-empty. I'd like to drop the --listen default, to make it an explicit opt-in, but I don't want to lose metrics when folks update from 4.6 -> 4.7. With this commit we start setting --listen explicitly when we launch child CVOs, and in 4.8 we can drop: ListenAddr: "0.0.0.0:9099", from pkg/start. It's possible that the manifest for the incoming CVO is constructed from the incoming release image, in which case we may be able to drop the --listen default now. I'm not setting --listen in the bootstrap manifest, because we don't need to serve metrics then (it's long before we have Prometheus around to scrape us).
We began serving metrics over HTTPS with 6132bc3 (Bug 1809195: Send CVO metrics over https, 2020-05-07, openshift#358), which also requested monitoring to scrape us over HTTPS. Now that that is all in place in 4.6, we no longer need to serve over HTTP in 4.7 and later. This commit pivots us to always serving over HTTPS. Because we are no longer serving HTTP, move to requiring --serving-cert-file and --serving-key-file when --listen is non-empty. I'd like to drop the --listen default, to make it an explicit opt-in, but I don't want to lose metrics when folks update from 4.6 -> 4.7. With this commit we start setting --listen explicitly when we launch child CVOs, and in 4.8 we can drop: ListenAddr: "0.0.0.0:9099", from pkg/start. It's possible that the manifest for the incoming CVO is constructed from the incoming release image, in which case we may be able to drop the --listen default now. I'm not setting --listen in the bootstrap manifest, because we don't need to serve metrics then (it's long before we have Prometheus around to scrape us).
We began serving metrics over HTTPS with 6132bc3 (Bug 1809195: Send CVO metrics over https, 2020-05-07, openshift#358), which also requested monitoring to scrape us over HTTPS. Now that that is all in place in 4.6, we no longer need to serve over HTTP in 4.7 and later. This commit pivots us to always serving over HTTPS. Because we are no longer serving HTTP, move to requiring --serving-cert-file and --serving-key-file when --listen is non-empty. I'd like to drop the --listen default, to make it an explicit opt-in, but I don't want to lose metrics when folks update from 4.6 -> 4.7. With this commit we start setting --listen explicitly when we launch child CVOs, and in 4.8 we can drop: ListenAddr: "0.0.0.0:9099", from pkg/start. It's possible that the manifest for the incoming CVO is constructed from the incoming release image, in which case we may be able to drop the --listen default now. I'm not setting --listen in the bootstrap manifest, because we don't need to serve metrics then (it's long before we have Prometheus around to scrape us).
CVO metrics are currently being sent insecurely over http. Change
metrics yaml file to use https and TLS encryption.