-
Notifications
You must be signed in to change notification settings - Fork 176
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
Allow specifying baseArn to prepend to role names. #95
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -60,6 +60,7 @@ func main() { | |||||
// annotation/volume configurations | ||||||
annotationPrefix := flag.String("annotation-prefix", "eks.amazonaws.com", "The Service Account annotation to look for") | ||||||
audience := flag.String("token-audience", "sts.amazonaws.com", "The default audience for tokens. Can be overridden by annotation") | ||||||
baseArn := flag.String("base-arn", "", "The base arn to use if a non fully qualified role is detected") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have few thoughts on this: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The prefix is always going to be specific to the cluster. It will be So one possibility would be to make this not configurable, simply prepending unqualified values with the prefix described above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the comments. I did not forget about this PR, and I'll try and make some time to work on it soon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
One of the main purposes of this feature is to hide the details of the environment (ie the AWS account ID) from the end user. If the end user had to add an annotation with the base ARN that included the account ID, then they could just write the fully qualified ARN and not use this feature at all, so I'm not sure if it would be that useful.
I can do this. If an end user sets both, the most reasonable (and backwards compatible) behavior to me seems to be to take the original annotation
Do you think this is required if we do 2. and make the new annotation only a fallback if the original one doesn't exist?
WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
mountPath := flag.String("token-mount-path", "/var/run/secrets/eks.amazonaws.com/serviceaccount", "The path to mount tokens") | ||||||
tokenExpiration := flag.Int64("token-expiration", 86400, "The token expiration") | ||||||
region := flag.String("aws-default-region", "", "If set, AWS_DEFAULT_REGION and AWS_REGION will be set to this value in mutated containers") | ||||||
|
@@ -112,6 +113,7 @@ func main() { | |||||
handler.WithServiceAccountCache(saCache), | ||||||
handler.WithRegion(*region), | ||||||
handler.WithRegionalSTS(*regionalSTS), | ||||||
handler.WithBaseArn(*baseArn), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
) | ||||||
|
||||||
addr := fmt.Sprintf(":%d", *port) | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -74,6 +74,8 @@ var ( | |||||||||
deserializer = codecs.UniversalDeserializer() | ||||||||||
) | ||||||||||
|
||||||||||
const arnPrefix = "arn:" | ||||||||||
|
||||||||||
// ModifierOpt is an option type for setting up a Modifier | ||||||||||
type ModifierOpt func(*Modifier) | ||||||||||
|
||||||||||
|
@@ -102,6 +104,11 @@ func WithRegionalSTS(enabled bool) ModifierOpt { | |||||||||
return func(m *Modifier) { m.RegionalSTSEndpoint = enabled } | ||||||||||
} | ||||||||||
|
||||||||||
// WithBaseArn sets the baseArn to use if we detect role name is not fully qualified | ||||||||||
func WithBaseArn(baseArn string) ModifierOpt { | ||||||||||
Comment on lines
+107
to
+108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
return func(m *Modifier) { m.BaseArn = baseArn } | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
} | ||||||||||
|
||||||||||
// WithAnnotationDomain adds an annotation domain | ||||||||||
func WithAnnotationDomain(domain string) ModifierOpt { | ||||||||||
return func(m *Modifier) { m.AnnotationDomain = domain } | ||||||||||
|
@@ -127,6 +134,7 @@ func NewModifier(opts ...ModifierOpt) *Modifier { | |||||||||
// Modifier holds configuration values for pod modifications | ||||||||||
type Modifier struct { | ||||||||||
AnnotationDomain string | ||||||||||
BaseArn string | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
Expiration int64 | ||||||||||
MountPath string | ||||||||||
Region string | ||||||||||
|
@@ -155,7 +163,7 @@ func logContext(podName, podGenerateName, serviceAccountName, namespace string) | |||||||||
namespace) | ||||||||||
} | ||||||||||
|
||||||||||
func (m *Modifier) addEnvToContainer(container *corev1.Container, tokenFilePath, roleName string, podSettings *podUpdateSettings) bool { | ||||||||||
func (m *Modifier) addEnvToContainer(container *corev1.Container, tokenFilePath, fullRoleArn string, podSettings *podUpdateSettings) bool { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
// return if this is a named skipped container | ||||||||||
if _, ok := podSettings.skipContainers[container.Name]; ok { | ||||||||||
klog.V(4).Infof("Container %s was annotated to be skipped", container.Name) | ||||||||||
|
@@ -225,7 +233,7 @@ func (m *Modifier) addEnvToContainer(container *corev1.Container, tokenFilePath, | |||||||||
if !reservedKeysDefined { | ||||||||||
env = append(env, corev1.EnvVar{ | ||||||||||
Name: "AWS_ROLE_ARN", | ||||||||||
Value: roleName, | ||||||||||
Value: fullRoleArn, | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
}) | ||||||||||
|
||||||||||
env = append(env, corev1.EnvVar{ | ||||||||||
|
@@ -258,6 +266,14 @@ func (m *Modifier) addEnvToContainer(container *corev1.Container, tokenFilePath, | |||||||||
return changed | ||||||||||
} | ||||||||||
|
||||||||||
func (m *Modifier) fullRoleArn(arn string) string { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
if strings.HasPrefix(arn, arnPrefix) { | ||||||||||
return arn | ||||||||||
} | ||||||||||
|
||||||||||
return fmt.Sprintf("%s%s", m.BaseArn, arn) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
} | ||||||||||
|
||||||||||
func (m *Modifier) updatePodSpec(pod *corev1.Pod, roleName, audience string, regionalSTS bool) ([]patchOperation, bool) { | ||||||||||
updateSettings := newPodUpdateSettings(m.AnnotationDomain, pod, regionalSTS) | ||||||||||
|
||||||||||
|
@@ -272,17 +288,19 @@ func (m *Modifier) updatePodSpec(pod *corev1.Pod, roleName, audience string, reg | |||||||||
tokenFilePath = "C:" + strings.Replace(tokenFilePath, `/`, `\`, -1) | ||||||||||
} | ||||||||||
|
||||||||||
fullRoleArn := m.fullRoleArn(roleName) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
var changed bool | ||||||||||
var initContainers = []corev1.Container{} | ||||||||||
for i := range pod.Spec.InitContainers { | ||||||||||
container := pod.Spec.InitContainers[i] | ||||||||||
changed = m.addEnvToContainer(&container, tokenFilePath, roleName, updateSettings) | ||||||||||
changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleArn, updateSettings) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
initContainers = append(initContainers, container) | ||||||||||
} | ||||||||||
var containers = []corev1.Container{} | ||||||||||
for i := range pod.Spec.Containers { | ||||||||||
container := pod.Spec.Containers[i] | ||||||||||
changed = m.addEnvToContainer(&container, tokenFilePath, roleName, updateSettings) | ||||||||||
changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleArn, updateSettings) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
containers = append(containers, container) | ||||||||||
} | ||||||||||
|
||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -48,6 +48,7 @@ var ( | |||||||||
handlerExpirationAnnotation = "testing.eks.amazonaws.com/handler/expiration" | ||||||||||
handlerRegionAnnotation = "testing.eks.amazonaws.com/handler/region" | ||||||||||
handlerSTSAnnotation = "testing.eks.amazonaws.com/handler/injectSTS" | ||||||||||
handlerBaseArnAnnotation = "testing.eks.amazonaws.com/handler/baseArn" | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
) | ||||||||||
|
||||||||||
func getModifierFromPod(pod corev1.Pod) (*Modifier, error) { | ||||||||||
|
@@ -73,6 +74,9 @@ func getModifierFromPod(pod corev1.Pod) (*Modifier, error) { | |||||||||
} | ||||||||||
modifiers = append(modifiers, WithRegionalSTS(value)) | ||||||||||
} | ||||||||||
if baseArnAnnotation, ok := pod.Annotations[handlerBaseArnAnnotation]; ok { | ||||||||||
modifiers = append(modifiers, WithBaseArn(baseArnAnnotation)) | ||||||||||
Comment on lines
+77
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
} | ||||||||||
return NewModifier(modifiers...), nil | ||||||||||
} | ||||||||||
|
||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,17 @@ | ||||||
apiVersion: v1 | ||||||
kind: Pod | ||||||
metadata: | ||||||
name: balajilovesoreos | ||||||
annotations: | ||||||
testing.eks.amazonaws.com/handler/baseArn: 'arn:aws:iam::111122223333:role/' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
testing.eks.amazonaws.com/skip: "false" | ||||||
testing.eks.amazonaws.com/serviceAccount/roleArn: "s3-reader" | ||||||
testing.eks.amazonaws.com/serviceAccount/audience: "sts.amazonaws.com" | ||||||
testing.eks.amazonaws.com/expectedPatch: '[{"op":"add","path":"/spec/volumes/0","value":{"name":"aws-iam-token","projected":{"sources":[{"serviceAccountToken":{"audience":"sts.amazonaws.com","expirationSeconds":86400,"path":"token"}}]}}},{"op":"add","path":"/spec/containers","value":[{"name":"balajilovesoreos","image":"amazonlinux","env":[{"name":"AWS_ROLE_ARN","value":"arn:aws:iam::111122223333:role/s3-reader"},{"name":"AWS_WEB_IDENTITY_TOKEN_FILE","value":"/var/run/secrets/eks.amazonaws.com/serviceaccount/token"}],"resources":{},"volumeMounts":[{"name":"aws-iam-token","readOnly":true,"mountPath":"/var/run/secrets/eks.amazonaws.com/serviceaccount"}]}]}]' | ||||||
spec: | ||||||
containers: | ||||||
- image: amazonlinux | ||||||
name: balajilovesoreos | ||||||
serviceAccountName: default | ||||||
volumes: | ||||||
- name: my-volume |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.