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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ Usage of amazon-eks-pod-identity-webhook:
--alsologtostderr log to standard error as well as files
--annotation-prefix string The Service Account annotation to look for (default "eks.amazonaws.com")
--aws-default-region string If set, AWS_DEFAULT_REGION and AWS_REGION will be set to this value in mutated containers
--base-arn string The base arn to use if a non fully qualified role is detected
Copy link

Choose a reason for hiding this comment

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

Suggested change
--base-arn string The base arn to use if a non fully qualified role is detected
--role-base-arn string The base ARN to use as a prefix for completing role ARNs that are not fully qualified

--in-cluster Use in-cluster authentication and certificate request API (default true)
--enable-debugging-handlers Enable debugging handlers on the metrics port (http). Currently /debug/alpha/cache is supported (default false) [ALPHA]
--kube-api string (out-of-cluster) The url to the API server
Expand Down
2 changes: 2 additions & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

I have few thoughts on this:
1- This mechanism allows usage only when the user has access to change the parameters. Non cluster admins would not be able to use this functionality. wdyt about making this extensible by adding an new annotations on service accounts?
2- rollout and rollback(if needed) of this might become tricky due to using the same rolearn annotation. wdyt about using entirely different parameters for annotation like eks.amazonaws.com/role and eks.amazonaws.com/role-prefix that clarifies the intent of the fields explicitly
3. Can we add a feature flag for this functionality (set to alpha first and disabled by default) so that only interested users can flip this on.

Choose a reason for hiding this comment

The 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 arn:aws:iam::, followed by the ID of the account the cluster is in, followed by :role/, followed by the name of the cluster, followed by some fixed separator (I use _).

So one possibility would be to make this not configurable, simply prepending unqualified values with the prefix described above.

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

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

1- This mechanism allows usage only when the user has access to change the parameters. Non cluster admins would not be able to use this functionality. wdyt about making this extensible by adding an new annotations on service accounts?

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.

2- rollout and rollback(if needed) of this might become tricky due to using the same rolearn annotation. wdyt about using entirely different parameters for annotation like eks.amazonaws.com/role and eks.amazonaws.com/role-prefix that clarifies the intent of the fields explicitly

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 eks.amazonaws.com/role verbatim and not make any changes.

  1. Can we add a feature flag for this functionality (set to alpha first and disabled by default) so that only interested users can flip this on.

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?

The prefix is always going to be specific to the cluster. It will be arn:aws:iam::, followed by the ID of the account the cluster is in, followed by :role/, followed by the name of the cluster, followed by some fixed separator (I use _).

So one possibility would be to make this not configurable, simply prepending unqualified values with the prefix described above.
This seems reasonable to me too. I can add a flag for --aws-default-account-id similar to how we have flags for other AWS environment like region.

WDYT?

Copy link

Choose a reason for hiding this comment

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

Suggested change
baseArn := flag.String("base-arn", "", "The base arn to use if a non fully qualified role is detected")
roleBaseARN := flag.String("role-base-arn", "", "The base ARN to use as a prefix for completing role ARNs that are not fully qualified")

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")
Expand Down Expand Up @@ -112,6 +113,7 @@ func main() {
handler.WithServiceAccountCache(saCache),
handler.WithRegion(*region),
handler.WithRegionalSTS(*regionalSTS),
handler.WithBaseArn(*baseArn),
Copy link

Choose a reason for hiding this comment

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

Suggested change
handler.WithBaseArn(*baseArn),
handler.WithRoleBaseARN(*roleBaseARN),

)

addr := fmt.Sprintf(":%d", *port)
Expand Down
26 changes: 22 additions & 4 deletions pkg/handler/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

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

Suggested change
// WithBaseArn sets the baseArn to use if we detect role name is not fully qualified
func WithBaseArn(baseArn string) ModifierOpt {
// WithRoleBaseARN sets the base ARN to use as a prefix for completing role ARNs that are not fully qualified
func WithRoleBaseARN(baseARN string) ModifierOpt {

return func(m *Modifier) { m.BaseArn = baseArn }
Copy link

Choose a reason for hiding this comment

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

Suggested change
return func(m *Modifier) { m.BaseArn = baseArn }
return func(m *Modifier) { m.RoleBaseARN = baseARN }

}

// WithAnnotationDomain adds an annotation domain
func WithAnnotationDomain(domain string) ModifierOpt {
return func(m *Modifier) { m.AnnotationDomain = domain }
Expand All @@ -127,6 +134,7 @@ func NewModifier(opts ...ModifierOpt) *Modifier {
// Modifier holds configuration values for pod modifications
type Modifier struct {
AnnotationDomain string
BaseArn string
Copy link

Choose a reason for hiding this comment

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

Suggested change
BaseArn string
RoleBaseARN string

Expiration int64
MountPath string
Region string
Expand Down Expand Up @@ -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 {
Copy link

Choose a reason for hiding this comment

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

Suggested change
func (m *Modifier) addEnvToContainer(container *corev1.Container, tokenFilePath, fullRoleArn string, podSettings *podUpdateSettings) bool {
func (m *Modifier) addEnvToContainer(container *corev1.Container, tokenFilePath, fullRoleARN string, podSettings *podUpdateSettings) bool {

// 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)
Expand Down Expand Up @@ -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,
Copy link

Choose a reason for hiding this comment

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

Suggested change
Value: fullRoleArn,
Value: fullRoleARN,

})

env = append(env, corev1.EnvVar{
Expand Down Expand Up @@ -258,6 +266,14 @@ func (m *Modifier) addEnvToContainer(container *corev1.Container, tokenFilePath,
return changed
}

func (m *Modifier) fullRoleArn(arn string) string {
Copy link

Choose a reason for hiding this comment

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

Suggested change
func (m *Modifier) fullRoleArn(arn string) string {
func (m *Modifier) fullRoleARN(arn string) string {

if strings.HasPrefix(arn, arnPrefix) {
return arn
}

return fmt.Sprintf("%s%s", m.BaseArn, arn)
Copy link

Choose a reason for hiding this comment

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

Suggested change
return fmt.Sprintf("%s%s", m.BaseArn, arn)
return fmt.Sprintf("%s%s", m.RoleBaseARN, arn)

}

func (m *Modifier) updatePodSpec(pod *corev1.Pod, roleName, audience string, regionalSTS bool) ([]patchOperation, bool) {
updateSettings := newPodUpdateSettings(m.AnnotationDomain, pod, regionalSTS)

Expand All @@ -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)
Copy link

Choose a reason for hiding this comment

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

Suggested change
fullRoleArn := m.fullRoleArn(roleName)
fullRoleARN := m.fullRoleARN(roleName)


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)
Copy link

Choose a reason for hiding this comment

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

Suggested change
changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleArn, updateSettings)
changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleARN, updateSettings)

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)
Copy link

Choose a reason for hiding this comment

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

Suggested change
changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleArn, updateSettings)
changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleARN, updateSettings)

containers = append(containers, container)
}

Expand Down
4 changes: 4 additions & 0 deletions pkg/handler/handler_pod_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link

Choose a reason for hiding this comment

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

Suggested change
handlerBaseArnAnnotation = "testing.eks.amazonaws.com/handler/baseArn"
handlerBaseARNAnnotation = "testing.eks.amazonaws.com/handler/baseARN"

)

func getModifierFromPod(pod corev1.Pod) (*Modifier, error) {
Expand All @@ -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
Copy link

Choose a reason for hiding this comment

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

Suggested change
if baseArnAnnotation, ok := pod.Annotations[handlerBaseArnAnnotation]; ok {
modifiers = append(modifiers, WithBaseArn(baseArnAnnotation))
if baseARNAnnotation, ok := pod.Annotations[handlerBaseARNAnnotation]; ok {
modifiers = append(modifiers, WithBaseARN(baseARNAnnotation))

}
return NewModifier(modifiers...), nil
}

Expand Down
17 changes: 17 additions & 0 deletions pkg/handler/testdata/rawPodNoBaseArn.pod.yaml
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/'
Copy link

Choose a reason for hiding this comment

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

Suggested change
testing.eks.amazonaws.com/handler/baseArn: 'arn:aws:iam::111122223333:role/'
testing.eks.amazonaws.com/handler/baseARN: 'arn:aws:iam::111122223333:role/'

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