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

EKS Provisioning #10581

Merged
merged 53 commits into from
Apr 26, 2024
Merged

Conversation

mantis-toboggan-md
Copy link
Member

@mantis-toboggan-md mantis-toboggan-md commented Mar 8, 2024

Summary

Fixes #8966
#6840
#10510
#7295

Also fixes #10416

I hit problems with KeyValue and LabeledSelect in this PR, which I'm now looking into:
#9668
#10587
The LabeledSelect bug in particular is blocking this PR.

Occurred changes and/or fixed issues

All EKS provisioning functionality from Ember should be replicated here. The only changes are some additional validation, and fewer hardcoded lists: version, region, and instance type are dynamically loaded in this form and hardcoded in the old EKS form.

Technical notes summary

  • it is expected that some users wont have permission to load k8s versions nor kms keys - the describeAddonVersions and listKeys aws api calls can fail silently
  • regions can't be loaded until we have authetnicated with aws...using a region...so the credential dropdown still uses a hardcoded list of regions, but the eks form's dropdown doesn't
  • the description field in Ember doesn't work
  • nodegroupName is not auto-generated in the Ember UI - these ec2 resources have other tags identifying them in aws so I think it's ok for us to default to something relatively meaningless like group1

Areas or cases that should be tested

  1. Creating new EKS Clusters
  2. Editing existing EKS clusters
  3. Editing EKS clusters before they've finished provisioning (confirm which fields are editable at which stage)
  4. EKS Launch templates Launch template support in Rancher UI rancher#30613 (comment)

Areas which could experience regressions

Ensure the correct page still loads when creating/editing/importing other cluster types (aks, gke, rke1, rke2)

Screenshot/Video

Screenshot 2024-03-08 at 8 44 28 AM
Screenshot 2024-03-08 at 8 44 51 AM
Screenshot 2024-03-08 at 8 45 18 AM

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

I've given everything a brief view. As like the AKS PR i didn't go in to detail around config options, validating how they're actually set in aws, etc. That one's best left for someone QA side with lots of AWS knowledge.

Clusters come up as before and can be edited fine.

I spotted the following generally things

Remaining / New

Resolved

  • With no creds (or multiple?) the create creds page shows the cluster name, description and region inputs at top
  • Create Amazon EKS - Main page
    • Top of page - There's a difference between AKS and EKS
      • New EKS has row 1 - name, description. row 2 - region. row 3 - cloud credential
      • New ALS has row 1 - cloud crendential. row 2 - name, description, region, kube version
      • Could these align somehow?
        • Would think that EKS looks best, but maybe move cloud credential on the same row as region (order tbc). atm the values look real lost given the large width of the controls
        • AKS would become row - 1 name, description. row 2 - region, cloud credential, kube version
    • Node Pools
      • Missing Node Pools header (this is in new AKS, though is pretty small there, should it be bigger?)
      • Node Template Details
        • Can the width of the Node Template Details input be expanded to display the default option without expanding the drop down?
        • Applying a launch template populates some of the fields, but switching back to Default doesn't reset those back to their original 'default' values
        • To confirm, in old world i see no Launch Template options. It's a bug fix we now show them in the new world?
        • There's some nice info text below the old AMI ID option. If that's still relevant can it come across?
        • User Data
          • Useful text in old world lost in new
          • We've lost the Read from a file option for User Data. Super low priority though.
    • Networking
      • Public / Private Networking options were grouping in an API Server Endpoint Access section with a nice blurb. If that's still relevant can we keep? Maybe even docs link optional
      • The endpoints also have a small blurb that's missing
    • Labels and Annotations
      • This is now an empty drop down (no way to add either type)
  • Edit Amazon EKS - Main page
    • Changes from previous disabled/enabled state (are these fixes?)
      • Credentials
        • Can change the cloud credential
      • Node Pools
        • Unable to change the desired/min/max of an existing pool
        • Can change the gpu enabled instance and request spot instances
      • Labels and Annotations
        • Some of these (both types) are now editable. From memory i think there's a hardcoded list that defines them
  • Generally
    • I created a cluster and assigned a standard user as a cluster member
    • The user can then see the cluster. They can't edit... but going to the cluster detail's Config page gives a number of warning notifications (below). I think these shouldn't happen (user can make requests via meta/proxy with cloud credential)? It might be a general problem that applies to other cluster providers as well, if so this can be a different issue to agree a pattern?
      • Request is missing Authentication Token
      • Error fetching regions: MissingParameter: The request must contain the parameter AWSAccessKeyId
      • The request must contain the parameter AWSAccessKeyId
    • After creating a new cluster it failed to come up with the error
      • Waiting for API to be available:error getting or creating launch template: error creating launch template:~ InvalidLaunchTemplateName.AlreadyExistsException: Launch template name already in use. status code: 400, request id: 35b6b40b-45c5-4436-a0fc-0771aab78ba5
      • the launch template is set to Default
      • i probably re-used the cluster name of rc-rke and node pool name of rc-rke
      • do we pass through an id for the launch template, is it based on the pool name? if so do we need to make these context unique?
      • [BUG]Unable to re-use EKS cluster names rancher#45044

