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

Whitelist "internal" annotations/labels that the user is managing in the resource #325

Merged
merged 7 commits into from
May 10, 2019

Conversation

mbarrien
Copy link
Contributor

This PR is an alternative to #244 that closes #199 and closes #200. It is mostly a cherry picking of the strategy used in the fork of @sl1pm4t with a few small updates from me to catch up with the recent changes. I'm hoping it is okay to submit his code, and given that it follows the same licensing, should be mergeable.

At a high level, instead of having the user actively manage a whitelist as #244 does, this instead "automatically whitelists" any annotations/labels that the user is actively stating that they want to manage by its presence in the annotations or labels metadata. To do this, flattenMetadata is passed the resource spec, from which it extracts the list of annotations that the user has specified.

Any annotations/labels added by the Kubernetes system itself with the .kubernetes.io domain are still ignored the same way as before and will not come up as false changes in the plan. If the user does try to modify the annotation/label that is actually internal and system managed, they will get the issues of either continually being asked to update the resource or Kubernetes rejecting the change; the user will learn quickly not to try to manage the annotation. With this, the user no longer has to manage the whitelist at the provider level, reducing confusion.

@rpatrick00
Copy link

This is a much better solution than PR #244.

@dh-harald
Copy link
Contributor

I agree, this solution is much better

@jhoblitt
Copy link
Contributor

@alexsomesan Any thoughts on this approach? I suspect there is a very captive audience for this feature.

@ereOn
Copy link

ereOn commented Apr 25, 2019

I can confirm it works perfectly well and unblocked an otherwise unresolvable situation. I can't comment on the implementation level, but at the user level, this has the behavior I expected to begin with.

@rpatrick00
Copy link

#Hashicorp has a conflict of interest in their Kubernetes competitive product and basically don’t seem to care if the Kubernetes provider works or not. They should let the community maintain this provider.

@legal90
Copy link

legal90 commented Apr 26, 2019

@alexsomesan Could you please take a look at this PR?

@alexsomesan
Copy link
Member

I agree this is an nicer approach. I'm looking over and testing this over the next day or two.

@pdecat
Copy link
Contributor

pdecat commented Apr 26, 2019

I do like that way more than the provider level manual whitelisting option I proposed in 2017 😄

@sl1pm4t
Copy link
Contributor

sl1pm4t commented Apr 27, 2019

Thanks for pushing this across to the official provider @mbarrien !

sl1pm4t and others added 7 commits May 10, 2019 22:47
The official Kubernetes explicitly prevents the use of annotations and labels that have “kubernetes.io” as part of the key name. This is because these are often added server side, by the Kubernetes API and would cause a diff on the Terraform side which has no knowledge of the key.

This commit updates the Terraform logic to allow kubernetes.io keys, but on Read, strip any that aren’t found in the Terraform config / schema. This way a diff is not triggered.
@ghost ghost added size/XL and removed size/M labels May 10, 2019
@alexsomesan
Copy link
Member

I've tested this change manually with a couple of scenarios under EKS, AKS and GKE. Looks great.

I added some mentions of the new behaviour around the docs.
I've also added another acceptance test that uses internal annotations (for load balancers).
Finally, I rebased this on current master to include the Ingress resource and enable it's annotations test.

Running the whole thing in CI now.

Copy link
Member

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is green.
All looks good. Merging.

@alexsomesan alexsomesan merged commit 4fa0271 into hashicorp:master May 10, 2019
@alexsomesan alexsomesan added this to the v1.7.0 milestone May 10, 2019
@tdmalone
Copy link
Contributor

This has now been released in 1.7.0: https://github.com/terraform-providers/terraform-provider-kubernetes/blob/master/CHANGELOG.md#170-may-22-2019
🎉 🚀

@pdecat
Copy link
Contributor

pdecat commented May 29, 2019

this instead "automatically whitelists" any annotations/labels that the user is actively stating that they want to manage by its presence in the annotations or labels metadata.

FTR, the behavior with this implementation when importing existing ingresses is that:

  • configured annotations are not set on the state,
  • a following plan will reveal a diff for those missing annotations despite the fact they are actually set server side,
  • applying will update the ingress' annotations and remove the non-terraform managed annotations (e.g. on GCP: ingress.kubernetes.io/backends, ingress.kubernetes.io/forwarding-rule, etc.)
  • after a few seconds, the ingress controller re-applies the annotations it manages by itself.
  • no diff remains during following plans.

@mbarrien mbarrien deleted the internal-anno branch October 21, 2019 06:26
@ghost ghost locked and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metadata annotations generate an internal Kubernetes annotation errors
10 participants