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

Set ASG subnet to only private subnets #2191

Closed
sedefsavas opened this issue Jan 6, 2021 · 4 comments · Fixed by #2302
Closed

Set ASG subnet to only private subnets #2191

sedefsavas opened this issue Jan 6, 2021 · 4 comments · Fixed by #2302
Labels
kind/bug Categorizes issue or PR as related to a bug.
Milestone

Comments

@sedefsavas
Copy link
Contributor

In AWSMachinePool, if subnets are not specified, we add all available subnets to the AutoScalingGroup.
Instead, the default should be only private subnets IMO.

if len(subnetIDs) == 0 {
for _, subnet := range scope.InfraCluster.Subnets() {
subnetIDs = append(subnetIDs, subnet.ID)
}
}

What do you all think?

@randomvariable randomvariable added this to the v0.7.0 milestone Mar 11, 2021
@randomvariable
Copy link
Member

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 11, 2021
@paurosello
Copy link

Hello @sedefsavas I think this feature is pretty much required in most scenarios.

I cannot think of a case were we want to have machines in public subnets.

I tried using filters for subnets in the MachinePool to overcome this issue but as pointed out in the code on the initial comment, only the ID is used, filters are ignored. This is how it would look like with a filter:

   subnets:
     - filters:
       - name: sigs.k8s.io/cluster-api-provider-aws/role
         values:
           - private

@sedefsavas
Copy link
Contributor Author

@paurosello
This PR fixed this issue by only selecting the private control-plane subnets: #2302

// 4. All the private subnets from the control plane are used
// In Cluster API Availability Zone can also be referred to by the name `Failure Domain`
func (p *defaultSubnetPlacementStrategy) Place(input *placementInput) ([]string, error) {

Is there another scenario this fix does not cover?

@paurosello
Copy link

Oh, that's nice already. We were thinking on using the filters to assign specific MachinePools to specific subnets with filters but I think it would be a completely different issue right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants