Build Scenarios

[agriffit79]

We've had some churn in the code-base the last year with conflicting PRs around the foreman_host resource. PRs were submitted and accepted to make changes to the build process which broke some build scenarios. PRs were then submitted by other contributors to revert those changes. The root-cause seems to be that there are a number of different ways of using Foreman to build/manage hosts and it's sometimes difficult for contributors to understand how their proposed changes will impact build methods they don't personally use.

What I would like to achieve in this discussion is to list all the possible build scenarios, the desired behaviour of the plugin in each case and what the resource declaration should look like. Below is a list of what I believe to be the possible scenarios. Please comment if there are any I have missed.

• Kickstart. PXE or ISO boot with customisation via kickstart pre/post scripts.
  • Bare-metal with power management. Calls are made to some BMC to toggle power and request network boot.
  • Bare-metal with no power management. Power toggle and network boot are performed out-of-band of Foreman (e.g. manually on the console).
  • Managed VM. Calls to some virtualisation layer (e.g. KVM, oVirt, vSphere) to create VM instance and retrieve MAC address(es).
  • Un-managed VM. No integration with virtualisation layer. VMs instance is created out-of-band of Foreman and MAC(s) manually added to Foreman. This is essentially the same as bare-metal with no power management.
• Image based. Calls to some virtualisation/cloud platform (e.g. AWS, GCP, Openstack, oVirt) to clone from an existing image. Build is customised either through user-data or shell script over SSH.
• Un-managed. Host entry is created in Foreman but the build/clone process is orchestrated independently. A use-case for this method is to use Foreman as an ENC for Puppet (how I'm planning to use it). In this case it may be desired (but not mandatory) to have the build flag set to true as ForemanProxy/PuppetCA will refuse to sign certificate requests otherwise (depending on Foreman configuration driven by security requirements).

It's also my opinion that host rebuilds should not be orchestrated through the plugin. My view is that Terraform declarations should be idempotent and requesting a transitional state like "rebuild" fundamentally conflicts with that.

[thatsk]

This is what we want and think about build method or provision method to clear confusion

[bitkeks]

Hi @agriffit79, thank you so much for the detailed write-up!

It's also my opinion that host rebuilds should not be orchestrated through the plugin. My view is that Terraform declarations should be idempotent and requesting a transitional state like "rebuild" fundamentally conflicts with that.

True, and IMO one of the most important points to consider in the discussion around the build-flag.

Re-building a host with Foreman, without destroying the resource via Terraform, should be kept out of the Terraform plugin. Users that need this use case should use the Foreman UI (using the "Build" button on the host's page) or the Foreman API directly.

@agriffit79 would you consider implementing your use case outside of Terraform? Triggering the build flag for ENC certificate creation? From my POV this scenario would be better covered by Tools like Puppet/Chef/Ansible than Terraform.

[agriffit79]

Hi @bitkeks , glad we're in agreement on the rebuild functionality.

Regards build flag. The purpose is to bootstrap Puppet, so I cannot really use Puppet to bootstrap its self. But that's just one use case, there may well be others. But don't worry, it's a feature I'll add later myself. I'm not using this in production yet, so won't break anything for me.

[bitkeks]

I see, maybe not Puppet but a simple script that talks to the Foreman API..

Regarding breaking changes: Merging the PRs and then releasing the next version, e.g. 0.6.0 will not break existing setups anyway if they still stay on 0.5.x.

[agriffit79]

Sure, let's park it for the moment.

[thatsk]

@agriffit79 when we are expecting with all these features to test it out? officially in terraform provider registry

[bitkeks]

FYI, I already compiled the master branch and all my PRs into a new binary. We did some testing and found no blockers so far. Especially image-based provisioning seems to be on a good path now.

[togoschi]

From the beginning since @lhw has forked from https://github.com/wayfair/terraform-provider-foreman, our main concern in further development of the provider was the deployment of bare-metal machines without any manual intervention. For this purpose @lhw has extended the provider with the functions of power management (BMC), amongst many other helpful things. Our machines are either completely built using Foreman templates or configured with Ansible after built. We use the same build workflow for pre-production enviroments, but with the difference that we use the virtualization layer in Foreman (VMware) instead. As @lhw already wrote we don't use Puppet at all with Foreman.

In order to put the provider development on a broader basis, @lhw moved the code base to a public repository when he left our company. But the changes like deprecation of method and introduction of build and managed flags since the first 0.5 release have caused insecurities in our production environment. Since then every code change was potentially dangerous because of toggling the build flag of foreman_host resources. In the early days of using 0.5 releases at least more than once a machine was unintenionally rebuilt after reboot (caused by power outage e.g.). I think this behaviour is absolutely inacceptable for production environments!

To fix these (and further) issues we have asked @bitkeks a while ago to work on the provider. We still appreciate if a broader community will develop the provider furthermore. But at first we need to consolidate the code regarding this issue - i think.

[agriffit79]

Thanks for the background. TBH - I had not realised the build flag was such a big problem as there isn't actually an open issue for it. I thought the biggest problem was image based building was broken as reported in #106.

[thatsk]

one more thing: I saw the official terraform provider I saw there needs an upgrade for SDK any insights on the official provider for that?

[bitkeks]

What message did you see? Do you mean https://registry.terraform.io/providers/terraform-coop/foreman/latest ?

[bitkeks]

@agriffit79 picking up your comment in https://github.com/terraform-coop/terraform-provider-foreman/pull/122/files#r1199028040

I think this is too restrictive and misses some use cases. If provision method is image then a finish script will still be run on the host and that will call back to Foreman to update build status.

Referring to https://theforeman.org/manuals/3.6/index.html#4.4.6Workflow, you probably mean this stage?:

I also have a use case where I provision hosts un-managed but still rely on build status to sign Puppet certificates.
I wonder if we need an explicit parameter that controls whether build=true will be set on host creation?

See #125 (comment)

I'm therefore continuing with the PR as-is and we will find a way to support the ENC-only use-case in the next release iteration 👍

[bitkeks]

Moving on to release v0.6.0

From my POV we covered all open discussions around use cases and the issue of the host build/method arguments.
Are there any issues left that block merging the PRs and releasing v0.6.0?

@agriffit79 unfortunately, you seem to be the only available reviewer right now. Do you have capacity to approve my PRs? 😅 The tests might fail due to an existing error in the master branch, as mentioned in #124.

[agriffit79]

Sorry, really busy at work at the mo. Difficult to keep track of this. Don't want to hold you guys up, but don't want to push a bad build ether.

I think my main outstanding concern is the validation of compute_attributes. Not all resources require image_id field, so your checks will break them. More fundamentally I don't think it's practical to do any validation on this field as the payloads vary for each backend provider.

[bitkeks]

Are you referring to https://github.com/terraform-coop/terraform-provider-foreman/pull/122/files#diff-f00877e0ef958de1f3b9af8377bbbfc516fc38305f5329b0e82ed92fe1f1f1b9R1066 ? You are absolutely right that this differs depending on the compute resource provider. That's why I tried to insert a check to figure out the backend, which unfortunately did not work. I'll remove the code and we will have to work more with the example files.

[bitkeks]

Removed in 6d60bff

[thatsk]

@agriffit79 are we pushing to an official provider registry
which is terraform registry?

[bitkeks]

@thatsk Yes, the newly tagged version 0.6.0 is available through https://registry.terraform.io/providers/terraform-coop/foreman/0.6.0. It supports the usage of provision_method = "image".

@agriffit79 originally, the PRs

• Add more fields to Foreman host  #123
• foreman_host: Replace the "name" attribute with "fqdn" (computed) and "shortname" (required) #121
• Implement settings API as data source #120

were meant to be included in 0.6.0 as well. Do you see any open issues that block merging them into master?

I will review the merge conflict in PR #121 and my comment https://github.com/terraform-coop/terraform-provider-foreman/pull/121/files#r1226612772.

We'll have to release 0.6.1 then, including the new features and fields.

[thatsk]

Just curious @bitkeks @agriffit79 if we are changing the subnet id and network of the existing machine it should destroy and recreate the machine but looks like terraform lifecycle messed up this one ideally it should destroy as change. Thoughts?

~ update in-place

Terraform will perform the following actions:

  # module.foreman.foreman_host.host["node"] will be updated in-place
  ~ resource "foreman_host" "host" {
        id                      = "7448"
        name                    = "nodename"
        # (22 unchanged attributes hidden)

      ~ interfaces_attributes {
          ~ compute_attributes = {
              ~ "network" = "VM-164 Network" -> "VM-24 Network"
                # (1 unchanged element hidden)
            }
            id                 = 7449
            name               = "nodename"
          ~ subnet_id          = 6 -> 9
            # (7 unchanged attributes hidden)
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.
module.foreman.foreman_host.host["node"]: Modifying... [id=7448]
╷
│ Error: HTTP Error:{
│   endpoint:   [https://foreman/api/hosts/7448]
│   statusCode: [422]
│   respBody:   [{
│   "error": {"id":7448,"errors":{"interfaces.ip":["does not match selected subnet"]},"full_messages":["Ip does not match selected subnet"]}
│ }
│ ]
│ }
│ 
│   with module.foreman.foreman_host.host["node"],
│   on .terraform/modules/foreman/main.tf line 25, in resource "foreman_host" "host":
│   25: resource "foreman_host" "host" {
│ 

[thatsk]

are we adding this ?

[agriffit79]

Would all users want or even expect that behaviour? For example on bare-metal maybe you change the subnet but don't want to delete the host? You might be able to achieve that same behaviour by using the replace_triggered_by meta paramater without requiring any changes in the provider.

[thatsk]

Can you give me example with resource? So that I can try

[agriffit79]

This is the documentation - I've never personally used the feature, so it might not do what you want.

[thatsk]

let say if we use this and add some attributes here does it gonna override our existing one which is mentioned by @bitkeks ?

[holmesb]

Hi,

"Re-building a host with Foreman, without destroying the resource via Terraform, should be kept out of the Terraform plugin. Users that need this use case should use the Foreman UI"

I introduced the build argument. It is needed because the world has shifted from snowflake servers that are built once and then endlessly configured (often manually) to immutable phoenix servers. Instead of endlessly configuring, upgrading, and patching, hoping new problems\bugs aren't introduced, you replace\rebuild, testing in less critical environments first, and can quickly return to a known good state when problems arise. This approach is in keeping with modern software development practices (eg 12 factor app).

My company has remained on the version of this provider with my build argument, so we haven't tested @bitkeks subsequent PR and I'm just catching up with this discussion.

I think build's removal was done without knowledge of how the this flag is used. When this provider is run with build = true, just as with the webui, your host gets built\rebuilt. Foreman provides a "built" REST endpoint that you toggle in your code (eg kickstart\cloud-init script) to disable the build flag and avoid a repeated build on next reboot. Also, we never commit build = true to git, only = false. We only pass true as a terraform -var flag in our CI\CD pipeline script. So there is never a rebuild loop, and rebuilds only occur when this pipeline is run, or someone toggles it on their local and runs terraform. So rebuilds cannot be accidental. All that was needed IMO, was advice or a reminder in documentation that if you leave build = true (or commit it permanently), you'll be stuck in a rebuild loop. At the time, I thought this would be obvious enough, but apparently not. I think @bitkeks saying this is "dangerous and should be avoided" is excessive. The perceived\actual risk could have been reduced instead of removing the rebuild functionality completely.

The Cobbler provider for terraform, which I also contributed to, supports rebuilds, and I believe this provider should too. Phoenix cattle servers are good, endlessly groomed pet servers are a legacy mindset. Rebuilding should be a frequent\routine operation that is supported by this provider. I agree terraform is declarative and should not accommodate an intermediate state, but the desired end-state is immutable infrastructure, which the build flag facilitated. Seems the only way to achieve immutability now (>= v6) is by point-and-click in the webui, rather than Terraform. Removal of the ability to rebuild is anti-infrastructure-as-code.

We'll stay on < v6 for the time being. I hope it's possible to somehow reinstate the ability to rebuild hosts using this provider. Would you accept a new argument on foreman_host that makes it clearer what the purpose is? Eg a rebuild boolean instead of build that is clearly documented? I propose that unlike build, rebuild will default to false, so you would have to specifically change it to cause something "dangerous".

Cheers.

[bitkeks]

Hi @holmesb,
thanks for joining the discussion! Yeah, from your POV it might seem like a decision made too drasticly.

Let me start with your last paragraph:

I hope it's possible to somehow reinstate the ability to rebuild hosts using this provider. Would you accept a new argument on foreman_host that makes it clearer what the purpose is? Eg a rebuild boolean instead of build that is clearly documented?

Yes, that's what I proposed in the same PR #122 you referenced:

We will have to find an additional attribute to support the use case of "enforced rebuild" that originally lead to the switch.

---

On to the problem of the "build" flag:

rebuilds cannot be accidental

I think @bitkeks saying this is #122 (comment) is excessive

As far as I see it, interpreting your text and viewing the change from your perspective, my comment might seem excessive in your context: you understand what the build flag does and that setting/leaving it on true recreates existing hosts. You might think this would be obvious enough but given that at least one user accidentally destroyed existing servers by a simple terraform apply, it is not. Especially since the rebuilt only happens after a host reboot, so that there might be a time delay between running Terraform and the rebuild.

Why did I call it dangerous? Because the default value for the build flag was set to true (snippet from v0.5.8):

terraform-provider-foreman/foreman/resource_foreman_host.go

Lines 404 to 410 in 4a410d1

	"build": {
	Type: schema.TypeBool,
	Optional: true,
	Default: true,
	Description: "Whether or not this host's build flag will be enabled in Foreman. Default is true, " +
	"which means host will be built at next boot.",
	},

This means that a second run of terraform apply will reset the build flag on all existing hosts in Foreman, if the build flag is not explicitly set to false in Terraform. But this requires understanding the flag..

Don't get me wrong, I know what the build flag does and how the Foreman API and provisioning workflows use it. And I understand your use case. But to an unsuspecting user of the provider plugin, the implementation in v0.5.x can lead to destructive and unexpected behaviour that should be avoided.

Your proposal for a rebuild flag is the way we'd go as well. If you don't mind we will stay on this track then.

PS: As seen in this thread, use cases differ. That's why enforcing a cattle mindset with immuntable servers might not suffice all users of the provider.

PPS: And not to forget, my PR also re-introduced the provision_method (was called method) parameter, which was removed completely before, making it impossible to use image-based provisioning.

[holmesb]

OK, I'm with you on changing default from true to false. If there are no objections, I'll reintroduce build as a new argument: rebuild. Will need to test how this interacts with current behaviour.

1. Now (v6): no build or rebuild exists, initial creation of foreman_host results in it being built, since this is the Foreman default. Re-running Terraform will not rebuild.
2. Proposed: rebuild = false (default): How I expect it will work: this provider does not tell Foreman API to disable the build flag. Instead, since Foreman defaults to true (ie build), at initial creation of foreman_host in Terraform still results in host being built. Re-running of Terraform will not rebuild.
3. Proposed: rebuild = true: Initial creation of foreman_host results in it being built. Re-run of Terraform will rebuild. Changing back to false will prevent rebuild.

Only downside vs pre-v6's build is:
A. Setting rebuild = true initially is a bit confusing, IE, rebuilding a new host (that doesn't exist yet) is counter-intuitive. But I doubt people will ever set = true in this context, since new hosts will be initially built regardless of rebuild flag.
B. With old build = false, could create a host in Foreman, but not build it. I don't think we've ever needed to do this, and I can't imagine a use case, but maybe is some demand. I don't think this is a deal-breaker.

If there are no objections, I'll transfer the outcome of this discussion into a new issue: "Re-add ability to rebuild".

Re your PPS, TBH, with the name method, I didn't really know what this arg was for, as docs weren't clear. I understand why it's needed now, so thanks for renaming to provisioning_method.

[bitkeks]

Looks good to me! I'll defer working on a rebuild-PR then and let you create a proposal.

How I expect it will work: this provider does not tell Foreman API to disable the build flag.

Check.

Re-run of Terraform will rebuild.

Check, but in Foreman speak: will reset the build flag in the API, causing a host AFTER REBOOT to be re-provisioned. See the following:

Setting rebuild = true initially is a bit confusing, IE, rebuilding a new host (that doesn't exist yet) is counter-intuitive. But I doubt people will ever set = true in this context, since new hosts will be initially built regardless of rebuild flag.

I was thinking about an alternative name for the argument to find a better match for the use case. The most obvious alternative would be to use set_build_flag instead of rebuild. It best describes what is done, because "rebuild" is vague in Foreman context. Terraform does not rebuild a host - and neither does Foreman if you do not additionally reboot the host after setting the flag in the Foreman API.

With old build = false, could create a host in Foreman, but not build it. I don't think we've ever needed to do this, and I can't imagine a use case, but maybe is some demand. I don't think this is a deal-breaker.

We had the use case for "registering hosts", so adding hosts that are already provisioned by another tool. @agriffit79 might have meant to include this use case in his list under "un-managed". Either way, that's the only use case I can think of that needs the build flag to be disabled from the start. But then again, this raises the question about support of this use case in the Terraform provider.

[holmesb]

"that's the only use case I can think of that needs the build flag to be disabled from the start" - well currently (v6), we don't support this scenario. I don't think there's any way. If we force set_build_flag = false to actually change the API, hosts won't initially build, and you don't want default to be true.

[thatsk]

@bitkeks Another just idea is after creating VM resource how do we ensure VM is up can't we add local exec to check uptime whether the VM is fully up and running. ?

[thatsk]

rather then creating another null_resource for maintaining it.

[holmesb]

In the absence of further comments on the rebuild subject, I've created issue #130. @bitkeks, I probably won't have time to deliver a PR for some weeks, so seeing as we are aligned, happy for you to step in if you've more bandwidth.

[bitkeks]

Will do, thanks!