Repository: kubernetes/kops (go)

Issue:
additionalSecurityGroups for bastion ELB is documented but isn't implemented
/kind bug

**1. What `kops` version are you running? The command `kops version`, will display
 this information.**

1.29.2

**2. What Kubernetes version are you running? `kubectl version` will print the
 version if a cluster is running or provide the Kubernetes version specified as
 a `kops` flag.**

1.29.8

**3. What cloud provider are you using?**

AWS

**4. What commands did you run?  What is the simplest way to reproduce this issue?**

kops edit cluster and add the security groups

**5. What happened after the commands executed?**

The security groups aren't added

**6. What did you expect to happen?**

The security groups to be added

**7. Please provide your cluster manifest. Execute
  `kops get --name my.example.com -o yaml` to display your cluster manifest.
  You may want to remove your cluster name and other sensitive information.**

My desired manifest is:
```yaml
spec:
  topology:
    bastion:
      bastionPublicName: xxx
      loadBalancer:
        additionalSecurityGroups:
        - sg-xxx
        - sg-xxx
```

But the result of `kops get cluster` includes
```yaml
  topology:
    bastion:
      bastionPublicName: xxx
      loadBalancer: {}
```

**8. Please run the commands with most verbose logging by adding the `-v 10` flag.
  Paste the logs into this report, or in a gist and provide the gist link here.**

```
    "topology": {
      "bastion": {
        "publicName": "xxx",
        "loadBalancer": {}
      },
      "dns": "Public"
    },
```

**9. Anything else do we need to know?**

The docs describe how to add additional SGs here https://github.com/kubernetes/kops/blob/v1.29.2/docs/bastion.md#additional-security-groups-to-elb

But the code says otherwise:
https://github.com/kubernetes/kops/blob/v1.29.2/pkg/apis/kops/v1alpha2/bastion.go#L28
and
https://github.com/kubernetes/kops/blob/v1.29.2/k8s/crds/kops.k8s.io_clusters.yaml#L6197-L6198

So really this is a documentation error from what I can tell, but it wasted an hour of my time trying to figure out why my security groups weren't applying because I assumed I'd made an error.

Note: I'm linking to v1.29.2 tags here but it's still the same in the main branch and later tags.

Pull request description:
Re-enable additionalSecurityGroups for bastion LB
fix #17250 

This feature is documented: https://github.com/kubernetes/kops/blob/v1.32.0/docs/bastion.md#additional-security-groups-to-elb
But it is not implemented.

Once this feature was added: https://github.com/kubernetes/kops/pull/8921
However, this was removed: https://github.com/kubernetes/kops/pull/14468

I guess
- At first, Classic load balancer was used as a bastion LB. So, we could use security groups for the bastion LB.
- When CLB was changed to NLB, this feature was removed. Because NLB didn't support security groups at that time.
- However, after that change, AWS started to support security groups for NLB: https://aws.amazon.com/jp/blogs/containers/network-load-balancers-now-support-security-groups/

So, I think we can re-enable this feature currently.

What do you think?
If we don't need this feature, I'm going to delete it from the document.

Patch:
diff --git a/k8s/crds/kops.k8s.io_clusters.yaml b/k8s/crds/kops.k8s.io_clusters.yaml
index d90ce0220c..19ce428bf4 100644
--- a/k8s/crds/kops.k8s.io_clusters.yaml
+++ b/k8s/crds/kops.k8s.io_clusters.yaml
@@ -6386,7 +6386,6 @@ spec:
                       loadBalancer:
                         properties:
                           additionalSecurityGroups:
-                            description: AdditionalSecurityGroups is unused
                             items:
                               type: string
                             type: array
diff --git a/pkg/apis/kops/bastion.go b/pkg/apis/kops/bastion.go
index ff59c92dd1..4e1116f549 100644
--- a/pkg/apis/kops/bastion.go
+++ b/pkg/apis/kops/bastion.go
@@ -24,6 +24,7 @@ type BastionSpec struct {
 }
 
 type BastionLoadBalancerSpec struct {
+	AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"`
 	// Type of load balancer to create, it can be Public or Internal.
 	Type LoadBalancerType `json:"type,omitempty"`
 }
diff --git a/pkg/apis/kops/v1alpha2/bastion.go b/pkg/apis/kops/v1alpha2/bastion.go
index 74552fd9ed..14262463ce 100644
--- a/pkg/apis/kops/v1alpha2/bastion.go
+++ b/pkg/apis/kops/v1alpha2/bastion.go
@@ -25,8 +25,6 @@ type BastionSpec struct {
 }
 
 type BastionLoadBalancerSpec struct {
-	// AdditionalSecurityGroups is unused
-	// +k8s:conversion-gen=false
 	AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"`
 	// Type of load balancer to create, it can be Public or Internal.
 	Type LoadBalancerType `json:"type,omitempty"`
diff --git a/pkg/apis/kops/v1alpha2/zz_generated.conversion.go b/pkg/apis/kops/v1alpha2/zz_generated.conversion.go
index 5428af1ca2..974963c57d 100644
--- a/pkg/apis/kops/v1alpha2/zz_generated.conversion.go
+++ b/pkg/apis/kops/v1alpha2/zz_generated.conversion.go
@@ -1742,7 +1742,7 @@ func Convert_kops_AzureSpec_To_v1alpha2_AzureSpec(in *kops.AzureSpec, out *Azure
 }
 
 func autoConvert_v1alpha2_BastionLoadBalancerSpec_To_kops_BastionLoadBalancerSpec(in *BastionLoadBalancerSpec, out *kops.BastionLoadBalancerSpec, s conversion.Scope) error {
-	// INFO: in.AdditionalSecurityGroups opted out of conversion generation
+	out.AdditionalSecurityGroups = in.AdditionalSecurityGroups
 	out.Type = kops.LoadBalancerType(in.Type)
 	return nil
 }
@@ -1753,6 +1753,7 @@ func Convert_v1alpha2_BastionLoadBalancerSpec_To_kops_BastionLoadBalancerSpec(in
 }
 
 func autoConvert_kops_BastionLoadBalancerSpec_To_v1alpha2_BastionLoadBalancerSpec(in *kops.BastionLoadBalancerSpec, out *BastionLoadBalancerSpec, s conversion.Scope) error {
+	out.AdditionalSecurityGroups = in.AdditionalSecurityGroups
 	out.Type = LoadBalancerType(in.Type)
 	return nil
 }
diff --git a/pkg/apis/kops/v1alpha3/bastion.go b/pkg/apis/kops/v1alpha3/bastion.go
index 5b159bcd71..8e4fe2342e 100644
--- a/pkg/apis/kops/v1alpha3/bastion.go
+++ b/pkg/apis/kops/v1alpha3/bastion.go
@@ -24,6 +24,7 @@ type BastionSpec struct {
 }
 
 type BastionLoadBalancerSpec struct {
+	AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"`
 	// Type of load balancer to create, it can be Public or Internal.
 	Type LoadBalancerType `json:"type,omitempty"`
 }
diff --git a/pkg/apis/kops/v1alpha3/zz_generated.conversion.go b/pkg/apis/kops/v1alpha3/zz_generated.conversion.go
index 65aecd8991..a5556c352a 100644
--- a/pkg/apis/kops/v1alpha3/zz_generated.conversion.go
+++ b/pkg/apis/kops/v1alpha3/zz_generated.conversion.go
@@ -1918,6 +1918,7 @@ func Convert_kops_AzureSpec_To_v1alpha3_AzureSpec(in *kops.AzureSpec, out *Azure
 }
 
 func autoConvert_v1alpha3_BastionLoadBalancerSpec_To_kops_BastionLoadBalancerSpec(in *BastionLoadBalancerSpec, out *kops.BastionLoadBalancerSpec, s conversion.Scope) error {
+	out.AdditionalSecurityGroups = in.AdditionalSecurityGroups
 	out.Type = kops.LoadBalancerType(in.Type)
 	return nil
 }
@@ -1928,6 +1929,7 @@ func Convert_v1alpha3_BastionLoadBalancerSpec_To_kops_BastionLoadBalancerSpec(in
 }
 
 func autoConvert_kops_BastionLoadBalancerSpec_To_v1alpha3_BastionLoadBalancerSpec(in *kops.BastionLoadBalancerSpec, out *BastionLoadBalancerSpec, s conversion.Scope) error {
+	out.AdditionalSecurityGroups = in.AdditionalSecurityGroups
 	out.Type = LoadBalancerType(in.Type)
 	return nil
 }
diff --git a/pkg/apis/kops/v1alpha3/zz_generated.deepcopy.go b/pkg/apis/kops/v1alpha3/zz_generated.deepcopy.go
index b3c024f466..2f0d276d3c 100644
--- a/pkg/apis/kops/v1alpha3/zz_generated.deepcopy.go
+++ b/pkg/apis/kops/v1alpha3/zz_generated.deepcopy.go
@@ -423,6 +423,11 @@ func (in *AzureSpec) DeepCopy() *AzureSpec {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *BastionLoadBalancerSpec) DeepCopyInto(out *BastionLoadBalancerSpec) {
 	*out = *in
+	if in.AdditionalSecurityGroups != nil {
+		in, out := &in.AdditionalSecurityGroups, &out.AdditionalSecurityGroups
+		*out = make([]string, len(*in))
+		copy(*out, *in)
+	}
 	return
 }
 
@@ -442,7 +447,7 @@ func (in *BastionSpec) DeepCopyInto(out *BastionSpec) {
 	if in.LoadBalancer != nil {
 		in, out := &in.LoadBalancer, &out.LoadBalancer
 		*out = new(BastionLoadBalancerSpec)
-		**out = **in
+		(*in).DeepCopyInto(*out)
 	}
 	return
 }
diff --git a/pkg/apis/kops/zz_generated.deepcopy.go b/pkg/apis/kops/zz_generated.deepcopy.go
index 9ca9ca1ae8..df8602ca7c 100644
--- a/pkg/apis/kops/zz_generated.deepcopy.go
+++ b/pkg/apis/kops/zz_generated.deepcopy.go
@@ -422,6 +422,11 @@ func (in *AzureSpec) DeepCopy() *AzureSpec {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *BastionLoadBalancerSpec) DeepCopyInto(out *BastionLoadBalancerSpec) {
 	*out = *in
+	if in.AdditionalSecurityGroups != nil {
+		in, out := &in.AdditionalSecurityGroups, &out.AdditionalSecurityGroups
+		*out = make([]string, len(*in))
+		copy(*out, *in)
+	}
 	return
 }
 
@@ -441,7 +446,7 @@ func (in *BastionSpec) DeepCopyInto(out *BastionSpec) {
 	if in.LoadBalancer != nil {
 		in, out := &in.LoadBalancer, &out.LoadBalancer
 		*out = new(BastionLoadBalancerSpec)
-		**out = **in
+		(*in).DeepCopyInto(*out)
 	}
 	return
 }
diff --git a/pkg/model/awsmodel/bastion.go b/pkg/model/awsmodel/bastion.go
index 55fa519108..339a1eb36f 100644
--- a/pkg/model/awsmodel/bastion.go
+++ b/pkg/model/awsmodel/bastion.go
@@ -393,6 +393,20 @@ func (b *BastionModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 
 		c.AddTask(tg)
 
+		// Add additional security groups to the NLB
+		if b.Cluster.Spec.Networking.Topology != nil && b.Cluster.Spec.Networking.Topology.Bastion != nil && b.Cluster.Spec.Networking.Topology.Bastion.LoadBalancer != nil && b.Cluster.Spec.Networking.Topology.Bastion.LoadBalancer.AdditionalSecurityGroups != nil {
+			for _, id := range b.Cluster.Spec.Networking.Topology.Bastion.LoadBalancer.AdditionalSecurityGroups {
+				t := &awstasks.SecurityGroup{
+					Name:      fi.PtrTo(id),
+					Lifecycle: b.SecurityLifecycle,
+					ID:        fi.PtrTo(id),
+					Shared:    fi.PtrTo(true),
+				}
+				c.EnsureTask(t)
+				nlb.SecurityGroups = append(nlb.SecurityGroups, t)
+			}
+		}
+
 		c.AddTask(nlb)
 	}
 


Test patch:
diff --git a/tests/integration/update_cluster/bastionadditional_user-data/data/aws_s3_object_cluster-completed.spec_content b/tests/integration/update_cluster/bastionadditional_user-data/data/aws_s3_object_cluster-completed.spec_content
index 775b2018d2..34f6d57b41 100644
--- a/tests/integration/update_cluster/bastionadditional_user-data/data/aws_s3_object_cluster-completed.spec_content
+++ b/tests/integration/update_cluster/bastionadditional_user-data/data/aws_s3_object_cluster-completed.spec_content
@@ -209,6 +209,8 @@ spec:
     zone: us-test-1a
   topology:
     bastion:
-      loadBalancer: {}
+      loadBalancer:
+        additionalSecurityGroups:
+        - sg-exampleid
     dns:
       type: Public
diff --git a/tests/integration/update_cluster/bastionadditional_user-data/kubernetes.tf b/tests/integration/update_cluster/bastionadditional_user-data/kubernetes.tf
index 46012ac231..46ff96364e 100644
--- a/tests/integration/update_cluster/bastionadditional_user-data/kubernetes.tf
+++ b/tests/integration/update_cluster/bastionadditional_user-data/kubernetes.tf
@@ -773,7 +773,7 @@ resource "aws_lb" "bastion-bastionuserdata-example-com" {
   internal                         = false
   load_balancer_type               = "network"
   name                             = "bastion-bastionuserdata-e-4grhsv"
-  security_groups                  = [aws_security_group.bastion-elb-bastionuserdata-example-com.id]
+  security_groups                  = ["sg-exampleid", aws_security_group.bastion-elb-bastionuserdata-example-com.id]
   subnet_mapping {
     subnet_id = aws_subnet.utility-us-test-1a-bastionuserdata-example-com.id
   }