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

Azure Private DNS will replace Azure DNS Private Zones #1073

Closed
t0mmyt opened this issue Jun 18, 2019 · 68 comments
Closed

Azure Private DNS will replace Azure DNS Private Zones #1073

t0mmyt opened this issue Jun 18, 2019 · 68 comments
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@t0mmyt
Copy link

t0mmyt commented Jun 18, 2019

The current method of creating a private zone in Azure (a regular DNS zone set to type private) is going to be deprecated in coming months and the new method will be out of private preview in the next few weeks I am told.

I've had a quick look at what's involved and it will require a much newer version of the Azure SDK which in turn requires a new Azure rest client which in turn requires updating client-go.

I am happy to take on the Azure specific changes (updating the existing provider and adding a new one for private DNS) but updating to client-go v11.0.0 will require significantly more work than I have time to take on.

Is anyone able to get the client-go and k8s machinary up to date?

@bhicks329
Copy link

They’re going to be changing the top-level resource provider to separate it from the current DNS zones. At the moment they’re both covered under the same one.

https://azure.microsoft.com/en-gb/updates/announcing-preview-refresh-for-azure-dns-private-zones-2/

In a month or so, this functionality with Private DNS zones will break.

@lukasmrtvy
Copy link

lukasmrtvy commented Aug 20, 2019

@t0mmyt
I guess that --publish-internal-services will handle private zone, right ? ( I did not find any info, how to use external-dns with private zone ) ... seems that now its broken, because even with --publish-internal-services external-dns is publishing records to public zone.

@timja
Copy link
Contributor

timja commented Aug 20, 2019

There needs to be code changes done to support it, private zone is a different top level resource and uses a different API and needs a new SDK version...

@szuecs
Copy link
Contributor

szuecs commented Sep 16, 2019

Anyone that would like to work on this issue?
We have no Azure account and not the capacity to work on this.

@timja
Copy link
Contributor

timja commented Sep 16, 2019

Happy to make the change, but ideally upgrading client-go and the azure-sdk too first would make this easier

I already tried to update it awhile ago, but I had trouble updating the dependencies

@lukasmrtvy
Copy link

lukasmrtvy commented Sep 16, 2019

@timja
What was wrong?

I used:

