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

Deployment from scratch fails with latest changes in backend Lambda log group creation #1190

Closed
dlpzx opened this issue Apr 18, 2024 · 1 comment
Assignees
Labels

Comments

@dlpzx
Copy link
Contributor

dlpzx commented Apr 18, 2024

Describe the bug

When deploying data.all in a fresh account using the latest code in main the CICD fails in the deployment of the backend stack. The failure comes from the creation of CloudWatch alarms for the log-groups of the backend lambdas.
In this PR #1134 the behavior on how the log groups for the Lambda functions changes, instead of letting Lambda create the groups automatically, it explicitly checks for the log group name and then performs the check. Unfortunately, the output of from_log_group_name is not None even when the log group does not exist.

       esproxy_loggroup = logs.LogGroup.from_log_group_name(
            self, 'esproxyloggroup', f'/aws/lambda/{resource_prefix}-{envname}-esproxy'
        )
....

        self.elasticsearch_proxy_handler = _lambda.DockerImageFunction(
            self,
            'ElasticSearchProxyHandler',
            function_name=f'{resource_prefix}-{envname}-esproxy',
            log_group=esproxy_loggroup
            if esproxy_loggroup is not None
            else logs.LogGroup(
                self, 'esproxyloggroup', log_group_name=f'/aws/lambda/{resource_prefix}-{envname}-esproxy'
            ),

How to Reproduce

Deploy data.all with the current changes including #1134

Expected behavior

Smooth behavior, log groups created and cloudwatch alarms created referencing the created log groups

Your project

No response

Screenshots

No response

OS

n/a

Python version

n/a

AWS data.all version

2.3+additional PRs

Additional context

No response

@dlpzx dlpzx self-assigned this Apr 18, 2024
@dlpzx
Copy link
Contributor Author

dlpzx commented Apr 18, 2024

From testing we can conclude:

  1. If we do not specify the log_group in the Lambda creation, it will automatically create a LogGroup for the Lambda using the name of the function, which is exactly what we create using logs.LogGroup
  2. If we create the logs.LogGroup but it already exists, the whole stack fails creation
  3. if we create the logs.LogGroup and it did not exist before, it succeeds

I think because of number 2, the check was added, however the check does not effectively check the existence of a previous stack.

I am inclined to revert the changes and go for the previous setup in which we did not define the log-group and use the automatically created ones (like in 1). But maybe there is something that I am missing @mourya-33

dlpzx added a commit that referenced this issue Apr 19, 2024
### Feature or Bugfix
- Bugfix

### Detail
In #1134 log groups are created explicitly for each Lambda in the
backend, which results in errors when the log-groups do not exist. (see
#1190 )

This PR removes the `log_group` parameter from the definition of the
Lambdas. It reverts back to the previous (2.3) settings.

Tested: on a fresh deployment, making sure that no log-groups were
previously created, the stack creates the log-groups with the Lambda
name. For example `/aws/lambda/<resource_prefix>-<env_name>-esproxy`.

Update: we need to explicitly define the log groups to avoid the log
retention lambdas and their roles and policies that CDK automatically
creates. So to cope with existing groups and non-existing groups this PR
adds a CfnCondition to the creation of the log-group

### Relates
- #1134

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/). `n/a`

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
@dlpzx dlpzx closed this as completed Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant