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

Add metadata annotations to generated apex objects #1034

Merged
merged 1 commit into from
Oct 27, 2021
Merged

Add metadata annotations to generated apex objects #1034

merged 1 commit into from
Oct 27, 2021

Conversation

jonnylangefeld
Copy link
Contributor

Some third party software relies on annotations and labels on istios VirtualServices. For instance external-dns makes use of the external-dns.alpha.kubernetes.io/controller annotation. Currently there is no way to set labels and annotations on the VirtualService resource.

This change takes the metadata from the canary.Spec.Service.Apex property to replicate exactly what is already possible for a traefik resource:

tsMetadata := canary.Spec.Service.Apex
if tsMetadata == nil {
tsMetadata = &flaggerv1.CustomMetadata{}
}
if tsMetadata.Labels == nil {
tsMetadata.Labels = make(map[string]string)
}
if tsMetadata.Annotations == nil {
tsMetadata.Annotations = make(map[string]string)
}

Fix #854

Signed-off-by: Jonny Langefeld [email protected]

@stefanprodan stefanprodan changed the title Add metadata to istio VirtualService Add metadata to generated apex objects Oct 15, 2021
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @jonnylangefeld 🏅

@stefanprodan stefanprodan added the kind/enhancement Improvement request for an existing feature label Oct 26, 2021
@stefanprodan stefanprodan changed the title Add metadata to generated apex objects Add metadata annotations to generated apex objects Oct 26, 2021
Some third party software relies on annotations and labels on istios VirtualServices. For instance external-dns makes use of the `external-dns.alpha.kubernetes.io/controller` annotation. Currently there is no way to set labels and annotations on the VirtualService resource.

This change takes the metadata from the `canary.Spec.Service.Apex` property to replicate exactly what is already possible for a traefik resource:
https://github.com/fluxcd/flagger/blob/c36a13ccffefbda1502bf02e8cac2f1b3ca9d027/pkg/router/traefik.go#L59-L68

Fix #854

Signed-off-by: Jonny Langefeld <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Merging #1034 (d5994ac) into main (01d4780) will increase coverage by 0.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1034      +/-   ##
==========================================
+ Coverage   56.88%   57.15%   +0.26%     
==========================================
  Files          76       76              
  Lines        6093     6138      +45     
==========================================
+ Hits         3466     3508      +42     
- Misses       2102     2103       +1     
- Partials      525      527       +2     
Impacted Files Coverage Δ
pkg/router/appmesh.go 85.42% <100.00%> (+0.45%) ⬆️
pkg/router/appmesh_v1beta2.go 88.12% <100.00%> (+0.30%) ⬆️
pkg/router/contour.go 88.54% <100.00%> (+0.36%) ⬆️
pkg/router/gloo.go 75.79% <100.00%> (+1.03%) ⬆️
pkg/router/istio.go 81.07% <100.00%> (-0.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01d4780...d5994ac. Read the comment docs.

@stefanprodan stefanprodan merged commit d832937 into fluxcd:main Oct 27, 2021
@MaesterZ
Copy link
Contributor

MaesterZ commented Oct 28, 2021

If I am not mistaken this will add annotations to both the Kubernetes services and Istio virtualservices, not sure what happens with external-dns.alpha.kubernetes.io/target in this case especially if you want the annotation only on one of them (the service or the virtual service).

Also this new feature/behavior should be mentioned in the docs?

@stefanprodan
Copy link
Member

@Misteur-Z my understanding is that external-dns doesn't look at VirtualServices by default, you need to enable it with --source=istio-virtualservice and doing so, Kubernetes services are ignored.

@stefanprodan
Copy link
Member

Also this new feature/behavior should be mentioned in the docs?

Thanks! I've added a note to docs here 6f65907

@MaesterZ
Copy link
Contributor

MaesterZ commented Oct 29, 2021

@Misteur-Z my understanding is that external-dns doesn't look at VirtualServices by default, you need to enable it with --source=istio-virtualservice and doing so, Kubernetes services are ignored.

It's fine if you only use the virtual service source for External DNS, but you could also enable multiple ones (service + ingress + virtualservice), there is no limitations.
https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/istio.md

@stefanprodan
Copy link
Member

@Misteur-Z could you please make a PR and add a warning on external-dns? Thanks for raising this, I think users should be made aware that enabling both Istio and Service sources would result in conflicts.

@jonnylangefeld
Copy link
Contributor Author

An alternative would be to create a new field on the canary to propagate metadata just to a specific apex (like only VirtualServiec or only Service). But that obviously would overload the canary object quite a bit

@jonnylangefeld jonnylangefeld deleted the jlf/add-metadata-to-istio-vs branch October 29, 2021 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvement request for an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotate Istio VirtualService
4 participants