fix(opensearchservice): access denied when creating a new domain in regions without cognito support by MikeJamesWhite · Pull Request #21395 · aws/aws-cdk

MikeJamesWhite · 2022-07-30T18:57:17Z

Currently the CognitoOptions attribute is always populated in the CloudFormation output when creating a new OpenSearch Domain, even if cognitoDashboardsAuth is not passed into the constructor.

Including this attribute, even with just the default body of { "Enabled": false }, causes deployments to fail in regions where Cognito isn't supported (e.g. cn-north-1), with the message: You don't have permissions to integrate with Cognito. Contact your admin if you need help. This change fixes this behavior by completely omitting the attribute if cognitoDashboardsAuth is not passed into the constructor.

I also updated the package.json file to fix several build warnings, and the README and default Rosetta fixture to fix a build error and make the docs align with the style recommendations in the contribution guidelines.

NOTE: Current snapshots are updated after running the integration tests, since the existing templates should no longer have CognitoOptions populated. However, the snapshot for the newly added integration test has this attribute populated.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-07-30T18:57:23Z

MikeJamesWhite · 2022-07-31T13:24:06Z

Update: I've sorted out access to an account for running the integration tests, so I'll run them myself and update the PR (reviewer will no longer need to execute them as indicated in the original PR body).

I also found that the documentation currently fails to build a couple of code examples -- dove a bit deeper and noticed that the README is not following the recommendations from the contributions guide. Since I'm adding to the README in this PR anyway, I'm also going to add a commit to update existing code examples to follow the guidelines, and make them all build successfully in the process.

TheRealAmazonKendra

Thank you for your contribution. Just a few small comments inline.

TheRealAmazonKendra · 2022-08-02T06:26:21Z

packages/@aws-cdk/aws-opensearchservice/rosetta/default.ts-fixture

 import * as iam from '@aws-cdk/aws-iam';

