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.

Sign up for GitHub

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 support for Headless hostPort services #324

Merged
merged 5 commits into from
Nov 20, 2017
Merged

Add support for Headless hostPort services #324

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
10d7b2c
Added initial support for Headless services
Aug 29, 2017
b1fde0a
service.go: Fixed some formatting
Aug 29, 2017
7eb493a
forgot to run gometalinter
Aug 29, 2017
20e9e11
service: implemented alternative proposal, to reuse existing annotation
Aug 31, 2017
325a0bc
source: some refactoring
Aug 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added source/debug.test
View file
Open in desktop
Binary file not shown.
49 changes: 18 additions & 31 deletions source/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,6 @@ func (sc *serviceSource) Endpoints() ([]*endpoint.Endpoint, error) {
svcEndpoints = legacyEndpointsFromService(&svc, sc.compatibility)
}

// check for headless service
if len(svcEndpoints) == 0 {
svcEndpoints, err = sc.endpointsFromHeadless(&svc)
if err != nil {
return nil, err
}
}

// apply template if none of the above is found
if len(svcEndpoints) == 0 && sc.fqdnTemplate != nil {
svcEndpoints, err = sc.endpointsFromTemplate(&svc)
Expand All @@ -123,37 +115,28 @@ func (sc *serviceSource) Endpoints() ([]*endpoint.Endpoint, error) {
return endpoints, nil
}

func (sc *serviceSource) endpointsFromHeadless(svc *v1.Service) ([]*endpoint.Endpoint, error) {
func (sc *serviceSource) extractHeadlessEndpoint(svc *v1.Service, hostname string) []*endpoint.Endpoint {

var endpoints []*endpoint.Endpoint

// Check if a Headless Service definition and we need to generate the hostnames differently
headlessDomain, ok := svc.Annotations[headlessDomainAnnotationKey]

// We require some domain to be set, maybe should also use the fqdn-template?
if ok && headlessDomain != "" {

// Get all the Pods
if pods, err := sc.client.CoreV1().Pods(svc.Namespace).List(metav1.ListOptions{LabelSelector: labels.Set(svc.Spec.Selector).AsSelectorPreValidated().String()}); err != nil {
log.Errorf("List Pods of service[%s] error:%v", svc.GetName(), err)
} else {
for _, v := range pods.Items {

log.Debugf("Generating matching endpoint %s%s with HostIP %s", v.Spec.Hostname+headlessDomain, v.GetName(), v.Status.HostIP)
// To reduce traffice on the DNS API only add record for running Pods. Good Idea?
if v.Status.Phase == v1.PodRunning {
endpoints = append(endpoints, endpoint.NewEndpoint(v.Spec.Hostname+headlessDomain, v.Status.HostIP, endpoint.RecordTypeA))
} else {
log.Debugf("Pod %s is not in running phase", v.Spec.Hostname)
}
// Get all the Pods
if pods, err := sc.client.CoreV1().Pods(svc.Namespace).List(metav1.ListOptions{LabelSelector: labels.Set(svc.Spec.Selector).AsSelectorPreValidated().String()}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the spirit of the original request around convention and suggestion. I personally have not see this style of expression used much, where the else is referencing the result of the assignment in the guarded if statement and it took me a few seconds to figure out where that was even coming from.

Personally I find it more clear to simply make the assignment and have a small err != nil check that short circuits which eventually you get used to reading all over the place.

That's a completely arbitrary assessment though and may not be true to those who have worked and looked at larger code bases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I will take a look shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean something like this?

pods, err := sc.client.CoreV1().Pods(svc.Namespace).List(metav1.ListOptions{LabelSelector: labels.Set(svc.Spec.Selector).AsSelectorPreValidated().String()})
	// Get all the Pods
	if err != nil {
		log.Errorf("List Pods of service[%s] error:%v", svc.GetName(), err)
	} else {}

Copy link
Contributor

Choose a reason for hiding this comment

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

You can have an empty return to short circuit: https://play.golang.org/p/jpRxJ19fl5

Ex.

pods, err := sc.client.CoreV1().Pods(svc.Namespace).List(metav1.ListOptions{LabelSelector: labels.Set(svc.Spec.Selector).AsSelectorPreValidated().String()}); 

if err != nil {
 	log.Errorf("List Pods of service[%s] error:%v", svc.GetName(), err)
 	return 
} 

for _, v := range pods.Items {
	headlessDomain := v.Spec.Hostname + "." + hostname
	log.Debugf("Generating matching endpoint %s with HostIP %s", headlessDomain, v.Status.HostIP)
	// To reduce traffice on the DNS API only add record for running Pods. Good Idea?
	if v.Status.Phase == v1.PodRunning {
		endpoints = append(endpoints, endpoint.NewEndpoint(headlessDomain, v.Status.HostIP, endpoint.RecordTypeA))
	} else {
		log.Debugf("Pod %s is not in running phase", v.Spec.Hostname)
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the function should return the Endpoints no? Or should we redesign that part?

log.Errorf("List Pods of service[%s] error:%v", svc.GetName(), err)
} else {
for _, v := range pods.Items {
headlessDomain := v.Spec.Hostname + "." + hostname
log.Debugf("Generating matching endpoint %s with HostIP %s", headlessDomain, v.Status.HostIP)
// To reduce traffice on the DNS API only add record for running Pods. Good Idea?
if v.Status.Phase == v1.PodRunning {
endpoints = append(endpoints, endpoint.NewEndpoint(headlessDomain, v.Status.HostIP, endpoint.RecordTypeA))
} else {
log.Debugf("Pod %s is not in running phase", v.Spec.Hostname)
}
}

}

return endpoints, nil
return endpoints
}

func (sc *serviceSource) endpointsFromTemplate(svc *v1.Service) ([]*endpoint.Endpoint, error) {
var endpoints []*endpoint.Endpoint

Expand Down Expand Up @@ -199,6 +182,10 @@ func (sc *serviceSource) generateEndpoints(svc *v1.Service, hostname string) []*
if sc.publishInternal {
endpoints = append(endpoints, extractServiceIps(svc, hostname)...)
}
if svc.Spec.ClusterIP == v1.ClusterIPNone {
endpoints = append(endpoints, sc.extractHeadlessEndpoint(svc, hostname)...)
}

}

return endpoints
Expand Down
10 changes: 5 additions & 5 deletions source/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ func TestHeadlessServices(t *testing.T) {
"",
map[string]string{"component": "foo"},
map[string]string{
headlessDomainAnnotationKey: ".example.org.",
hostnameAnnotationKey: "service.example.org",
},
v1.ClusterIPNone,
"1.1.1.1",
Expand All @@ -716,8 +716,8 @@ func TestHeadlessServices(t *testing.T) {
[]string{"foo-0", "foo-1"},
[]v1.PodPhase{v1.PodRunning, v1.PodRunning},
[]*endpoint.Endpoint{
{DNSName: "foo-0.example.org", Target: "1.1.1.1"},
{DNSName: "foo-1.example.org", Target: "1.1.1.1"},
{DNSName: "foo-0.service.example.org", Target: "1.1.1.1"},
{DNSName: "foo-1.service.example.org", Target: "1.1.1.1"},
},
false,
},
Expand All @@ -731,7 +731,7 @@ func TestHeadlessServices(t *testing.T) {
"",
map[string]string{"component": "foo"},
map[string]string{
headlessDomainAnnotationKey: ".example.org.",
hostnameAnnotationKey: "service.example.org",
},
v1.ClusterIPNone,
"1.1.1.1",
Expand All @@ -742,7 +742,7 @@ func TestHeadlessServices(t *testing.T) {
[]string{"foo-0", "foo-1"},
[]v1.PodPhase{v1.PodRunning, v1.PodFailed},
[]*endpoint.Endpoint{
{DNSName: "foo-0.example.org", Target: "1.1.1.1"},
{DNSName: "foo-0.service.example.org", Target: "1.1.1.1"},
},
false,
},
Expand Down
2 changes: 0 additions & 2 deletions source/source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ const (
hostnameAnnotationKey = "external-dns.alpha.kubernetes.io/hostname"
// The annotation used for defining the desired ingress target
targetAnnotationKey = "external-dns.alpha.kubernetes.io/target"
// The annotation used for defining a subdomain the Pods of a headless service are going to be injected into
headlessDomainAnnotationKey = "external-dns.alpha.kubernetes.io/headlessDomain"
// The value of the controller annotation so that we feel resposible
controllerAnnotationValue = "dns-controller"
)
Expand Down