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

Removed hardcoding for AMIs in bastion host and added latest AMI lookup #3298

Merged
merged 2 commits into from Mar 11, 2022

Conversation

Ankitasw
Copy link
Member

@Ankitasw Ankitasw commented Mar 10, 2022

What type of PR is this?
/kind feature

What this PR does / why we need it:
Currently, default AMIs used for bastion are hardcoded. This PR enables to lookup the latest images published by Ubuntu.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3108

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

- Added latest AMI lookup for bastion host and removed hardcoded AMIs.
- Added unit tests for bastion.go

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 10, 2022
@k8s-ci-robot
Copy link
Contributor

@Ankitasw: This issue is currently awaiting triage.

If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 10, 2022
@Ankitasw Ankitasw changed the title Removed hardcoding for AMIs in bastion host and used AMI lookup Removed hardcoding for AMIs in bastion host and added latest AMI lookup Mar 10, 2022
@Ankitasw
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e

@Ankitasw
Copy link
Member Author

/retest

@sedefsavas
Copy link
Contributor

sedefsavas commented Mar 10, 2022

Would be good to have a section that explains which images we use by default for bastions under accessing ec2 instances section in the book.

Just to confirm, have you tested this locally by bringing up a bastion host?

@Ankitasw
Copy link
Member Author

I will add the documentation.
I have not tested it locally but i checked e2e tests looks fine, would that not suffice? If not, i will check it locally.

@sedefsavas
Copy link
Contributor

I will add the documentation. I have not tested it locally but i checked e2e tests looks fine, would that not suffice? If not, i will check it locally.

If we have a bastion host enabled template probably enough, but probably need to look at the controller logs if creating bastion instance is successful or not because we don't have any checks in the tests. Would be good to make sure locally as well.

@Ankitasw
Copy link
Member Author

Ankitasw commented Mar 11, 2022

If we have a bastion host enabled template probably enough, but probably need to look at the controller logs if creating bastion instance is successful or not because we don't have any checks in the tests. Would be good to make sure locally as well.

I checked that in any of E2E templates we are not setting bastion as enabled, shall we add it to one of the existing tests so that this is tested?
Also, I have tested locally, and checked the logs, it's creating bastion host successfully.

I0311 08:37:10.465774 8 bastion.go:91] controller/awscluster "msg"="Created new bastion host" "cluster"="capi-bastion-test" "name"="capi-bastion-test" "namespace"="default" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="AWSCluster" "id"="i-02b3c36d4135c037d"

And the AMI fetched is the latest one:

aws ec2 describe-images --image-ids ami-01896de1f162f0ab7 --region us-east-1                                                                                 ✔ 

{
    "Images": [
        {
            "Architecture": "x86_64",
            "CreationDate": "2022-03-08T23:53:51.000Z",
            "ImageId": "ami-01896de1f162f0ab7",
            "ImageLocation": "099720109477/ubuntu/images/hvm-ssd/ubuntu-focal-20.04-amd64-server-20220308",
            "ImageType": "machine",
            "Public": true,
            "OwnerId": "099720109477",
            "PlatformDetails": "Linux/UNIX",
            "UsageOperation": "RunInstances",
            "State": "available",
            "BlockDeviceMappings": [
                {
                    "DeviceName": "/dev/sda1",
                    "Ebs": {
                        "DeleteOnTermination": true,
                        "SnapshotId": "snap-0747283fe9fceac10",
                        "VolumeSize": 8,
                        "VolumeType": "gp2",
                        "Encrypted": false
                    }
                },
                {
                    "DeviceName": "/dev/sdb",
                    "VirtualName": "ephemeral0"
                },
                {
                    "DeviceName": "/dev/sdc",
                    "VirtualName": "ephemeral1"
                }
            ],
            "Description": "Canonical, Ubuntu, 20.04 LTS, amd64 focal image build on 2022-03-08",
            "EnaSupport": true,
            "Hypervisor": "xen",
            "Name": "ubuntu/images/hvm-ssd/ubuntu-focal-20.04-amd64-server-20220308",
            "RootDeviceName": "/dev/sda1",
            "RootDeviceType": "ebs",
            "SriovNetSupport": "simple",
            "VirtualizationType": "hvm"
        }
    ]
}

@Ankitasw
Copy link
Member Author

Ankitasw commented Mar 11, 2022

Would be good to have a section that explains which images we use by default for bastions under accessing ec2 instances section in the book.

@sedefsavas The default latest AMIs that are being used as of now shall be listed there? If yes, we will need to update it everytime the latest AMIs are published right? Or do we just want to specify that we use ubuntu cloud images?

@Ankitasw Ankitasw force-pushed the bastion-ami-lookup branch 4 times, most recently from 49ecfcd to a3dcb7b Compare March 11, 2022 10:44
@@ -31,6 +31,7 @@ spec:
bastion:
enabled: true
```
If this field is set, then by default the latest AMI(Ubuntu 20.04 LTS OS) is looked up from [Ubuntu cloud images](https://ubuntu.com/server/docs/cloud-images/amazon-ec2) by CAPA controller and used in bastion host creation.
Copy link
Member Author

@Ankitasw Ankitasw Mar 11, 2022

Choose a reason for hiding this comment

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

@sedefsavas Let me know if this statement would suffice? Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just added this is true in case AMI ID is not specified.

@sedefsavas
Copy link
Contributor

sedefsavas commented Mar 11, 2022

No need to publish those images in the book since not providing useful info, users can go to that link and see for themselves, and if users want more control, they can specify the AMI ID for the bastion host.

Also, enabling bastion in e2e might be difficult to test, what will be the success condition there? Merging this PR now as this looks complete as is, if you want to do add an e2e test, feel free to do a follow up PR.

@sedefsavas
Copy link
Contributor

sedefsavas commented Mar 11, 2022

/lgtm
/approve

We can check BastionHostReadyCondition in the e2e test.
Also, might add a new reason for this condition: BastionAMILookupFailedReason.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sedefsavas

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2022
@k8s-ci-robot k8s-ci-robot merged commit 0934a16 into kubernetes-sigs:main Mar 11, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.x milestone Mar 11, 2022
@Ankitasw Ankitasw deleted the bastion-ami-lookup branch March 11, 2022 21:34
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use AMI lookup for bastion hosts instead of using hardcoded values
3 participants