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

AWSResourceReference unimplemented logic #2087

Closed
MarcusNoble opened this issue Nov 2, 2020 · 15 comments · Fixed by #3257
Closed

AWSResourceReference unimplemented logic #2087

MarcusNoble opened this issue Nov 2, 2020 · 15 comments · Fixed by #3257
Assignees
Labels
kind/release-blocking Issues or PRs that need to be closed before the next release lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@MarcusNoble
Copy link
Contributor

MarcusNoble commented Nov 2, 2020

Collating all instances of AWSResourceReference being used in the codebase without implementing all lookup logic (e.g. only using the ID property)

Type Property References ID ARN Filters
AWSMachineSpec AMI
if scope.AWSMachine.Spec.AMI.ID != nil { // nolint:nestif
input.ImageID = *scope.AWSMachine.Spec.AMI.ID
} else {
AWSMachineSpec AdditionalSecurityGroups
for _, id := range additional {
newAnnotation[*id.ID] = struct{}{}
}
AWSMachineSpec Subnet
switch {
case scope.AWSMachine.Spec.Subnet != nil && scope.AWSMachine.Spec.Subnet.ID != nil:
if failureDomain != nil {
subnet := s.scope.Subnets().FindByID(*scope.AWSMachine.Spec.Subnet.ID)
if subnet == nil {
record.Warnf(scope.AWSMachine, "FailedCreate",
"Failed to create instance: subnet with id %q not found", aws.StringValue(scope.AWSMachine.Spec.Subnet.ID))
return "", awserrors.NewFailedDependency(
fmt.Sprintf("failed to run machine %q, subnet with id %q not found",
scope.Name(),
aws.StringValue(scope.AWSMachine.Spec.Subnet.ID),
),
)
}
if subnet.AvailabilityZone != *failureDomain {
record.Warnf(scope.AWSMachine, "FailedCreate",
"Failed to create instance: subnet's availability zone %q does not match with the failure domain %q",
subnet.AvailabilityZone,
*failureDomain)
return "", awserrors.NewFailedDependency(
fmt.Sprintf("failed to run machine %q, subnet's availability zone %q does not match with the failure domain %q",
scope.Name(),
subnet.AvailabilityZone,
*failureDomain,
),
)
}
}
return *scope.AWSMachine.Spec.Subnet.ID, nil
case scope.AWSMachine.Spec.Subnet != nil && scope.AWSMachine.Spec.Subnet.Filters != nil:
criteria := []*ec2.Filter{
filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable),
filter.EC2.VPC(s.scope.VPC().ID),
}
if failureDomain != nil {
criteria = append(criteria, filter.EC2.AvailabilityZone(*failureDomain))
}
for _, f := range scope.AWSMachine.Spec.Subnet.Filters {
criteria = append(criteria, &ec2.Filter{Name: aws.String(f.Name), Values: aws.StringSlice(f.Values)})
}
subnets, err := s.getFilteredSubnets(criteria...)
if err != nil {
return "", errors.Wrapf(err, "failed to filter subnets for criteria %q", criteria)
}
if len(subnets) == 0 {
record.Warnf(scope.AWSMachine, "FailedCreate",
"Failed to create instance: no subnets available matching filters %q", scope.AWSMachine.Spec.Subnet.Filters)
return "", awserrors.NewFailedDependency(
fmt.Sprintf("failed to run machine %q, no subnets available matching filters %q",
scope.Name(),
scope.AWSMachine.Spec.Subnet.Filters,
),
)
}
return *subnets[0].SubnetId, nil
case failureDomain != nil:
subnets := s.scope.Subnets().FilterPrivate().FilterByZone(*failureDomain)
if len(subnets) == 0 {
record.Warnf(scope.AWSMachine, "FailedCreate",
"Failed to create instance: no subnets available in availability zone %q", *failureDomain)
return "", awserrors.NewFailedDependency(
fmt.Sprintf("failed to run machine %q, no subnets available in availability zone %q",
scope.Name(),
*failureDomain,
),
)
}
return subnets[0].ID, nil
// TODO(vincepri): Define a tag that would allow to pick a preferred subnet in an AZ when working
// with control plane machines.
default:
sns := s.scope.Subnets().FilterPrivate()
if len(sns) == 0 {
record.Eventf(s.scope.InfraCluster(), "FailedCreateInstance", "Failed to run machine %q, no subnets available", scope.Name())
return "", awserrors.NewFailedDependency(fmt.Sprintf("failed to run machine %q, no subnets available", scope.Name()))
}
return sns[0].ID, nil
}
AWSMachinePoolSpec Subnets
for i, v := range scope.AWSMachinePool.Spec.Subnets {
subnetIDs[i] = aws.StringValue(v.ID)
}
@MarcusNoble
Copy link
Contributor Author

It looks as though there aren't as many instances as first thought.

If ignoring test files and generated files AWSResourceReference is only made use of in 4 places.

All 4 make use of the ID property, one makes use of the Filters property and there are no instances where ARN is used.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 7, 2021
@MarcusNoble
Copy link
Contributor Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 8, 2021
@randomvariable randomvariable added the kind/release-blocking Issues or PRs that need to be closed before the next release label Apr 15, 2021
@sedefsavas
Copy link
Contributor

/assign @randomvariable

@randomvariable
Copy link
Member

/lifecycle active

@k8s-ci-robot k8s-ci-robot added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Jul 22, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. labels Oct 20, 2021
@randomvariable randomvariable modified the milestones: v0.7.0, v1.x Nov 3, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 3, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

@sedefsavas
Copy link
Contributor

lifecycle/stale

@sedefsavas sedefsavas reopened this Jan 3, 2022
@k8s-ci-robot k8s-ci-robot added needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 3, 2022
@sedefsavas sedefsavas added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jan 3, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 3, 2022
@sedefsavas sedefsavas added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 3, 2022
@sedefsavas sedefsavas added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Jan 3, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 2, 2022
@sedefsavas sedefsavas added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Feb 4, 2022
@Ankitasw
Copy link
Member

/assign

@Ankitasw
Copy link
Member

Ankitasw commented Feb 25, 2022

@sedefsavas @richardcase Just to clarify, here we need to implement lookup logic for fields which are of type AWSResourceReference and making use of any of the properties(ID, Filters) from AWSResourceReference in the respective implementation.

@Ankitasw
Copy link
Member

/lifecycle active

@k8s-ci-robot k8s-ci-robot added lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 25, 2022
@shivi28
Copy link
Contributor

shivi28 commented Mar 2, 2022

Pasting slack discussion here for others reference:
Scope of this issue has multiple parts, and one of them is

if subnet id is there, do a lookup using that
if filter is there, do a lookup using that
If both are given then throw a validation error 

But currently in our code we are only doing lookup by IDs.

So a PR is raised which will implement above logic for AWSResourceReference in AWSMachinePool.Spec.Subnets during ASG creation/updation.

/assign @shivi28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/release-blocking Issues or PRs that need to be closed before the next release lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants