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

Pass tags when creating EBS ? #603

Closed
eLvErDe opened this issue Apr 20, 2021 · 15 comments
Closed

Pass tags when creating EBS ? #603

eLvErDe opened this issue Apr 20, 2021 · 15 comments

Comments

@eLvErDe
Copy link
Contributor

eLvErDe commented Apr 20, 2021

Hello,

It seems there's no such feature available according to:
https://github.com/fog/fog-aws/blob/master/lib/fog/aws/requests/compute/run_instances.rb#L24

Would you be kind enough to implement that ?

Best regards, Adam.

@geemus
Copy link
Member

geemus commented Apr 20, 2021

Although it's not documented there, it does look like it should pass through any configuration nested there (even if the comments don't mention it). Have you tried to pass tags in that way? I haven't used this in some time, so I feel ill equipped to change/fix it, but I would certainly be happy to support someone else working on it.

@eLvErDe
Copy link
Contributor Author

eLvErDe commented May 18, 2021

Hello @geemus

Thanks for your response, actually, I'm trying to pass custom tags to volume when using fog-aws through Foreman and I'm not sure to understand how I can pass it as a nested configuration.

If you could have a look and let me know what you think... The relevant code snippet is here:

      args[:groups].reject!(&:empty?) if args.has_key?(:groups)
      args[:security_group_ids].reject!(&:empty?) if args.has_key?(:security_group_ids)
      args[:associate_public_ip] = subnet_implies_is_vpc?(args) && args[:managed_ip] == 'public'
      args[:private_ip_address] = args[:interfaces_attributes][:"0"][:ip]
      # Extract root device name and default size from AMI definition
      root_device_definition = root_device_from_ami(image.uuid)
      # Can be used to override root size
      root_device_definition["Ebs.VolumeSize"] = 500
      args[:block_device_mapping] = [ root_device_definition ]

[...]

    def root_device_from_ami(ami_id)
      ami_desc = client.describe_images('image-id' => ami_id)
      root_device_name = ami_desc.body["imagesSet"][0]["rootDeviceName"]
      root_device_definition = ami_desc.body["imagesSet"][0]["blockDeviceMapping"].find {|x| x["deviceName"] == root_device_name}
      root_device_size = root_device_definition["volumeSize"]
      return {:DeviceName => root_device_name, 'Ebs.VolumeSize' => root_device_size}
    end

Thanks a lot in advance

@geemus
Copy link
Member

geemus commented May 18, 2021

Hey, looking at RunInstances documentation I don't see a way to tag EBS specifically/separately. To clarify, do you want to tag just the volumes, or tag everything? It looks like if you set tag specifications on the instance it will also appear on the volume, but it wasn't clear to me if that is what you were after. Could you clarify? Also, have you had luck doing this with other libraries/sdks? Just looking for good examples to better understand what I might be missing. Thanks!

@eLvErDe
Copy link
Contributor Author

eLvErDe commented May 18, 2021

Hello,

Actually the tag are already set on the instance but not added to the volumes (and this is what I need).
I'm sure it's possible because terraform offers a way to add EBS tags:
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/instance#volume_tags

Maybe it has to be set at EC2 instance level ? That would explain because we cannot find anything related to volumes.

Thanks a lot for your help :-)

Adam

@eLvErDe
Copy link
Contributor Author

eLvErDe commented May 18, 2021

Found some volumes/tags ruby SDK doc here:
https://docs.aws.amazon.com/sdk-for-ruby/v3/api/Aws/EC2/Volume.html

Is this the low level SDK you're using ?

@geemus
Copy link
Member

geemus commented May 18, 2021

Ok, I think I found what we needed buried in the docs. Thanks for the extra details to help me narrow in on it.

So, technically I think you can pass through things if you are willing to do a bit of data munging yourself. We can (and probably should) add some handling similar to some of the other parameters to the do the numbering/naming/munging for you though. It looks like what you want is something like:

{ 'TagSpecification.1.ResourceType': 'volume', 'TagSpecification.1.Tag.1.Key': 'foo', 'TagSpecification.1.Tag.1.Value': 'bar' }

