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

aws-opensearchservice: cannot disable logging #29200

Closed
sigJoe opened this issue Feb 21, 2024 · 7 comments · Fixed by #29202
Closed

aws-opensearchservice: cannot disable logging #29200

sigJoe opened this issue Feb 21, 2024 · 7 comments · Fixed by #29202
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 p2

Comments

@sigJoe
Copy link
Contributor

sigJoe commented Feb 21, 2024

Describe the bug

Enabling logging on OpenSearch clusters will populate LogPublishingOptions in the output template, but disabling logging leaves LogPublishingOptions empty so the cluster will retain the previous logging config. I don't see any documentation saying that enabling logging is a one-way trip, so I think this is a bug.

Specifically, I'm talking about these logging props:

new es.Domain(scope, id, {
 ...
 logging: {
    slowSearchLogEnabled: false,
    appLogEnabled: false,
    slowIndexLogEnabled: false,
  },
})

Expected Behavior

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

Current Behavior

    "LogPublishingOptions": {},

Reproduction Steps

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 }

Possible Solution

If you don't need the current behaviour, then have it perform the expected behaviour from above.

If there is a good reason it works that way or you want backwards-compatibility, then maybe have logging props like appLogEnabled default to some non-boolean value (e.g. -1). If left default (not specified by user), then LogPublishingOptions can be empty. If the user explicitly specifies either true or false, then populate the LogPublishingOptions to enable or disable logging as appropriate.

Additional Information/Context

An environment hit the limit on cloudwatch resource policies, so I'm working on a change related to the new (much appreciated) suppressLogsResourcePolicy prop. The way things are currently, I'll have to manually (shudder) disable logging on a cluster to clear up policy space before I can deploy the new multi-cluster logging resource policy.

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 21, 2024
@github-actions github-actions bot added the @aws-cdk/aws-opensearch Related to the @aws-cdk/aws-opensearchservice package label Feb 21, 2024
@pahud
Copy link
Contributor

pahud commented Feb 21, 2024

I am not sure if this is a bug from cloudformation but we should note this in the CDK doc. Thank you for the report.

@pahud pahud added p2 effort/medium Medium work item – several days of effort labels Feb 21, 2024
@pahud pahud removed the needs-triage This issue or PR still needs to be triaged. label Feb 21, 2024
@mergify mergify bot closed this as completed in #29202 Feb 21, 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.

@sigJoe
Copy link
Contributor Author

sigJoe commented Feb 21, 2024

@pahud

I think perhaps there was a miscommunication here, as I feel the PR doesn't accurately describe the situation and actually increases confusion. The PR creates the assumption that it is possible to disable OpenSearch logging if you set the parameters explicitly to false, but the CDK in its current form does not behave that way.

Take a look at

for example. If true, it populates logPublishingOptions. If false, it does not populate the config.

Cloudformation needs THOSE logPublishingOptions to contain "enabled: false" for it to have any effect

@pahud
Copy link
Contributor

pahud commented Feb 21, 2024

@SigniantJoe

Do you mean if I specify this way

new opensearch.Domain(this, 'Domain', {
	version: opensearch.EngineVersion.OPENSEARCH_2_11,
	logging: {
		slowSearchLogEnabled: false,
		appLogEnabled: false,
		slowIndexLogEnabled: false,
	},
})

it should synthesize to

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

but we are actually getting

"LogPublishingOptions": {},

If yes, I agree with you.

My question is - if I have enabled it like

new opensearch.Domain(this, 'Domain', {
	version: opensearch.EngineVersion.OPENSEARCH_2_11,
	logging: {
		slowSearchLogEnabled: true,
		appLogEnabled: true,
		slowIndexLogEnabled: true,
	},
})

Now I need to disable them and update to

new opensearch.Domain(this, 'Domain', {
	version: opensearch.EngineVersion.OPENSEARCH_2_11,
})

Will they be disabled from the enabled state? If not, I think #29202 is still relevant?

@pahud pahud reopened this Feb 21, 2024
@pahud
Copy link
Contributor

pahud commented Feb 21, 2024

Reopen this issue as it's still relevant.

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.

GavinZZ pushed a commit that referenced this issue Feb 22, 2024
### Issue # (if applicable)

Closes #29200

### Reason for this change

Disabling the logging from the enabled status requires an explicit `false`. This PR adds the description in the doc.

### Description of changes



### Description of how you validated changes



### 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*
@sigJoe
Copy link
Contributor Author

sigJoe commented Feb 22, 2024

@pahud you are correct in your latest interpretation about getting empty logPublishingOptions when it should be populated. This means that if you deploy with logging enabled then try to deploy with logging disabled, logging will remain enabled.

I actually made my own PR with unit tests for this, which I think explains the issue: #29205 It's currently stuck waiting on me to updating snapshots, which I'm looking into

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 p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants