Revert "meta: Treat internal k8s annotations as invalid"

[mbarrien]

This reverts #50 and closes #60 by allowing "internal" annotations that have "kubernetes.io" in the domain name.

Examples abound for which this "feature" breaks basic usage, such as with:

https://kubernetes.io/docs/concepts/services-networking/service/#ssl-support-on-aws
https://docs.microsoft.com/en-us/azure/aks/internal-lb
https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/
https://github.com/kubernetes-sigs/aws-alb-ingress-controller/blob/master/docs/ingress-resources.md#annotations

For 3rd party Kubernetes plugins, Kubernetes has no control over what annotation names they use, and many others have created custom annotations for their tools that end with kubernetes.io in the part before the slash, and for which the annotation is absolutely essential to get it working.

There was an argument made that this plugin is not meant to support alpha or beta features. The problem is these "alpha" or "beta" feature are heavily relied on in production by many users because of the long beta cycles and the the usefulness of those "beta" but in practice stable features. This provider should not be in the business of saying "this annotation should not be supported because it's beta"; let the developer/ops person decide.

With this over-protectiveness, people will start using direct calls to kubectl via null_resource local-execs; that's even more of a nightmare that lets people shoot themselves in the foot more than having the potential for a few broken annotations here and there (for which a good developer will notice the false changes in the plan, and eventually realize how to fix it).

Let the engineers make their mistakes and trust them to figure it out; don't be so over-protective as to cripple core functionality. If necessary, put a big warning up saying "you better know what you're doing", but don't stop the innovation.

[willscripted]

👏 👏 thank you @mbarrien

This PR unblocks our transition to terraform-defined kubernetes load balancers in eks.

Is the gist of #50 this: Terraform is not the only actor annotating kubernetes resources. Since there are others, reality drifts away from terraform state in acceptable way? But terraform sees diffs and trys to "correct" them by resetting the annotations set by other actors. To avoid this overwrite of legitimate drifted state, #50 flat-out prohibits use of any "risky" annotations.

Did I get that right @mbarrien / @radeksimko ?

[TechnicalMercenary]

Is this going to happen ?

[alexsomesan]

Yes, but not on it’s own.
If we just merge this we are going to break the use cases that depend on the present behavior.
So this has to be paired with a white / blacklisting feature that would allow for control over the set of annotations to be ignored.
I just didn’t have the time to get to that part yet.

[rpatrick00]

How would you propose the user defines/manages this white/blacklisting feature? I am happy to take a shot at implementing it but it would be good to make sure that we agree on how/where the list(s) are defined.

[pdecat]

FYI, @dh-harald has made a POC for the whitelisting feature: #60 (comment)

[nmartin413]

FWIW - not rolling back this restriction makes the resource unusable for non-trivial functionality in the Azure AKS product:

https://docs.microsoft.com/en-us/azure/aks/internal-lb

[rpatrick00]

@nmartin413, with PR #244, you can simply add the list of annotations you want to allow to the provider's whitelist. I believe that this allows us to move forward and use the provider without having to fight the battle of reverting the change. Both AWS EKS and GCP GKE have the same issues with blocking the kubernetes.io annotations.

[rpatrick00]

@alexsomesan, can someone please look at PR #244 and either merge it or provide feedback?

[trevor403]

Is there a timeline for getting this PR accepted and merged?

[tdmalone]

I don't normally do this, but since this provider is almost unusable.... is there a timeline for getting this PR accepted and merged?

[alexsomesan]

Yes, this is the biggest pain point right now. However, it will most likely happen once the support for Terraform 0.12 is fully tested and merged as that work has the top priority in order to deliver a functional provider with the release of 0.12. Also, merging additional functionality is mostly on hold right now as we don’t want to introduce additional uncertainty while we test. Once we merge support for 0.12, development will pick back up as well as merging community work.

On Fri 22. Mar 2019 at 01:26, Tim Malone ***@***.***> wrote: I don't normally do this, but since this provider is almost unusable.... is there a timeline for getting this PR accepted and merged? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#199 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABC-mxvhWCExDG40zHlMUAut3ab-D_T_ks5vZCM7gaJpZM4YCeqS> .

-- — Sent from my phone.

[tdmalone]

While I appreciate these priorities...

...deliver a functional provider with the release of 0.12
...merging additional functionality is mostly on hold right now

I beg to differ, as:

• this provider is almost not functional without this being merged
• this isn't additional functionality; it's a fix for a bug introduced by the merge of meta: Treat internal k8s annotations as invalid #50 in August 2017

[rpatrick00]

The only way to use this provider is to take one of the branches that is functional, build it, and install it as a custom provider. Clearly, no one at Hashicorp cares about the impact of their lack of action on terraform users. Maybe it’s time to move onto Ansible...

…

On Thu, Mar 21, 2019 at 7:51 PM Tim Malone ***@***.***> wrote: While I appreciate these priorities... ...deliver a functional provider with the release of 0.12 ...merging additional functionality is mostly on hold right now I beg to differ, as: - this provider is almost not functional without this being merged - this isn't additional functionality; it's a fix for a bug introduced by the merge of #50 <#50> in August 2017 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#199 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHoqVfKmeRQBnCBuw2fp9UWvNQbs650-ks5vZCkPgaJpZM4YCeqS> .

[tdmalone]

I have created some additional noise at the main Terraform repo: hashicorp/terraform#20788

[jjgrayston]

For anyone else who is currently blocked until this is fixed - my temporary workaround has been to write the service definition to a local file and invoke kubectl apply using a local provisioner. Then use the kubernetes_service data source to access attributes of the newly created service. Ugly but functional.