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 in separate PRs: #11131 and #11132

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.

For release testing: you should also be able to create clusters in the prod env, but for creating clusters without IPACL, see Talmai's comment (and use the prod env)

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
- general spotcheck of Kubernetes with both the APL flag on and off

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: 87.08%
Current Coverage: 87.01%

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

bnussman-akamai

Looking good! Let me know when you need a final review!

packages/manager/src/queries/kubernetes.ts

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

bnussman-akamai · 2024-10-17T13:52:41Z

packages/validation/src/kubernetes.schema.ts

@@ -58,3 +59,28 @@ export const createKubeClusterSchema = object().shape({
    .of(nodePoolSchema)
    .min(1, 'Please add at least one node pool.'),
 });
+
+export const ipv4Address = string().test({


Good callout. Nice job using validation here. Looks clean and seems to work work well with react hook form

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

bnussman-akamai · 2024-10-17T13:59:18Z

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

This poor form 😭

It really needs the react-hook-form treatment.

This looks good for now, but hopefully we can find some time in the future to refactor this page.

bnussman-akamai · 2024-10-17T14:11:57Z

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

I do wonder if it would make sense to just not use MultipleIPInput at all for cases like this (in the future. we can leave it for now unless you want to do more work 😅 ). I do think abstractions are good, but I feel like building the same thing without the abstraction is also fine. MultipeIPInput just seems a bit rigid and doesn't enable us to be as flexible. For example, I wonder if react-hook-form's useFieldArray could be used for the IPs (similar to packages/manager/src/features/Linodes/LinodeCreate/VPC/VPCRanges.tsx) if we weren't locked into using MultipleIPInput .

Most of that is me thing thinking out-loud, but what you did here seems fair to me.

😅 I'm gonna leave as is for now since I still need to finish the e2e tests, but will hopefully revisit this down the line! Maybe this could be part of the work for transitioning ClusterCreate to react-hook-form, since that also uses MultipleIPInput

update: [M3-8777]

packages/manager/src/queries/kubernetes.ts

abailly-akamai

@coliu-akamai Fantastic job taking over a large PR that should have been broken down in the first place. Should have been flagged right away and rejected in its current shape. It's really hard to digest and ended up taking far more time it would have taken to make several small PRs.

I am going to spend some extra time with the UI in the coming days but so far I have not caught any issue with the UI & functionality. Left some minor comments and will get back to this, with fresh eyes a bit later.

packages/api-v4/src/kubernetes/kubernetes.ts

abailly-akamai · 2024-10-21T13:32:03Z

packages/api-v4/src/kubernetes/types.ts

+
+export interface ControlPlaneACLOptions {
+  enabled?: boolean;
+  'revision-id'?: string;


It seems quite unusual for the API to introduce a hyphened id property. It doesn't usually play very well with downstream consumers...

abailly-akamai · 2024-10-21T13:46:27Z

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

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


can we just pass SX ?

abailly-akamai · 2024-10-21T14:05:12Z

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

+    const payload: KubernetesControlPlaneACLPayload = {
+      acl: {
+        enabled: acl.enabled,
+        'revision-id': acl['revision-id'],


see my comment above.. not this PR's fault, but... why

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

packages/validation/src/firewalls.schema.ts

packages/manager/src/queries/kubernetes.ts

coliu-akamai · 2024-10-21T21:45:02Z

@abailly-akamai

double checked regarding the drawer - we're wanting to allow people to update IPs and tag it with a revision ID independent of enabling ACL
created internal ticket for LKE summary mobile/smaller views! [M3-8764]

abailly-akamai

This is looking great on my end - I did not find functionality issues after the second pass, tho I may still miss some context on the feature as a whole. Thanks for taking this to the finish line @coliu-akamai

cypress · 2024-10-22T20:04:03Z

Cloud Manager E2E Run #6715

Run Properties: Passed #6715 • 8b9d956cb1: feat: [LKEAPIFW-428] LKE clusters should have IP ACL integration on CM (part 1) ...

Project	`Cloud Manager E2E`
Branch Review	`develop`
Run status	`Passed #6715`
Run duration	`29m 04s`
Commit	`8b9d956cb1: feat: [LKEAPIFW-428] LKE clusters should have IP ACL integration on CM (part 1) ...`
Committer	`Talmai Oliveira`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`5`
Pending	`2`
Skipped	`0`
Passing	`438`
View all changes introduced in this branch ↗︎

j-zimnowoda · 2024-11-04T17:10:04Z

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

@@ -30,12 +29,8 @@ export const KubernetesClusterDetail = () => {
  const id = Number(clusterID);
  const location = useLocation();
  const showAPL = useGetAPLAvailability();
-  const kubernetesClusterBetaQuery = useKubernetesClusterBetaQuery(id);


@talmai , why this was removed ?

we consolidated the logic for this query with the already existing kubernetesClusterQuery so that we'd only need one query key. The functionality should still exist, it's just been moved. Apologies if this caused any confusion!

see here for reasoning/discussion for the change, and here for the file changes (lines 112-118)

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

coliu-akamai added 2 commits October 16, 2024 15:39

allow empty addresses

c0af136

update copies to most recent, add enabled to query

af7080d

bnussman-akamai reviewed Oct 17, 2024

View reviewed changes

add footer, address feedback, tag bug

0188f60

coliu-akamai reviewed Oct 17, 2024

View reviewed changes

packages/manager/src/queries/kubernetes.ts Outdated Show resolved Hide resolved

coliu-akamai added 2 commits October 17, 2024 12:42

address feedback + quick design feedback

1582ac5

design feedback quick change

c871570

coliu-akamai force-pushed the feature/lkeapifw-428 branch from 4ab6aaa to c871570 Compare October 17, 2024 17:19

notice style update

b9e12c7

bnussman-akamai approved these changes Oct 17, 2024

View reviewed changes

coliu-akamai added the Add'tl Approval Needed Waiting on another approval! label Oct 17, 2024

abailly-akamai mentioned this pull request Oct 21, 2024

fix: [APL-272] - Make useAPLAvailability check for beta enrollment #11110

Merged

abailly-akamai reviewed Oct 21, 2024

View reviewed changes

coliu-akamai added 2 commits October 21, 2024 16:14

Merge branch 'develop' into feature/lkeapifw-428

2504e61

address feedback

b0d9a8a

This was referenced Oct 21, 2024

test: [M3-8725] - Add cypress tests for updating ACL in LKE clusters (LKE ACL integration part 2) #11131

Merged

test: [M3-8725] - Add cypress tests for creating LKE clusters with ACL (LKE ACL integration part 3) #11132

Merged

update copies

169427a

abailly-akamai approved these changes Oct 22, 2024

View reviewed changes

coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Oct 22, 2024

fix merge conflict

23b2797

coliu-akamai merged commit 8b9d956 into linode:develop Oct 22, 2024
23 checks passed

This was referenced Oct 23, 2024

Release v1.131.0 - release → staging #11153

Merged

Release v1.131.0 - staging → master #11175

Merged

j-zimnowoda reviewed Nov 4, 2024

View reviewed changes

coliu-akamai mentioned this pull request Nov 5, 2024

hotfix: [APL-343] - APL summary panel is not being loaded in staging #11207

Merged

mjac0bs mentioned this pull request Nov 15, 2024

feat: [M3-8837] - Add LKE-E feature flag #11259

Merged

8 tasks

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

bnussman-akamai left a comment

bnussman-akamai Oct 17, 2024

bnussman-akamai Oct 17, 2024

bnussman-akamai Oct 17, 2024 •

edited

Loading

coliu-akamai Oct 17, 2024 •

edited

Loading

abailly-akamai left a comment

abailly-akamai Oct 21, 2024

abailly-akamai Oct 21, 2024

abailly-akamai Oct 21, 2024

coliu-akamai commented Oct 21, 2024

abailly-akamai left a comment

cypress bot commented Oct 22, 2024

j-zimnowoda Nov 4, 2024 •

edited

Loading

coliu-akamai Nov 4, 2024

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

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

bnussman-akamai left a comment

Choose a reason for hiding this comment

bnussman-akamai Oct 17, 2024

Choose a reason for hiding this comment

bnussman-akamai Oct 17, 2024

Choose a reason for hiding this comment

bnussman-akamai Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

coliu-akamai Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

abailly-akamai left a comment

Choose a reason for hiding this comment

abailly-akamai Oct 21, 2024

Choose a reason for hiding this comment

abailly-akamai Oct 21, 2024

Choose a reason for hiding this comment

abailly-akamai Oct 21, 2024

Choose a reason for hiding this comment

coliu-akamai commented Oct 21, 2024

abailly-akamai left a comment

Choose a reason for hiding this comment

cypress bot commented Oct 22, 2024

Cloud Manager E2E Run #6715

j-zimnowoda Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

coliu-akamai Nov 4, 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

bnussman-akamai Oct 17, 2024 •

edited

Loading

coliu-akamai Oct 17, 2024 •

edited

Loading

j-zimnowoda Nov 4, 2024 •

edited

Loading