feat: [LKEAPIFW-428] LKE clusters should have IP ACL integration on CM (part 1) #10968

talmai · 2024-09-19T06:05:12Z

Description 📝

Enables creation of LKE clusters with IP ACLS; enables updates of already migrated clusters

Changes 🔄

Adds new UI components
adds respective library calls
some style cleanup

*** tests will be on a separate PR

Target release date 🗓️

Oct 28th release

Preview 📷

Cluster creation:

Cluster summary:

IPACL enabled	IPACL not enabled

New IPACL Drawer:

Cluster migrated (IPACL enabled/not enabled)	Cluster not yet migrated

How to test 🧪

Spin up this branch, update to reach alpha's apinext backend. Once all set, attempt to create new clusters with ipacls and then update them.

Prerequisites

See how to test

Verification steps

Create a Cluster with IPACL enabled (toggled on). Confirm button on summary page lists number of IPs added
Create a Cluster with IPACL disabled (toggled off). Confirm button on summary page says 'Enable'. If you added any IPs, confirm they show up in the drawer
Create a Cluster that does not have IPACL. Confirm button on summary page says 'Enable' (it will take several seconds to load) and that drawer matches the screenshot for 'Cluster not yet migrated'. Update/toggle IPACL, confirm flow now follows a cluster with IPACL enabled
- See Talmai's comment for how to create these clusters. I'v also just been creating clusters on alpha (but not on this branch/using the develop branch), and then going back to this branch

As an Author I have considered 🤔

Check all that apply

✅ 👀 Doing a self review
✅ ❔ Our contribution guidelines
✅ 🤏 Splitting feature into small PRs
[NO see comment] ➕ Adding a changeset
[NO ] 🧪 Providing/Improving test coverage
✅ 🔐 Removing all sensitive information from the code and PR description
[NO] 🚩 Using a feature flag to protect the release
✅ 👣 Providing comprehensive reproduction steps
[NO] 📑 Providing or updating our documentation
[NO] 🕛 Scheduling a pair reviewing session
[N/A] 📱 Providing mobile support
[N/A] ♿ Providing accessibility support

jcallahan-akamai · 2024-09-19T14:24:52Z

Thanks for the contribution @talmai. Would you please move this to Draft until it's ready to be reviewed?

…s; IPACL drawer showing enabled only when enabled wip

talmai · 2024-10-02T20:21:39Z

Can someone help me get the changeset together? I keep hitting the following error:

toliveir@bos-mpdj0 manager % yarn changeset
yarn run v1.22.22
$ node scripts/changelog/changeset.mjs

======================================================

Failed to get pull request number.
Please open a pull request for the current branch and let's try this again!

➔ Error: Pull request number not found.

======================================================

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
toliveir@bos-mpdj0 manager % yarn changeset
yarn run v1.22.22
$ node scripts/changelog/changeset.mjs

======================================================

Failed to get pull request number.
Please open a pull request for the current branch and let's try this again!

➔ Error: Pull request number not found.

======================================================

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

coliu-akamai · 2024-10-02T20:51:11Z

Just pushed up a changeset! (I had to run the gh repo set-default command and set linode/manager as the default repo - not sure what could have caused this)
edit: forgot one for the api-v4 package, so just pushed up another changeset for that too

Giving a quick glance over the files - seems like there are a few new components and flows. Will there be test coverage added later/is there a ticket for it already?

github-actions · 2024-10-02T21:16:22Z

Coverage Report: ✅
Base Coverage: 86.95%
Current Coverage: 86.95%

packages/manager/src/features/Kubernetes/CreateCluster/ControlPlaneACLPane.tsx

jdamore-linode · 2024-10-03T15:51:14Z

Hi @talmai! I'm seeing a test has failed repeatedly in lke-create.spec.ts against this branch. Would you mind taking a quick look? We can disregard if it's unrelated to your PRs changes.

yarn && yarn build && yarn start:manager:ci, then in another console:

yarn cy:run -s "cypress/e2e/core/kubernetes/lke-create.spec.ts"

(You might have to follow our Cypress first time setup docs, but feel free to reach out on Slack if you have any questions or run into any trouble)

coliu-akamai · 2024-10-03T18:13:54Z

Thanks Talmai! Overall this is looking really solid! 🎉
Will also reach out to Daniel to set up a UX review

✅ Tested the following flows so far:

Creating LKE clusters with only IPv4 IPs, only IPv6s, both, and having IP ACL disabled (and then later enabled via drawer)
Validation/API error handling for nonsense IPs for both the create flow and the drawer
Adding/updating/removing IPs in the drawer
Enabling/disabling IP ACL in the drawer

Still need to go through the code, had a few questions/comments with what I tested:

Should there be error handling for when I enter the same IP twice? This will say two IP addresses in the cluster summary's IPACL section, but the IPs are the same:

There's some extra spacing at the bottom of the HA Control Plane on the Create flow now (in comparison to the top of the section):

On prod, because there's no IP ACL section yet, there's an extra divider:

in the drawer, the API error appears in the IP section but in the create flow, the error appears at the top of the page - wondering if there should be consistency? (and wondering how other pages/drawers handle this - will need to check)
In the Create flow: if I type a bad IP, then flip off the Enable IPACL toggle, and try to create a cluster, the bad IP will still get sent to the backend and trigger an error - wondering if toggling off 'Enable ACL' should clear the IPs that might still exist there

coliu-akamai · 2024-10-07T17:54:43Z

Thanks @talmai! I'm not familiar with the timelines for this project besides that this PR needs to make it to the next release, but I think a future ticket for API work to be done would make sense! And then we'd just be able to surface that new error for the frontend.

I just pushed up some changes after meeting with Daniel:

Made button in the IPACL drawer says 'Install IPACL" when Summary pannel says "Install IPACL"
the IPACL toggle now appears and the button wording is "Update IPACL" in the drawer as long as IPACL is already installed (toggle shows up even if IPACL is installed but not enabled)
Moved the error we were talking about in the Create flow to the IPACL panel's section
in the create flow, if we toggle off IPACL and then click 'Create', any IPs we'd previously added will be ignored and not sent to the backend

There's still one margin/padding issue that is being a bit more finicky than I expected, so looking into that a bit more 😅

packages/manager/src/features/Kubernetes/KubernetesClusterDetail/KubeControlPaneACLDrawer.tsx

coliu-akamai · 2024-10-07T20:48:07Z

Checked with Daniel - the design should be all set now, but we will need some new copies. Hopefully they'll be in by Wednesday/in time for the release branch being cut, otherwise this might need to be pointed to the release branch.

packages/manager/src/features/Kubernetes/KubernetesClusterDetail/KubeClusterControlPlaneACL.tsx

packages/manager/src/features/Kubernetes/KubernetesClusterDetail/KubeSummaryPanel.styles.tsx

jaalah-akamai · 2024-10-08T18:58:25Z

@coliu-akamai will handle the rest of the work needed here. @talmai will leave code comments surrounding the parts where we need to ensure we're accounting for certain edge cases when creating clusters.

coliu-akamai · 2024-10-08T19:20:10Z

packages/manager/cypress/e2e/core/cloudpulse/linode-widget-verification.spec.ts

@@ -46,8 +46,14 @@ import { Interception } from 'cypress/types/net-stubbing';
 const expectedGranularityArray = ['Auto', '1 day', '1 hr', '5 min'];
 const timeDurationToSelect = 'Last 24 Hours';

-const { metrics, id, serviceType, dashboardName, region, resource } =
-  widgetDetails.linode;
+const {


looks like this file and factories/dashboards.ts snuck in as eslint fixes while I was resolving conflicts - gonna leave them in this PR

coliu-akamai

@talmai ty for the additional comments! They've been super helpful in providing context.

@jdamore-linode took a look at lke-create.spec.ts - I've seen it failing pretty consistently in the github checks, but then it's been passing relatively consistently for me, locally... 😕🤔 it failed for me once when some region was selected (I think Amsterdam?) that led to some checkbox not being checked above the Create button, disabling the Create button - could that have been an issue?

I'm planning to write e2e tests for the new flows (see my comment on M3-8725), will create another PR for them since this PR is already about ~900 added lines. Will be putting this up for review shortly - hopefully by tomorrow, just have a couple more things I want to look into

packages/manager/src/features/Kubernetes/KubernetesClusterDetail/KubeControlPaneACLDrawer.tsx

coliu-akamai · 2024-10-10T20:31:54Z

packages/manager/src/features/Kubernetes/KubernetesClusterDetail/KubeControlPaneACLDrawer.tsx

+  } = useForm({
+    values: {
+      enabled: !!enabled,
+      ipv4: ipv4 ?? [stringToExtendedIP('')],
+      ipv6: ipv6 ?? [stringToExtendedIP('')],
+      'revision-id': revisionID,
+    },
+  });


I'd like to get these values to match KubernetesControlPlaneACLPayload and do something like useForm<KubernetesControlPlaneACLPayload>. However, we're using the ExtendedIP[] type for the MultipleIPInput components whereas KubernetesControlPlaneACLPayload's IP fields are just string[] - it's making it a bit difficult, but will test some more stuff out

update: will be keeping this for the most part - it's now in the shape of { acl: { ...those above fields}}, which is a bit closer to the shape of KubernetesControlPlaneACLPayload 🥲

I really think we should eliminate ExtendedIP and align with KubernetesControlPlaneACLPayload if possible. I don't find ExtendedIP to be very maintainable because it diverges from the API's types and has error on the type, which is bad because react-hook-form has its own more correct way to handle errors.

How hard would the change be?

Happy to help look into this if needed. I really don't like "extended" types like this 😖

coliu-akamai · 2024-10-10T20:35:05Z

packages/manager/src/features/Kubernetes/KubernetesClusterDetail/KubeControlPaneACLDrawer.tsx

+  const {
+    data: data,
+    error: isErrorKubernetesACL,
+    isFetching: isFetchingKubernetesACL,
+    isLoading: isLoadingKubernetesACL,
+    refetch: refetchKubernetesACL,
+  } = useKubernetesControlPlaneACLQuery(clusterId);


will be making another version to lift this query and the data up into the parent and pass it down to the drawer >> didn't go too deep with the other version, prefer this version more tbh (but I can return to it if needed!)

coliu-akamai · 2024-10-11T19:23:26Z

packages/manager/src/components/MultipleIPInput/MultipleIPInput.tsx

