Restrict subnets only to subnets from regular availability zones in ELB auto-discovery procedure #499

lobziik · 2022-09-08T16:42:48Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
Outpost, wavelength, and local zone don't support NLB or CLB at the moment.
This pr adds check for availability zone type and includes only subnets from availability-zone typed zones for the auto-discovery.

Which issue(s) this PR fixes:

Fixes #442

Special notes for your reviewer:
I'm not sure about PR type, guess that feature is the most suitable one.

Does this PR introduce a user-facing change?:

Added check  for availability zone type within subnets auto-discovery procedure for a load-balancers
Auto-discovery will include only subnets from `availability-zone` typed zones.

IMPORTANT: This change requires an IAM permissions update. IAM policy for control-plane nodes
should be extended with `ec2:DescribeAvailabilityZones` permission.

k8s-ci-robot · 2022-09-08T16:42:57Z

@lobziik: This issue is currently awaiting triage.

If cloud-provider-aws contributors determine 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 · 2022-09-08T16:42:58Z

Welcome @lobziik!

It looks like this is your first PR to kubernetes/cloud-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/cloud-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-09-08T16:42:59Z

Hi @lobziik. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

JoelSpeed · 2022-09-09T15:23:16Z

/ok-to-test

JoelSpeed · 2022-09-12T09:51:17Z

/lgtm

Will need an AWS approver to review as well, but the code looks clean and makes sense to me

lobziik · 2022-09-12T10:06:13Z

e2e is failing, i need some time to reproduce and investigate it.
/hold

lobziik · 2022-09-12T12:17:55Z

Apparently this requires additional iam permissions for describing availability zones. I need to figure out where it does set within this e2es and give an another thought about this api call error handling.

Will appreciate some advices here, for now i'm not entirely sure how to proceed.

pkg/providers/v1/aws.go

lobziik · 2022-10-18T14:34:57Z

Updated PR, added a check for the response type. Insufficient permissions does not threat as an error anymore, warning will be raised instead: https://github.com/kubernetes/cloud-provider-aws/pull/499/files#diff-97a31d0f193ea0b4e2c538595daf93405351054377458053a5010f6fdb97f4dcR3539 for provide backward compatability and do not break existing clusters after ccm upgrade.

Added missed permission into documentation also.

lobziik · 2022-10-18T14:35:39Z

/hold cancel
/cc @kishorj

lobziik · 2022-10-18T14:37:08Z

/cc @kishorj @JoelSpeed

Sry, I misclicked and break review requests.

JoelSpeed

Changes look ok, couple of nits about errors but otherwise LGTM, lmk what you think about using the error wrapping, this file already imports the standard library errors package so there's no new dependency on this

pkg/providers/v1/aws.go

JoelSpeed · 2022-11-01T07:22:43Z

/lgtm

olemarkus · 2022-11-02T08:20:37Z

/lgtm

olemarkus · 2022-11-03T14:53:40Z

/pull-kops-e2e-cni-amazonvpc

kishorj · 2022-11-09T18:04:41Z

pkg/providers/v1/aws.go

+
+	azs, err := c.ec2.DescribeAvailabilityZones(azRequest)
+	if err != nil {
+		if awsErr, ok := err.(awserr.RequestFailure); ok && awsErr.Code() == "UnauthorizedOperation" {


Tbh, if we ignore the error we will keep the existing behavior unless the IAM permissions get updated. I favor throwing the error here instead.

We will have to update the IAM permissions. As for EKS, we can start process to update the managed policies. For kops, we can request the permission to be included.

Once the policies get updated, we can backport the changes to older releases as well.

I updated pr, now permissions error will be propagated back.

IAM permissions in kops were extended in kubernetes/kops#14650.

@kishorj can you please take a look? :)

lobziik · 2022-11-24T15:38:49Z

I attempted to update permission list for kops. @olemarkus can you please help with that, when you will have time? :) kubernetes/kops#14650 - think it will be a prerequisite for having pull-cloud-provider-aws-e2e passed. Then I can update this PR.

To workaround the issue with subnets auto-discovery [1] AWS ccm needs to have permission to retrieve information about availability zones (specifically to detect outpost, wavelength, and local zones [2]). [1] kubernetes/cloud-provider-aws#442 [2] kubernetes/cloud-provider-aws#499

Outpost, wavelength, and local zone don't support NLB or CLB for the moment. This commit adds check for availability zone type and includes only subnets from `availability-zone` typed zones for the auto-discovery.

olemarkus · 2022-12-09T13:04:19Z

/retest

JoelSpeed · 2022-12-12T16:52:13Z

Thanks for the fixups @lobziik

/lgtm

kishorj · 2022-12-16T18:42:19Z

Thanks
/lgtm

Need updates to the AmazonEKSClusterPolicy for EKS clusters. Hold merge until we initiate the process to update the managed policy.

k8s-ci-robot · 2022-12-17T14:22:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus

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:

~~OWNERS~~ [olemarkus]

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 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 8, 2022

k8s-ci-robot requested review from jaypipes and nckturner September 8, 2022 16:43

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 8, 2022

lobziik changed the title ~~Restrict subnets to subnets from regular availability zones~~ Restrict subnets to subnets from regular availability zones in auto-discovery procedure Sep 8, 2022

lobziik changed the title ~~Restrict subnets to subnets from regular availability zones in auto-discovery procedure~~ Restrict subnets only to subnets from regular availability zones in ELB auto-discovery procedure Sep 8, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 9, 2022

k8s-ci-robot assigned JoelSpeed Sep 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2022

mtulio reviewed Oct 3, 2022

View reviewed changes

pkg/providers/v1/aws.go Show resolved Hide resolved

lobziik mentioned this pull request Oct 17, 2022

REQUEST: New membership for @lobziik kubernetes/org#3773

Closed

9 tasks

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2022

k8s-ci-robot requested a review from kishorj October 18, 2022 14:35

lobziik removed the request for review from jaypipes October 18, 2022 14:36

lobziik requested review from mtulio and removed request for nckturner and kishorj October 18, 2022 14:36

k8s-ci-robot requested review from JoelSpeed and kishorj and removed request for mtulio October 18, 2022 14:37

JoelSpeed reviewed Oct 19, 2022

View reviewed changes

pkg/providers/v1/aws.go Outdated Show resolved Hide resolved

pkg/providers/v1/aws.go Outdated Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2022

k8s-ci-robot assigned olemarkus Nov 2, 2022

kishorj reviewed Nov 9, 2022

View reviewed changes

lobziik mentioned this pull request Nov 24, 2022

aws: Add ec2:DescribeAvailabilityZones to the AWS CCM permissions list kubernetes/kops#14650

Merged

Restrict subnets to subnets from regular availability zones

c3e6626

Outpost, wavelength, and local zone don't support NLB or CLB for the moment. This commit adds check for availability zone type and includes only subnets from `availability-zone` typed zones for the auto-discovery.

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 9, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2022

k8s-ci-robot assigned kishorj Dec 16, 2022

olemarkus approved these changes Dec 17, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2022

k8s-ci-robot merged commit 1afaa94 into kubernetes:master Dec 17, 2022

weseven mentioned this pull request Jul 8, 2024

Add ec2:DescribeAvailabilityZones to control plane template. giantswarm/capa-iam-operator#310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict subnets only to subnets from regular availability zones in ELB auto-discovery procedure #499

Restrict subnets only to subnets from regular availability zones in ELB auto-discovery procedure #499

lobziik commented Sep 8, 2022 •

edited

Loading

k8s-ci-robot commented Sep 8, 2022

k8s-ci-robot commented Sep 8, 2022

k8s-ci-robot commented Sep 8, 2022

JoelSpeed commented Sep 9, 2022

JoelSpeed commented Sep 12, 2022

lobziik commented Sep 12, 2022

lobziik commented Sep 12, 2022

lobziik commented Oct 18, 2022

lobziik commented Oct 18, 2022

lobziik commented Oct 18, 2022

JoelSpeed left a comment

JoelSpeed commented Nov 1, 2022

olemarkus commented Nov 2, 2022

olemarkus commented Nov 3, 2022

kishorj Nov 9, 2022

kishorj Nov 9, 2022 •

edited

Loading

lobziik Dec 9, 2022

lobziik commented Nov 24, 2022 •

edited

Loading

olemarkus commented Dec 9, 2022

JoelSpeed commented Dec 12, 2022

kishorj commented Dec 16, 2022

k8s-ci-robot commented Dec 17, 2022

Restrict subnets only to subnets from regular availability zones in ELB auto-discovery procedure #499

Restrict subnets only to subnets from regular availability zones in ELB auto-discovery procedure #499

Conversation

lobziik commented Sep 8, 2022 • edited Loading

k8s-ci-robot commented Sep 8, 2022

k8s-ci-robot commented Sep 8, 2022

k8s-ci-robot commented Sep 8, 2022

JoelSpeed commented Sep 9, 2022

JoelSpeed commented Sep 12, 2022

lobziik commented Sep 12, 2022

lobziik commented Sep 12, 2022

lobziik commented Oct 18, 2022

lobziik commented Oct 18, 2022

lobziik commented Oct 18, 2022

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed commented Nov 1, 2022

olemarkus commented Nov 2, 2022

olemarkus commented Nov 3, 2022

kishorj Nov 9, 2022

Choose a reason for hiding this comment

kishorj Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

lobziik Dec 9, 2022

Choose a reason for hiding this comment

lobziik commented Nov 24, 2022 • edited Loading

olemarkus commented Dec 9, 2022

JoelSpeed commented Dec 12, 2022

kishorj commented Dec 16, 2022

k8s-ci-robot commented Dec 17, 2022

lobziik commented Sep 8, 2022 •

edited

Loading

kishorj Nov 9, 2022 •

edited

Loading

lobziik commented Nov 24, 2022 •

edited

Loading