OSDOCS#6222: Adding custom security groups in AWS #60921
Conversation
openshift-ci
bot
added
the
size/M
mjpytlak
added
branch/enterprise-4.14
and removed
size/M
| additionalSecurityGroupIDs: | ||
| - sg-1 | ||
| - sg-2 | ||
| replicas: 3 |
There was a problem hiding this comment.
I think it's important to make sure that feature also can work with the edge compute pool with Local Zones.
https://docs.openshift.com/container-platform/4.13/installing/installing_aws/installing-aws-localzone.html
@barbacbd can you see any restriction on that?
There was a problem hiding this comment.
I have not seen a restriction but I can look. Technically the placement in the install config allows the security groups to be added to any AWS machine type, but I haven't run specific tests for those cases.
There was a problem hiding this comment.
@mtulio as far as local zones go, security groups don't require anything special. The security groups just need to be attached to the VPC with the local zone subnet. Also we would require that these local zone subnets are provided in the subnets field of the install config.
There was a problem hiding this comment.
@mtulio as far as edge computing, what concerns do you have? Security groups can be shared by many resources as they are just rules.
There was a problem hiding this comment.
@mtulio as far as edge computing, what concerns do you have? Security groups can be shared by many resources as they are just rules.
Yes, SGs can be assigned to any supported resource between the same VPC.
My concern was on the install side, I just wanna make sure the configuration below can be used - considering edge compute pool uses the same flow of worker, I expect only sg-5 and sg-6 in MachineSet manifests for Local Zones, just confirming it.
compute:
- hyperthreading: Enabled
name: worker
platform:
aws:
additionalSecurityGroupIDs:
- sg-1 <1>
- sg-2
replicas: 3
- hyperthreading: Enabled
name: edge
platform:
aws:
additionalSecurityGroupIDs:
- sg-5
- sg-6There was a problem hiding this comment.
Confirmed! The new machineset does show that it is for edge nodes and it has the security group that I mentioned in the install config and not the one used for the workers
openshift-ci
bot
added
the
size/M
|
🤖 Updated build preview is available at: Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/20502 |
|
/cc @barbacbd |
| = Applying existing security groups to the cluster | ||
| By default, the installation program creates and attaches security groups to control plane and compute machines. The rules associated with the default security groups cannot be modified. | ||
|
|
||
| If you are deploying an {product-title} cluster to an existing AWS Virtual Private Cloud (VPC), you can apply additional existing AWS security groups (custom security groups) to your control plane and compute machines. Applying custom security groups can help you meet the security needs of your organization, in such cases where you need to control the incoming and outgoing traffic of your control plane and compute machines. |
There was a problem hiding this comment.
| If you are deploying an {product-title} cluster to an existing AWS Virtual Private Cloud (VPC), you can apply additional existing AWS security groups (custom security groups) to your control plane and compute machines. Applying custom security groups can help you meet the security needs of your organization, in such cases where you need to control the incoming and outgoing traffic of your control plane and compute machines. | |
| If you are deploying an {product-title} cluster to an existing AWS Virtual Private Cloud (VPC), you can apply additional existing AWS security groups (custom security groups) to your control plane and compute machines. Applying custom security groups can help you meet the security needs of your organization, in such cases where you need to control the incoming or outgoing traffic of your control plane and compute machines. |
Sorry kind of nit picky =)
|
|
||
| .Prerequisites | ||
| * You have created the security groups in AWS. For more information, see the AWS documentation about working with link:https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-security-groups.html[security groups]. | ||
| * The security groups must be part of the existing VPC to which you are deploying the cluster. The security groups cannot be associated with another VPC. |
There was a problem hiding this comment.
| * The security groups must be part of the existing VPC to which you are deploying the cluster. The security groups cannot be associated with another VPC. | |
| * The security groups must be associated with the existing VPC to which you are deploying the cluster. The security groups cannot be associated with another VPC. |
nit: using the same words as AWS regarding this topic.
| additionalSecurityGroupIDs: | ||
| - sg-1 | ||
| - sg-2 | ||
| replicas: 3 |
There was a problem hiding this comment.
I have not seen a restriction but I can look. Technically the placement in the install config allows the security groups to be added to any AWS machine type, but I haven't run specific tests for those cases.
| platform: | ||
| aws: | ||
| region: us-east-1 | ||
| subnets: |
There was a problem hiding this comment.
@mjpytlak did we want to note that the subnets are required or just let the user figure that out during install?
There was a problem hiding this comment.
did we want to note that the subnets are required or just let the user figure that out during install?
@barbacbd I was originally thinking that given an earlier sample file in the topic states that the subnets are required, I would let it be. However - can't hurt. I updated code block with a few annotations. Outside of that, I made your proposed changes. Thanks for those. Please let me know when you have had a chance to look into the Local Zones case.
|
@nirarg I am working with @barbacbd on the ability to apply existing AWS security groups when users BYON. [1] I noticed that the AWS Outposts case requires users to BYON. The content I am updating is shared by the Outposts doc. Do either you or Brent have concerns about this content appear in that installation use case? [2] [1] https://issues.redhat.com/browse/OCPSTRAT-148 |
|
/lgtm |
openshift-ci
bot
added
size/L
|
@mtulio I have added this content to the Local Zones doc. Notably I have updated the sample example to include local zone specific content. PTAL and let me know if we are OK as is or if updates are required. Thanks in advance. |
| name: worker | ||
| endif::local-zones[] | ||
| ifdef::local-zones[] | ||
| - name: edge |
There was a problem hiding this comment.
The hyphen must be removed, otherwise will create a wrong yaml /install-config. (Double check the rendered page for Local zones)
There was a problem hiding this comment.
@mtulio I do not see the behavior. Are you looking at this page? https://60921--docspreview.netlify.app/openshift-enterprise/latest/installing/installing_aws/installing-aws-localzone.html#installation-aws-vpc-security-groups_installing-aws-localzone?
If so, is it possible you were looking at the page before I added the condition for local zones and you are getting cached version?
There was a problem hiding this comment.
I am interpreting your comment as ifdef::local-zones[] is incorrect. Are you referring to another hyphen in this example? If you are referring to - name: edge I used the syntax from https://docs.openshift.com/container-platform/4.13/installing/installing_aws/installing-aws-localzone.html#machines-edge-machine-pool_installing-aws-localzone. Is that a doc bug?
There was a problem hiding this comment.
There was a problem hiding this comment.
@mjpytlak Also looking at where that section is placed, I can see the docs is providing different examples of optional machine configuration (like Security Groups) here:
https://60921--docspreview.netlify.app/openshift-enterprise/latest/installing/installing_aws/installing-aws-localzone.html#machines-edge-machine-pool_installing-aws-localzone
- Configuration that uses an edge pool with default settings
- Configuration that uses an edge pool with a custom instance type
- Configuration that uses an edge pool with a custom EBS type
Do you think that optional security group deployment would fit in that list too, as it is also optional?
There was a problem hiding this comment.
Do you think that optional security group deployment would fit in that list too, as it is also optional?
@mtulio Yes - I agree. I have added an example to this section. I have also removed the - from https://github.com/openshift/openshift-docs/pull/60921/files#diff-c1a8e33036504600b04232682d3adb385bd0cb256c8bb0fa17b50c6daec36635R39
Please take one more look.
There was a problem hiding this comment.
@mjpytlak ok, thanks, I can see there. Do we need two examples about that topic on the Local Zone page? As you agreed to add the example[1] in the section, I can't see a benefit of [2] (as we already have detailed in the other pages). As a user [reading the rendered page] it seems the SG section ([2]Applying existing AWS security groups to the cluster) is required in the installation process for Local Zones - which is not true.
- (1) Installing a cluster using AWS Local Zone > The edge compute pool for AWS Local Zones
- (2) Installing a cluster using AWS Local Zone > Applying existing security groups to a cluster
I am ok having only 1 on the Local Zones install page. Let me know your thoughts.
There was a problem hiding this comment.
I am ok having only 1 on the Local Zones install page. Let me know your thoughts.
@mtulio I agree and that was an oversight on my part. I removed Applying existing AWS security groups to the cluster.
There was a problem hiding this comment.
@mjpytlak np at all! Thanks for revisiting.
| compute: | ||
| - hyperthreading: Enabled | ||
| ifndef::local-zones[] | ||
| name: worker | ||
| endif::local-zones[] | ||
| ifdef::local-zones[] | ||
| - name: edge |
There was a problem hiding this comment.
IIUC I think this can fix:
| compute: | |
| - hyperthreading: Enabled | |
| ifndef::local-zones[] | |
| name: worker | |
| endif::local-zones[] | |
| ifdef::local-zones[] | |
| - name: edge | |
| compute: | |
| - hyperthreading: Enabled | |
| ifndef::local-zones[] | |
| name: worker | |
| endif::local-zones[] | |
| ifdef::local-zones[] | |
| name: edge |
There was a problem hiding this comment.
Done. Thank you for the clarification on Friday. I did not realize that the - from hyperthreading was inherited.
mjpytlak
force-pushed
the
osdocs-6222
branch
2 times, most recently
from
66fb021
to
0319f7b
Compare
|
LGTM Local Zones |
|
@yunjiang29 Draft docs ready for QE review. PTAL. Thank you. |
|
@gpei PTAL, thanks |
|
/lgtm |
mjpytlak
added
the
peer-review-needed
|
/label peer-review-in-progress |
openshift-ci
bot
added
the
peer-review-in-progress
|
|
||
| .Prerequisites | ||
| * You have created the security groups in AWS. For more information, see the AWS documentation about working with link:https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-security-groups.html[security groups]. | ||
| * The security groups must associated with the existing VPC to which you are deploying the cluster. The security groups cannot be associated with another VPC. |
There was a problem hiding this comment.
| * The security groups must associated with the existing VPC to which you are deploying the cluster. The security groups cannot be associated with another VPC. | |
| * The security groups must be associated with the existing VPC to which you are deploying the cluster. The security groups cannot be associated with another VPC. |
Also, it's fine the way it is, but per ISG you are allowed to end in a preposition to avoid an awkward sentence, in case you care to reword to get rid of the "to which" part. (And instead say "... existing VPC that you are deploying the cluster to."
|
|
||
| .Procedure | ||
|
|
||
| . Edit the `compute.platform.aws.additionalSecurityGroupIDs` parameter to specify one or more custom security groups for your compute machines. |
There was a problem hiding this comment.
Where are we editing, in the install-config.yaml file? I'd clarify that.
| .Procedure | ||
|
|
||
| . Edit the `compute.platform.aws.additionalSecurityGroupIDs` parameter to specify one or more custom security groups for your compute machines. | ||
| . Edit `controlPlane.platform.aws.additionalSecurityGroupIDs` parameter to specify one or more custom security groups for your control plane machines. |
There was a problem hiding this comment.
| . Edit `controlPlane.platform.aws.additionalSecurityGroupIDs` parameter to specify one or more custom security groups for your control plane machines. | |
| . Edit the `controlPlane.platform.aws.additionalSecurityGroupIDs` parameter to specify one or more custom security groups for your control plane machines. |
| . Edit `controlPlane.platform.aws.additionalSecurityGroupIDs` parameter to specify one or more custom security groups for your control plane machines. | ||
| . Save the file and reference it when deploying the cluster. | ||
|
|
||
| .Sample `install-config.yaml` files that specifies custom security groups |
There was a problem hiding this comment.
| .Sample `install-config.yaml` files that specifies custom security groups | |
| .Sample `install-config.yaml` file that specifies custom security groups |
| .Sample `install-config.yaml` files that specifies custom security groups | ||
| [source,yaml] | ||
| ---- | ||
| #... |
There was a problem hiding this comment.
There should be a space, # ...
|
|
||
| By default, the installation program creates and attaches security groups to control plane and compute machines. The rules associated with the default security groups cannot be modified. | ||
|
|
||
| However, you can apply additional existing AWS security groups (custom security groups), which are associated with your existing VPC, to control plane and compute machines. Applying custom security groups can help you meet the security needs of your organization, in such cases where you need to control the incoming or outgoing traffic of these machines. |
There was a problem hiding this comment.
Not critical, but I'd argue that you don't need the (custom security groups)
|
/label peer-review-done |
openshift-ci
bot
added
peer-review-done
|
New changes are detected. LGTM label has been removed. |
|
Appreciate the review @bergerhoffer. Always helpful. |
|
/cherrypick enterprise-4.14 |
|
@mjpytlak: new pull request created: #62781 In response to this:
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. |
Version(s):
4.14+
Issue:
This issue addresses osdocs-6222.
Link to docs preview:
QE review: