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

Don't use old hardcoded PD API key secret #107

Merged

Conversation

grdryn
Copy link
Member

@grdryn grdryn commented Jul 27, 2020

It should be specified in the PDI CR.

@grdryn
Copy link
Member Author

grdryn commented Jul 27, 2020

/cc @jewzaam

@grdryn
Copy link
Member Author

grdryn commented Aug 1, 2020

@jewzaam I've attempted to add a metric around the ability to load the API key from the secret specified in the CR, but I'm having a little trouble testing it out: I can't see any of the custom metrics showing up on the metrics endpoint (so they don't get to prometheus either). In the operator logs, I can regularly see the log entry for the existing pagerduty API heartbeat metric, but I don't see that show up on the served page at the metrics endpoint. Log:

{"level":"info","ts":1596291862.8925786,"logger":"localmetrics","msg":"Metrics for PD API","Namespace":"pagerduty-operator"}

I don't really know enough about the operator-custom-metrics library that's being used here, and don't currently have time to debug (maybe next week). Could you check if those existing custom metrics get served as expected in stage?

@2uasimojo
Copy link
Member

I'm having a little trouble testing it out: I can't see any of the custom metrics showing up on the metrics endpoint (so they don't get to prometheus either).

Hi @grdryn.

You've been hit by a recent change in the controller-runtime. In older versions, if you didn't bootstrap the controller-runtime Manager with a MetricsBindAddress, it would not switch on the default metrics service. But since 0.2, which included this PR, the default changed to serving up metrics at port 8080. You accidentally crossed this boundary when you upgraded the operator-sdk via #100.

You normally wouldn't care that controller-runtime enables metrics you don't use. But if you try to register to the same port via operator-custom-metrics, it fails (silently [1]).

Thus, you will have to fix pagerduty-operator so it doesn't encounter this port conflict. The recommended way is to explicitly disable the default metrics service. You can do this by passing MetricsBindAddress: "0" to the Options you pass to manager.New(). Here's an example of such a change.

Hope this helps!

[1] @fahlmant (maintainer of operator-custom-metrics) is aware and is working on getting it to blow up when it encounters this problem.

@grdryn
Copy link
Member Author

grdryn commented Aug 1, 2020

@2uasimojo thanks for the tip! I've fixed it now and everything seems fine now.

I've also added the pdi name as a label to the existing create/delete metrics.

I've also put in a check to make sure the old finalizer won't block deletion.

@grdryn grdryn requested a review from jewzaam August 1, 2020 21:42
cmd/manager/main.go Outdated Show resolved Hide resolved
It should be specified in the PDI CR. If the API key can't be loaded
from the specified API key, then trigger an alert.
@grdryn
Copy link
Member Author

grdryn commented Aug 5, 2020

I have rebased this (conflict with another PR merged earlier today), and it's ready for review again.

@jewzaam
Copy link
Member

jewzaam commented Aug 6, 2020

/lgtm
/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grdryn, jewzaam

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2020
@openshift-merge-robot openshift-merge-robot merged commit 686d3de into openshift:master Aug 6, 2020
@grdryn grdryn deleted the use-custom-apikey-secret branch October 14, 2020 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants