Set up pod identity association in E2E tests #329
Conversation
| podIdentityAddon := Addon{ | ||
| Name: podIdentityAgent, | ||
| Cluster: v.Cluster, | ||
| podIdentityAddon := PodIdentityAddon{ |
There was a problem hiding this comment.
can this just be a Addon instead?
it seems like we don't need the pod identity struct since we only want to wait until it's active?
| var err error | ||
| if err = podIdentityAddon.WaitUtilActive(timeoutCtx, v.EKSClient, v.Logger); err != nil { |
There was a problem hiding this comment.
This is to ensure the add-on is in Active state.
| return fmt.Errorf("creating kubernetes client: %w", err) | ||
| } | ||
|
|
||
| podIdentityAddon := addon.PodIdentityAddon{ |
There was a problem hiding this comment.
could we have a constructor for this?
I'm not against creating this "service" objects with a struct directly, but in this case it seems like to do so we have to reveal a lot of the internal details: the fact that wraps an Addon, the name of the add-on, the configuration... It appears that those details could be hidden in a constructor so they remain internal to the addon package.
| } | ||
|
|
||
| err = podIdentityAddon.Create(ctx, c.eks, c.logger) | ||
| if err != nil && !errors.IsType(err, &types.ResourceInUseException{}) { |
There was a problem hiding this comment.
aren't you handling that error now in the Create method itself? it does seem like it's a good thing to hide if the intent is to make the create operation idempotent
|
|
||
| func NewServiceAccount(ctx context.Context, logger logr.Logger, k8s *kubernetes.Clientset, namespace, name string) error { | ||
| if _, err := k8s.CoreV1().ServiceAccounts(namespace).Get(ctx, name, metav1.GetOptions{}); err == nil { | ||
| logger.Info("Service account has already been created", "namespace", namespace, name) |
There was a problem hiding this comment.
super nit
| logger.Info("Service account has already been created", "namespace", namespace, name) | |
| logger.Info("Service account already exists", "namespace", namespace, "name", name) |
Issue #, if available:
This is part 2 of setting up E2E tests for pod identity. Part 1 PR
Description of changes:
This PR covers pod identity association setup.
(Since this PR already contains enough changes, I will create another PR to actually deploy pods and verify EKS Pod Identity volume is correctly attached.)
I followed this public document to set up pod identity association.
In summary, the following is required in order to set up pod identity association.
The creation of service account and pod identity association can only be done after the EKS cluster is provisioned successfully. We can put this into either
Here I choose to put it into creation of pod identity add-on. The reason for it is we don't need to check or set up this association in every
OS/auth providercombination. It should be a one-time setup for the whole EKS cluster.The creation of IAM role of pod identity can be put into
nodeadm-stack.tsto provision one IAM role for allkube/CNIcombinationsetup-cfn.yamlto provision one IAM role for allOS/Auth providerunder onekube/CNIcombinationOS/Auth providerOption 3 is not applicable for two reasons.
OS/Auth providerOption 1 and 2 are both working but I choose option 2 because it makes more sense as the role is tied to one cluster, which the lifecycle is managed by CloudFormation.
Testing (if applicable):
Documentation added/planned (if applicable):
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.