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

Fix SecurityGroup check when there are no Network Interfaces associated with a LaunchTemplate #8666

Merged

Conversation

KashifSaadat
Copy link
Contributor

No description provided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 3, 2020
@KashifSaadat
Copy link
Contributor Author

On deleting a cluster the following runtime error occurs:

$ kops delete cluster --yes

panic: runtime error: index out of range [0] with length 0

goroutine 1 [running]:
k8s.io/kops/pkg/resources/aws.FindAutoScalingLaunchTemplateConfigurations(0x7f7bcdc5c178, 0xc0001f91f0, 0xc00045df80, 0x7f7bcdc5c178, 0xc0001f91f0, 0x4, 0x0, 0x0)
	/go/src/k8s.io/kops/pkg/resources/aws/aws.go:1208 +0x92c
k8s.io/kops/pkg/resources/aws.ListResourcesAWS(0x476e440, 0xc0001f91f0, 0xc000052052, 0x25, 0xc0001f91f0, 0x3472200, 0xc000827928)
	/go/src/k8s.io/kops/pkg/resources/aws/aws.go:149 +0xab6
k8s.io/kops/pkg/resources/ops.ListResources(0x7f7bcdc5c178, 0xc0001f91f0, 0xc000052052, 0x25, 0x0, 0x0, 0x0, 0x10, 0x3472200)
	/go/src/k8s.io/kops/pkg/resources/ops/collector.go:41 +0x2d8
main.RunDeleteCluster(0xc00071aa20, 0x4691460, 0xc00000e018, 0xc000429440, 0x0, 0x0)
	/go/src/k8s.io/kops/cmd/kops/delete_cluster.go:133 +0x5e7
main.NewCmdDeleteCluster.func1(0xc000280a00, 0xc0000c3ae0, 0x0, 0x1)
	/go/src/k8s.io/kops/cmd/kops/delete_cluster.go:80 +0xcd
github.com/spf13/cobra.(*Command).execute(0xc000280a00, 0xc0000c3ac0, 0x1, 0x1, 0xc000280a00, 0xc0000c3ac0)
	/go/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:830 +0x2aa
github.com/spf13/cobra.(*Command).ExecuteC(0x67332a0, 0x676dda8, 0x0, 0x0)
	/go/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:914 +0x2fb
github.com/spf13/cobra.(*Command).Execute(...)
	/go/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:864
main.Execute()
	/go/src/k8s.io/kops/cmd/kops/root.go:95 +0x8f
main.main()
	/go/src/k8s.io/kops/cmd/kops/main.go:25 +0x20

Looks like this may have been introduced in PR #8639

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KashifSaadat

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 3, 2020
@hakman
Copy link
Member

hakman commented Mar 3, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2020
@rifelpet
Copy link
Member

rifelpet commented Mar 3, 2020

@KashifSaadat can you explain how a LaunchTemplate would not have Network Interfaces? Just curious

@k8s-ci-robot k8s-ci-robot merged commit cb983ff into kubernetes:master Mar 3, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Mar 3, 2020
@hakman
Copy link
Member

hakman commented Mar 4, 2020

@KashifSaadat any special reason there are 2 ways of specifying the security groups here?

if fi.BoolValue(t.AssociatePublicIP) {
lc.NetworkInterfaces = append(lc.NetworkInterfaces,
&ec2.LaunchTemplateInstanceNetworkInterfaceSpecificationRequest{
AssociatePublicIpAddress: t.AssociatePublicIP,
DeleteOnTermination: aws.Bool(true),
DeviceIndex: fi.Int64(0),
Groups: securityGroups,
})
} else {
lc.SecurityGroupIds = securityGroups
}

For example, for terraform we only use the NetworkInterfaces way.

tf := terraformLaunchTemplate{
NamePrefix: fi.String(fi.StringValue(e.Name) + "-"),
EBSOptimized: e.RootVolumeOptimization,
ImageID: image,
InstanceType: e.InstanceType,
Lifecycle: &terraform.Lifecycle{CreateBeforeDestroy: fi.Bool(true)},
NetworkInterfaces: []*terraformLaunchTemplateNetworkInterfaces{
{AssociatePublicIPAddress: e.AssociatePublicIP,
DeleteOnTermination: fi.Bool(true)},
},
}

@KashifSaadat
Copy link
Contributor Author

@rifelpet : If there are no customised settings to be provided (such as AssociatePublicIP) then a NetworkInterfaceSpecificationRequest isn't created as there is no need, and so it falls back to the default, i.e. Currently no network interface details are specified and therefore the instance will launch with the template default network interface settings

@hakman I'm not sure if there was a specific reason for that approach, maybe just taking the stance that if there's no explicit need to define a NetworkInterface template (other than for security groups) then to avoid it to not complicate the configuration? If there are no issues either way, then that logic can be simplified and made consistent with how it's generated in the terraform output.

@KashifSaadat KashifSaadat deleted the bugfix/aws-lt-delete-cluster branch March 4, 2020 16:47
@hakman
Copy link
Member

hakman commented Mar 4, 2020

Thanks @KashifSaadat. I am trying to add LaunchTemplate tags to CF and TF and noticed that there are quite a few inconsistencies. I will try to make things more uniform in a future PR.

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants