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

feat(opensearchservice): L2 properties for offPeakWindowOptions and softwareUpdateOptions #26403

Merged
merged 11 commits into from
Jul 27, 2023

Conversation

lpizzinidev
Copy link
Contributor

@lpizzinidev lpizzinidev commented Jul 18, 2023

The OffPeakWindowOptions and SoftwareUpdateOptions are supported by OpenSearch Domain, but not by the CDK high-level construct.

This change adds the corresponding properties to the Domain construct:

const domain = new Domain(this, 'Domain', {
  version: EngineVersion.OPENSEARCH_1_3,
  offPeakWindowEnabled: true, // can be omitted if offPeakWindowStart is set
  offPeakWindowStart: {
    hours: 20,
    minutes: 0,
  },
  enableAutoSoftwareUpdate: true,
});

Closes #26388.


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

@aws-cdk-automation aws-cdk-automation requested a review from a team July 18, 2023 12:18
@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK labels Jul 18, 2023
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 18, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good @lpizzinidev! Just a couple comments about defaults

packages/aws-cdk-lib/aws-opensearchservice/lib/domain.ts Outdated Show resolved Hide resolved
* Options for a domain's off-peak window, during which OpenSearch Service can perform mandatory
* configuration changes on the domain.
*/
readonly offPeakWindowOptions?: OffPeakWindowOptions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specifically the default for this prop may be confusing. looks like the default should be that it is configured by default and cannot be turned off, but still, what will the default window be?

Off-peak windows were introduced on February 16, 2023. All domains created before this date have the off-peak window disabled by default. You must manually enable and configure the off-peak window for these domains. All domains created after this date will have the off-peak window enabled by default. You can't disable the off-peak window for a domain after it's enabled.

https://docs.aws.amazon.com/opensearch-service/latest/developerguide/off-peak.html

Copy link
Contributor Author

@lpizzinidev lpizzinidev Jul 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defaulted it to 00:00 UTC

If you don't specify a custom window start time, it defaults to 00:00 UTC.

https://docs.aws.amazon.com/opensearch-service/latest/developerguide/off-peak.html (in Enabling the off-peak window > CLI)

@mergify mergify bot dismissed kaizencc’s stale review July 22, 2023 13:12

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 22, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lpizzinidev, a couple more comments

/**
* Options for configuring service software updates for a domain.
*/
export interface SoftwareUpdateOptions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont think we need an interface for this, lets default to a flatter API. Think we should just expose a boolean enableAutoSoftwareUpdate

Comment on lines 362 to 375
export interface OffPeakWindowOptions {
/**
* Specifies whether off-peak window settings are enabled for the domain.
*
* @default - true
*/
readonly enabled?: boolean;

/**
* Off-peak window settings for the domain.
*
* @default - default window start time is 00:00 UTC
*/
readonly offPeakWindow?: OffPeakWindow;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this to be an interface either. We should be able to get away with just offPeakWindow?: OffPeakWindow. If its specified, users probably want it to be enabled as well. In general I like to default to a single prop rather than specifying interfaces.

@@ -1558,6 +1632,23 @@ export class Domain extends DomainBase implements IDomain, ec2.IConnectable {
}
}

let offPeakWindowOptions: OffPeakWindowOptions = {
enabled: props.offPeakWindowOptions?.enabled ?? true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All domains created before this date have the off-peak window disabled by default.

So we can't just default to true here, as that will represent a behavior change to existing domains. Rather, without setting offPeakWindow (i.e. offPeakWindow: undefined) what we have to do is document the service-level behavior (enabled post 2/16/23, disabled prior). Then, we just pass undefined into the L1 and let the service handle the default.

@mergify mergify bot dismissed kaizencc’s stale review July 25, 2023 07:53

Pull request has been modified.

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies @lpizzinidev I feel like I've been giving you half-baked reviews on this PR. But this last little redesign should be enough I think. Thanks for the quick turnarounds.

* You can't disable the off-peak window for a domain after it's enabled.
*
* @see https://docs.aws.amazon.com/it_it/AWSCloudFormation/latest/UserGuide/aws-properties-opensearchservice-domain-offpeakwindow.html
* @default - no off-peak window will be configured
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @default - no off-peak window will be configured
* @default - disabled for domains created before February 16, 2023. enabled for domains created after.

Comment on lines 1687 to 1695
offPeakWindowOptions: props.offPeakWindow ? {
enabled: true,
offPeakWindow: {
windowStartTime: props.offPeakWindow.windowStartTime ?? {
hours: 22,
minutes: 0,
},
},
} : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the reason why I'm not in love with this API yet is because to specify offPeakWindow with the given defaults we have to do:

offPeakWindow: {},

That's not very intuitive. Which now makes me understand why enabled was a prop in the first place :). Still, though, this potential experience to get the default window isn't great either:

offPeakWindow: {
  enabled: true,
},

It feels weird to supply an object with just a boolean prop. So I suggest we do the following:

offPeakWindowEnabled: true,
// if people want to customize the window
// offPeakWindowStart: {
//  hours: 22,
//  minutes: 0,
// },

I think having two separate properties on the base construct is the way to go and what we've generally done in the past for similar situations.

Of course, the default for offPeakWindowEnabled is true if offPeakWindowStart is set, false otherwise.

@mergify mergify bot dismissed kaizencc’s stale review July 26, 2023 07:09

Pull request has been modified.

@kaizencc kaizencc changed the title feat(opensearchservice): added support for offPeakWindowOptions and softwareUpdateOptions properties feat(opensearchservice): L2 properties for offPeakWindowOptions and softwareUpdateOptions Jul 26, 2023
kaizencc
kaizencc previously approved these changes Jul 26, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lpizzinidev! Just added one last thing that allows offPeakWindowEnabled to be omitted if you specify offPeakWindowStart. Thanks for the contribution!

@mergify
Copy link
Contributor

mergify bot commented Jul 26, 2023

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 mergify bot dismissed kaizencc’s stale review July 27, 2023 17:50

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Jul 27, 2023

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 16ba32f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 02e8d58 into aws:main Jul 27, 2023
7 checks passed
@mergify
Copy link
Contributor

mergify bot commented Jul 27, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opensearch: High Level Constructs For OpenSearchOffPeakWindowOptions and Software Update Options Feature
3 participants