NE-226: Support both DNSZone and PrivateDNSZone resources #300

abhinavdahiya · 2019-09-20T18:34:54Z

https://github.com/openshift/api/blob/c4807bb1ed5f3830874a2f78c6acd1235bdbe79c/config/v1/types_dns.go#L55

on Azure currently we use DNSZone resource with type Private to store the DNS records private to the cluster.

There are certain shortcomings with this resource when using type Private

This has been deprecated by Azure for Private DNS Zones preview
This does not allow attaching itself to pre-existing Virtual Networks

The new resource Private DNS Zone solves all the shortcoming of the previous generation and is required for Installing OpenShift in pre-exisiting VNets in Azure.

abhinavdahiya · 2019-09-20T18:35:05Z

/test e2e-azure

ironcladlou · 2019-09-23T20:06:51Z

/approve

@abhinavdahiya thanks for this... what's left to do?

abhinavdahiya · 2019-09-23T20:08:37Z

/approve

@abhinavdahiya thanks for this... what's left to do?

still doing some test with the PrivateDNSZone resource.

abhinavdahiya · 2019-09-24T18:20:33Z

/retest

abhinavdahiya · 2019-09-24T20:20:12Z

This is what I did to test this change.

$ OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE=registry.svc.ci.openshift.org/ci-op-62nsqh5i/release:latest ./bin/openshift-install --dir dev create manifests

# edit the manifests/cluster-dns-02-config.yml
# to have 
#  privateZone:
#    id: /subscriptions/xxxxxxxxxxxxxxxxxx/resourceGroups/adahiya-4-jhd4v-rg/providers/Microsoft.Network/privateDnsZones/adahiya-4.installer-azure.devcluster.openshift.com
# This will force the cluster-ingress-operator to use the new resource when trying to create DNS records.

$ OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE=registry.svc.ci.openshift.org/ci-op-62nsqh5i/release:latest ./bin/openshift-install --dir dev create cluster

# create a privateDNSZone manually as the installer isn't creating it.

$ oc get dnsrecords.ingress.operator.openshift.io -n openshift-ingress-operator default-wildcard -oyaml
apiVersion: ingress.operator.openshift.io/v1
kind: DNSRecord
metadata:
  creationTimestamp: "2019-09-24T18:52:29Z"
  finalizers:
  - operator.openshift.io/ingress-dns
  generation: 1
  labels:
    ingresscontroller.operator.openshift.io/owning-ingresscontroller: default
  name: default-wildcard
  namespace: openshift-ingress-operator
  ownerReferences:
  - apiVersion: operator.openshift.io/v1
    blockOwnerDeletion: true
    controller: true
    kind: IngressController
    name: default
    uid: a9585766-8a5e-406b-919b-732c0cad7d2c
  resourceVersion: "35292"
  selfLink: /apis/ingress.operator.openshift.io/v1/namespaces/openshift-ingress-operator/dnsrecords/default-wildcard
  uid: 181c6c1a-0a08-4e06-96eb-d4ff9d2e8654
spec:
  dnsName: '*.apps.adahiya-4.installer-azure.devcluster.openshift.com.'
  recordTTL: 30
  recordType: A
  targets:
  - 13.89.142.136
status:
  zones:
  - dnsZone:
      id: /subscriptions/433715e6-37fe-4328-af75-3661e13b15fc/resourceGroups/adahiya-4-jhd4v-rg/providers/Microsoft.Network/privateDnsZones/adahiya-4.installer-azure.devcluster.openshift.com
  - dnsZone:
      id: /subscriptions/433715e6-37fe-4328-af75-3661e13b15fc/resourceGroups/os4-common/providers/Microsoft.Network/dnszones/installer-azure.devcluster.openshift.com

# ingress created the record correctly.

$ az network private-dns record-set a list -g adahiya-4-jhd4v-rg --zone-name "adahiya-4.installer-azure.devcluster.openshift.com" -otable
Name    ResourceGroup       Ttl    Type    AutoRegistered    Metadata
------  ------------------  -----  ------  ----------------  ----------
*.apps  adahiya-4-jhd4v-rg  30     A       False

abhinavdahiya · 2019-09-24T20:25:57Z

/test e2e-azure

abhinavdahiya · 2019-09-27T19:17:38Z

/retest

Miciah · 2019-09-27T19:30:30Z

