-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: add annotation and label filters to Ambassador Host Source #2633
base: master
Are you sure you want to change the base?
feat: add annotation and label filters to Ambassador Host Source #2633
Conversation
Welcome @KyleMartin901! |
8c6851c
to
a427434
Compare
I have also added the label filter to the Ambassador host source as per suggestion by @alebedev87 in #2043 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just some cosmetic comments.
source/ambassador_host.go
Outdated
// Filter Ambassador Hosts | ||
ambassadorHosts, err = sc.filterByAnnotations(ambassadorHosts) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "failed to filter Ambassador Hosts") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, errors.Wrap(err, "failed to filter Ambassador Hosts") | |
return nil, errors.Wrap(err, "failed to filter Ambassador Hosts by annotation") |
source/ambassador_host_test.go
Outdated
title: "no host", | ||
targetNamespace: "", | ||
labelSelector: labels.Everything(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
title: "no host", | |
targetNamespace: "", | |
labelSelector: labels.Everything(), | |
title: "no host", | |
labelSelector: labels.Everything(), |
Just to safe 1 line from each test case which doesn't need the target namespace.
BTW I didn't see any test case using the targetNamespace, specifying it is supposed to filter hosts too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry forgot to add the test case in. I was going to test to make sure the Ambassador host was only added if it is within the External DNS targeted namespace like the following sources do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the Target Namespace test now 51e3633
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the test case for the namespace!
The other tests cases can remove targetNamespace
field as it's set to the empty string by default - will save 1 line for each line.
expectError: true, | ||
}, | ||
{ | ||
title: "valid matching annotation filter label", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this test case? The one with the filter expression has already tested the parsing into the labelSelector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you again copied the test from Contour, Ingress, Istio Gateway and the service tests. Happy to remove it if you would like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it doesn't bring any added value, I'd prefer to remove it - smaller is simpler for the further maintenance.
source/ambassador_host_test.go
Outdated
{ | ||
title: "valid matching label filter expression", | ||
targetNamespace: "", | ||
// annotationFilter: "kubernetes.io/ingress.class in (external-ingress)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed if not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned this up 1629661 sorry forgot to go through and make sure was all clean
source/ambassador_host_test.go
Outdated
hostname: "fake1.org", | ||
annotations: map[string]string{ | ||
"external-dns.ambassador-service": "emissary-ingress/emissary", | ||
"kubernetes.io/ingress.class": "external-ingress", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This annotation can be removed as we don't use any annotation filter, just to keep the test to absolute minimum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I have removed the kubernetes.io/ingress.class
annotations from the label tests where they weren't being used. I still need to keep the external-dns.ambassador-service
annotation as that is how External DNS knows to assign the DNS record to the correct Ambassador Host service/endpoint.
source/ambassador_host_test.go
Outdated
expected: []*endpoint.Endpoint{}, | ||
}, | ||
{ | ||
title: "valid matching label filter expression for single host", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case seems to be a "superset" of valid matching label filter expression
one, so why not keeping only this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now removed valid matching label filter expression
as you are correct I am already matching valid label within the valid matching label filter expression for single host
test.
Sorry I was trying to match tests like what was done within the annotations without really thinking about it.
source/ambassador_host_test.go
Outdated
ti := ti | ||
t.Run(ti.title, func(t *testing.T) { | ||
// Create a slice of Ambassador Hosts | ||
ambassadorHosts := make([]*ambassador.Host, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal at all but I've seen all the possible ways of creating a slice of ambassador hosts in this PR:
var ambassadorHosts []*ambassador.Host
---
filteredList := []*ambassador.Host{}
---
ambassadorHosts := make([]*ambassador.Host, 0)
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that copy and paste issues. Thanks for picking that up for me totally missed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have cleaned this up in 2482ef5 decided to use the short declaration operator
f10f6eb
to
25aff9e
Compare
…notation As suggested by @alebedev87 kubernetes-sigs#2633 (comment) Co-authored-by: Andrey Lebedev <alebedev87@gmail.com>
Correcting the inconsistancy in the way an empty slice of Ambassador Hosts were declared so it is clean and clearer. Thanks to @alebedev87 for catching this kubernetes-sigs#2633 (comment)
@alebedev87 thanks for picking up those issues. I wasn't sure on the process of adding in the changes if it is preferred for the changes to be added in as new commits or squashed so the PR still only had the two commits for adding annotation filter and label filter. I have just added them as seperate commits so it is easy to squash if that's what is preferred. Let me know if you would prefer me to squash them or happy as is. |
Removing the `valid matching label filter expression` test in favour of just using `valid matching label filter expression for single host` as it is testing the same thing that a Ambassador Host with a valid label is matched. Disscussed with @alebedev87 in kubernetes-sigs#2633 (comment)
Removing the annoations that are not required for the label tests to keep the test to an absolute minimum based on conversations with @alebedev87 kubernetes-sigs#2633 (comment)
…notation As suggested by alebedev87 in kubernetes-sigs#2633 (comment) Co-authored-by: Andrey Lebedev <alebedev87@gmail.com>
Correcting the inconsistancy in the way an empty slice of Ambassador Hosts were declared so it is clean and clearer. Thanks to alebedev87 for catching this in kubernetes-sigs#2633 (comment)
Removing the `valid matching label filter expression` test in favour of just using `valid matching label filter expression for single host` as it is testing the same thing that a Ambassador Host with a valid label is matched. Disscussed with alebedev87 in kubernetes-sigs#2633 (comment)
Removing the annoations that are not required for the label tests to keep the test to an absolute minimum based on conversations with alebedev87 kubernetes-sigs#2633 (comment)
f166698
to
ed61d9b
Compare
@KyleMartin901: I'm not aware of any strict rules about the commits. However I don't think that the commits made to address the review remarks really need to be upstream. |
f702e14
to
eda671a
Compare
Since author is not answering, I'm closing this PR. |
@mloiseleur: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Sorry I am just getting back from 6 weeks holiday where I had my notifications turned off. I’ll fix up the issues this weekend @mloiseleur |
sure, no problem |
@mloiseleur: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This change makes the Ambassador Host source respect the External-DNS annotationFilter allowing for an Ambassador Host resource to specify what External-DNS deployment to use when there are multiple External-DNS deployments within the same cluster. Before this change if you had two External-DNS deployments within the cluster and used the Ambassador Host source the first External-DNS to process the resource will create the record and not the one that was specified in the filter annotation. I added the `filterByAnnotations` function so that it matched the same way the other sources have implemented annotation filtering. I didn't add the controller check only because I wanted to keep this change to implementing the annotationFilter. I added Endpoint tests to validate that the filterByAnnotations function works as expected. Again these tests were based of the Endpoint tests that other sources use. To keep the tests simpler I only allow for a single load balancer to be used. Example: Create two External-DNS deployments 1 public and 1 private and set the Ambassador Host to use the public External-DNS using the annotation filter. ``` --- apiVersion: apps/v1 kind: Deployment metadata: name: external-dns-private spec: strategy: type: Recreate selector: matchLabels: app: external-dns-private template: metadata: labels: app: external-dns-private annotations: iam.amazonaws.com/role: {ARN} # AWS ARN role spec: serviceAccountName: external-dns containers: - name: external-dns image: k8s.gcr.io/external-dns/external-dns:latest args: - --source=ambassador-host - --domain-filter=example.net # will make ExternalDNS see only the hosted zones matching provided domain, omit to process all available hosted zones - --provider=aws - --policy=upsert-only # would prevent ExternalDNS from deleting any records, omit to enable full synchronization - --aws-zone-type=private # only look at public hosted zones (valid values are public, private or no value for both) - --registry=txt - --txt-owner-id= {Hosted Zone ID} # Insert Route53 Hosted Zone ID here - --annotation-filter=kubernetes.io/ingress.class in (private) --- apiVersion: apps/v1 kind: Deployment metadata: name: external-dns-public spec: strategy: type: Recreate selector: matchLabels: app: external-dns-public template: metadata: labels: app: external-dns-public annotations: iam.amazonaws.com/role: {ARN} # AWS ARN role spec: serviceAccountName: external-dns containers: - name: external-dns image: k8s.gcr.io/external-dns/external-dns:latest args: - --source=ambassador-host - --domain-filter=example.net # will make ExternalDNS see only the hosted zones matching provided domain, omit to process all available hosted zones - --provider=aws - --policy=upsert-only # would prevent ExternalDNS from deleting any records, omit to enable full synchronization - --aws-zone-type= # only look at public hosted zones (valid values are public, private or no value for both) - --registry=txt - --txt-owner-id= {Hosted Zone ID} # Insert Route53 Hosted Zone ID here - --annotation-filter=kubernetes.io/ingress.class in (public) --- apiVersion: getambassador.io/v3alpha1 kind: Host metadata: name: your-hostname annotations: external-dns.ambassador-service: emissary-ingress/emissary kubernetes.io/ingress.class: public spec: acmeProvider: authority: none hostname: your-hostname.example.com ``` Fixes kubernetes-sigs#2632
Currently the `--label-filter` flag can only be used to filter CRDs, Ingress, Service and Openshift Route objects which match the label selector passed through that flag. This change extends the functionality to the Ambassador Host type object. When the flag is not specified the default value is `labels.Everything()` which is an empty string, the same as before. Annotation based filter is inefficient because the filtering has to be done in the controller instead of the API server like with label filtering. The Annotation based filtering has been left in for legacy reasons so the Ambassador Host source can be used inconjunction with the other sources that don't yet support label filltering. It is possible to use label based filltering with annotation based filltering so you can initially filter by label then filter the returned hosts by annotation. This is not recomended
86da547
to
c7ae7f1
Compare
Add that the Ambassador Host source now supports both annotation and label filltering.
@mloiseleur thanks for reopening the PR and giving me the time to rebase and update the docs. I have now pushed those changes, let me know if there is anything else you would like me to do. |
/ok-to-test |
Add the expected record type to the Ambassador Host source tests so they pass.
@mloiseleur sorry about the failed tests, I have fixed them up by adding the now required record type. Sorry I was lazy and just did the rebase and didn't rerun the tests but I have now run the tests and they are all passing. I added a lazy commit let me know if you want it squashed in or changed. |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: szuecs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@KyleMartin901: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@KyleMartin901 This PR has been approved. It means that it just needs a rebase, to pass tests and it will be merged. |
Thanks @mloiseleur i will attempt to get this done this week. Looks like someone got tests merged in before mine so going to refactor to match what has already been merged |
This change makes the Ambassador Host source respect the External-DNS annotation-filter and label-filter allowing for an Ambassador Host resource to specify what External-DNS deployment to use when there are multiple External-DNS deployments within the same cluster. Before this change if you had two External-DNS deployments within the cluster and used the Ambassador Host source the first External-DNS to process the resource will create the record and not the one that was specified in the filter annotation.
Annotation Fillter
I added the
filterByAnnotations
function so that it matched the same way the other sources have implemented annotation filtering. I didn't add the controller check only because I wanted to keep this change to implementing the annotationFilter.I added Endpoint tests to validate that the filterByAnnotations function works as expected. Again these tests were based of the Endpoint tests that other sources use. To keep the tests simpler I only allow for a single load balancer to be used.
Example: Create two External-DNS deployments 1 public and 1 private and set the Ambassador Host to use the public External-DNS using the annotation filter.
Fixes #2632
Label Filter
Currently, the
--label-filter
flag can only be used to filter CRDs, Ingress, Service and Openshift Route objects that match the label selector passed through that flag. This change extends the functionality to the Ambassador Host type object.When the flag is not specified the default value is
labels.Everything()
which is an empty string, the same as before.Annotation based filter is inefficient because the filtering has to be done in the controller instead of the API server like with label filtering. The Annotation based filtering has been left in for legacy reasons so the Ambassador Host source can be used in conjunction with the other sources that don't yet support label filtering.
It is possible to use label based filtering with annotation based filtering so you can initially filter by label and then filter the returned hosts by annotation. This is not recommended
Fixes #2761
Checklist