Add PoC EKS node membership validation #5969
Conversation
There was a problem hiding this comment.
Hi @meyskens, thanks for proposing these changes! The high-level approach in terms of AWS integration looks good to me. I have some preliminary comments but haven't had a chance to review all the implementation details in the PR yet (I also assumed this PR is not quite complete since it is in Draft status).
Since we are going to the extent of validating that a node belongs to a particular EKS cluster, I feel it would also be worthwhile to emit the EKS cluster as a node selector in case anyone wants to match a given set of entries to a specific set of clusters. This relates to my comment in the PR regarding opening up the configuration to allow for nodes running in multiple clusters to attest to the same SPIRE Server.
| ) | ||
|
|
||
| type eksValidationConfig struct { | ||
| EKSClusterName string `hcl:"eks_cluster_name"` |
There was a problem hiding this comment.
I think this should be a list of cluster names rather than an individual one. I don't think we need to restrict how users map clusters to their SPIRE Server deployment.
| type eksValidationConfig struct { | ||
| EKSClusterName string `hcl:"eks_cluster_name"` | ||
|
|
||
| NodesListTTL string `hcl:"eks_nodes_map_ttl"` |
There was a problem hiding this comment.
I would prefer this not be a user-controlled configuration if we can avoid it. Can we find a reasonable default value?
There was a problem hiding this comment.
Will remove it from the config file, the idea came from keeping it similar to the account check
| } | ||
|
|
||
| for { | ||
| for _, instance := range describeASGOp.AutoScalingGroups[0].Instances { |
There was a problem hiding this comment.
Only examining the first autoscaling group looks like a bug to me. Is there any reason for limiting to only the first autoscaling group in the node group?
There was a problem hiding this comment.
It was one of those quick PoC code bugs 😄 Will fix it in the next commit
|
Hi @meyskens, just checking, is this something you are still interested in contributing? I noticed there hasn't been any update here in a while. |
|
Hi @meyskens, I think we should be ok on the design other than the comments I left in the PR here. We were mostly interested in seeing the details of how we would associate nodes to EKS clusters to make sure the attestation looked secure. This part looks good to me based on how things are implemented in this PR. |
Signed-off-by: Maartje Eyskens <[email protected]>
Signed-off-by: Maartje Eyskens <[email protected]>
meyskens
force-pushed
the
aws-eks-attestattion
branch
from
7a68e6b to
086d2b5
Compare
Signed-off-by: Maartje Eyskens <[email protected]>
|
Hey @meyskens, I noticed there's some recent pushes to this PR as a force push. It's been a bit since I last looked at these changes so it's a bit hard for me to tell what's changed since I last reviewed. Whenever this is ready for review, feel free to move this PR out of draft state, and I'll have another look at it. |
Draft PR for discussion in #5956
Pull Request check list
Affected functionality
Description of change
Which issue this PR fixes