pkg/dns/azure/client/client.go

 }

 // New returns an authenticated DNSClient
 func New(config Config, userAgentExtension string) (DNSClient, error) {
+	rsc, err := newRecordSetClient(config, userAgentExtension)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to create newRecordSetClient")


newRecordSetClient→recordSetClient.

Miciah · 2019-09-27T19:30:47Z

pkg/dns/azure/client/client.go

+
+	prsc, err := newPrivateRecordSetClient(config, userAgentExtension)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to create newPrivateRecordSetClient")


newPrivateRecordSetClient→privateRecordSetClient.

Miciah · 2019-09-27T19:45:59Z

pkg/dns/azure/client/zone.go

@@ -16,5 +18,5 @@ func ParseZone(id string) (*Zone, error) {
 	if len(s) < 9 {
 		return nil, errors.New("invalid azure dns zone id")
 	}
-	return &Zone{SubscriptionID: s[2], ResourceGroup: s[4], Name: s[8]}, nil
+	return &Zone{SubscriptionID: s[2], ResourceGroup: s[4], Provider: fmt.Sprintf("%s/%s", s[6], s[7]), Name: s[8]}, nil


Is the structure of the string id documented?

well i couldn't find any documentations around it, but https://docs.microsoft.com/en-us/rest/api/resources/resources/getbyid

has information on the resource id structure.

secondly, i think i have errors for unknown cases, which should be clear to identify when things are out of place.

Hmm, maybe I'm misunderstanding, but maybe it has to be okay because we're using IDs instead of tags to address zones generally in Azure. So if that ID format isn't stable, some day we're going to lose track of all our zones. I'm not sure how to assess the stability of the documented ID format.

This PR certainly isn't making the situation worse and there's literally nothing else right now for it to go by.

A regex with named capture groups might at least be more readable at this point, but that's a nit.

If the reliability of ID's format is in question, should we open a parallel investigation into our strategy for addressing Azure zones?

Err, wait a sec, if you have the opaque resource ID, can you not do some versioned API call to get back a resource containing the info we're currently extracting from the ID itself (presumed to be opaque)?

you can take the ID and request a get using https://docs.microsoft.com/en-us/rest/api/resources/resources/getbyid to get a response..
https://docs.microsoft.com/en-us/rest/api/resources/resources/getbyid#genericresource

but the response doesn't give us any more information that we would need, compared to the already parsing one.

I think for now the resource ID is standard enough like https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/helpers/azure/resourceid.go

that it should be good enough IMO.

ironcladlou · 2019-09-27T23:08:05Z

Will look at this more closely Monday, thanks.

I'm going to hold it until #301 is merged since it's just about ready.

/hold

ironcladlou · 2019-09-30T17:12:17Z

/hold cancel

ironcladlou · 2019-09-30T17:15:15Z

@abhinavdahiya looks like just a couple of nits from @Miciah to address and then we can merge this

… zones Uses the ZoneID to differentiate the type of zone, and calls the corresponding resource-type handler to PUT and DELETE the records. * creates `recordSetClient` to manage the records for DNSZone resource type. * creates `privateRecordSetClient` to manage the records for PrivateDNSZone resource type. * `dnsClient` now picks one of the above based on the Zone's provider

abhinavdahiya · 2019-09-30T20:45:41Z

/test e2e-azure

abhinavdahiya · 2019-09-30T20:59:11Z

@abhinavdahiya looks like just a couple of nits from @Miciah to address and then we can merge this

@ironcladlou @Miciah fixed the nits in 0038ee3...5265fad

abhinavdahiya · 2019-10-01T18:03:16Z

/retest

abhinavdahiya · 2019-10-07T18:28:25Z

Also this is blocking installer from moving on https://jira.coreos.com/browse/CORS-1205, so if we can move forward on this for now and come back and improve later on that would be super helpful. @ironcladlou @Miciah

ironcladlou · 2019-10-07T18:32:29Z

I want to believe there's some API we're just missing that would let us avoid parsing the IDs, but I agree that nobody knows that that would be yet, and the parsing works, and we're already implicitly relying on the parsing even before this change, so let's merge it and move on.

/lgtm
/approve

openshift-ci-robot · 2019-10-07T18:32:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, ironcladlou

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:

~~OWNERS~~ [ironcladlou]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

the ingress-operator can handle the Private DNS Zone since [1] [1]: openshift/cluster-ingress-operator#300

the ingress-operator can handle the Private DNS Zone since [1] [1]: openshift/cluster-ingress-operator#300 (cherry picked from commit 7d3119f)

Miciah · 2020-01-07T02:18:11Z

/cherrypick release-4.2

openshift-cherrypick-robot · 2020-01-07T02:18:21Z

@Miciah: new pull request created: #344

In response to this:

/cherrypick release-4.2

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.

vendor: refresh to include privatedns service

68878a7

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2019

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 20, 2019

openshift-ci-robot requested review from ironcladlou and knobunc September 20, 2019 18:35

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2019

abhinavdahiya force-pushed the azure_dns_change branch from c34ce28 to 0038ee3 Compare September 24, 2019 20:25

abhinavdahiya changed the title ~~WIP: NE-226: Support both DNSZone and PrivateDNSZone resources~~ NE-226: Support both DNSZone and PrivateDNSZone resources Sep 24, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2019

Miciah reviewed Sep 27, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2019

abhinavdahiya force-pushed the azure_dns_change branch from 0038ee3 to 5265fad Compare September 30, 2019 20:45

openshift-ci-robot assigned ironcladlou Oct 7, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2019

openshift-merge-robot merged commit 3c24f65 into openshift:master Oct 7, 2019

abhinavdahiya added a commit to abhinavdahiya/installer that referenced this pull request Oct 8, 2019

asset: update dns manifest to use Private DNS Zone

7d3119f

the ingress-operator can handle the Private DNS Zone since [1] [1]: openshift/cluster-ingress-operator#300

jhixson74 pushed a commit to jhixson74/installer that referenced this pull request Dec 6, 2019

asset: update dns manifest to use Private DNS Zone

e601267

the ingress-operator can handle the Private DNS Zone since [1] [1]: openshift/cluster-ingress-operator#300

openshift-cherrypick-robot mentioned this pull request Jan 7, 2020

Bug 1788707: [release-4.2] NE-250: Support both DNSZone and PrivateDNSZone resources #344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NE-226: Support both DNSZone and PrivateDNSZone resources #300

NE-226: Support both DNSZone and PrivateDNSZone resources #300

abhinavdahiya commented Sep 20, 2019

abhinavdahiya commented Sep 20, 2019

ironcladlou commented Sep 23, 2019

abhinavdahiya commented Sep 23, 2019

abhinavdahiya commented Sep 24, 2019

abhinavdahiya commented Sep 24, 2019

abhinavdahiya commented Sep 24, 2019

abhinavdahiya commented Sep 27, 2019

Miciah Sep 27, 2019

Miciah Sep 27, 2019

Miciah Sep 27, 2019

abhinavdahiya Sep 27, 2019

ironcladlou Oct 2, 2019

ironcladlou Oct 2, 2019

abhinavdahiya Oct 7, 2019

ironcladlou commented Sep 27, 2019

ironcladlou commented Sep 30, 2019

ironcladlou commented Sep 30, 2019

abhinavdahiya commented Sep 30, 2019

abhinavdahiya commented Sep 30, 2019 •

edited

Loading

abhinavdahiya commented Oct 1, 2019

abhinavdahiya commented Oct 7, 2019

ironcladlou commented Oct 7, 2019

openshift-ci-robot commented Oct 7, 2019

Miciah commented Jan 7, 2020

openshift-cherrypick-robot commented Jan 7, 2020

NE-226: Support both DNSZone and PrivateDNSZone resources #300

NE-226: Support both DNSZone and PrivateDNSZone resources #300

Conversation

abhinavdahiya commented Sep 20, 2019

abhinavdahiya commented Sep 20, 2019

ironcladlou commented Sep 23, 2019

abhinavdahiya commented Sep 23, 2019

abhinavdahiya commented Sep 24, 2019

abhinavdahiya commented Sep 24, 2019

abhinavdahiya commented Sep 24, 2019

abhinavdahiya commented Sep 27, 2019

Miciah Sep 27, 2019

Choose a reason for hiding this comment

Miciah Sep 27, 2019

Choose a reason for hiding this comment

Miciah Sep 27, 2019

Choose a reason for hiding this comment

abhinavdahiya Sep 27, 2019

Choose a reason for hiding this comment

ironcladlou Oct 2, 2019

Choose a reason for hiding this comment

ironcladlou Oct 2, 2019

Choose a reason for hiding this comment

abhinavdahiya Oct 7, 2019

Choose a reason for hiding this comment

ironcladlou commented Sep 27, 2019

ironcladlou commented Sep 30, 2019

ironcladlou commented Sep 30, 2019

abhinavdahiya commented Sep 30, 2019

abhinavdahiya commented Sep 30, 2019 • edited Loading

abhinavdahiya commented Oct 1, 2019

abhinavdahiya commented Oct 7, 2019

ironcladlou commented Oct 7, 2019

openshift-ci-robot commented Oct 7, 2019

Miciah commented Jan 7, 2020

openshift-cherrypick-robot commented Jan 7, 2020

abhinavdahiya commented Sep 30, 2019 •

edited

Loading