Obviously managing all the indexed params manually is a bit of a pain (that's what some of the other special handling code is for in run_instances), but I think it will let you pass something like this through in the mean time.

Does that help? I'd certainly welcome help with a nicer version that would take nested hashes and automatically build the indexed keys and things for the user, but that is obviously a bit more involved.

@geemus
Copy link
Member

geemus commented May 18, 2021

P.S. In particular I found Example 7 helpful here: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_RunInstances.html

@eLvErDe
Copy link
Contributor Author

eLvErDe commented May 18, 2021

Thanks, I'll try to implement this tomorrow in foreman and will let you know if it does something.
Can you explain where the tagspecification hash should be passed ? When instantiating the ec2 instance ?

@geemus
Copy link
Member

geemus commented May 18, 2021

For sure. Doing my best to help out, especially since I feel a bit out of touch with this stuff having not used it recently.

The stuff I was showing would be for during instance instantiation (as it seemed like that is what you were asking for if I understood you). There are also calls for adding tags to existing resources, but they are a bit different. We can talk about that part too if you need it, but again I didn't think that is what the question was about.

So yes, I would expect those tagspecification bits to be passed to the run_instances call, where I think/hope they should just pass through and end up with a running instance and attached EBS with the desired tags.

@eLvErDe
Copy link
Contributor Author

eLvErDe commented May 19, 2021

Hello,

I tried several things but I haven't been able to make it work :/

Have you seen the example here:
https://docs.aws.amazon.com/sdk-for-ruby/v3/api/Aws/EC2/Resource.html

It seems tags_specification can be passed somehow as an array:

  tag_specifications: [
    {
      resource_type: "client-vpn-endpoint", # accepts client-vpn-endpoint, customer-gateway, dedicated-host, dhcp-options, egress-only-internet-gateway, elastic-ip, elastic-gpu, export-image-task, export-instance-task, fleet, fpga-image, host-reservation, image, import-image-task, import-snapshot-task, instance, internet-gateway, key-pair, launch-template, local-gateway-route-table-vpc-association, natgateway, network-acl, network-interface, network-insights-analysis, network-insights-path, placement-group, reserved-instances, route-table, security-group, snapshot, spot-fleet-request, spot-instances-request, subnet, traffic-mirror-filter, traffic-mirror-session, traffic-mirror-target, transit-gateway, transit-gateway-attachment, transit-gateway-connect-peer, transit-gateway-multicast-domain, transit-gateway-route-table, volume, vpc, vpc-peering-connection, vpn-connection, vpn-gateway, vpc-flow-log
      tags: [
        {
          key: "String",
          value: "String",
        },
      ],
    },
  ],

But to be honest I'm completely lost if Foreman's code, I don't even find any call to run_instances.
The file spinning up VMs is here:
https://github.com/theforeman/foreman/blob/develop/app/models/compute_resources/foreman/model/ec2.rb

Can you help me figuring out how it works ? :/
I tried adding the following code:

    # Convert tags to tags specifications, this method is way more powerfull than using :tags API parameter
    # and allow to tag associated volumes with the same tags as the EC2 instances
    # See: https://github.com/fog/fog-aws/issues/603
    # See: https://docs.aws.amazon.com/sdk-for-ruby/v3/api/Aws/EC2/Resource.html
    # Parameter tags must be a validated hash of tag name and tag value
    def tags_to_tags_specs(tags)
      tags_spec = [
        {:resource_type => 'instance', :tags => []},
        {:resource_type => 'volume', :tags => []},
      ]
      tags.each do |key, value|
        tags_spec[0][:tags].push({:key => key, :value => value})
        tags_spec[1][:tags].push({:key => key, :value => value})
      end
      tags_spec
    end

  def create_vm(args = { })
    [...]
    args[:tag_specifications] = tags_to_tags_specs(args[:tags])

I also tried passing TagSpecification.1.ResourceType directly to args hashmap, but in both case I ended up with machine and volume having no tags at all...

Thanks a lot !

@geemus
Copy link
Member

geemus commented May 21, 2021

There is definitely some indirection happening here. I'll try to break it down:

  1. create_vm call here: https://github.com/theforeman/foreman/blob/develop/app/models/compute_resources/foreman/model/ec2.rb#L53
  2. supers to the create_vm call here: https://github.com/theforeman/foreman/blob/develop/app/models/compute_resource.rb#L194
  3. which calls servers.create ending up here: https://github.com/fog/fog-core/blob/master/lib/fog/core/collection.rb#L48
  4. which first calls server.initialize here: https://github.com/fog/fog-aws/blob/master/lib/fog/aws/models/compute/server.rb#L60
  5. then server.save here: https://github.com/fog/fog-aws/blob/master/lib/fog/aws/models/compute/server.rb#L202
  6. which calls servers.save_many here: https://github.com/fog/fog-aws/blob/master/lib/fog/aws/models/compute/servers.rb#L158
  7. which finally calls run_instances

I wouldn't call that particularly clear, but hopefully you can see how it flows now. In building that out, I think it mostly was just passing data through. You could probably test with EXCON_DEBUG=true to see what ends up going over the wire and see what params (or not) are filtering through to the actual call. In addition, your code above looks alright at a glance but it might be good to see the full args data you are working with (just take care to remove anything that might be sensitive/secret/private). I know that doesn't quite answer your question, but hopefully it gives you more to work with.

@eLvErDe
Copy link
Contributor Author

eLvErDe commented May 25, 2021

Hello,

Thanks a lot, I finally managed to get it working, thanks to your latest message and the very first one :D
I just sent a PR which is working fine for me, it allows creation of tags for both ec2 instance and associated volumes just fine !

Regards, Adam.

@geemus
Copy link
Member

geemus commented May 26, 2021

Awesome, thanks for your patience in working through it with me, glad to hear we made it there in the end!

@eLvErDe
Copy link
Contributor Author

eLvErDe commented May 26, 2021

Yeah thanks a lot, I would never managed to sort this out by myself !

@geemus
Copy link
Member

geemus commented May 26, 2021

Definitely, it was a bit surprising even to me how many layers of things this had to navigate.

@geemus geemus closed this as completed in 8d6f0ca Jun 2, 2021
geemus added a commit that referenced this issue Jun 2, 2021
…ations

Implement AWS TagSpecifications (closes #603)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants