OCPBUGS-29391: Fix instance PrivateDNSName when domain-name is set in dhcpOpts

jparrill · jparrill · commit 362a226b10ef · 2024-04-11T12:03:50.000+02:00
When a VPC sets a DHCPOption's domain-name with a custom DNS name, it's being ignored. Now, with this PR, ensure that during instance creation, the controller checks the DHCPOptions and sets the proper PrivateDNSName.

Signed-off-by: Juan Manuel Parrilla Madrid &lt;jparrill@redhat.com&gt;
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go b/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go
@@ -131,6 +131,7 @@ func (t Template) ControllersPolicy() *iamv1.PolicyDocument {
 				"ec2:DescribeSecurityGroups",
 				"ec2:DescribeSubnets",
 				"ec2:DescribeVpcs",
+				"ec2:DescribeDhcpOptions",
 				"ec2:DescribeVpcAttribute",
 				"ec2:DescribeVpcEndpoints",
 				"ec2:DescribeVolumes",
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/customsuffix.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/customsuffix.yaml
@@ -190,6 +190,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/default.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/default.yaml
@@ -190,6 +190,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_all_secret_backends.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_all_secret_backends.yaml
@@ -196,6 +196,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_allow_assume_role.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_allow_assume_role.yaml
@@ -190,6 +190,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_bootstrap_user.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_bootstrap_user.yaml
@@ -196,6 +196,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_custom_bootstrap_user.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_custom_bootstrap_user.yaml
@@ -196,6 +196,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_different_instance_profiles.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_different_instance_profiles.yaml
@@ -190,6 +190,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_console.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_console.yaml
@@ -190,6 +190,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_default_roles.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_default_roles.yaml
@@ -190,6 +190,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_disable.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_disable.yaml
@@ -190,6 +190,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_kms_prefix.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_kms_prefix.yaml
@@ -190,6 +190,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_extra_statements.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_extra_statements.yaml
@@ -196,6 +196,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml
@@ -190,6 +190,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_ssm_secret_backend.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_ssm_secret_backend.yaml
@@ -190,6 +190,7 @@ Resources:
           - ec2:DescribeSecurityGroups
           - ec2:DescribeSubnets
           - ec2:DescribeVpcs
+          - ec2:DescribeDhcpOptions
           - ec2:DescribeVpcAttribute
           - ec2:DescribeVpcEndpoints
           - ec2:DescribeVolumes
diff --git a/controllers/awsmachine_controller_test.go b/controllers/awsmachine_controller_test.go
@@ -114,6 +114,10 @@ func TestAWSMachineReconcilerIntegrationTests(t *testing.T) {
 		}}})
 		g.Expect(err).To(BeNil())
 		cs.Cluster = &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"}}
+		cs.AWSCluster.Spec.NetworkSpec.VPC = infrav1.VPCSpec{
+			ID:        "vpc-exists",
+			CidrBlock: "10.0.0.0/16",
+		}
 		cs.AWSCluster.Status.Network.APIServerELB.DNSName = DNSName
 		cs.AWSCluster.Spec.ControlPlaneLoadBalancer = &infrav1.AWSLoadBalancerSpec{
 			LoadBalancerType: infrav1.LoadBalancerTypeClassic,
@@ -283,6 +287,10 @@ func TestAWSMachineReconcilerIntegrationTests(t *testing.T) {
 		g.Expect(err).To(BeNil())
 		cs.Cluster = &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"}}
 		cs.AWSCluster.Status.Network.APIServerELB.DNSName = DNSName
+		cs.AWSCluster.Spec.NetworkSpec.VPC = infrav1.VPCSpec{
+			ID:        "vpc-exists",
+			CidrBlock: "10.0.0.0/16",
+		}
 		cs.AWSCluster.Spec.ControlPlaneLoadBalancer = &infrav1.AWSLoadBalancerSpec{
 			LoadBalancerType: infrav1.LoadBalancerTypeClassic,
 		}
diff --git a/pkg/cloud/services/ec2/instances.go b/pkg/cloud/services/ec2/instances.go
@@ -25,6 +25,7 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
 	"github.com/pkg/errors"
 	"k8s.io/utils/ptr"
 
@@ -913,6 +914,8 @@ func (s *Service) SDKToInstance(v *ec2.Instance) (*infrav1.Instance, error) {
 
 func (s *Service) getInstanceAddresses(instance *ec2.Instance) []clusterv1.MachineAddress {
 	addresses := []clusterv1.MachineAddress{}
+	// Check if the DHCP Option Set has domain name set
+	domainName := s.GetDHCPOptionSetDomainName(s.EC2Client, instance.VpcId)
 	for _, eni := range instance.NetworkInterfaces {
 		privateDNSAddress := clusterv1.MachineAddress{
 			Type:    clusterv1.MachineInternalDNS,
@@ -922,8 +925,18 @@ func (s *Service) getInstanceAddresses(instance *ec2.Instance) []clusterv1.Machi
 			Type:    clusterv1.MachineInternalIP,
 			Address: aws.StringValue(eni.PrivateIpAddress),
 		}
+
 		addresses = append(addresses, privateDNSAddress, privateIPAddress)
 
+		if domainName != nil {
+			// Add secondary private DNS Name with domain name set in DHCP Option Set
+			additionalPrivateDNSAddress := clusterv1.MachineAddress{
+				Type:    clusterv1.MachineInternalDNS,
+				Address: fmt.Sprintf("%s.%s", strings.Split(privateDNSAddress.Address, ".")[0], *domainName),
+			}
+			addresses = append(addresses, additionalPrivateDNSAddress)
+		}
+
 		// An elastic IP is attached if association is non nil pointer
 		if eni.Association != nil {
 			publicDNSAddress := clusterv1.MachineAddress{
@@ -937,6 +950,7 @@ func (s *Service) getInstanceAddresses(instance *ec2.Instance) []clusterv1.Machi
 			addresses = append(addresses, publicDNSAddress, publicIPAddress)
 		}
 	}
+
 	return addresses
 }
 
@@ -1035,6 +1049,54 @@ func (s *Service) ModifyInstanceMetadataOptions(instanceID string, options *infr
 	return nil
 }
 
+// GetDHCPOptionSetDomainName returns the domain DNS name for the VPC from the DHCP Options.
+func (s *Service) GetDHCPOptionSetDomainName(ec2client ec2iface.EC2API, vpcID *string) *string {
+	log := s.scope.GetLogger()
+
+	if vpcID == nil {
+		log.Info("vpcID is nil, skipping DHCP Option Set discovery")
+		return nil
+	}
+
+	vpcInput := &ec2.DescribeVpcsInput{
+		VpcIds: []*string{vpcID},
+	}
+
+	vpcResult, err := ec2client.DescribeVpcs(vpcInput)
+	if err != nil {
+		log.Info("failed to describe VPC, skipping DHCP Option Set discovery", "vpcID", *vpcID, "Error", err.Error())
+		return nil
+	}
+
+	dhcpInput := &ec2.DescribeDhcpOptionsInput{
+		DhcpOptionsIds: []*string{vpcResult.Vpcs[0].DhcpOptionsId},
+	}
+
+	dhcpResult, err := ec2client.DescribeDhcpOptions(dhcpInput)
+	if err != nil {
+		log.Error(err, "failed to describe DHCP Options Set", "DhcpOptionsSet", *dhcpResult)
+		return nil
+	}
+
+	for _, dhcpConfig := range dhcpResult.DhcpOptions[0].DhcpConfigurations {
+		if *dhcpConfig.Key == "domain-name" {
+			if len(dhcpConfig.Values) == 0 {
+				return nil
+			}
+			domainName := dhcpConfig.Values[0].Value
+			// default domainName is 'ec2.internal' in us-east-1 and 'region.compute.internal' in the other regions.
+			if (s.scope.Region() == "us-east-1" && *domainName == "ec2.internal") ||
+				(s.scope.Region() != "us-east-1" && *domainName == fmt.Sprintf("%s.compute.internal", s.scope.Region())) {
+				return nil
+			}
+
+			return domainName
+		}
+	}
+
+	return nil
+}
+
 // filterGroups filters a list for a string.
 func filterGroups(list []string, strToFilter string) (newList []string) {
 	for _, item := range list {
diff --git a/pkg/cloud/services/ec2/instances_test.go b/pkg/cloud/services/ec2/instances_test.go
