Add a new template variable {{EC2PrivateDNSName}} #65

nckturner · 2018-03-27T18:10:56Z

kubelet reports itself as system:node:<aws-private-dns>
modifies authenticator to be able to discover an ec2 instance's private dns
and use it for the node name when {{EC2PrivateDNSName}} is used in the
template username
allows a role to be specified in the config file to assume for the describe
instances call, in case it is in a separate account.

Fixes: #57
Alternative to: #61

Signed-off-by: Nick Turner nic@amazon.com

mattlandis

Overall I like this approach better. It allows for more extensiblity and limits the number of mapping types to deal with.

Just a couple minor comments. None of them are blockers to me. If you don't want to make the change to handle malformed usernames in the PR please create an issue.

mattlandis · 2018-03-27T18:23:00Z

pkg/server/server.go

 	template = strings.Replace(template, "{{SessionName}}", sessionName, -1)
-	return template
+


Since usernames must be DNS-1123 compliant should we have a final check to ensure the rendered template is valid and return an error if it is not? For instance if I accidentally put in {{AccountId}} instead of {{AccountID}} it will not get replaced and we will return an invalid username to k8s.

mattlandis · 2018-03-27T18:27:45Z

pkg/server/server.go

+		}
+	}
+	if privateDNSName == "" {
+		return "", errors.New(fmt.Sprintf("failed to find node %s", id))


You can use fmt.Errorf here.

mattlandis · 2018-03-27T18:33:29Z

pkg/server/server.go

+	return template, nil
+}
+
+type EC2Provider interface {


This can be local scope.

mattlandis · 2018-03-27T18:54:56Z

Also can we open two issues

add metrics for this since it is a place latency could be introduced
Examine managing cache lifetime so that it won't be an issue for long running clusters with frequent node changes

dbenhur · 2018-03-27T19:41:20Z

pkg/server/server.go

+
+	// Private DNS requires EC2 API call
+	if strings.Contains(template, "{{EC2PrivateDNSName}}") {
+		re := regexp.MustCompile("^i-(\\w{8}|\\w{17})$")


I've seen (^i-(\w{8}|\w{17})$)|(^mi-\w{17}$) used elsewhere. EC2 apparently also has managed instances for hybrid environments with "mi-" id prefix. Not sure if these should be allowed or not (are they covered by getPrivateDNSName?).

@dbenhur Let's open a issue for this. I think it requires more investigation to make sure there are no other side effects we aren't thinking about.

@nckturner Add the sign-off and I think this is good to go.

Cool, will rebase and sign off.

I think it'd also be fine to do minimal validation here and just trust that ec2:DescribeInstances is going to return an appropriate error if the instance ID is invalid.

We could also stick this regex in a global var instanceIDPattern = regexp.MustCompile(...) so we only compile once at init.

mattmoyer

A few small comments but LGTM. Thanks!

mattmoyer · 2018-03-27T20:36:53Z

README.md

@@ -173,6 +173,9 @@ server:
  # output `path` where a generated webhook kubeconfig will be stored.
  generateKubeconfig: /etc/kubernetes/heptio-authenticator-aws.kubeconfig # (default)

+  # role to assume before querying EC2 API in order to discover metadata like EC2 private DNS Name
+  defaultEC2DescribeInstancesRoleARN: arn:aws:iam::000000000000:role/DescribeInstancesRole


Nit: the "default" part here is slightly confusing to me. Maybe something like serverAssumeRoleARN?

Ah, default is incorrect now. I like having EC2DescribeInstances in the name though, as I think it helps differentiate it from the other references to roles. How about serverEC2DescribeInstancesRoleARN?

Yeah I like that.

mattmoyer · 2018-03-27T20:37:53Z

pkg/config/constants.go

@@ -29,4 +29,7 @@ const (

 	// certLifetime is the lifetime of the CA certificate (100 years)
 	certLifetime = time.Hour * 24 * 365 * 100
+
+	// nodeNamePrefix is the username prefix that the apiserver expects of kubelet
+	NodeNamePrefix = "system:node:"


This is no longer needed as far as I can tell.

mattmoyer · 2018-03-27T20:39:07Z

pkg/server/server.go

+			groups = append(groups, group)
+		}
+		return username, groups, nil
+	} else if userMapping, exists := h.lowercaseUserMap[arn]; exists {


Nit: these else if clauses could now be plain if clauses since we return early at the end of each one.

mattmoyer · 2018-03-27T20:41:05Z

pkg/server/server.go

+
+	// Private DNS requires EC2 API call
+	if strings.Contains(template, "{{EC2PrivateDNSName}}") {
+		re := regexp.MustCompile("^i-(\\w{8}|\\w{17})$")


I think it'd also be fine to do minimal validation here and just trust that ec2:DescribeInstances is going to return an appropriate error if the instance ID is invalid.

We could also stick this regex in a global var instanceIDPattern = regexp.MustCompile(...) so we only compile once at init.

mattmoyer · 2018-03-27T20:43:39Z

pkg/server/server.go

 	template = strings.Replace(template, "{{SessionName}}", sessionName, -1)
-	return template
+	re := regexp.MustCompile("[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*")


Same note here re: using a global var dns1123Pattern = regexp.MustCompile(...) compiled at init.

Moved to global.

…te DNS - kubelet reports itself as system:node:<aws-private-dns> - modifies authenticator to be able to discover an ec2 instance's private dns and use it for the node name when {{EC2PrivateDNSName}} is used in the template username - allows a role to be specified in the config file to assume for the describe instances call, in case it is in a separate account. Signed-off-by: Nick Turner <nic@amazon.com>

nckturner · 2018-03-27T21:31:33Z

Squashed review commit.

nckturner mentioned this pull request Mar 27, 2018

Add a NodeMapping that automatically maps a role to the name used by kubelet #61

Closed

mattlandis approved these changes Mar 27, 2018

View reviewed changes

dbenhur reviewed Mar 27, 2018

View reviewed changes

nckturner force-pushed the kubelet-username-templatized branch from 5ad7b14 to 00bca85 Compare March 27, 2018 20:41

mattmoyer approved these changes Mar 27, 2018

View reviewed changes

nckturner force-pushed the kubelet-username-templatized branch from 14d4f73 to 30b4923 Compare March 27, 2018 21:22

nckturner force-pushed the kubelet-username-templatized branch from 30b4923 to 9e6702c Compare March 27, 2018 21:31

nckturner merged commit bdea007 into kubernetes-sigs:master Mar 27, 2018

This was referenced Mar 27, 2018

Improve caching for ec2 DescribeInstances data #67

Closed

Add metrics for ec2 DescribeInstances calls #68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new template variable {{EC2PrivateDNSName}} #65

Add a new template variable {{EC2PrivateDNSName}} #65

nckturner commented Mar 27, 2018 •

edited

Loading

mattlandis left a comment

mattlandis Mar 27, 2018

mattlandis Mar 27, 2018

mattlandis Mar 27, 2018

mattlandis commented Mar 27, 2018

dbenhur Mar 27, 2018

mattlandis Mar 27, 2018

nckturner Mar 27, 2018

mattmoyer Mar 27, 2018

mattmoyer left a comment

mattmoyer Mar 27, 2018

nckturner Mar 27, 2018

mattmoyer Mar 27, 2018

mattmoyer Mar 27, 2018

nckturner Mar 27, 2018

mattmoyer Mar 27, 2018

nckturner Mar 27, 2018

mattmoyer Mar 27, 2018

mattmoyer Mar 27, 2018

nckturner Mar 27, 2018

nckturner commented Mar 27, 2018

		template = strings.Replace(template, "{{SessionName}}", sessionName, -1)
		return template

Add a new template variable {{EC2PrivateDNSName}} #65

Add a new template variable {{EC2PrivateDNSName}} #65

Conversation

nckturner commented Mar 27, 2018 • edited Loading

mattlandis 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

mattlandis commented Mar 27, 2018

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

mattmoyer 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

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

nckturner commented Mar 27, 2018

nckturner commented Mar 27, 2018 •

edited

Loading