Skip to content

Hard fail if api_key is not provided #505

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

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

gopivalleru
Copy link
Contributor

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: #504

@gopivalleru gopivalleru requested a review from a team as a code owner July 18, 2023 18:28
@gopivalleru
Copy link
Contributor Author

@bkabrda Can someone take a look at this PR.

@chouquette
Copy link
Contributor

chouquette commented Aug 9, 2023

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 chouquette self-assigned this Aug 9, 2023
@chouquette chouquette merged commit 82a0f86 into DataDog:main Aug 29, 2023
@gopivalleru
Copy link
Contributor Author

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.

@gopivalleru gopivalleru deleted the hard_fail_when_api_key_is_null branch August 29, 2023 15:58
@anth0d
Copy link

anth0d commented Dec 28, 2023

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
Copy link
Contributor

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: #538, please feel free to chime in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants