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

Since v0.5.x foreman_host.<name>.name is inconsistent #116

Closed
togoschi opened this issue Feb 2, 2023 · 3 comments
Closed

Since v0.5.x foreman_host.<name>.name is inconsistent #116

togoschi opened this issue Feb 2, 2023 · 3 comments

Comments

@togoschi
Copy link
Contributor

togoschi commented Feb 2, 2023

With provider versions < 0.5.0 a reference of the foreman_host name attribute points to the short hostname. After upgrading to 0.5.x the name reference becomes inconsistent. An initial plan will still show the short name but apply will set the FQDN.

An example terraform snippet for this misbehaviour is this HashiCorp Vault PKI resource using a reference to name attribute of the foreman_host resource as common name attribute for the subject DN of the certificate

resource "vault_pki_secret_backend_cert" "dashboard" {
...
  common_name = foreman_host.osd[0].name
...
}

tf apply will lead to the following error

╷
│ Error: Provider produced inconsistent final plan
│ 
│ When expanding the plan for vault_pki_secret_backend_cert.dashboard to
│ include new values learned so far during apply, provider
│ "registry.terraform.io/hashicorp/vault" produced an invalid new value for
│ .common_name: was cty.StringVal("vxstocph101"), but now
│ cty.StringVal("vxstocph101.hcp-dev-infra.hanse-merkur.de").
│ 
│ This is a bug in the provider, which should be reported in the provider's
│ own issue tracker.
╵
ERRO[0107] 1 error occurred:
	* exit status 1

Anyway the next tf apply will fix and create the failing resource.

But above all with 0.5.x i have to change common name attributes in all resources like above because the common name should be the FQDN in my deployment. With a version < v0.5.x the common name attribute pointed to "shortname.domain" ("${foreman_host.osd[0].name}.${data.foreman_domain.hcp-infra.name}") but after upgrading the provider i need to change the common name of these resources to foreman_host.osd[0].name as shown above because the name reference of the foreman_host resource points finally already to the FQDN

@agriffit79
Copy link
Collaborator

I've just hit the same issue. It was introduced in 2fa9ff2 when the custom host unmashall function was removed.

It's really a bug in the Foreman API - you set one value and another is returned. But we'll have to figure out some way to handle it in the provider.

bitkeks added a commit to mindfulsecurity/terraform-provider-foreman that referenced this issue May 17, 2023
Implements the API calls for ReadSetting and QuerySetting, allowing a
read-only usage of the Foreman settings API.

As an example, the setting "append_domain_name_for_hosts" is read in the
resourceForemanHostCreate func to handle the issue with short names and
FQDNs as return value.

Refs terraform-coop#116
bitkeks added a commit to mindfulsecurity/terraform-provider-foreman that referenced this issue May 17, 2023
This commit introduces the new attributes "fqdn" and "shortname" to the
Terraform provider for foreman_host objects. Formerly, the "name"
attribute was used to configure and read the hostname of a host. But
this value can be inconsistent, because Foreman might expand a short
hostname to an FQDN, depending on a setting. (this is not a bug)

Because of this unexpected behaviour (results in "inconstistent plan" in
Terraform) the name attribute is now bypassed. Shortname is the new
required input variable.

Summary:
* name: was required, is now optional and computed
* fqdn: new, read-only, always returns the FQDN
* shortname: required and checked for dots (".") to issue warnings

Refs terraform-coop#116
@bitkeks
Copy link
Collaborator

bitkeks commented May 17, 2023

Hi, PR #121 introduces a possible solution to handle the name field.

It's really a bug in the Foreman API - you set one value and another is returned. But we'll have to figure out some way to handle it in the provider.

It's probably not a bug, but a feature. The setting append_domain_name_for_hosts does exactly what it says: you put in a short hostname and Foreman converts it to an FQDN. This FQDN is then returned to the provider. If you do not cut the domain part, the provider will be confused because it gave another input. This breaks if the returned value is used in the same apply workflow (Terraform uses the term "learned during apply").

bitkeks added a commit that referenced this issue Jul 19, 2023
… "shortname" (required) (#121)

* Add more tracef log entries; small formattings

* resourceForemanHost: Move resourceForemanHostCustomizeDiff in customdiff.All

* Add fqdn and shortname attributes to Foreman host, handle name

This commit introduces the new attributes "fqdn" and "shortname" to the
Terraform provider for foreman_host objects. Formerly, the "name"
attribute was used to configure and read the hostname of a host. But
this value can be inconsistent, because Foreman might expand a short
hostname to an FQDN, depending on a setting. (this is not a bug)

Because of this unexpected behaviour (results in "inconstistent plan" in
Terraform) the name attribute is now bypassed. Shortname is the new
required input variable.

Summary:
* name: was required, is now optional and computed
* fqdn: new, read-only, always returns the FQDN
* shortname: required and checked for dots (".") to issue warnings

Refs #116

* Add constructShortname func to Foreman host API wrapper

"Shortname" does not exist in the Foreman API, only "name" which is
either shortname or FQDN depending on a setting. For a more expected
behaviour the shortname and fqdn fields were introduced. This commit
fills the Shortname field in the ForemanHost API struct.

* Allow name to not have dots

* Fix shortname/fqdn tests; make name argument deprecated and read-only

The tests use an API mock response file to check against a mocked
Terraform schema. Since neither "shortname" nor "fqdn" host attributes
are in the Foreman API, we exclude them in the tests. They are
constructed from the "name" given by Foreman, or passed in in case of
the shortname.

To enforce the switch from "name" to "shortname", name is now a
deprecated argument. It is still accessible via the name attribute,
which is filled when the resource is read from the API.

* Move resourceForemanHostCustomizeDiffComputeAttributes into extra func again
bitkeks added a commit that referenced this issue Jul 19, 2023
* Implement settings API as data source

Implements the API calls for ReadSetting and QuerySetting, allowing a
read-only usage of the Foreman settings API.

As an example, the setting "append_domain_name_for_hosts" is read in the
resourceForemanHostCreate func to handle the issue with short names and
FQDNs as return value.

Refs #116

* Add settings_type to Terraform setting data resource; add example .tf

Adds the settings_type field to the Terraform data resource definition.
See the created example on how to use the setting DR in Terraform.

* Disable hostname check in resourceForemanHostCreate using settings API

For testing and as an example, the resourceForemanHostCreate used the
newly added settings API to check the hostname of possible
inconsistencies. As @agriffit79 mentioned in the PR #120 the DomainName
value will always be empty / evaluate to false since it is computed.
Therefore, the check is redundant.

It might happen that the DomainName attribute will become available in
other use cases (e.g. directly specified on the host?) - that's why the
check is disabled and not removed completely at the moment.

* Settings API: remove TF warnings on type conversion

Refs #120

* Remove commented leftover code for settings API query example

* Add tests for settings API / settings data resource

Adds Terraform schema tests and testdata JSON files for
the Foreman settings API.

The test structure is quite complicated..
@bitkeks
Copy link
Collaborator

bitkeks commented Jul 25, 2023

Fixed in v0.6.0 and documented in README after merge of #132

@bitkeks bitkeks closed this as completed Jul 25, 2023
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

3 participants