pkg/eks/components/CruEKS.vue Show resolved Hide resolved
pkg/eks/provisioner.ts Outdated Show resolved Hide resolved
pkg/eks/components/AccountAccess.vue Outdated Show resolved Hide resolved
pkg/eks/components/AccountAccess.vue Outdated Show resolved Hide resolved
pkg/eks/components/CruEKS.vue Outdated Show resolved Hide resolved
pkg/eks/components/Networking.vue Outdated Show resolved Hide resolved
pkg/eks/components/CruEKS.vue Outdated Show resolved Hide resolved
pkg/eks/components/CruEKS.vue Outdated Show resolved Hide resolved
pkg/eks/components/NodeGroup.vue Outdated Show resolved Hide resolved
pkg/eks/components/NodeGroup.vue Outdated Show resolved Hide resolved
@mantis-toboggan-md
Copy link
Member Author

  * To confirm, in old world i see no `Launch Template` options. It's a bug fix we now show them in the new world?

What options do you see in the new world that you dont in the old world? Is this a new cluster or editing an existing cluster? Same region and credential selected in both UIs? I haven't seen discrepancies.

  • With no creds (or multiple?) the create creds page shows the cluster name, description and region inputs at top

  • Create Amazon EKS - Main page

    • Top of page - There's a difference between AKS and EKS

      • New EKS has row 1 - name, description. row 2 - region. row 3 - cloud credential

      • New ALS has row 1 - cloud crendential. row 2 - name, description, region, kube version

      • Could these align somehow?

        • Would think that EKS looks best, but maybe move cloud credential on the same row as region (order tbc). atm the values look real lost given the large width of the controls
        • AKS would become row - 1 name, description. row 2 - region, cloud credential, kube version
        • If the above sounds good, i can create an issue for the aks change

I've updated the top area to show the region and credential dropdown on the same row, then give the credential component the whole width of the form when creating a new credential.

  • Node Pools

    • Missing Node Pools header (this is in new AKS, though is pretty small there, should it be bigger?)

    • Node Template Details

      • Can the width of the Node Template Details input be expanded to display the default option without expanding the drop down?

I've added the header here and increased the one in AKS from h4 to h3. Played around with input widths and added various info boxes/warnings

  * Applying a launch template populates some of the fields, but switching back to `Default` doesn't reset those back to their original 'default' values

I updated the form so that if you clear your launch template selection or select a new version without a value for a particular field, the default value is re-applied. Also added a test for this

  * There's some nice info text below the old `AMI ID` option. If that's still relevant can it come across?
  * User Data
    
    * Useful text in old world lost in new
    * We've lost the `Read from a file` option for User Data. Super low priority though.

Both added now.

  • Networking

    • Public / Private Networking options were grouping in an API Server Endpoint Access section with a nice blurb. If that's still relevant can we keep? Maybe even docs link optional
    • The endpoints also have a small blurb that's missing

I readded these as an info banner

  • Edit Amazon EKS - Main page
  • Changes from previous disabled/enabled state (are these fixes?)

    • Credentials

      • Can change the cloud credential
    • Node Pools

      • Unable to change the desired/min/max of an existing pool
      • Can change the gpu enabled instance and request spot instances

There was a bug here with detecting the active/provisioning state of the underlying eks cluster...these inputs should match up with the old ui now. I also updated the launch template dropdown to clarify when a nodegroup has a rancher-managed template as that affects which fields can be edited per rancher/rancher#30613 (comment)

* Labels and Annotations
  
  * Some of these (both types) are now editable. From memory i think there's a hardcoded list that defines them

