Skip to content

Commit

Permalink
Merge pull request #402 from alexander-demichev/tags
Browse files Browse the repository at this point in the history 
Bug 1952611: [OCPCLOUD-1115] Get instance tags from infrastructure object
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored Apr 22, 2021
2 parents fd44227 + 2fac8aa commit ec9ced7
Show file tree
Hide file tree
Showing 7 changed files with 231 additions and 20 deletions.
10 changes: 10 additions & 0 deletions pkg/actuators/machine/actuator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ import (
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/golang/mock/gomock"
. "github.com/onsi/gomega"
configv1 "github.com/openshift/api/config/v1"
machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/tools/record"
Expand All @@ -23,6 +25,7 @@ import (
func init() {
// Add types to scheme
machinev1.AddToScheme(scheme.Scheme)
configv1.AddToScheme(scheme.Scheme)
}

func TestMachineEvents(t *testing.T) {
Expand Down Expand Up @@ -145,6 +148,13 @@ func TestMachineEvents(t *testing.T) {
gs.Expect(k8sClient.Delete(ctx, machine)).To(Succeed())
}()

// Create infrastructure object
infra := &configv1.Infrastructure{ObjectMeta: metav1.ObjectMeta{Name: awsclient.GlobalInfrastuctureName}}
gs.Expect(k8sClient.Create(ctx, infra)).To(Succeed())
defer func() {
gs.Expect(k8sClient.Delete(ctx, infra)).To(Succeed())
}()

// Ensure the machine has synced to the cache
getMachine := func() error {
machineKey := types.NamespacedName{Namespace: machine.Namespace, Name: machine.Name}
Expand Down
33 changes: 28 additions & 5 deletions pkg/actuators/machine/instances.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

"github.com/aws/aws-sdk-go/aws/awserr"
configv1 "github.com/openshift/api/config/v1"
machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
mapierrors "github.com/openshift/machine-api-operator/pkg/controller/machine"
"github.com/openshift/machine-api-operator/pkg/metrics"
Expand All @@ -23,7 +24,7 @@ import (
awsclient "sigs.k8s.io/cluster-api-provider-aws/pkg/client"
)

// Scan machine tags, and return a deduped tags list
// Scan machine tags, and return a deduped tags list. The first found value gets precedence.
func removeDuplicatedTags(tags []*ec2.Tag) []*ec2.Tag {
m := make(map[string]bool)
result := []*ec2.Tag{}
Expand Down Expand Up @@ -272,7 +273,7 @@ func getBlockDeviceMappings(machine runtimeclient.ObjectKey, blockDeviceMappingS
return blockDeviceMappings, nil
}

func launchInstance(machine *machinev1.Machine, machineProviderConfig *awsproviderv1.AWSMachineProviderConfig, userData []byte, client awsclient.Client) (*ec2.Instance, error) {
func launchInstance(machine *machinev1.Machine, machineProviderConfig *awsproviderv1.AWSMachineProviderConfig, userData []byte, client awsclient.Client, infra *configv1.Infrastructure) (*ec2.Instance, error) {
machineKey := runtimeclient.ObjectKey{
Name: machine.Name,
Namespace: machine.Namespace,
Expand Down Expand Up @@ -315,7 +316,7 @@ func launchInstance(machine *machinev1.Machine, machineProviderConfig *awsprovid
return nil, mapierrors.InvalidMachineConfiguration("Unable to get cluster ID for machine: %q", machine.Name)
}
// Add tags to the created machine
tagList := buildTagList(machine.Name, clusterID, machineProviderConfig.Tags)
tagList := buildTagList(machine.Name, clusterID, machineProviderConfig.Tags, infra)

tagInstance := &ec2.TagSpecification{
ResourceType: aws.String("instance"),
Expand Down Expand Up @@ -410,9 +411,13 @@ func launchInstance(machine *machinev1.Machine, machineProviderConfig *awsprovid
return runResult.Instances[0], nil
}

func buildTagList(machineName string, clusterID string, machineTags []awsproviderv1.TagSpecification) []*ec2.Tag {
// buildTagList compile a list of ec2 tags from machine provider spec and infrastructure object platform spec
func buildTagList(machineName string, clusterID string, machineTags []awsproviderv1.TagSpecification, infra *configv1.Infrastructure) []*ec2.Tag {
rawTagList := []*ec2.Tag{}
for _, tag := range machineTags {

mergedTags := mergeInfrastructureAndMachineSpecTags(machineTags, infra)

for _, tag := range mergedTags {
// AWS tags are case sensitive, so we don't need to worry about other casing of "Name"
if !strings.HasPrefix(tag.Name, "kubernetes.io/cluster/") && tag.Name != "Name" {
rawTagList = append(rawTagList, &ec2.Tag{Key: aws.String(tag.Name), Value: aws.String(tag.Value)})
Expand All @@ -422,9 +427,27 @@ func buildTagList(machineName string, clusterID string, machineTags []awsprovide
{Key: aws.String("kubernetes.io/cluster/" + clusterID), Value: aws.String("owned")},
{Key: aws.String("Name"), Value: aws.String(machineName)},
}...)

return removeDuplicatedTags(rawTagList)
}

// mergeInfrastructureAndMachineSpecTags merge list of tags from machine provider spec and Infrastructure object platform spec.
// Machine tags have precedence over Infrastructure
func mergeInfrastructureAndMachineSpecTags(machineSpecTags []awsproviderv1.TagSpecification, infra *configv1.Infrastructure) []awsproviderv1.TagSpecification {
if infra == nil || infra.Status.PlatformStatus == nil || infra.Status.PlatformStatus.AWS == nil || infra.Status.PlatformStatus.AWS.ResourceTags == nil {
return machineSpecTags
}

mergedList := []awsproviderv1.TagSpecification{}
mergedList = append(mergedList, machineSpecTags...)

for _, tag := range infra.Status.PlatformStatus.AWS.ResourceTags {
mergedList = append(mergedList, awsproviderv1.TagSpecification{Name: tag.Key, Value: tag.Value})
}

return mergedList
}

type instanceList []*ec2.Instance

func (il instanceList) Len() int {
Expand Down
169 changes: 159 additions & 10 deletions pkg/actuators/machine/instances_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
configv1 "github.com/openshift/api/config/v1"
awsproviderv1 "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsprovider/v1beta1"
"sigs.k8s.io/controller-runtime/pkg/client"

Expand Down Expand Up @@ -58,35 +59,128 @@ func TestRemoveDuplicatedTags(t *testing.T) {

func TestBuildTagList(t *testing.T) {
cases := []struct {
tagList []awsproviderv1.TagSpecification
expected []*ec2.Tag
name string
machineSpecTags []awsproviderv1.TagSpecification
infra *configv1.Infrastructure
expected []*ec2.Tag
}{
{
tagList: []awsproviderv1.TagSpecification{},
name: "with empty infra and provider spec should return default tags",
machineSpecTags: []awsproviderv1.TagSpecification{},
infra: &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
PlatformStatus: &configv1.PlatformStatus{
AWS: &configv1.AWSPlatformStatus{
ResourceTags: []configv1.AWSResourceTag{},
},
},
},
},
expected: []*ec2.Tag{
{Key: aws.String("kubernetes.io/cluster/clusterID"), Value: aws.String("owned")},
{Key: aws.String("Name"), Value: aws.String("machineName")},
},
},
{
name: "with empty infra should return default tags",
machineSpecTags: []awsproviderv1.TagSpecification{},
infra: &configv1.Infrastructure{}, // should work with empty infra object
expected: []*ec2.Tag{
{Key: aws.String("kubernetes.io/cluster/clusterID"), Value: aws.String("owned")},
{Key: aws.String("Name"), Value: aws.String("machineName")},
},
},
{
tagList: []awsproviderv1.TagSpecification{
name: "with nil infra should return default tags",
machineSpecTags: []awsproviderv1.TagSpecification{},
infra: nil, // should work with nil infra object
expected: []*ec2.Tag{
{Key: aws.String("kubernetes.io/cluster/clusterID"), Value: aws.String("owned")},
{Key: aws.String("Name"), Value: aws.String("machineName")},
},
},
{
name: "should filter out bad tags from provider spec",
machineSpecTags: []awsproviderv1.TagSpecification{
{Name: "Name", Value: "badname"},
{Name: "kubernetes.io/cluster/badid", Value: "badvalue"},
{Name: "good", Value: "goodvalue"},
},
infra: nil,
// Invalid tags get dropped and the valid clusterID and Name get applied last.
expected: []*ec2.Tag{
{Key: aws.String("good"), Value: aws.String("goodvalue")},
{Key: aws.String("kubernetes.io/cluster/clusterID"), Value: aws.String("owned")},
{Key: aws.String("Name"), Value: aws.String("machineName")},
},
},
{
name: "should filter out bad tags from infra object",
machineSpecTags: []awsproviderv1.TagSpecification{},
infra: &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
PlatformStatus: &configv1.PlatformStatus{
AWS: &configv1.AWSPlatformStatus{
ResourceTags: []configv1.AWSResourceTag{
{
Key: "kubernetes.io/cluster/badid",
Value: "badvalue",
},
{
Key: "Name",
Value: "badname",
},
{
Key: "good",
Value: "goodvalue",
},
},
},
},
},
},
// Invalid tags get dropped and the valid clusterID and Name get applied last.
expected: []*ec2.Tag{
{Key: aws.String("good"), Value: aws.String("goodvalue")},
{Key: aws.String("kubernetes.io/cluster/clusterID"), Value: aws.String("owned")},
{Key: aws.String("Name"), Value: aws.String("machineName")},
},
},
{
name: "tags from machine object should have precedence",
machineSpecTags: []awsproviderv1.TagSpecification{
{Name: "Name", Value: "badname"},
{Name: "kubernetes.io/cluster/badid", Value: "badvalue"},
{Name: "good", Value: "goodvalue"},
},
infra: &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
PlatformStatus: &configv1.PlatformStatus{
AWS: &configv1.AWSPlatformStatus{
ResourceTags: []configv1.AWSResourceTag{
{
Key: "good",
Value: "should-be-overwritten",
},
},
},
},
},
},
expected: []*ec2.Tag{
{Key: aws.String("good"), Value: aws.String("goodvalue")},
{Key: aws.String("kubernetes.io/cluster/clusterID"), Value: aws.String("owned")},
{Key: aws.String("Name"), Value: aws.String("machineName")},
},
},
}
for i, c := range cases {
actual := buildTagList("machineName", "clusterID", c.tagList)
if !reflect.DeepEqual(c.expected, actual) {
t.Errorf("test #%d: expected %+v, got %+v", i, c.expected, actual)
}
t.Run(c.name, func(t *testing.T) {
actual := buildTagList("machineName", "clusterID", c.machineSpecTags, c.infra)
if !reflect.DeepEqual(c.expected, actual) {
t.Errorf("test #%d: expected %+v, got %+v", i, c.expected, actual)
}
})
}
}

Expand Down Expand Up @@ -350,7 +444,24 @@ func TestLaunchInstance(t *testing.T) {
}

providerConfig := stubProviderConfig()
stubTagList := buildTagList(machine.Name, stubClusterID, providerConfig.Tags)
stubTagList := buildTagList(machine.Name, stubClusterID, providerConfig.Tags, nil)

infra := &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
PlatformStatus: &configv1.PlatformStatus{
AWS: &configv1.AWSPlatformStatus{
ResourceTags: []configv1.AWSResourceTag{
{
Key: "infra-tag-key",
Value: "infra-tag-value",
},
},
},
},
},
}

stubTagListWithInfraObject := buildTagList(machine.Name, stubClusterID, providerConfig.Tags, infra)

cases := []struct {
name string
Expand All @@ -366,6 +477,7 @@ func TestLaunchInstance(t *testing.T) {
instancesErr error
succeeds bool
runInstancesInput *ec2.RunInstancesInput
infra *configv1.Infrastructure
}{
{
name: "Security groups with filters",
Expand Down Expand Up @@ -738,6 +850,43 @@ func TestLaunchInstance(t *testing.T) {
name: "Dedicated instance tenancy",
providerConfig: stubInvalidInstanceTenancy(),
},
{
name: "Attach infrastructure object tags",
providerConfig: providerConfig,
infra: infra,
runInstancesInput: &ec2.RunInstancesInput{
IamInstanceProfile: &ec2.IamInstanceProfileSpecification{
Name: aws.String(*providerConfig.IAMInstanceProfile.ID),
},
ImageId: aws.String(*providerConfig.AMI.ID),
InstanceType: &providerConfig.InstanceType,
MinCount: aws.Int64(1),
MaxCount: aws.Int64(1),
KeyName: providerConfig.KeyName,
TagSpecifications: []*ec2.TagSpecification{{
ResourceType: aws.String("instance"),
Tags: stubTagListWithInfraObject,
}, {
ResourceType: aws.String("volume"),
Tags: stubTagListWithInfraObject,
}},
NetworkInterfaces: []*ec2.InstanceNetworkInterfaceSpecification{
{
DeviceIndex: aws.Int64(providerConfig.DeviceIndex),
AssociatePublicIpAddress: providerConfig.PublicIP,
SubnetId: providerConfig.Subnet.ID,
Groups: []*string{
aws.String("sg-00868b02fbe29de17"),
aws.String("sg-0a4658991dc5eb40a"),
aws.String("sg-009a70e28fa4ba84e"),
aws.String("sg-07323d56fb932c84c"),
aws.String("sg-08b1ffd32874d59a2"),
},
},
},
UserData: aws.String(""),
},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
Expand All @@ -750,7 +899,7 @@ func TestLaunchInstance(t *testing.T) {
mockAWSClient.EXPECT().DescribeImages(gomock.Any()).Return(tc.imageOutput, tc.imageErr).AnyTimes()
mockAWSClient.EXPECT().RunInstances(tc.runInstancesInput).Return(tc.instancesOutput, tc.instancesErr).AnyTimes()

_, launchErr := launchInstance(machine, tc.providerConfig, nil, mockAWSClient)
_, launchErr := launchInstance(machine, tc.providerConfig, nil, mockAWSClient, tc.infra)
t.Log(launchErr)
if launchErr == nil {
if !tc.succeeds {
Expand Down
12 changes: 11 additions & 1 deletion pkg/actuators/machine/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,15 @@ import (

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
configv1 "github.com/openshift/api/config/v1"
machinecontroller "github.com/openshift/machine-api-operator/pkg/controller/machine"
"github.com/openshift/machine-api-operator/pkg/metrics"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/types"
errorutil "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/klog/v2"
awsclient "sigs.k8s.io/cluster-api-provider-aws/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client"

awsproviderv1 "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsprovider/v1beta1"
)
Expand Down Expand Up @@ -64,7 +67,14 @@ func (r *Reconciler) create() error {
return fmt.Errorf("failed to get user data: %w", err)
}

instance, err := launchInstance(r.machine, r.providerSpec, userData, r.awsClient)
infra := &configv1.Infrastructure{}
infraName := client.ObjectKey{Name: awsclient.GlobalInfrastuctureName}

if err := r.client.Get(r.Context, infraName, infra); err != nil {
return err
}

instance, err := launchInstance(r.machine, r.providerSpec, userData, r.awsClient, infra)
if err != nil {
klog.Errorf("%s: error creating machine: %v", r.machine.Name, err)
conditionFailed := conditionFailed()
Expand Down
Loading

0 comments on commit ec9ced7

Please sign in to comment.