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: replace aws copy for encryption with encrypted EBS volumes #3293

Merged
merged 3 commits into from Mar 17, 2020

Conversation

abhinavdahiya
Copy link
Contributor

xref: https://issues.redhat.com/browse/CORS-1352
depends on openshift/cluster-api-provider-aws#303

Amazon supports using encrypted root EBS volumes. So instead of copying the AMI and encrypting in the process we can use the encypted EBS volumes instead which saves us making the AMI copy during install.

The change also allows the user to provide KMS key to the terraform instead of the default KMS key.

Using this change the instance boots with public RHCOS AMI, and the EBS volume is encrypted with default KMS key for the account and region.

[4:42:15]installer git:(ami_no_copy) ✗ AWS_PROFILE=openshift-dev aws ec2 describe-volumes --region ca-central-1 --volume-ids vol-09ceb58bc9ac93602
{
    "Volumes": [
        {
            "Attachments": [
                {
                    "AttachTime": "2020-03-13T23:30:09.000Z",
                    "Device": "/dev/xvda",
                    "InstanceId": "i-0bdec7b64f2b9a607",
                    "State": "attached",
                    "VolumeId": "vol-09ceb58bc9ac93602",
                    "DeleteOnTermination": true
                }
            ],
            "AvailabilityZone": "ca-central-1b",
            "CreateTime": "2020-03-13T23:30:09.789Z",
            "Encrypted": true,
            "KmsKeyId": "arn:aws:kms:ca-central-1:<REDACTED>:key/1ffbfa61-7250-42de-9a75-cac64a8f0e4d",
            "Size": 120,
            "SnapshotId": "snap-0c8421bbe06f5f69d",
            "State": "in-use",
            "VolumeId": "vol-09ceb58bc9ac93602",
            "Iops": 360,
            "Tags": [
                {
                    "Key": "kubernetes.io/cluster/adahiya-1-djk4q",
                    "Value": "owned"
                },
                {
                    "Key": "Name",
                    "Value": "adahiya-1-djk4q-master-1-vol"
                }
            ],
            "VolumeType": "gp2"
        }
    ]
}
[4:42:28]installer git:(ami_no_copy) ✗ AWS_PROFILE=openshift-dev aws ec2 describe-instances --region ca-central-1 --instance-ids i-0bdec7b64f2b9a607
{
    "Reservations": [
        {
            "Groups": [],
            "Instances": [
                {
                    "AmiLaunchIndex": 0,
                    "ImageId": "ami-0d235b4920e9def96",
                    "InstanceId": "i-0bdec7b64f2b9a607",
                    "InstanceType": "m4.xlarge",
                    "LaunchTime": "2020-03-13T23:30:09.000Z",
...
                    "BlockDeviceMappings": [
                        {
                            "DeviceName": "/dev/xvda",
                            "Ebs": {
                                "AttachTime": "2020-03-13T23:30:09.000Z",
                                "DeleteOnTermination": true,
                                "Status": "attached",
                                "VolumeId": "vol-09ceb58bc9ac93602"
                            }
                        }
                    ],
...
                    "Tags": [
                        {
                            "Key": "Name",
                            "Value": "adahiya-1-djk4q-master-1"
                        },
                        {
                            "Key": "kubernetes.io/cluster/adahiya-1-djk4q",
                            "Value": "owned"
                        }
                    ],
...
        }
    ]
}
[4:44:31]installer git:(ami_no_copy) ✗

Users can provide the KMS Key ARN which should be used to encrypt the EBS volumes otherwise the default KMS key for the region will be used.

@abhinavdahiya
Copy link
Contributor Author

tried to bring the cluster up using cluster-bot using cmd: /msg @clusterbot launch openshift/cluster-api-provider-aws#303,openshift/installer#3293 aws

@abhinavdahiya
Copy link
Contributor Author

/hold as 25e7902 is using local fork

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2020
@abhinavdahiya
Copy link
Contributor Author

We have measured the impact of this change and decided that it does not improve the situation

From quick testing, this reduces the 4-5 minutes on less traffic account and maybe a lot more in ci account as the AMI copy is completely removed.
But from the testing also saw that even if the control-plane booted very quickly, they were stuck waiting for internal DNS zone and records, around 2 minutes, to be created to reach out to the machine-config-server for Ingition.

OR We have implemented this change and it proven that it reduces the overhead associated with copying and encrypting our boot images

I think not copying does reduce any overhead

Understand how non default KMS keys can be used for encryption.

non-default KMS keys can easily be provided to the install-config per machine pool i.e. separate for control-plane and compute which is maybe nice.

Are there any limitations?

  • from encrypted EBS volume docs, they list Supported Instance Types. These seems to include all the known instance classes at least the m4 and m5 so we should be good.

  • Still figuring out if this is supported in non-public regions like US Gov Cloud, China

@abhinavdahiya
Copy link
Contributor Author

/test e2e-aws

@cgwalters
Copy link
Member

We clearly can't ever delete bootimages that has ever been pinned by a released OpenShift version.

I'd also hesitate at deleting bootimages that were ever pinned (e.g. during a devel cycle).

Beyond that, I don't see a reason for us not to prune the bootimages eventually.

So this should clearly be fine.

```
$ go version
go version go1.14 linux/amd64
$ go mod edit -replace sigs.k8s.io/cluster-api-provider-aws=github.com/openshift/cluster-api-provider-aws@master
$ go mod tidy
warning: ignoring symlink /home/adahiya/go/src/github.com/openshift/installer/pkg/asset/store/data
go: downloading github.com/openshift/cluster-api-provider-aws v0.2.1-0.20200316201703-923caeb1d0d8
go: downloading github.com/openshift/machine-api-operator v0.2.1-0.20200310180732-c63fa2b143f0
$ go mod vendor
```
@abhinavdahiya
Copy link
Contributor Author

/hold cancel
deaa20d beings in the correct vendor bump for cluster-apir-provider-aws

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2020
Amazon supports using encrypted root EBS volumes [1]. So instead of copying the AMI and encrypting in the process we can use the encypted EBS volumes instead which saves us making the AMI copy during install.

The change also allows the user to provide KMS key to the terraform instead of the default KMS key.

Using this change the instance boots with public RHCOS AMI, and the EBS volume is encrypted with default KMS key for the account and region.
```console
[4:42:15] ➜  installer git:(ami_no_copy) ✗ AWS_PROFILE=openshift-dev aws ec2 describe-volumes --region ca-central-1 --volume-ids vol-09ceb58bc9ac93602
{
    "Volumes": [
        {
            "Attachments": [
                {
                    "AttachTime": "2020-03-13T23:30:09.000Z",
                    "Device": "/dev/xvda",
                    "InstanceId": "i-0bdec7b64f2b9a607",
                    "State": "attached",
                    "VolumeId": "vol-09ceb58bc9ac93602",
                    "DeleteOnTermination": true
                }
            ],
            "AvailabilityZone": "ca-central-1b",
            "CreateTime": "2020-03-13T23:30:09.789Z",
            "Encrypted": true,
            "KmsKeyId": "arn:aws:kms:ca-central-1:<REDACTED>:key/1ffbfa61-7250-42de-9a75-cac64a8f0e4d",
            "Size": 120,
            "SnapshotId": "snap-0c8421bbe06f5f69d",
            "State": "in-use",
            "VolumeId": "vol-09ceb58bc9ac93602",
            "Iops": 360,
            "Tags": [
                {
                    "Key": "kubernetes.io/cluster/adahiya-1-djk4q",
                    "Value": "owned"
                },
                {
                    "Key": "Name",
                    "Value": "adahiya-1-djk4q-master-1-vol"
                }
            ],
            "VolumeType": "gp2"
        }
    ]
}
[4:42:28] ➜  installer git:(ami_no_copy) ✗ AWS_PROFILE=openshift-dev aws ec2 describe-instances --region ca-central-1 --instance-ids i-0bdec7b64f2b9a607
{
    "Reservations": [
        {
            "Groups": [],
            "Instances": [
                {
                    "AmiLaunchIndex": 0,
                    "ImageId": "ami-0d235b4920e9def96",
                    "InstanceId": "i-0bdec7b64f2b9a607",
                    "InstanceType": "m4.xlarge",
                    "LaunchTime": "2020-03-13T23:30:09.000Z",
...
                    "BlockDeviceMappings": [
                        {
                            "DeviceName": "/dev/xvda",
                            "Ebs": {
                                "AttachTime": "2020-03-13T23:30:09.000Z",
                                "DeleteOnTermination": true,
                                "Status": "attached",
                                "VolumeId": "vol-09ceb58bc9ac93602"
                            }
                        }
                    ],
...
                    "Tags": [
                        {
                            "Key": "Name",
                            "Value": "adahiya-1-djk4q-master-1"
                        },
                        {
                            "Key": "kubernetes.io/cluster/adahiya-1-djk4q",
                            "Value": "owned"
                        }
                    ],
...
        }
    ]
}
[4:44:31] ➜  installer git:(ami_no_copy) ✗
```

the bootstrap host is always encrypted with either the default KMS key or using the user specified KMS key.

[1]: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/EBSEncryption.html
Users can provide the KMS Key ARN which should be used to encrypt the EBS volumes otherwise the default KMS key for the region will be used.
@wking
Copy link
Member

wking commented Mar 17, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2020
@abhinavdahiya
Copy link
Contributor Author

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 344e38f into openshift:master Mar 17, 2020
@openshift-ci-robot
Copy link
Contributor

@abhinavdahiya: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-libvirt 17030b3 link /test e2e-libvirt

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

wking added a commit to wking/openshift-installer that referenced this pull request Mar 16, 2021
Fix a disconnect about the slug to use for the reference link from
17030b3 (aws: allow users to set the KMS key id for encrypting EBS
volumes, 2020-03-13, openshift#3293).
smrowley pushed a commit to smrowley/installer that referenced this pull request Mar 31, 2021
Fix a disconnect about the slug to use for the reference link from
17030b3 (aws: allow users to set the KMS key id for encrypting EBS
volumes, 2020-03-13, openshift#3293).
AnnaZivkovic pushed a commit to AnnaZivkovic/installer that referenced this pull request Apr 1, 2022
Fix a disconnect about the slug to use for the reference link from
17030b3 (aws: allow users to set the KMS key id for encrypting EBS
volumes, 2020-03-13, openshift#3293).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants