karpenter-provider-aws
karpenter-provider-aws copied to clipboard
aws
feat: select subnets by subnet cidr
Fixes https://github.com/kubernetes-sigs/karpenter/issues/2201
Description
Allow users to specify a cidr block like 10.0.1.0/24 in the subnetSelectorTerms like so
spec:
subnetSelectorTerms:
- cidrBlock: "10.0.1.0/24"
How was this change tested?
By the tests added in this PR
Does this change impact docs?
- [x] Yes, PR includes docs updates
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Deploy Preview for karpenter-docs-prod canceled.
| Name | Link |
|---|---|
| Latest commit | 057d5334cb033561a7341ce7fb2265c35757179d |
| Latest deploy log | https://app.netlify.com/projects/karpenter-docs-prod/deploys/686447db83d3e6000857fedd |
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
@mariuskimmina I'd like to request that you draft an RFC (Request for Comments) for this pull request to better explain the necessity of the additional API. Following the design guide available at https://karpenter.sh/docs/contributing/design-guide/, please outline both the problem you're addressing and your proposed solution. This document should clearly articulate the challenges you've identified, explore various potential solutions, and explain why your chosen approach is optimal.
@mariuskimmina I'd like to request that you draft an RFC (Request for Comments) for this pull request to better explain the necessity of the additional API. Following the design guide available at https://karpenter.sh/docs/contributing/design-guide/, please outline both the problem you're addressing and your proposed solution. This document should clearly articulate the challenges you've identified, explore various potential solutions, and explain why your chosen approach is optimal.
Hi, I am wondering what determines when an RFC is necessary? There have been very similar changes in recent times, such as https://github.com/aws/karpenter-provider-aws/pull/7341 that didn't seem to require such a heavy-handed process.
Of course I don't mind doing it, I just want to understand the processes a bit better as I am still fairly new in contributing to Karpenter.
I am wondering what determines when an RFC is necessary
I think we generally reserve RFCs for "larger" changes that have larger impact across the project or its API surface. I'd actually agree that I don't think this needs a full RFC since it's just one additional field in the API; however, I think we'd like to understand how someone would use this field to understand if it's a truly necessary change to the API surface.
In general, we're pretty wary about adding additional surface to the API without strong use-cases for doing so since it can definitely quickly spiral if we're not careful.
I think the primary use case would be a user with many subnets which makes using id cumbersome but that also lags proper tags on it's subnets. In such a case the user might be able specify all the subnets they need as follows
spec:
subnetSelectorTerms:
- cidrBlock: "10.0.*"
The issue https://github.com/kubernetes-sigs/karpenter/issues/2201 shows that there are users where this is the case.
Now, to be fair, the aws documentation reads as follows
cidr-block - The IPv4 CIDR block of the subnet. The CIDR block you specify must exactly match the subnet’s CIDR block for information to be returned for the subnet. You can also use cidr or cidrBlock as the filter names.
but that does not seem to match the actual implementation as doing the following works just fine for me
aws ec2 describe-subnets --filters "Name=cidr-block,Values=10.*"
I also created a small test program to verify that using wildcards like this also works with aws-sdk-go-v2
When trying to use a wildcard in my tests on this PR tho, they failed, so I am still figuring that part out.
Found the problem with the wildcard on my test - filtered wrong on the fake ec2api. But of course now the test relies on some made up behavior that isn't really documented on the AWS api.
I think it can be useful to have these kind of wildcard matching, I could also see people do something like
spec:
subnetSelectorTerms:
- cidrBlock: "*/24"
Which also works fine from the aws cli.
Please let me know what you think from the AWS point of view @jonathan-innis
As an update, I tried it out on one of our test environment clusters and using any of following worked fine
subnetSelectorTerms:
- cidrBlock: "*/20"
subnetSelectorTerms:
- cidrBlock: "10.60.*"
Pull Request Test Coverage Report for Build 16009708407
Warning: This coverage report may be inaccurate.
This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
- For more information on this, see Tracking coverage changes with pull request builds.
- To avoid this issue with future PRs, see these Recommended CI Configurations.
- For a quick fix, rebase this PR at GitHub. Your next report should be accurate.
Details
- 30 of 30 (100.0%) changed or added relevant lines in 3 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+0.06%) to 67.396%
| Totals | |
|---|---|
| Change from base Build 15980637143: | 0.06% |
| Covered Lines: | 7301 |
| Relevant Lines: | 10833 |
💛 - Coveralls
@mariuskimmina can you share a bit about why tagging the subnets wouldn't work / be difficult?
@akestner It isn't my own problem I am trying to solve here but rather the issue https://github.com/kubernetes-sigs/karpenter/issues/2201 - see this comment specifically: https://github.com/kubernetes-sigs/karpenter/issues/2201#issuecomment-2867117504
Hey, I see nothing happened here in some time, I rebased the changes now and wanted to bring this back to your attention, what are your current thoughts on this? @jonathan-innis @engedaam
If there is interest in getting this in I would be happy to make any adjustments required.