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

opensearchservice: cannot disable logging (still) #29294

Closed
sigJoe opened this issue Feb 28, 2024 · 2 comments · May be fixed by NOUIY/aws-solutions-constructs#98, NOUIY/aws-solutions-constructs#99 or NOUIY/aws-solutions-constructs#101
Labels
@aws-cdk/aws-opensearch Related to the @aws-cdk/aws-opensearchservice package bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@sigJoe
Copy link
Contributor

sigJoe commented Feb 28, 2024

Describe the bug

Related to #29200, although that one was closed due to a misunderstanding. The documentation was updated to suggest that things work the way I would expect, but that is not how the CDK currently behaves.

The problem is that if you enable logging (e.g. appLogEnabled: true) on an OpenSearch Cluster then try to disable logging (e.g. appLogEnabled: false), the OpenSearch Cluster will still have logging enabled.

Specifically, I'm talking about these configuration properties:

logging: {
  slowSearchLogEnabled: false,
  appLogEnabled: false,
  slowIndexLogEnabled: false,
  auditLogEnabled: false,
}

Expected Behavior

When told to disable logging, CDK should populate LogPublishingOptions with configuration to disable logging:

    "LogPublishingOptions": {
        "ES_APPLICATION_LOGS": {
            "Enabled": false
        },
        "SEARCH_SLOW_LOGS": {
            "Enabled": false
        },
        "INDEX_SLOW_LOGS": {
            "Enabled": false
        }
    },

Current Behavior

When told to disable logging, CDK will leave LogPublishingOptions empty:

    "LogPublishingOptions": {},

This means that CloudFormation will not enforce any configuration and OpenSearch logging configuration remains in its previous state.

Reproduction Steps

Deploy a template such as this:

const { Stack, Duration } = require('aws-cdk-lib');
const es = require('aws-cdk-lib/aws-opensearchservice');

class PocStack extends Stack {
  constructor(scope, id, props) {
    super(scope, id, props);

    const cluster = new es.Domain(this, 'OpenSearchCluster', {
      domainName: 'test-opensearch-cluster',
      version: es.EngineVersion.OPENSEARCH_2_11,
      capacity: {
        dataNodes: 1,
        dataNodeInstanceType: "t4g.medium.search",
      },
      ebs: {
        volumeSize: 10,
      },
      logging: {
        appLogEnabled: true,
      }
    });
  }
}

module.exports = { PocStack }

Then set appLogEnabled: false and observe that the OpenSearch cluster still has logging enabled.

Possible Solution

PR #29205 makes it so that:

  1. If a logging parameter (e.g. appLogEnabled) is explicitly enabled, LogPublishingOptions will be configured to enable that type of logging
  2. If a logging parameter (e.g. appLogEnabled) is explicitly disabled, LogPublishingOptions will be configured to disable that type of logging
  3. If a logging parameter (e.g. appLogEnabled) is left undefined, then LogPublishing Options will omit configuration for that type of logging (for backwards compatibility)

Additional Information/Context

No response

CDK CLI Version

2.128.0

Framework Version

No response

Node.js Version

v18.18.2

OS

OSX

Language

TypeScript

Language Version

No response

Other information

No response

@sigJoe sigJoe added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 28, 2024
@github-actions github-actions bot added the @aws-cdk/aws-opensearch Related to the @aws-cdk/aws-opensearchservice package label Feb 28, 2024
@pahud
Copy link
Contributor

pahud commented Feb 28, 2024

Yes! Thank you for the PR and clarifying.

@pahud pahud added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 28, 2024
@pahud pahud changed the title aws-opensearchservice: cannot disable logging (still) opensearchservice: cannot disable logging (still) Feb 28, 2024
GavinZZ added a commit that referenced this issue Mar 18, 2024
### Issue # (if applicable)

#29294

### Reason for this change

Currently cannot disable opensearch logging

### Description of changes

If log parameters are explicitly set to false rather than undefined, it
populates the logPublishingOptions with config to disable that logging

### Description of how you validated changes

I added unit tests, although to be honest jest is giving me lots of
trouble and I'm out of time for the day so I'll just create this pR and
see what happens. Fingers crossed the PR test check is clean and I can
pretend I know what I'm doing.

### Checklist
- [x] My code adheres to the [CONTRIBUTING
GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
[DESIGN
GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license*

---------

Co-authored-by: GZ <yuanhaoz@amazon.com>
@sigJoe sigJoe closed this as completed Mar 19, 2024
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

ahammond pushed a commit to ahammond/aws-cdk that referenced this issue Mar 26, 2024
### Issue # (if applicable)

aws#29294

### Reason for this change

Currently cannot disable opensearch logging

### Description of changes

If log parameters are explicitly set to false rather than undefined, it
populates the logPublishingOptions with config to disable that logging

### Description of how you validated changes

I added unit tests, although to be honest jest is giving me lots of
trouble and I'm out of time for the day so I'll just create this pR and
see what happens. Fingers crossed the PR test check is clean and I can
pretend I know what I'm doing.

### Checklist
- [x] My code adheres to the [CONTRIBUTING
GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
[DESIGN
GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license*

---------

Co-authored-by: GZ <yuanhaoz@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-opensearch Related to the @aws-cdk/aws-opensearchservice package bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
2 participants