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

After upgrade to 4.0.0 get error "extraneous key [DatadogCredentials] is not permitted" #158

Open
sunshineo opened this issue Aug 5, 2021 · 33 comments
Labels
kind/bug Bug related issue stale Stale - Bot reminder

Comments

@sunshineo
Copy link

We are using serverless, here is our yml

resources:
  Resources:
    DatadogMonitorP1:
      Type: Datadog::Monitors::Monitor
      Properties:
        Type: log alert
        Query: 'logs("service:${self:service} env:${env:CI_COMMIT_BRANCH} status:error").index("main").rollup("count").last("1h") > 100'
        Name: ${self:service}-${env:CI_COMMIT_BRANCH}-p1
        Message: '{{#is_alert}} ALERT! {{event.text}} {{/is_alert}} ${self:custom.datadogP1.${env:CI_COMMIT_BRANCH}}'
        DatadogCredentials:
          ApiKey: ${self:custom.datadogSecrets.apiKey}
          ApplicationKey: ${self:custom.datadogSecrets.applicationKey}

What is the new way to do this? I did not find any documentation

@allistera
Copy link

Hi,

I also ran into this issue. It seems that DatadogCredentials has been moved from being a property to be a registry option:

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/registry-register.html?icmpid=docs_cfn_console#registry-set-configuration

As part of the 2.x release: #142

@sunshineo
Copy link
Author

@allistera Thank you for the info. I still do not fully get it but my understanding so far: when register Datadog::Monitors::Monitor, we provide ApiKey and ApplicationKey, and in serverless yml we just remove those?

@allistera
Copy link

Yes that is correct. There is an example in README.md of the data required when registering the type:

{
  "DatadogCredentials": {
      "ApiKey": "{{resolve:secretsmanager:MySecret:SecretString:SecretAPIKey}}",
      "ApplicationKey": "{{resolve:secretsmanager:MySecret:SecretString:SecretAppKey}}"
  }
}

In this example it pulls in the keys from Secrets Manager - although you could hard-code it at this place it is highly recommended to store them within Secrets Manager.

The primary advantage of this approach is that you do not need to give your users (those that area creating CF stacks) credentials.

So the setup approach would be:

  1. Create Secrets Manager Key containing the APP and API key
  2. Register Type. Using above default config
  3. Create CF stack using 'Datadog::Monitors::Monitor' resource

@sunshineo
Copy link
Author

We believe it is a mistake to reject the key and fail. It made the deployments not backward compatible. It is kind of impossible for us to upgrade the version and change all project templates currently providing the DatadogCredentials at the same time. Can we accept the key and ignore it? Is this something DataDog team can fix or AWS screwed up?

@allistera
Copy link

Hi @sunshineo

Just a heads up I don't work at Datadog so can't speak on their behalf. I am also in the same boat with 60+ AWS accounts needing updated.

It is worth of note that the project seems to follow semver which permits backboard compatible breaking changes for the 1.x.x to 2.x.x release.

One possible solution - and one we may adopt is to centralise the API/APP key in one account as Secrets Manager allows cross account access

@github-actions
Copy link

Thanks for your contribution!

This issue has been automatically marked as stale because it has not had activity in the last 30 days. Note that the issue will not be automatically closed, but this notification will remind us to investigate why there's been inactivity. Thank you for participating in the Datadog open source community.

If you would like this issue to remain open:

  1. Verify that you can still reproduce the issue in the latest version of this project.

  2. Comment that the issue is still reproducible and include updated details requested in the issue template.

@github-actions github-actions bot added the stale Stale - Bot reminder label Oct 25, 2021
@sunshineo
Copy link
Author

I have opened a ticket to Datadog and hopefully someone will take a look

@skarimo
Copy link
Member

skarimo commented Jan 21, 2022

Hi all, apologies for the delayed response on this issue.

The steps for setting up the resources from aws public registry are outlined here

  • Resource example: link

  • See additional docs on public extensions usage in aws docs

If you are privately registering the resource, please make sure to set the TypeConfiguration after upgrading to the latest version https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/registry-register.html#registry-set-configuration

The monitors resource 4.x.x release is not backward compatible in this case with the previous major versions as we switched over to use TypeConfiguration for added security as the api/app keys were not hidden. To upgrade your existing resources to new extension versions, there are 2 options at the moment:

  1. Recreate the existing stacks with the new version - granted this will delete, for example, monitors previous history
  2. Delete the stack while retaining the resources and reimport them: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resource-import-existing-stack.html

@elruwen
Copy link
Contributor

elruwen commented Jan 24, 2022

If you would put the major version into the resource type name, then people could move gradually to the new resources from the private registry. AWS does the same, see for example AWS::ElasticLoadBalancingV2.

@sunshineo
Copy link
Author

@elruwen Very good idea

@therve
Copy link
Contributor

therve commented Jan 25, 2022

This is unfortunately too late for that approach.

@elruwen
Copy link
Contributor

elruwen commented Jan 27, 2022

Is it? You can still publish Datadog::Monitors::Monitor as Datadog::Monitors::MonitorV4, too.
Then people like me can install that one and migrate one monitor at a time across.

I got over 100 monitors, owned by various teams.

Manually deleting all those monitors from cloudformation, then uninstall the extension and then reimport them takes quite a bit of time. And while that is in progress, people need to create new monitors by hand. And temporarily deleting all those monitors is also not really an option.

@therve
Copy link
Contributor

therve commented Jan 27, 2022

The problem is not just the monitor resource though, we have 5 other resources, and we'd need to publish on 26 regions.
You should be able to reimport the resources, we'll look into instructions how to make that happen.

@sunshineo
Copy link
Author

@therve Have you received any complains about the other 4 resources? Have you received complains from customers from all 26 regions or just North America?
Have a MonitorV4 on North America will unblock us. (We are paying customer)
But also looking forward to your reimport instruction (if it can switch 100+ services smoothly)

@elruwen
Copy link
Contributor

elruwen commented Feb 17, 2022

The problem is not just the monitor resource though, we have 5 other resources, and we'd need to publish on 26 regions. You should be able to reimport the resources, we'll look into instructions how to make that happen.

Any updates on that?

@skarimo
Copy link
Member

skarimo commented Feb 23, 2022

Hi all, we have the following branch up to help ease the upgrade process: #200
The above PR will be targeted for v3.1.0

S3 link: s3://datadog-cloudformation-resources/datadog-monitors-monitor/datadog-monitors-monitor-3.1.0b2.zip

The 2 main changes in the PR are:

  1. Support both TypeConfiguration and DatadogCredentials in single version
  2. Handle previous resource id format for the monitor resource.

Upgrade instructions for resource major versions v2/v3 --> v3.1.0 --> v4:

  1. Register the above version using the steps outline here and set the newly registered resource version as default. If you would like to manually build and submit the resource using cfn cli, checkout the branch and follow the steps outlined here
  2. Set the TypeConfiguration for the newly registered resource version. AWS docs for setting Type Configuration:
{
  "DatadogCredentials": {
      "ApiKey": "{{resolve:secretsmanager:MySecret:SecretString:SecretAPIKey}}",
      "ApplicationKey": "{{resolve:secretsmanager:MySecret:SecretString:SecretAppKey}}"
  }
}
  1. Remove the DatadogCredentials property from the monitor resource definition.
  2. Apply the changes.
    • Note: When upgrading from 2.x.x -> v3.1.0, the first apply will fail with an error Handler returned a new physical resource identifier during update.. This is expected as the format of the resources physical id changes from str -> int. However the second apply will succeed.
  3. You can now upgrade to latest major version v4.

@sunshineo
Copy link
Author

sunshineo commented Apr 21, 2022

@skarimo What I see in the 3.1.0b1 schema.json and after register the type on AWS, the configuration json shows:

"DatadogCredentials": {
    "description": "Credentials for the Datadog API",
    "properties": {
        "ApiKey": {
            "description": "Datadog API key",
            "type": "string"
        },
        "ApplicationKey": {
            "description": "Datadog application key",
            "type": "string"
        },
        "ApiURL": {
            "description": "Datadog API URL (defaults to https://api.datadoghq.com) Use https://api.datadoghq.eu for EU accounts.",
            "type": "string"
        }
    },
    "required": [
        "ApiKey",
        "ApplicationKey"
    ],
    "type": "object",
    "additionalProperties": false
}

What should we do?

@sunshineo
Copy link
Author

I think I understand now. We provide default value for these using the type configuration. Then we can remove from each service. And then upgrade to v4 and no more of these
image

@sunshineo
Copy link
Author

sunshineo commented Apr 21, 2022

It's not working for us. We simply see "Internal Failure" when try to update the monitor for the deploy after remove the credentials from the service. No details of the error was returned from Datadog
image

@sunshineo
Copy link
Author

I cannot even rollback and I cannot even delete the stack. This is very broken

serverless remove --stage develop
Serverless: DOTENV: Could not find .env file.
Serverless: Getting all objects in S3 bucket...
Serverless: Removing objects in S3 bucket...
Serverless: Removing Stack...
Serverless: Checking Stack delete progress...
....................
Serverless: Operation failed!
Serverless: View the full error output: https://us-west-2.console.aws.amazon.com/cloudformation/home?region=us-west-2#/stack/detail?stackId=dynamic-deploy-js-develop

 Serverless Error ----------------------------------------

  An error occurred: DatadogMonitorP3 - Internal Failure.

  Get Support --------------------------------------------
     Docs:          docs.serverless.com
     Bugs:          github.com/serverless/serverless/issues
     Issues:        forum.serverless.com

  Your Environment Information ---------------------------
     Operating System:          darwin
     Node Version:              14.17.5
     Framework Version:         2.72.3 (local)
     Plugin Version:            5.5.4
     SDK Version:               4.3.2
     Components Version:        3.18.2

@skarimo
Copy link
Member

