Rely properly on the asset_dir, proper abort on kubectl fails

[frimik]

PR o'clock

Description

Implement a more reliable way for kubectl errors to propagate. Reliability is also discussed in #199

Add asset_dir variable where auth and manifests get written. Manifest and config write-outs are no longer optional. Make them "first-class" instead. Operator responsibility to clean up if clean-up is needed.
Since files are written temporarily anyway, I see no clear motivation to keeping them "hidden" other than to avoid admin mistakes (git commit etc).

Variables removed:

• config_output_path
• write_kubeconfig
• write_aws_auth_config

Checklist

• terraform fmt and terraform validate both work from the root and examples/eks_test_fixture directories (look in CI for an example)
• Tests for the changes have been added and passing (for bug fixes/features)
• Test results are pasted in this PR (in lieu of CI)
• I've added my change to CHANGELOG.md
• Any breaking changes are highlighted above

[max-rocket-internet]

Thanks for the PR @frimik!

Implement a more reliable way for kubectl errors to propagate.

That sounds great and would help #199 but how is this related to "write-outs are no longer optional"?

Manifest and config write-outs are no longer optional

I'm not sure everyone wants this so let's wait and get some more opinions 🙂

Currently, if you want to save the files you can just set config_output_path variable. If you don't want the files then you don't have to set anything.

[frimik]

Thanks for the PR @frimik!

Implement a more reliable way for kubectl errors to propagate.

That sounds great and would help #199 but how is this related to "write-outs are no longer optional"?

It was just the simplest way to avoid some of the complexity in the shell script parts. Just write the files on disk once. Leaving the clean-up to the operator to decide.

Manifest and config write-outs are no longer optional

I'm not sure everyone wants this so let's wait and get some more opinions

Yep.. understand.

I'm using this module now from a "wrapper" module.
The wrapper module sets up EKS CNI CIDRs and Subnets, launches the cluster using terraform-aws-eks, then utilizes the outputted kubeconfig to stuff more "base setup yaml" inside the cluster. (ENIConfigs etc).

So it just makes sense to me to remove the "temporary write-out to disk", and piggy-back on the implicit asset_dir output instead.

The kubectl apply parts could obviously be done in a different way. But today, all methods in terraform concerning "apply manifests" are hacks anyway...

Either way, the reliability part seems to be fixed by this. I yet haven't seen a single false terraform run in about 50 of them since fixing it.

[max-rocket-internet]

I'm using this module now from a "wrapper" module. The wrapper module sets up EKS CNI CIDRs and Subnets, launches the cluster using terraform-aws-eks, then utilizes the outputted kubeconfig to stuff more "base setup yaml" inside the cluster. (ENIConfigs etc).

Sounds nice! Is it public? Perhaps you could paste a link here so people will find it??

[max-rocket-internet]

Either way, the reliability part seems to be fixed by this. I yet haven't seen a single false terraform run in about 50 of them since fixing it.

OK cool but then what is this PR for now? Because I don't think people will be happy with removing those 2 write_xx options. And the asset_dir option is really just the same as what we have as config_output_path, right?

[max-rocket-internet]

No update in almost a month so will close. Feel free to reopen, rebase and answer the questions when you are ready 🙂

[morganchristiansson]

His work in progress ENIConfig work is here #316. This is not the wrapper however.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.