+    forVPCIPv4Ranges || isLinkStyled ? (
+      <StyledLinkButtonBox sx={{ marginTop: isLinkStyled ? '8px' : '12px' }}>
+        <LinkButton onClick={addNewInput}>{buttonText}</LinkButton>
+      </StyledLinkButtonBox>


introduced a new prop, isLinkStyled, because of the margin differences (and forVPCIPv4Ranges does additional things) - this does feel a bit jank/not clean, but not too sure what else to do 🥲...will keep thinking but putting this up for review

talmai requested a review from a team as a code owner September 19, 2024 06:05

talmai requested review from bnussman-akamai and coliu-akamai and removed request for a team September 19, 2024 06:05

bnussman-akamai added the LKE Related to Linode Kubernetes Engine offerings label Sep 19, 2024

bnussman-akamai assigned talmai Sep 19, 2024

bnussman-akamai marked this pull request as draft September 19, 2024 16:07

abailly-akamai self-requested a review October 1, 2024 13:18

talmai force-pushed the feature/lkeapifw-428 branch from 7b7310e to 4cd457e Compare October 1, 2024 15:27

Talmai Oliveira and others added 8 commits October 2, 2024 16:06

[LKEAPIFW-428] LKE clusters now have IP ACLs

693eaf6

[LKEAPIFW-428] Migration of non-ipacl'd clusters now working

7817a17

[LKEAPIFW-428] Ongoing UI tweaks

13b644f

[LKEAPIFW-428] Additional UI tweaks

9ead96e

[LKEAPIFW-428] Substituition of UI components

1eec5c0

[LKEAPIFW-428] Another round of UI tweaks: multiline IP default value…

8c5534f

…s; IPACL drawer showing enabled only when enabled wip

fix multiple ip css issues

d41ff96

[LKEAPIFW-428] Copy text adjustment

265a3ca

talmai force-pushed the feature/lkeapifw-428 branch from 8870af0 to 265a3ca Compare October 2, 2024 20:06

talmai marked this pull request as ready for review October 2, 2024 20:19

abailly-akamai removed their request for review October 2, 2024 20:36

add changeset

bc7af8a

Added changeset: ACL related endpoints and types for LKE clusters

903ce45

talmai commented Oct 3, 2024

View reviewed changes

packages/manager/src/features/Kubernetes/CreateCluster/ControlPlaneACLPane.tsx Outdated Show resolved Hide resolved

fix small eslint warnings + update spacing/design review with Daniel

2bb302b

updates as per Daniel's feedback

4af433e

bnussman-akamai reviewed Oct 7, 2024

View reviewed changes

packages/manager/src/features/Kubernetes/KubernetesClusterDetail/KubeControlPaneACLDrawer.tsx Outdated Show resolved Hide resolved

coliu-akamai added 2 commits October 7, 2024 16:12

margin fixes

1235902

height issues

39610eb

coliu-akamai reviewed Oct 8, 2024

View reviewed changes

packages/manager/src/features/Kubernetes/KubernetesClusterDetail/KubeClusterControlPlaneACL.tsx Outdated Show resolved Hide resolved

packages/manager/src/features/Kubernetes/KubernetesClusterDetail/KubeSummaryPanel.styles.tsx Outdated Show resolved Hide resolved

jaalah-akamai assigned coliu-akamai Oct 8, 2024

coliu-akamai marked this pull request as draft October 8, 2024 18:59

jaalah-akamai self-requested a review October 8, 2024 19:10

Merge branch 'develop' into feature/lkeapifw-428

672ad55

coliu-akamai reviewed Oct 8, 2024

View reviewed changes

coliu-akamai and others added 6 commits October 8, 2024 15:40

quick initial cleanup

900e18f

refactor some styling, initial transition to react-hook-form?

b324a6b

adding some code description around the update cluster calls

fcecc60

move button file to Kube summary panel

34da662

invalidate query after installing ipacl

1a70f0b

additional cleanup

ecbc30c

coliu-akamai reviewed Oct 10, 2024

View reviewed changes

coliu-akamai added 3 commits October 11, 2024 14:24

new UX changes, remove refresh

a5a6b4a

make function async

6183ec0

some bit more cleanup

dbe9387

coliu-akamai changed the title ~~feat: [LKEAPIFW-428] LKE clusters should have IP ACL integration on CM~~ feat: [LKEAPIFW-428] LKE clusters should have IP ACL integration on CM (part 1) Oct 11, 2024

coliu-akamai reviewed Oct 11, 2024

View reviewed changes

coliu-akamai marked this pull request as ready for review October 11, 2024 19:25

coliu-akamai requested a review from a team as a code owner October 11, 2024 19:26

coliu-akamai requested review from jdamore-linode and bnussman-akamai and removed request for a team October 11, 2024 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [LKEAPIFW-428] LKE clusters should have IP ACL integration on CM (part 1) #10968

feat: [LKEAPIFW-428] LKE clusters should have IP ACL integration on CM (part 1) #10968

talmai commented Sep 19, 2024 •

edited by coliu-akamai

Loading

jcallahan-akamai commented Sep 19, 2024

talmai commented Oct 2, 2024

coliu-akamai commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

jdamore-linode commented Oct 3, 2024

coliu-akamai commented Oct 3, 2024

coliu-akamai commented Oct 7, 2024

coliu-akamai commented Oct 7, 2024

jaalah-akamai commented Oct 8, 2024

coliu-akamai Oct 8, 2024

coliu-akamai left a comment

coliu-akamai Oct 10, 2024 •

edited

Loading

bnussman-akamai Oct 14, 2024

bnussman-akamai Oct 14, 2024

coliu-akamai Oct 10, 2024 •

edited

Loading

coliu-akamai Oct 11, 2024

feat: [LKEAPIFW-428] LKE clusters should have IP ACL integration on CM (part 1) #10968

Are you sure you want to change the base?

feat: [LKEAPIFW-428] LKE clusters should have IP ACL integration on CM (part 1) #10968

Conversation

talmai commented Sep 19, 2024 • edited by coliu-akamai Loading

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

jcallahan-akamai commented Sep 19, 2024

talmai commented Oct 2, 2024

coliu-akamai commented Oct 2, 2024 • edited Loading

github-actions bot commented Oct 2, 2024 • edited Loading

jdamore-linode commented Oct 3, 2024

coliu-akamai commented Oct 3, 2024

coliu-akamai commented Oct 7, 2024

coliu-akamai commented Oct 7, 2024

jaalah-akamai commented Oct 8, 2024

coliu-akamai Oct 8, 2024

Choose a reason for hiding this comment

coliu-akamai left a comment

Choose a reason for hiding this comment

coliu-akamai Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

bnussman-akamai Oct 14, 2024

Choose a reason for hiding this comment

bnussman-akamai Oct 14, 2024

Choose a reason for hiding this comment

coliu-akamai Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

coliu-akamai Oct 11, 2024

Choose a reason for hiding this comment

talmai commented Sep 19, 2024 •

edited by coliu-akamai

Loading

coliu-akamai commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

coliu-akamai Oct 10, 2024 •

edited

Loading

coliu-akamai Oct 10, 2024 •

edited

Loading