search

DataDog / ansible-datadog

Ansible role for Datadog Agent
Apache License 2.0
298 stars 222 forks source link

Hard fail if api_key is not provided #505

Closed gopivalleru closed 1 year ago

gopivalleru commented 1 year ago

datadog.conf and datadog.yaml role has below line which sets a invalid default value for api_key. If agent_datadog_config["api_key"] or datadog_api_key vars are not provided then api_key in the datadog has 'youshouldsetthis' which fails to start datadog agent. Setting default here actual installs datadog but fails to start the agent until user notices.

{% if agent_datadog_config["api_key"] is not defined -%} api_key: {{ datadog_api_key | default('youshouldsetthis') }} {% endif %} Its better to hard fail while the role is being executed so that the user will provide necessary mandator variables.

ref: https://github.com/DataDog/ansible-datadog/issues/504

gopivalleru commented 1 year ago

@bkabrda Can someone take a look at this PR.

chouquette commented 1 year ago

Hi, Thanks for your contribution! It looks good, although we'd prefer for the API_KEY to be optional when datadog_manage_config is false, so that the role only fails when it needs the API key to be set. We have an internal task for that that shouldn't take long to address, but if you include that change in your PR before we do, that's perfectly fine by us :) Thanks!

gopivalleru commented 1 year ago

Hi, Thanks for your contribution! It looks good, although we'd prefer for the API_KEY to be optional when datadog_manage_config is false, so that the role only fails when it needs the API key to be set. We have an internal task for that that shouldn't take long to address, but if you include that change in your PR before we do, that's perfectly fine by us :) Thanks!

@chouquette Thanks for the review and the comment. I just looked at github email notification as it got buried in other emails.

anth0d commented 8 months ago

FYI, @chouquette this is, strictly speaking, a breaking change and it disrupted our build pipeline. We use Ansible during AMI bake and as a rule we do not bake any credentials into our AMIs.

Longer term we can address this by excising the API key after the role has been executed but for now we are pinning the dependency at 4.20.1.

I'm happy with whichever direction you're going I just wanted to drop an experience report for you. Thanks!

chouquette commented 8 months ago

Hi @anth0d and thanks for reporting the issue!

Indeed that's breaking change which we failed to consider, and it should have come with a major version change. My apologies about that.

We are adding some notes about it as part of this PR: https://github.com/DataDog/ansible-datadog/pull/538, please feel free to chime in!