-class Fixture extends Stack {           
+class Fixture extends core.Stack {


These updates aren't really necessary.

While not strictly necessary, I thought it made sense to update it to follow the documentation recommendations in the contribution guidelines. Specifically Types from other modules should be qualified: and Types from the documented module should be un-qualified:

If you want to change formatting of the documentation, please do it as a separate PR. Large PRs with changes that get lost in formatting changes will not receive the same level of detailed feedback because it's harder to do with so much extra noise. We certainly don't mind better documentation!

Rosetta files, however, should not be manually touched. They have to do with how we translate the documentation into other languages and between v1 and v2.

Alright, I'll split it out into a new PR, though this does mean that the documentation build will continue to fail (it was failing before I made any changes due inconsistent use of "Domain" vs "opensearch.Domain" added in previous PRs).

That being said, in advance I'm just wondering about this: "Rosetta files, however, should not be manually touched". How is it expected to adjust how imports are represented in the documentation without adjusting the Rosetta file? I'm new to this, so may be missing something, but seems like this is necessary?

If you're adding new imports it's fine, but the formatting we use for importing from core is consistent across all packages. No lie, I don't know why that's our convention and why we do it that way, but it's consistent and we should keep it that way.

Also, there's a lot of stuff that we autogenerate in various processes through scripts and JSII. To your point about build failures from previous documentation inconsistency, if it's small, it's fine to include with other stuff but in general I'd just open the PR to make that fix first and then do the code change in a subsequent PR. Doc changes usually get merged pretty fast.

Makes sense -- thanks for the feedback!

So, yes, you can actually update the rosetta file, but you shouldn't change the style of imports. I misspoke earlier when I said they shouldn't be touched at all.

yarn.lock

TheRealAmazonKendra · 2022-08-02T06:29:09Z

packages/@aws-cdk/aws-opensearchservice/package.json

    "@aws-cdk/integ-runner": "0.0.0",
    "@aws-cdk/cfn2ts": "0.0.0",
    "@aws-cdk/pkglint": "0.0.0",
+    "@aws-cdk/aws-certificatemanager": "0.0.0",


Why are all these dependencies now necessary? I'm not seeing a change that would have necessitated it.

Each of these was causing a build warning -- they're not strictly necessary, but it also seemed odd for the build to produce so many warnings.

TheRealAmazonKendra · 2022-08-02T06:30:20Z

packages/@aws-cdk/aws-opensearchservice/package.json

+    "@aws-cdk/aws-secretsmanager": "0.0.0",
+    "@aws-cdk/core": "0.0.0",
+    "@aws-cdk/custom-resources": "0.0.0",
+    "@aws-cdk/integ-tests": "0.0.0",


As far as I can tell, this should be the only additional dependency needed.

TheRealAmazonKendra · 2022-08-02T06:30:41Z

packages/@aws-cdk/aws-opensearchservice/package.json

    "@aws-cdk/aws-secretsmanager": "0.0.0",
    "@aws-cdk/core": "0.0.0",
    "@aws-cdk/custom-resources": "0.0.0",
+    "@aws-cdk/integ-tests": "0.0.0",


But not needed here.

Is there a guideline of how internal dependencies should be added? From what I can see, it seems to be standard to add them as peerDependencies, but if not added as devDependencies as well then the build produces warnings, which doesn't seem right?

hmm. I'm not actually sure if we have this documented anywhere but basically, if it doesn't produce an error, don't add it.

Alright, will change this back

After taking a look, I think the warnings may actually be the result of these being declared as both peer dependencies and normal dependencies. But I'll check that theory in another PR with the documentation updates.

Keep in mind some of why we do these oddities is that we translate these libraries into a half dozen languages and between v1 and v2 structure/conventions. Some of it might not align with what you've seen elsewhere because of that.

Pull request has been modified.

TheRealAmazonKendra

Additional requested changes still pending.

…egions without cognito support

Pull request has been modified.

TheRealAmazonKendra

Looks good now.

mergify · 2022-08-03T02:39:19Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2022-08-03T02:43:08Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-08-03T03:41:16Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 981d8fe
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-08-03T03:41:36Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…ed by the contribution guidelines (#21453) Follow up to #21395, making documentation changes that were split out from that PR. This change updates the README file to make the docs align with the style recommendations in the contribution guidelines. Specifically, the current docs do not follow this guideline: `Types from the documented module should be un-qualified`. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added bug This issue is a bug. p2 labels Jul 30, 2022

aws-cdk-automation requested a review from a team July 30, 2022 18:57

MikeJamesWhite force-pushed the main branch from cfb6284 to 737c54b Compare July 31, 2022 20:20

TheRealAmazonKendra previously requested changes Aug 2, 2022

View reviewed changes

TheRealAmazonKendra changed the title ~~fix(opensearchservice): Access denied when creating a new Domain in regions without Cognito support (#21192)~~ fix(opensearchservice): access denied when creating a new domain in regions without cognito support Aug 2, 2022

MikeJamesWhite requested a review from TheRealAmazonKendra August 2, 2022 19:26

TheRealAmazonKendra previously requested changes Aug 2, 2022

View reviewed changes

fix(opensearchservice): access denied when creating a new domain in r…

d9912ff

…egions without cognito support

MikeJamesWhite force-pushed the main branch from 569f5e7 to d9912ff Compare August 2, 2022 21:55

MikeJamesWhite requested a review from TheRealAmazonKendra August 2, 2022 22:57

TheRealAmazonKendra approved these changes Aug 3, 2022

View reviewed changes

TheRealAmazonKendra added p1 and removed p2 labels Aug 3, 2022

Merge branch 'main' into main

981d8fe

mergify bot merged commit 0e49aed into aws:main Aug 3, 2022

MikeJamesWhite added a commit to MikeJamesWhite/aws-cdk that referenced this pull request Aug 3, 2022

Commit documentation changes split out from previous PR (aws#21395).

d23dff8

MikeJamesWhite mentioned this pull request Aug 3, 2022

docs(opensearchservice): align documentation with the style recommended by the contribution guidelines #21453

Merged

4 tasks

Conversation

MikeJamesWhite commented Jul 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Jul 30, 2022

Uh oh!

MikeJamesWhite commented Jul 31, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 3, 2022

Uh oh!

mergify bot commented Aug 3, 2022

Uh oh!

aws-cdk-automation commented Aug 3, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MikeJamesWhite commented Jul 30, 2022 •

edited

Loading

TheRealAmazonKendra Aug 2, 2022 •

edited

Loading

TheRealAmazonKendra Aug 3, 2022 •

edited

Loading