feat: [datadog metricprovider] Allow Datadog API and APP keys to be c… #1073
Conversation
There was a problem hiding this comment.
@jzyeezy the feature is a good improvement, but we'll need to reduce the API calls to k8s API server when getting each secret value.
Also, do you mind signing off for the DCO on your commit for us to accept this PR?
git commit --amend -s
git push -f
metricproviders/datadog/datadog.go
Outdated
| func getSecretValue(key, ns string, kubeclientset kubernetes.Interface, emptyValueOk bool) (string, error) { | ||
| envKey := strings.ToUpper(strings.ReplaceAll(key, "-", "_")) | ||
| if value, ok := os.LookupEnv(fmt.Sprintf("DD_%s", envKey)); ok { | ||
| return value, nil | ||
| } | ||
| secret, err := kubeclientset.CoreV1().Secrets(ns).Get(context.TODO(), DatadogTokensSecretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| _, valueMaybe := secret.Data[key] | ||
| if emptyValueOk && !valueMaybe { | ||
| return "", nil | ||
| } | ||
|
|
||
| return string(secret.Data[key]), nil | ||
| } | ||
|
|
||
| func NewDatadogProvider(logCtx log.Entry, kubeclientset kubernetes.Interface) (*Provider, error) { | ||
| ns := defaults.Namespace() | ||
| secret, err := kubeclientset.CoreV1().Secrets(ns).Get(context.TODO(), DatadogTokensSecretName, metav1.GetOptions{}) | ||
| apiKey, err := getSecretValue("api-key", ns, kubeclientset, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| apiKey := string(secret.Data["api-key"]) | ||
| appKey := string(secret.Data["app-key"]) | ||
| address := "" | ||
| if _, hasAddress := secret.Data["address"]; hasAddress { | ||
| address = string(secret.Data["address"]) | ||
| appKey, err := getSecretValue("app-key", ns, kubeclientset, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| address, err := getSecretValue("address", ns, kubeclientset, true) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
The change will result in three API calls to k8s instead of the previous one. Could you refactor such that it still only makes a single API call for when we need to get the secret from the API server?
…onsumed from env vars Signed-off-by: Joyce Yee <jzyee@paypal.com>
|
Okie @jessesuen I've addressed your comments. Thanks for the review! |
Codecov Report
@@ Coverage Diff @@
## master #1073 +/- ##
==========================================
+ Coverage 81.02% 81.28% +0.26%
==========================================
Files 103 104 +1
Lines 9174 9460 +286
==========================================
+ Hits 7433 7690 +257
- Misses 1246 1259 +13
- Partials 495 511 +16
Continue to review full report at Codecov.
|
metricproviders/datadog/datadog.go
Outdated
| secret, err := kubeclientset.CoreV1().Secrets(ns).Get(context.TODO(), DatadogTokensSecretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Note that by leaving the logic to fetch the secret here, it does not satisfy your previous benefit:
This also eliminates the need for granting the argo-rollouts pod the API permission to access the datadog k8s-secret.
Not sure if you care about this as a feature, but I just wanted to call out the fact that it does not really eliminate the RBAC necessary to get the datadog secret, because this call would fail.
There was a problem hiding this comment.
Wow you are right about that. 🤦🏻♀️ Thanks for catching that!
Let me address this one more time, properly.
There was a problem hiding this comment.
Alrighty pushed up 1308004, 3rd time's a charm?
Signed-off-by: Joyce Yee <jzyee@paypal.com>
|
Kudos, SonarCloud Quality Gate passed!
|
Why
My organization has a pattern where keys are stored in a secret cloud provider which get auto-synced as generic k8s secrets across regions. Rather than duplicate these keys in their own
datadogk8s-secret, it would be much more convenient for us to mount the original secret values as environment variables for argo-rollouts. This also eliminates the need for granting the argo-rollouts pod the API permission to access thedatadogk8s-secret.Checklist:
"fix(controller): Updates such and such. Fixes #1234".