I see that we do taht in some places with steve resources, it looks like the list is a little different with these clusters though. I've grabbed the list of protected labels/annotations from the old UI and brought them over for this resource.

  • Generally

    • I created a cluster and assigned a standard user as a cluster member

    • The user can then see the cluster. They can't edit... but going to the cluster detail's Config page gives a number of warning notifications (below). I think these shouldn't happen (user can make requests via meta/proxy with cloud credential)? It might be a general problem that applies to other cluster providers as well, if so this can be a different issue to agree a pattern?

      • Request is missing Authentication Token
      • Error fetching regions: MissingParameter: The request must contain the parameter AWSAccessKeyId
      • The request must contain the parameter AWSAccessKeyId

This is a more general problem. I tried a few other providers and they all have poor ux: showing errors instead of displaying the form if the user doesn't have permission to access the cloud credential. I tried aks and rke2 on ec2, digital ocean, and azure they all throw errors when a standard user/cluster member tries to 'view config'. I updated the EKS config view to not run any of those aws api calls in 'view' mode because they're uneeded anyway. I filed an issue for the other providers: #10734

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

I've updated the original review comment. i think two new, most resolved, a few left over

pkg/eks/components/CruEKS.vue Show resolved Hide resolved
pkg/eks/components/AccountAccess.vue Show resolved Hide resolved
pkg/eks/components/AccountAccess.vue Outdated Show resolved Hide resolved
@mantis-toboggan-md
Copy link
Member Author

mantis-toboggan-md commented Apr 8, 2024

With no creds (or multiple?) the create creds page shows the cluster name, description and region inputs at top

Updated this so they're hidden when there's no credential. I kept these inputs above the credential/region dropdown though, as I do think they look better there.

The endpoints also have a small blurb that's missing

I rolled this and the other networking blurb into one info banner because it seemed to work best with the input component options we have in the new UI, and they do seem like conceptually connected messages.

Labels and Annotations
This is now an empty drop down (no way to add either type)

Fixed this and added a couple unit tests in the area

After creating a new cluster it failed to come up with the error...

I think there may actually be a new backend issue around here?? I see this bug now, and did not in the past. I also see it with the old ui. I've used the same name in the past, including with the default launch template option (specifically I have created many all-default clusters in the old ui and new ui to compare). This morning I also tried reusing a cluster name with unique node group name, and still saw that error.

Selecting the 'default' option in the UI removes any launch template configuration from the cluster spec so I think this is going to be purely a backend fix. I opened a backend issue here:rancher/rancher#45044. I didn't label that as blocking the UI, not sure it qualifies?

@richard-cox thanks again for another review, sorry I missed a few requests last time, that was unintentional.

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

RE rancher/rancher#45044 - perfect, thanks

I've updated #10581 (review). One for me, one for you.

One inline comment remaining.

@mantis-toboggan-md
Copy link
Member Author

user data guide text overlaps button and link is not clickable (not sure why)

That threw me for a moment. Turns out all label tags within labeledinput have pointer-events: none;. I've added an exception for the sublabel.

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

Was just about to approve but noticed a possible regression.

Node groups aren't editable, in contrast to the old behaviour where a lot of the details were. Did we decided this was a bug in the old ui (given templates)? Everything else looks good

image

image

@mantis-toboggan-md
Copy link
Member Author

mantis-toboggan-md commented Apr 16, 2024

Nice catch Richard, that is a regression. Node group-level details should generally be editable (besides name and role): I updated the desired/min/max size inputs as well as labels and tags. I also verified that the Ember UI is handling those fields correctly, by confirming that updating them in Rancher will update the EKS cluster accordingly if done either before the underlying EKS cluster is created; after the cluster is created, but before the node groups are done creating; or after the node groups are running.

@mantis-toboggan-md mantis-toboggan-md merged commit 557d028 into rancher:master Apr 26, 2024
27 checks passed
instanceType: fvGetAndReportPathRules('instanceType'),
diskSize: fvGetAndReportPathRules('diskSize')
}"
:node-role.sync="node.nodeRole"
Copy link
Member

Choose a reason for hiding this comment

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

Won't .sync complicate our Vue3 migration?
https://eslint.vuejs.org/rules/no-deprecated-v-bind-sync.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AKS cluster page while editing cluster showing old UI Move EKS from Ember to Vue
3 participants