Refactored & Implemented new features for ServiceDNS #353

shashidharatd · 2018-10-23T14:36:35Z

/cc @danehans @pmorie @kubernetes-sigs/federation-v2-maintainers

k8s-ci-robot · 2018-10-23T14:36:38Z

@shashidharatd: GitHub didn't allow me to request PR reviews from the following users: danehans.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Ref: #348

/cc @danehans @pmorie @kubernetes-sigs/federation-v2-maintainers

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.

k8s-ci-robot · 2018-10-23T14:36:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shashidharatd

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~~ [shashidharatd]

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

cblecker · 2018-10-23T22:20:20Z

/cc @danehans
now that he's a k-sigs member

font

@shashidharatd I made some comments in #348 that are related to this PR.

danehans

Great work. Just one question for you.

danehans · 2018-10-25T16:36:54Z

pkg/apis/multiclusterdns/v1alpha1/federationdomain_types.go

+// FederationDomain
+// +k8s:openapi-gen=true
+// +kubebuilder:resource:path=federationdomains
+type FederationDomain struct {


Does it make sense to name this type Domain since it's part of the multiclusterdns.federation.k8s.io API group?

I am fine. @pmorie wdyt?

pkg/apis/multiclusterdns/v1alpha1/servicednsrecord_types.go

font · 2018-10-31T05:06:45Z

pkg/controller/dnsendpoint/service.go

+			endpoint := &feddnsv1a1.Endpoint{
+				DNSName:    dnsObject.Spec.DNSPrefix + "." + dnsObject.Status.Domain,
+				RecordTTL:  ttl,
+				RecordType: RecordTypeA,


What if we just create a CNAME record to point to the global DNS name? This would prevent having to manage a duplicate entry where the list of A records match the global DNS entry.

Agree, Updated to create CNAME record now.

test/e2e/servicedns.go

test/common/dns.go

test/integration/servicedns_test.go

pkg/apis/multiclusterdns/v1alpha1/servicednsrecord_types.go

font

Thanks @shashidharatd for these changes! I made some comments. Also, there are several places we're still referring to FederationDomain instead of Domain after the API name was updated. I didn't comment on all of them. Is this intentional?

shashidharatd

Thanks @font for the review. I have addressed the comments. PTAL

pkg/apis/multiclusterdns/v1alpha1/servicednsrecord_types.go

shashidharatd · 2018-10-31T14:57:08Z

pkg/controller/dnsendpoint/service.go

+			endpoint := &feddnsv1a1.Endpoint{
+				DNSName:    dnsObject.Spec.DNSPrefix + "." + dnsObject.Status.Domain,
+				RecordTTL:  ttl,
+				RecordType: RecordTypeA,


Agree, Updated to create CNAME record now.

test/common/dns.go

test/e2e/servicedns.go

test/integration/servicedns_test.go

font

@shashidharatd Thanks! There are just a few unresolved items and we can merge this.

test/e2e/servicedns.go

font · 2018-11-01T20:27:52Z

pkg/controller/dnsendpoint/service.go

@@ -101,6 +101,15 @@ func getServiceDNSEndpoints(obj interface{}) ([]*feddnsv1a1.Endpoint, error) {
 			}
 			endpoints = append(endpoints, endpoint)
 		}
+		if dnsObject.Spec.DNSPrefix != "" && len(globalTargets) > 0 {


I think we also need to check if AllowServiceWithoutEndpoints is set to determine if we need to check whether globalTargets contains an actual target. If a user sets AllowServiceWithoutEndpoints, then there should be a DNS entry created even if there are no targets set.

I agree with you for writing CNAME record even if there are no globalTargets. But AllowServiceWithoutEndpoints is for a different purpose (LB backed by endpoints/pods) and IMHO, we should not mix them.

font · 2018-11-01T20:59:47Z

pkg/controller/servicedns/controller.go

@@ -65,11 +65,18 @@ type Controller struct {
 	// Informer for the ServiceDNSRecord objects
 	serviceDNSController cache.Controller

+	// Store for the FederationDomain objects


s/FederationDomain/Domain/

font · 2018-11-01T20:59:53Z

pkg/controller/servicedns/controller.go

@@ -65,11 +65,18 @@ type Controller struct {
 	// Informer for the ServiceDNSRecord objects
 	serviceDNSController cache.Controller

+	// Store for the FederationDomain objects
+	federationDomainStore cache.Store
+	// Informer for the FederationDomain objects


s/FederationDomain/Domain/

…alpha1/Domain

shashidharatd

@font, have addressed the comments. ptal. Thanks !

test/e2e/servicedns.go

shashidharatd · 2018-11-02T06:45:41Z

pkg/controller/dnsendpoint/service.go

@@ -101,6 +101,15 @@ func getServiceDNSEndpoints(obj interface{}) ([]*feddnsv1a1.Endpoint, error) {
 			}
 			endpoints = append(endpoints, endpoint)
 		}
+		if dnsObject.Spec.DNSPrefix != "" && len(globalTargets) > 0 {


I agree with you for writing CNAME record even if there are no globalTargets. But AllowServiceWithoutEndpoints is for a different purpose (LB backed by endpoints/pods) and IMHO, we should not mix them.

shashidharatd · 2018-11-02T06:45:56Z

pkg/controller/servicedns/controller.go

@@ -65,11 +65,18 @@ type Controller struct {
 	// Informer for the ServiceDNSRecord objects
 	serviceDNSController cache.Controller

+	// Store for the FederationDomain objects


shashidharatd · 2018-11-02T06:46:01Z

pkg/controller/servicedns/controller.go

@@ -65,11 +65,18 @@ type Controller struct {
 	// Informer for the ServiceDNSRecord objects
 	serviceDNSController cache.Controller

+	// Store for the FederationDomain objects
+	federationDomainStore cache.Store
+	// Informer for the FederationDomain objects


shashidharatd · 2018-11-02T06:46:44Z

pkg/controller/servicedns/controller.go

@@ -123,6 +131,23 @@ func newController(fedClient fedclientset.Interface, kubeClient kubeclientset.In
 		util.NewTriggerOnAllChanges(s.worker.EnqueueObject),
 	)

+	// Informer for the FederationDomain resource


sorry, missed. now i did a grep and replace all of them.

font

Thanks @shashidharatd!
/lgtm

k8s-ci-robot requested a review from a team October 23, 2018 14:36

k8s-ci-robot requested a review from pmorie October 23, 2018 14:36

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 23, 2018

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 23, 2018

shashidharatd force-pushed the dns branch from 135eecd to 4b68fba Compare October 23, 2018 15:14

danehans mentioned this pull request Oct 23, 2018

Add new memberships - October 18th kubernetes/org#176

Merged

shashidharatd force-pushed the dns branch 2 times, most recently from fb4ecd5 to d18ac13 Compare October 23, 2018 17:11

k8s-ci-robot requested a review from danehans October 23, 2018 22:20

shashidharatd force-pushed the dns branch from d18ac13 to 1b823e2 Compare October 24, 2018 16:54

font reviewed Oct 25, 2018

View reviewed changes

danehans reviewed Oct 25, 2018

View reviewed changes

font reviewed Oct 26, 2018

View reviewed changes

pkg/apis/multiclusterdns/v1alpha1/servicednsrecord_types.go Outdated Show resolved Hide resolved

shashidharatd force-pushed the dns branch from 1b823e2 to 8c104a2 Compare October 29, 2018 06:05

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 29, 2018

shashidharatd force-pushed the dns branch from 8c104a2 to 038f44d Compare October 29, 2018 06:24

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 29, 2018

font reviewed Oct 31, 2018

View reviewed changes

shashidharatd commented Oct 31, 2018

View reviewed changes

shashidharatd force-pushed the dns branch from 038f44d to 6dfb416 Compare October 31, 2018 15:13

font reviewed Nov 1, 2018

View reviewed changes

shashidharatd added 2 commits November 2, 2018 11:37

Bootstrapped a new api resource multiclusterdns.federation.k8s.io./v1…

be4ff08

…alpha1/Domain

Added fields to Domain API type

f895b86

shashidharatd added 3 commits November 2, 2018 11:37

Refined ServiceDNSRecord API

76b5300

Updated generated code

ed21768

Update hack/install-latest.yaml

65f384c

shashidharatd commented Nov 2, 2018

View reviewed changes

Refactored and implement new features for ServiceDNSRecord

c346a55

shashidharatd force-pushed the dns branch from 6dfb416 to c346a55 Compare November 2, 2018 06:48

font reviewed Nov 2, 2018

View reviewed changes

k8s-ci-robot assigned font Nov 2, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2018

k8s-ci-robot merged commit 355fb6d into kubernetes-retired:master Nov 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored & Implemented new features for ServiceDNS #353

Refactored & Implemented new features for ServiceDNS #353

shashidharatd commented Oct 23, 2018

k8s-ci-robot commented Oct 23, 2018

k8s-ci-robot commented Oct 23, 2018

cblecker commented Oct 23, 2018

font left a comment

danehans left a comment

danehans Oct 25, 2018

shashidharatd Oct 26, 2018

font Oct 31, 2018 •

edited

Loading

shashidharatd Oct 31, 2018

font left a comment

shashidharatd left a comment

shashidharatd Oct 31, 2018

font left a comment

font Nov 1, 2018

shashidharatd Nov 2, 2018

font Nov 1, 2018

shashidharatd Nov 2, 2018

font Nov 1, 2018

shashidharatd Nov 2, 2018

shashidharatd left a comment

shashidharatd Nov 2, 2018

shashidharatd Nov 2, 2018

shashidharatd Nov 2, 2018

shashidharatd Nov 2, 2018

font left a comment

Refactored & Implemented new features for ServiceDNS #353

Refactored & Implemented new features for ServiceDNS #353

Conversation

shashidharatd commented Oct 23, 2018

k8s-ci-robot commented Oct 23, 2018

k8s-ci-robot commented Oct 23, 2018

cblecker commented Oct 23, 2018

font left a comment

Choose a reason for hiding this comment

danehans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

font Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

font left a comment

Choose a reason for hiding this comment

shashidharatd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

font left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashidharatd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

font left a comment

Choose a reason for hiding this comment

font Oct 31, 2018 •

edited

Loading