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

[FIX] Terraform provider version inconsistency within stages #2862

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

smokestacklightnin
Copy link

@smokestacklightnin smokestacklightnin commented Nov 24, 2024

Reference Issues or PRs

Fixes #2614

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

How to test this PR?

Any other comments?

The main change in this PR is that Terraform/OpenTofu versions and required providers are removed from the stage templates and instead injected via stages.tf_objects with version data from constants.py. stages.tf_objects.NebariOpentofuRequiredVersion and stages.tf_objects.NebariOpentofuRequiredProvider were added.

Now all subclasses of NebariTerraformStage present with a tf_objects method except terraform_state will call include the result of super().tf_objects in their return value:

class FooStage(NebariTerraformStage):
    def tf_objects(self) -> List[Dict]:
        return [
            *super().tf_objects(),
            ...,
        ]    

This way we can also inject NebariOpentofuRequiredVersion into NebariTerraformStage and have subclasses inherit it.

@smokestacklightnin smokestacklightnin requested review from marcelovilla and viniciusdc and removed request for marcelovilla and viniciusdc November 24, 2024 03:13
@smokestacklightnin smokestacklightnin force-pushed the opentofu/stages/sync-versions branch from fdddddb to dd64020 Compare November 25, 2024 03:14
@smokestacklightnin smokestacklightnin force-pushed the opentofu/stages/sync-versions branch from 5c422da to 8923dda Compare November 25, 2024 03:32
@smokestacklightnin smokestacklightnin force-pushed the opentofu/stages/sync-versions branch from d8f1409 to 1c0be1e Compare December 2, 2024 02:02
@smokestacklightnin
Copy link
Author

smokestacklightnin commented Dec 3, 2024

I've executed the changes I intended to make, however, when running Nebari, submodules are not inheriting required providers from the parent module. In particular, this is happening in some of the following modules:

./kubernetes_ingress/template/modules/kubernetes/ingress/
./kubernetes_services/template/modules/kubernetes/cephfs-mount/
./kubernetes_services/template/modules/kubernetes/
./kubernetes_services/template/modules/kubernetes/nfs-mount/
./kubernetes_services/template/modules/kubernetes/forwardauth/
./kubernetes_services/template/modules/kubernetes/nfs-server/
./kubernetes_services/template/modules/kubernetes/services/jupyterhub-ssh/
./kubernetes_services/template/modules/kubernetes/services/jupyterhub/
./kubernetes_services/template/modules/kubernetes/services/keycloak-client/
./kubernetes_services/template/modules/kubernetes/services/
./kubernetes_services/template/modules/kubernetes/services/redis/
./kubernetes_services/template/modules/kubernetes/services/monitoring/
./kubernetes_services/template/modules/kubernetes/services/monitoring/loki/
./kubernetes_services/template/modules/kubernetes/services/postgresql/
./kubernetes_services/template/modules/kubernetes/services/rook-ceph/
./kubernetes_services/template/modules/kubernetes/services/dask-gateway/
./kubernetes_services/template/modules/kubernetes/services/minio/
./kubernetes_services/template/modules/kubernetes/services/argo-workflows/
./kubernetes_services/template/modules/kubernetes/services/conda-store/
./nebari_tf_extensions/template/modules/nebariextension/
./nebari_tf_extensions/template/modules/helm-extensions/
./kubernetes_initialize/template/modules/
./kubernetes_initialize/template/modules/initialization/
./kubernetes_initialize/template/modules/cluster-autoscaler/
./kubernetes_initialize/template/modules/traefik_crds/
./kubernetes_initialize/template/modules/extcr/
./kubernetes_initialize/template/modules/nvidia-installer/
./kubernetes_keycloak/template/modules/kubernetes/keycloak-helm/

Just to see if it would work, I tried adding the correct required providers to those locations. It worked, so this suggest that it might be a good option to be able to dynamically inject required providers from constants.py this would be generalizing what is currently done at the root module to submodules as well.

@viniciusdc Do you have any input?

CC: @marcelovilla

@viniciusdc
Copy link
Contributor

Interesting. Based on the openTofu docs for the child module requirements (https://opentofu.org/docs/language/providers/requirements/), you are indeed right. We would need to inject the provider object into the modules as well. @smokestacklightnin Good catch!

@smokestacklightnin
Copy link
Author

smokestacklightnin commented Dec 10, 2024

I have a proof of concept (base class, method override).

The interface is up for discussion; I'd like others' opinions. If someone doesn't like my NebariTerraformStage.tf_objects_in_module method, it would help me to hear if they have a better idea.

To be honest, I don't love the idea of putting root module required Terraform providers in tf_objects and submodule Terraform required providers in render. I would like to come up with a more elegant solution, but I also want to hear anybody else's ideas or suggestions.

I could conceive of some method in NebariTerraformStage.render that automatically gets Terraform versions and required providers, but that might get messy. Let's see if I can do it cleanly.

CC: @viniciusdc @marcelovilla @dcmcand

@viniciusdc
Copy link
Contributor

Hi @smokestacklightnin thanks for the work so far!

To be honest, I don't love the idea of putting root module required Terraform providers in tf_objects and submodule Terraform required providers in render. I want to come up with a more elegant solution, but I also want to hear anybody else's ideas or suggestions.

I also don't like this. I will play around with these changes this afternoon and some opentofu/terraform configurations to see if we can make this more concise.

@viniciusdc
Copy link
Contributor

Maybe in a future PR we can have the provider settings in a separate json file, not a strong requirement though and only if viable enough

@smokestacklightnin
Copy link
Author

@viniciusdc

# This file is maintained automatically by "tofu init".
# Manual edits may be lost in future updates.

provider "registry.opentofu.org/hashicorp/helm" {
  version = "2.16.1"
  hashes = [
    "redacted"
  ]
}

provider "registry.opentofu.org/hashicorp/kubernetes" {
  version = "2.35.0"
  hashes = [
    "redacted"
  ]
}

provider "registry.opentofu.org/opentofu/helm" {
  version     = "2.1.2"
  constraints = "2.1.2"
  hashes = [
    "redacted"
  ]
}

provider "registry.opentofu.org/opentofu/kubernetes" {
  version     = "2.35.0"
  constraints = ">= 2.20.0"
  hashes = [
    "redacted"
  ]
}

@viniciusdc
Copy link
Contributor

To give more insight, the PR as is looks great! though, during a recent deep dive with @smokestacklightnin I noticed that the warning of misconfiguration terraform spits out when not passing the direct provider blocks into the child modules might be coming due to a possible internal error with how terraform interacts with opentofu.

Based on their docs https://opentofu.org/docs/internals/provider-registry-protocol/ the CLI should be able to import both available aliases for the providers nebari uses, though for many cases both /hashcorps and /opentofu "namespaces" exists leading to the behavior seen in these warnings (my assumption) and which can be seen in the lock file above... I am not entirely sure if they expect this, and I want to do a quick test with simpler k8s .tf deployment unrelated to nebari testing the behavior of provider imports and child modules

There might be a configuration we missed when we migrated.

@smokestacklightnin
Copy link
Author

I want to do a quick test with simpler k8s .tf deployment unrelated to nebari testing the behavior of provider imports and child modules

I have simple examples of using hashicorp and terraform providers with and without injecting providers in submodules. I also included the output of tofu init in output-init.log in each branch. Feel free to build upon what I have there. Long story short, we should inject into submodules so we can arbitrarily specify required providers and avoid misconfiguration errors. This also reflects the OpenTofu documentation:

Only provider configurations are inherited by child modules, not provider source or version requirements. Each module must declare its own provider requirements. This is especially important for non-HashiCorp providers.

@smokestacklightnin
Copy link
Author

Moving forward, @viniciusdc and I discussed two options:

  1. Use internal Nebari machinery to inject necessary required providers into submodules. I have a concept in mind for an implementation for this.
  2. Contribute upstream to Terraform/OpenTofu to implement the functionality of submodules inheriting required provider versions from parent modules

CC: @marcelovilla @dcmcand

@smokestacklightnin smokestacklightnin marked this pull request as ready for review December 23, 2024 19:53
@smokestacklightnin
Copy link
Author

My honest opinion is that it is easier to inject submodule required providers manually now and change to using inherited required providers from parent modules in the future if/when OpenTofu supports this, rather than waiting for another project to merge our changes. I also don't know Go, but am activetly trying to learn.

The code I have written would be easy to remove for the injection process in submodules if OpenTofu started supporting version requirement inheritance in the future. This is because it requires minimal additional machinery, just _tf_objects_required_providers.

We need this feature soon because other issues depend on updating versions (#2806, #2870, #2872).

Ideas? @marcelovilla @viniciusdc @dcmcand

@smokestacklightnin smokestacklightnin requested review from marcelovilla, dcmcand and viniciusdc and removed request for marcelovilla December 23, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New 🚦
Development

Successfully merging this pull request may close these issues.

[BUG] - Terraform provider version inconsistency within stages
2 participants