skarimo commented Apr 21, 2022

@sunshineo Could you make sure the type configuration is set up properly? It doesn't seem to conform to what aws expects. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/dynamic-references.html#dynamic-references-secretsmanager
is it suppose to be "{{resolve:secretsmanager:datadog-secrets:SecretString:apiKey}}"?

Try setting the TypeConfiguration to an empty object {} and adding back the DatadogCredentials property does that succeed?

@sunshineo
Copy link
Author

Thank you @skarimo . You are right. We need that "SecretString" exactly like that. It's a required hard coded thing.

secret-string
Currently, the only supported value is SecretString. The default is SecretString.

@casper-urhed-wcar
Copy link

I must say I have been disappointed how the version upgrades have been handled. We jumped on this pretty early in my organization and we are encountering these issues as well now when trying to upgrade. It's a not a big pain if you have a couple of resources in a single stack, but in our case we are talking dozens of CF stacks deployed across dozens of AWS and AWS CN accounts, handled by multiple teams. When we roll out the new version in an AWS account suddenly nobody can deploy their stacks due to a breaking change - and there is no way of doing a smooth transition of this, save for forking the resource type and making our own version if it with a different CloudFormation TypeName

The planned change in #200 is a step in the right direction and we very much appreciate that effort, and hope it will be available soon.

We would have preferred if you had

  1. Tried to keep the CloudFormation types backwards compatible to the extent where it's possible. The two non-backwards compatible changes that I know of is the issue mentioned here, and also early on for the Monitor resource type when the ApiURL attribute key name was changed.
  2. If really needing to do a breaking change/new major version, change the CloudFormation TypeName so that your consumers can do a smooth transition by having both the old type and the new available at the same time. For example from "Datadog::Monitors::Monitor" to "Datadog::Monitors::MonitorV2".

@skarimo
Copy link
Member

skarimo commented Apr 22, 2022

@sunshineo Great, were you able to update the resources successfully?

@sunshineo
Copy link
Author

@skarimo We have updated to 3.0.1b and removed the credentials from several of our services and it seems good. But it will take some time till all services remove the credentials. Then we will try to go to v4

@dogfish182
Copy link

dogfish182 commented Dec 6, 2022

it's possible to configure the datadog private extensions using Cfn/CDK like this

new CfnTypeActivation(
                this,
                `CfnTypeActivationDatadog${extension.name}`,
                /* all optional props */ {
                    autoUpdate: true,
                    majorVersion: extension.majorVersion,
                    publicTypeArn: extension.arn,
                    publisherId: extension.publisher,
                    executionRoleArn: extension.executionRoleArn,
                },
            );

but it seems NOT possible to then perform a SetTypeConfiguration via cdk or cloudformation directly. how are people automating this?

irritatingly
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/registry-register.html#registry-set-configuration
this takes you to a welcome page for cloud formation and I see no way to directly modify the config of the enabled extension to add the secrets param.

I guess downgrading is the way to go as I don't want to use CLI to enable this across my accounts?

@elruwen
Copy link
Contributor

elruwen commented Dec 6, 2022

Hi!

There is no other way that doing the CLI. Not even Terraform. Please create an AWS support ticket so they can add you to the internal feature request. There is already one ;)

We are doing it via CLI for now :(

@elruwen
Copy link
Contributor

elruwen commented Dec 7, 2022

@skarimo you can also create an AWS support ticket please?

@dogfish182
Copy link

dogfish182 commented Dec 7, 2022

Hi!

There is no other way that doing the CLI. Not even Terraform. Please create an AWS support ticket so they can add you to the internal feature request. There is already one ;)

We are doing it via CLI for now :(

Thanks, I've created a ticket for this.
Currently I think I will enable one time via cli, alternative being a custom resource to deal with this until it's supported by cloudformation.
https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_SetTypeConfiguration.html

For anyone else that winds up here reading this thread, the issue that would make this changeable via cloudformation/CDK is here and could use a thumbs up.
aws-cloudformation/cloudformation-coverage-roadmap#996

@elruwen
Copy link
Contributor

elruwen commented Dec 7, 2022

Yeah I was actually thinking about creating my own resource type for that ;)

@mtrspringer
Copy link

@dogfish182 i'm currently attempting to use the CfnHookTypeConfig resource. I can see in CloudTrail that it's trying to call SetTypeConfiguration, but i haven't got it to actually configure the types yet.

@dogfish182
Copy link

@dogfish182 i'm currently attempting to use the CfnHookTypeConfig resource. I can see in CloudTrail that it's trying to call SetTypeConfiguration, but i haven't got it to actually configure the types yet.

It seems like this is the resource for configuring cloudformation hooks though right? Don't think it actually relates to setting type config on an activated extension?

@mtrspringer
Copy link

@dogfish182 ah yes my mistake it must only work for hook types, which is probably why i was having trouble getting it to work. looks like a custom resource is the way to go then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Bug related issue stale Stale - Bot reminder
Projects
None yet
Development

No branches or pull requests

8 participants