Add auto enrolling capability to EKS Discover flow#38432
Conversation
d89a7c0 to
243366b
Compare
243366b to
07d2cce
Compare
gzdunek
left a comment
There was a problem hiding this comment.
Nit: the PR would be a bit simpler to review if extracting the shared components happened in a separate PR. Now it is difficult to distinguish between the moved code and the new one.
There was a problem hiding this comment.
TBH I'd just keep move content of this function to the catch clause, we use it only in that place.
Also we have a logic that isn't use at all (cfg.setAttempt).
|
@gzdunek thanks for the review! Yeah, you're right, it would be better if I at least did a dedicated commit for extraction. (AutoEnrollDialog was correctly identified as moved, SelfHostedAutoDiscoverDirections file content was copied almost exactly as is, except for changing databases text reference to "resources") |
There was a problem hiding this comment.
we should add a comment on what notifyAboutDelay means (i think skipDeployment made more sense tbh 🤷♀️ ). in the deployment section we let user know it'll take a few minutes, but since we are skipping deployment we have to show the note in the enrolling section
There was a problem hiding this comment.
Added comment. EKS doesn't have service deployment but has same need to warn user. I think it's a general capability that we want to notify user that for auto discovery resources will take some time to appear. be00e9c
There was a problem hiding this comment.
i'd leave it empty string if cloud, to make a point that that variable only applies to self-hosted
There was a problem hiding this comment.
i'm not sure this is necessary, think you can just do this?
export const WithTraitsAutoDiscovery = () => (
<MemoryRouter>
<SetupAccess {...props} agentMeta={...props.agentMeta, autoDiscovery= .....} />
</MemoryRouter>
);
i wasn't sure if you knew, but this copy is a shallow copy, nested things dont get copied and you could be modifying the original
There was a problem hiding this comment.
Yep, it's shallow copy, that's why we did it twice so we don't change original props. I didn't figure out syntax at first for the inline change (it needs extra braces agentMeta={{...props.agentMeta, autoDiscovery: {...} }} ), changed. 0fd4f00
There was a problem hiding this comment.
if auto discovering, does duplicated users/groups gets de-duplicated in the network call? i'd add a test for this
There was a problem hiding this comment.
Yep it's deduplicated.
There was a problem hiding this comment.
i'd add a test similar to what's being done in RDS
There was a problem hiding this comment.
I'll add test (and for useUserTraits), but will do it in a separate PR, since it will take me some time and I just want to make sure EKS auto discovery is merged for 15.1 release
There was a problem hiding this comment.
i think this is simple enough to just inline it like this:
.post(cfg.getDiscoveryConfigUrl(clusterId), {
name: req.name,
discoveryGroup: req.discoveryGroup,
aws: makeAwsMatchersReq(req.aws),
})
|
@kimlisa I changed the toggles to be stacked on top of each other, will ask Kenny later. |
9b6b1e8 to
ad5ab58
Compare
736bd91 to
fff35c4
Compare
1c5e0ff to
3cf8e63
Compare
a682f97 to
960584e
Compare
3cf8e63 to
3b16910
Compare
Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com>
Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com>
Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com>
Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com>
Co-authored-by: Lisa Kim <lisa@goteleport.com>
Co-authored-by: Lisa Kim <lisa@goteleport.com>
3b16910 to
79d30b4
Compare

This PR adds ability to setup autoenrollment of EKS clusters in the Discover UI using discover configs.
Autoenrollment of EKS is very similar to autoenrollment of RDS databases, so it follows same pattern, even a bit simpler because we don't need to think about deploying DB services. I extracted shared components -
SelfHostedAutoDiscoverDirectionsandAutoEnrollDialog.Demo in action: https://www.loom.com/share/5e4e6bb2d6494338ad96cad7964dc711
Changelog: Add auto-enrolling capabilities to EKS discover flow in the web UI