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.

Sign up for GitHub

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

Jump to bottom

Added optional AWS fields for OpenShift 4.5 AWS Install docs per BZ1865758 #31856

Closed
wants to merge 1 commit into from
Closed

Added optional AWS fields for OpenShift 4.5 AWS Install docs per BZ1865758 #31856

wants to merge 1 commit into from

Conversation

GroceryBoyJr
Copy link
Contributor

@GroceryBoyJr GroceryBoyJr commented Apr 23, 2021 •
edited

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 23, 2021
@netlify
Copy link

netlify bot commented Apr 23, 2021

Deploy preview for osdocs ready!

Built with commit e1b783e

https://deploy-preview-31856--osdocs.netlify.app

@GroceryBoyJr GroceryBoyJr changed the base branch from master to enterprise-4.5 April 23, 2021 16:04
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 23, 2021
@GroceryBoyJr
Copy link
Contributor Author

GroceryBoyJr commented Apr 23, 2021 •
edited

Describe the issue:
For KMS:
missing compute.platform.aws.rootVolume.kmsKeyARN and controlPlane.platform.aws.rootVolume.kmsKeyARN config parameters
according to https://issues.redhat.com/browse/CORS-1388 and openshift/installer#3293 ,
custom KMS key is supported by OCP 4.5, but the it is missing in the documents

For AMI:
missing compute.platform.aws.amiID and controlPlane.platform.aws.amiID config parameters
according to https://issues.redhat.com/browse/CORS-1401 and openshift/installer#3308 ,
custom AMI for machine pool is supported by OCP 4.5, but it is missing in the documents

Suggestions for improvement:
Add KMS and AMI parameter description in Table 3. Optional AWS parameters

Additional information:
I have added the following parameters to Table 3: Optional AWS parameters:
compute.platform.aws.amiID
compute.platform.aws.rootVolume.kmsKeyARN
controlPlane.platform.aws.amiID
I have not added the following parameter because it appears to be unknown to the OpenShift Installer:
controlPlane.platform.aws.rootVolume.kmsKeyARN
Please assess if this is correct or incorrect and comment in the BZ1865758 and this PR 31856*

@yunjiang29
Copy link
Contributor

yunjiang29 commented Apr 25, 2021 •
edited

@GroceryBoyJr thanks for your updates, here are some comments:

  1. for kmsKeyARN

  • it should be available for controlPlane.platform.aws.rootVolume and compute.platform.aws.rootVolume.kmsKeyARN.

    I saw you've added compute.platform.aws.rootVolume.kmsKeyARN, but controlPlane.platform.aws.rootVolume.kmsKeyARN is missing.

  • kmsKeyARN parameter is also missing in 4.6, 4.7, 4.8

  • kmsKeyARN should be added in sample install-config.yaml, you could refer to Sample customized install-config.yaml file for AWS in 4.7 doc [2]

  • The text of kmsKeyARN looks good to me, just one comment, the Values column, I think the user guide [3] is better than the API doc, WDYT?

  1. for amiID

  • it should be available for controlPlane.platform.aws.amiID, compute.platform.aws.amiID and platform.aws.amiID.

    I noticed that platform.aws.amiID is missing in your PR, you could refer to Table 3. Optional AWS parameters in 4.7 doc [1]

  • amiID should be added in sample install-config.yaml, you could refer to Sample customized install-config.yaml file for AWS in 4.7 doc [2]

  • The text of amiID looks good to me.

[1] https://docs.openshift.com/container-platform/4.7/installing/installing_aws/installing-aws-customizations.html#installation-configuration-parameters_installing-aws-customizations

[2] https://docs.openshift.com/container-platform/4.7/installing/installing_aws/installing-aws-customizations.html#installation-aws-config-yaml_installing-aws-customizations

[3] https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/EBSEncryption.html

staebler
staebler reviewed Apr 28, 2021
View reviewed changes
modules/installation-configuration-parameters.adoc
@@ -309,6 +309,14 @@ ifdef::aws[]
|====
|Parameter|Description|Values

|`compute.platform.aws.amiID`
|The The AWS AMI used to boot compute machines for the cluster. This is required for regions that require a custom RCOS AMI
Copy link

Choose a reason for hiding this comment

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

The field is not technically required, as there are other ways to specify the AMI to use. I see that this is the language that we use in more recent docs, too. I think it is confusing to state that all of the following are required.
compute.platform.aws.amiID
controlPlane.platform.aws.amiID
platform.aws.amiID
And there is no mention that the platform.aws.defaultMachinePlatform.amiID can be used instead of any of the preceding.
In these docs, we should at least add the section about the platform.aws.amiID field so that it is consistent with the docs for more recent OpenShfit versions. I see that Yunfei already mentioned this.

Suggested change
|The The AWS AMI used to boot compute machines for the cluster. This is required for regions that require a custom RCOS AMI
|The AWS AMI used to boot compute machines for the cluster. An AMI must be specified for regions that do not have a default RHCOS AMI.

modules/installation-configuration-parameters.adoc
@@ -309,6 +309,14 @@ ifdef::aws[]
|====
|Parameter|Description|Values

|`compute.platform.aws.amiID`
|The The AWS AMI used to boot compute machines for the cluster. This is required for regions that require a custom RCOS AMI
|Any published or custom RCOS AMI that belongs to the set AWS region.
Copy link

Choose a reason for hiding this comment

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

Suggested change
|Any published or custom RCOS AMI that belongs to the set AWS region.
|Any published or custom RHCOS AMI that belongs to the set AWS region.

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 27, 2021
@GroceryBoyJr
Copy link
Contributor Author

/remove-lifecycle stale

@openshift-ci openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 25, 2021
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 23, 2021
@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 23, 2021
@openshift-bot
Copy link

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci openshift-ci bot closed this Jan 22, 2022
@openshift-ci
Copy link

openshift-ci bot commented Jan 22, 2022

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staebler staebler staebler left review comments

Assignees
No one assigned
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@GroceryBoyJr @yunjiang29 @openshift-bot @staebler @openshift-ci-robot