-
Notifications
You must be signed in to change notification settings - Fork 203
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
fix: Remove unknown resource counts from derived inputs #884
fix: Remove unknown resource counts from derived inputs #884
Conversation
Signed-off-by: Christopher Kim <christopher.kim@originate.com>
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
@@ -2,7 +2,7 @@ | |||
# Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl | |||
|
|||
data "oci_core_vcn" "oke" { | |||
count = coalesce(var.vcn_id, "none") != "none" ? 1 : 0 | |||
count = var.create_vcn ? 0 : 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This data source is only used here: https://github.com/oracle-terraform-modules/terraform-oci-oke/blob/main/module-network.tf#L14-L16
As it currently works, if var.create_vcn == true
this gets ignored entirely, and if var.create_vcn == false
and var.vcn_id
is empty, then this module fails elsewhere already
tag_namespace_id = local.tag_namespace_id_found | ||
state = "ACTIVE" // TODO Support reactivation of retired tag w/ update | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only used on line 34 (which I removed)
@@ -31,7 +24,6 @@ locals { | |||
var.create_iam_resources, | |||
var.create_iam_tag_namespace, | |||
local.tag_namespace_id_found == null, | |||
one(data.oci_identity_tags.oke[*].tags) == null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the tag namespace id is found, then this looks for if the namespace has any tags in it, but the previous line/boolean already ensures that the tag namespace id should not be found, so this line never actually matters
modules/network/subnets.tf
Outdated
@@ -79,7 +79,7 @@ locals { | |||
# - Subnet is configured with newbits and/or netnum/cidr | |||
# - Not configured with create == 'never' | |||
# - Not configured with an existing 'id' | |||
subnets_to_create = length(var.vcn_cidrs) > 0 ? merge( | |||
subnets_to_create = merge( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason to safeguard against an empty var.vcn_cidrs
list, the plan will already fail elsewhere if length(var.vcn_cidrs) == 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's the case here, but some defensive coding in the project has related to errors on destroy, such as missing inputs that shouldn't matter blocking cleanup of known state, and we may need to handle the error vs. fail elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devoncrouse Thanks for looking through this PR. Understand the potential issue, any idea of a potential error case I can try to reproduce to see what workarounds are possible? Without a bit more context, one failsafe i can think of is to replace
subnets_to_create = length(var.vcn_cidrs) > 0 ? merge(...) : {}
with
subnets_to_create = try(merge(...), {})
(assuming the issue is that calculating subnets_to_create
throws an unwanted error when var.vcn_cidrs
is empty).
This avoids local.subnets_to_create
from having a dependency on data.oci_core_vcn.oke
from the root module through var.vcn_cidrs
, which is where this problem comes from.
Let me know thoughts, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrizkim 👍 that seems safe, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still fails if, in the subnets
variable, we are providing the newbits
instead of the cidr
attributes.
Here is an example of the subnets variable that can be used for test:
subnets = {
bastion = { newbits = 13 }
operator = { newbits = 13 }
cp = { newbits = 13 }
int_lb = { newbits = 11 }
pub_lb = { newbits = 11 }
workers = { newbits = 2 }
pods = { newbits = 2 }
# bastion = { cidr = "10.0.0.0/29" }
# operator = { cidr = "10.0.0.8/29" }
# cp = { cidr = "10.0.0.16/29" }
# int_lb = { cidr = "10.0.0.64/27" }
# pub_lb = { cidr = "10.0.0.128/27" }
# workers = { cidr = "10.0.8.0/21" }
}
To work around the problem with newbits
is necessary to remove one validation from the subnets_to_create
variable in the file subnets.tf
:
# contains(keys(local.subnet_cidrs_all), k), # has a calculated CIDR range (not id input)
@@ -2,7 +2,7 @@ | |||
# Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl | |||
|
|||
data "oci_identity_availability_domains" "all" { | |||
compartment_id = local.compartment_id | |||
compartment_id = local.tenancy_id != "unknown" ? local.tenancy_id : local.compartment_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is more a limitation of the OCI API and the Terraform provider. If the compartment_id
is not known during planning, then Terraform does not know how many data.oci_identity_availability_domains.all.availability_domains
will exist. When eventually used as a for_each
for module.workers.data.oci_identity_fault_domains.all
, it will fail.
Best option is to try to use the static Tenancy OCID if possible and only fallback to the Compartment OCID if that is unknown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one issue here is for some scenarios where auth != resource tenancy, and the returned AD prefixes are incorrect when the same provider value is used.
Another workaround I can think of might be to define a map local with 3 static entries each conditionally set if available from the data source, so the count is known?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devoncrouse Is the scenario you described that if the provider auth does not have access to the whole tenancy and is restricted to a subcompartment, then listing the availability domains against the tenancy OCID fails? Or do you mean there is a scenario where the subcompartment's ADs differ from the parent tenancy's ADs?
Regarding the local map, if I understood correctly, I still don't think it fully resolves the dependency. If the value of the local map depends on a data source, and that map gets used in a for_each
or count
, Terraform will still be relying on the data source to know the count. Only similar approach I can see is just hard-coding FD-1/2/3
and not bothering with data.oci_identity_fault_domains
at all. Let me know if I'm missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devoncrouse to cover the use case you described we should be more specific in the example we provide for the provider authentication.
tenancy_id
/tenancy_ocid
should be used to target the resource tenancy.
auth_tenancy_id
should be used to override the above in the provider configuration.
Most of the users of this module consider tenancy_id
as the resource tenancy and for me, it makes sense to use it for ADs name.
hi @chrizkim, can you please sign the contributor agreement so we can merge when this review is complete? |
@hyder Signed one a couple weeks back but I just checked and it seems that it got rejected with no note as to why. I'll ask our Oracle reps to look into what happened |
Thanks for letting us know @chrizkim. @alina-yur Is there anything we can do on our side to help? |
Hi @hyder. There's an email thread about this issue; the problem is in missing required information in the OCA request. Once we receive that information, we can approve the PR and the check will pass. |
Thank you for signing the OCA. |
Hi! I appreciate the work that's been done here. Is the plan that this PR will be merged, please? And if so, when will that happen? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved
Terraform plan and apply throw errors if a Compartment is created in the same configuration using this module and passed in through
var.compartment_id
. Same for VCNs created in the same configuration but not created by this module directly (i.e. passed in throughvar.vcn_id
withvar.create_vcn = false
)Fixes for #883