diff --git a/go.mod b/go.mod
index db42b9b9..c6e98b33 100644
--- a/go.mod
+++ b/go.mod
@@ -4,8 +4,8 @@ go 1.12
 
 require (
 	cloud.google.com/go v0.37.4
-	github.com/Azure/azure-sdk-for-go v10.0.4-beta+incompatible
-	github.com/Azure/go-autorest v10.9.0+incompatible
+	github.com/Azure/azure-sdk-for-go v33.2.0+incompatible
+	github.com/Azure/go-autorest v13.0.1+incompatible
 	github.com/alecthomas/assert v0.0.0-20170929043011-405dbfeb8e38 // indirect
 	github.com/alecthomas/colour v0.0.0-20160524082231-60882d9e2721 // indirect
 	github.com/alecthomas/kingpin v2.2.5+incompatible
diff --git a/provider/azure.go b/provider/azure.go
index 3f887e55..2fb19c2a 100644
--- a/provider/azure.go
+++ b/provider/azure.go
@@ -26,7 +26,7 @@ import (
 
 	yaml "gopkg.in/yaml.v2"
 
-	"github.com/Azure/azure-sdk-for-go/arm/dns"
+        "github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns"
 	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/adal"
 	"github.com/Azure/go-autorest/autorest/azure"
diff --git a/provider/azure_test.go b/provider/azure_test.go
index 36d69766..b1124e40 100644
--- a/provider/azure_test.go
+++ b/provider/azure_test.go
@@ -20,7 +20,7 @@ import (
 	"context"
 	"testing"
 
-	"github.com/Azure/azure-sdk-for-go/arm/dns"
+        "github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns"
 	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/azure"
 	"github.com/Azure/go-autorest/autorest/to"

@timja
Copy link
Contributor

timja commented Sep 16, 2019

It was awhile ago I'll try to have another look soon, could you create a PR with your change?

@timja
Copy link
Contributor

timja commented Sep 16, 2019

Compilation fails on this with those changes:

provider/azure.go:122:3: cannot use zonesClient (type "github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".ZonesClient) as type ZonesClient in field value:
	"github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".ZonesClient does not implement ZonesClient (wrong type for ListByResourceGroup method)
		have ListByResourceGroup("context".Context, string, *int32) ("github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".ZoneListResultPage, error)
		want ListByResourceGroup(string, *int32) ("github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".ZoneListResult, error)
provider/azure.go:123:3: cannot use recordsClient (type "github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".RecordSetsClient) as type RecordsClient in field value:
	"github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".RecordSetsClient does not implement RecordsClient (wrong type for CreateOrUpdate method)
		have CreateOrUpdate("context".Context, string, string, string, "github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".RecordType, "github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".RecordSet, string, string) ("github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".RecordSet, error)
		want CreateOrUpdate(string, string, string, "github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".RecordType, "github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".RecordSet, string, string) ("github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-05-01/dns".RecordSet, error)
make: *** [build/external-dns] Error 2

Think I just went down a rabbit hole trying to update the tests last time, will try have another look

@timja
Copy link
Contributor

timja commented Sep 17, 2019

Started on it in: #1195
some client-go issues to sort out, updating the azure_test and fixing the pagination of dns zones + records sets

@saidst
Copy link
Contributor

saidst commented Sep 21, 2019

Great you started to work on it.

I see you are quite far. Can I help to complete it?

@timja
Copy link
Contributor

timja commented Sep 21, 2019

See if you can figure out why it’s not compiling right now that would be much appreciated, I’ve got compatible Sdks working but the mock interface isn’t quite working, some go ism that I don’t understand...

@saidst
Copy link
Contributor

saidst commented Sep 24, 2019

FYI: I work on it. I hope I am able to push some first idea today. Compilation works.

I upgraded the azure dependencies once again.
Also, I figured, that authentication with Azure is not as MS wants it (https://github.com/Azure/azure-sdk-for-go#authentication). Thus, I was required to change that in order to keep code related to the Authorizer simple.

Maybee (or very probably), we should about splitting those changes in various PRs.

@timja
Copy link
Contributor

timja commented Sep 24, 2019

I wouldn't change the authentication in the PR unless it's required to make it work, can re-work it separately

@saidst
Copy link
Contributor

saidst commented Sep 24, 2019

Agreed. I will figure that out. But I am afraid, that code around ADAL/Manual token creation is at least deprecated if not more.

@timja
Copy link
Contributor

timja commented Sep 27, 2019

@stsaid how're you getting on?

@saidst
Copy link
Contributor

saidst commented Sep 27, 2019

I didn't work on it the last two days. Maybee you can have a look on my changes? See the PR on the branch in your forked repo.
If this looks OK, I would continue. There is some stuff to do in the tests (in the Azure-Provider but also in others.).

@timja
Copy link
Contributor

timja commented Sep 28, 2019

I took a look and managed to fix the tests, they now pass but need a CLA signature from you @stsaid as I merged your commit in.

@saidst
Copy link
Contributor

saidst commented Sep 29, 2019

I care for that today. Thanks for continuing work on the PR.

@saidst
Copy link
Contributor

saidst commented Sep 30, 2019

FYI: I've signed up at the Linux Foundation. So far, I haven't received the mail with the CLA Confirmation. Have filed a ticket requesting for support.

@saidst
Copy link
Contributor

saidst commented Oct 6, 2019

@timja, already thought how to handle the different types that come with Azure Private DNS?

So far, tests, services methods etc. are all bound to the specific types of Azure DNS.
An idea for that could be to create custom types for both private & public DNS which embed the Microsoft-ones, like e.g. a RecordSet. Additionally, those custom types implement "shared" interfaces which are independent from the semantics of public & private DNS.
Finally, adapt service methods etc. to use these interfaces and no longer bind to particular types of Azure DNS.

Alternatively, I tried to come up with an interlayer which does not embed Azure Types but exists separately. Example: Instead of Records()-function going straight to service methods of the Azure Clients, it invokes a "generic" function which again invokes both Azure DNS Client and Azure Private DNS client and returns "generic" types.
See: saidst@4130ea1

Lastly, one could even think of two distinct providers.

Opinions?

@timja
Copy link
Contributor

timja commented Oct 6, 2019

From what I can remember from implementing support for it in terraform all the models and api calls are different and the api is different under the hood (less consistent at least on the zone side)

I would lean on the side of a separate provider side

@saidst
Copy link
Contributor

saidst commented Oct 7, 2019

I think so too....
The "interlayer"-approach makes the code a bit complicated.
Also, for AWS-there are already two different providers.

Regarding provider-names we could go like...
--provider azure-dns|azure --> current provider
--provider azure-private-dns --> new additional provider

I will start on it this week.

@saidst
Copy link
Contributor

saidst commented Oct 10, 2019

Please check this out: saidst@07e46bc

I copy&pasted the existing provider but changed the Authorizer.

In the meantime, I try to figure out the credentials issues.

@rnkhouse
Copy link

rnkhouse commented Nov 6, 2019

Please let us know if there is any update on this. Thanks!
@saidst

@saidst
Copy link
Contributor

saidst commented Nov 6, 2019

First version incl. tutorials have been completed 2-3 weeks ago.
We depend on #1195 to move on. If this is accepted, we know what to include in the PR for this issue.

@saidst
Copy link
Contributor

saidst commented Nov 7, 2019

#1195 has been merged.
I will rebase my feature-branch on master and we can go from there.
@timja, have you also done anything on the actual private dns provider in the meantime?

@timja
Copy link
Contributor

timja commented Nov 7, 2019

#1195 has been merged.
I will rebase my feature-branch on master and we can go from there.
@timja, have you also done anything on the actual private dns provider in the meantime?

no I haven't, I'll test your changes out once you're able to open a PR and let me know if there's anything you're stuck on

@timja
Copy link
Contributor

timja commented Nov 13, 2019

There's now a PR up (thanks @saidst)
Any testing or feedback on it would be welcomed:
#1269

@dstampfli
Copy link

I finally got this working. Here's the parameters file that I used to create the Nginx ingress controller. I needed to add the controller.publishService.enabled: true parameter. After doing that and redeploying, Ingress resources are creating records in my private DNS zone. I've also included the sample service that I used for testing.

Helm parameters file:

name: nginx-ingress
publish-service: default/nginx-ingress-controller
controller:
  nodeSelector:
    beta.kubernetes.io/os: linux
  publishService:
    enabled: true
  service:
    annotations:
      external-dns.alpha.kubernetes.io/hostname: aksdemos.com  # https://github.com/helm/charts/tree/master/stable/nginx-ingress#externaldns-service-configuration
defaultBackend:
  nodeSelector:
    beta.kubernetes.io/os: linux

Sample service:

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  name: nginx1
spec:
  template:
    metadata:
      labels:
        app: nginx1
    spec:
      containers:
      - image: nginx
        name: nginx1
        ports:
        - containerPort: 80
---
apiVersion: v1
kind: Service
metadata:
  name: nginx1
spec:
  ports:
  - port: 80
    protocol: TCP
    targetPort: 80
  selector:
    app: nginx1
  type: ClusterIP
---
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: nginx1
  annotations:
    kubernetes.io/ingress.class: nginx
spec:
  rules:
  - host: sample1.aksdemos.com
    http:
      paths:
      - backend:
          serviceName: nginx1
          servicePort: 80
        path: /

@saidst
Copy link
Contributor

saidst commented Dec 10, 2019

Good and bad.
I thought (!?) made it work without explicitly setting publishService to true.

Sorry, I made a wrong suggestion because I overlooked my on scripts.

For ingresses we just need this:

publishService:
 enabled: true

Annotating the ingress-controller has no effect on the ingresses. I must have been too fast two days ago...

I think this page confirms it: https://github.com/kubernetes-sigs/external-dns/blob/9418e3acd83db8066d07efb80131c9c3ede03f82/docs/tutorials/aws.md#alias

@guitmz
Copy link

guitmz commented Dec 11, 2019

So is the new version of external-dns with azure private dns support being released anytime soon? PR was merged a couple of weeks ago, not sure if there is a release cycle in the project

@dstampfli
Copy link

No, I cloned master and built the Docker image locally and pushed it
to my container registry. Not sure when a new release is coming.

@lukasmrtvy
Copy link

lukasmrtvy commented Dec 12, 2019

From the Slack channel (2.12.2019):

sszuecs 9:37 AM
... we are working on it, but right now not possible until org change is done

@guitmz
Copy link

guitmz commented Jan 16, 2020

v0.5.18 was finally released yesterday containing this fix :)

@IbraheemAlSaady
Copy link

IbraheemAlSaady commented Jan 16, 2020

@guitmz does this work for the stable/external-dns helm chart as well? If yes, I'm not able to make it work for some reason, is there a documentation for the values.yml?

I'm doing:

provider: azure-private-dns

azure:
  resourceGroup:
  tenantId:
  subscriptionId:
  aadClientId:
  aadClientSecret:

Of course its filled with values. But I'm getting this error:

Failed to refresh the Token for request to https://management.azure.com/subscriptions//resourceGroups//providers/Microsoft.Network/privateDnsZones?api-version=2018-09-01

The subscription id and the resource group are not being picked.

I'm using this image:

image:
  registry: docker.io
  repository: bitnami/external-dns
  tag: 0.5.18

@guitmz
Copy link

guitmz commented Jan 21, 2020

@IbraheemAlSaady I'm currently updating my local chart with the changes of this new release and I should be able to test this week and let you know

@IbraheemAlSaady
Copy link

IbraheemAlSaady commented Jan 21, 2020

@guitmz
I have opened an issue and I have discovered a missing if statement in the deployment template, Check issue 20212

I'm gonna work on the PR today

@guitmz
Copy link

guitmz commented Jan 21, 2020

I have a weird issue when using

provider: azure-private-dns
azure:
  resourceGroup: "myGroup"

with this, the --azure-resource-group=myGroup is not added to the container arguments and I get this error:

Status=404 Code=\"SubscriptionNotFound\" Message=\"The subscription 'resourceGroups' could not be found.\""

If I do the same but with provider: azure, everything works well. I guess its the same as you mentioned in helm/charts#20212 (comment) @IbraheemAlSaady

@saidst
Copy link
Contributor

saidst commented Jan 21, 2020

Have you cared for also setting the subscription via an cli-arg?

https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/azure-private-dns.md#manifest-for-clusters-without-rbac-enabled:

--azure-subscription-id=<use the id of your subscription>

@guitmz
Copy link

guitmz commented Jan 21, 2020

also, looks like theres more to it @IbraheemAlSaady .. I have added the missing if statement to my local chart and deployed again, the issue still persists even though now the --azure-resource-group=myGroup is there..

I think its related to the comment in your issue where it says that private dns is relying only on env-args at this moment, also just did like @saidst just said and worked, its also necessary in the chart

@IbraheemAlSaady
Copy link

IbraheemAlSaady commented Jan 21, 2020

@guitmz @saidst, yes indeed the subscription should also be added. There is also the environment variables like tenantId, clientId, and clientSecret, which are required for private-dns. Which is a behavior I’ve noticed. I’m hoping to push the PR today at night (CET) or tomorrow morning.

@guitmz
Copy link

guitmz commented Jan 22, 2020

I was able to make it work without env vars, just by adding the subscription argument and fixing the if statement as you mentioned in your issue @IbraheemAlSaady , just FYI

@saidst
Copy link
Contributor

saidst commented Jan 22, 2020

Can you paste the (created) manifest for the deployment (of course without credentials) ?

@IbraheemAlSaady
Copy link

@guitmz weird, it didn't work for me without the env, it was throwing a 403 error

@guitmz
Copy link

guitmz commented Jan 22, 2020

heres the templates/deployment.yaml of my local copy of the chart http://dpaste.com/142RY63

My modifications are:

  • line 13 to include my azure managed identity annotation
  • line 136 (like @IbraheemAlSaady proposed)
  • line 184 (like @saidst proposed)

heres my values file for azure provider: http://dpaste.com/0P73S86
and for azure-private-dns provider: http://dpaste.com/2MZ0AR7

currently I have 2 deploys of external-dns, one for each provider since apparently theres no way of using multiple providers at once

rendered deployment for external-dns: http://dpaste.com/35D8MWQ
and for private-dns: http://dpaste.com/2M4M5S1 (named internal-dns just for differentiate both)

perhaps it works in my case because of pod-identity

@saidst
Copy link
Contributor

saidst commented Jan 22, 2020

Thx!

The args look OK (complete).

Do you use the managed-identity-extension for Azure Kubernetes Service (preview feature)?

If so, great. I've not tested that so far. But yes, in theory, the private-dns provider is able to pick up MSI. This is due to the used sdk's from microsoft.

I think you need to handle the case of not using msi. If not seen that while skimming through the code. In this case, you need to set the following env-vars:

AZURE_TENANT_ID (in which the SP exists)
AZURE_CLIENT_ID
AZURE_CLIENT_SECRET

@guitmz
Copy link

guitmz commented Jan 22, 2020

@saidst yeah, created a managed-identity in azure, gave it DNS zone contributor permissions, used this https://github.com/Azure/aad-pod-identity to attach to my external-dns pods and it works..

I haven't tested without MSI because my end goal is to use it, but I gave a second look at the chart and yeah, I believe it will fail without MSI if the env-vars are not included. Handling it should probably be included of the PR that fixes the chart in upstream if possible

@IbraheemAlSaady
Copy link

IbraheemAlSaady commented Jan 23, 2020

Changes have been merged

Use version 2.15.1. Here is the PR

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 22, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 22, 2020
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

@jaekunchoi
Copy link

heres the templates/deployment.yaml of my local copy of the chart http://dpaste.com/142RY63

My modifications are:

  • line 13 to include my azure managed identity annotation
  • line 136 (like @IbraheemAlSaady proposed)
  • line 184 (like @saidst proposed)

heres my values file for azure provider: http://dpaste.com/0P73S86
and for azure-private-dns provider: http://dpaste.com/2MZ0AR7

currently I have 2 deploys of external-dns, one for each provider since apparently theres no way of using multiple providers at once

rendered deployment for external-dns: http://dpaste.com/35D8MWQ
and for private-dns: http://dpaste.com/2M4M5S1 (named internal-dns just for differentiate both)

perhaps it works in my case because of pod-identity

were you able to get MSI working with azure-private-dns ? If so are you able to share your code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests