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

upcoming: [M3-9519] - Allow LKE-E IP ACL addresses to be optional with an explicit acknowledgement #11856

Merged
merged 13 commits into from
Mar 19, 2025

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Mar 14, 2025

Description 📝

Currently, the ACL section for LKE-E is enabled by default. Currently, this setting cannot be disabled and requires the user to enter at least 1 IP Address to proceed with cluster creation. In general, we do want to encourage that an ACL has an IP Address to avoid user error preventing an inability to access the cluster control plane.

However, there may be workflows where a customer does not want to specify an IP address upon creation and would prefer to add one later, which is acceptable as long as they understand that their cluster will not be accessible.

This PR will introduce an acknowledgement checkbox to confirm they explicitly intend to enable ACL without IP addresses.

Changes 🔄

  • Adds an acknowledgement checkbox to the ACL flows
    • If the user checks the checkbox, IP addresses will not be a required field (no validation error).
    • If the user does not check the checkbox and submits without at least 1 IP address, validation will run and show the existing error.
  • Updates test coverage

Target release date 🗓️

3/25

Preview 📷

LKE Create flow LKE details page - ACL drawer
Screenshot 2025-03-17 at 3 50 31 PM Screenshot 2025-03-17 at 3 49 57 PM

How to test 🧪

Prerequisites

(How to setup test environment)

  • Have LKE-E feature flag enabled and LKE-E customer tag on your account (see project tracker)
    • For confirming the ACL drawer: If you do not already have an LKE-E cluster and your account can no longer create one, ping me for which test account to use.

Reproduction steps

(How to reproduce the issue, if applicable)

  • Go to the create flow, try to submit an LKE-E cluster without an IP address defined, and observe the validation error
  • With an existing LKE-E cluster, go to the details page > ACL drawer > try to edit by saving 0 IP addresses and observe the validation error

Verification steps

(How to verify changes)

  • Go to the create flow, try to submit an LKE-E cluster without an IP address defined, and observe the validation error UNLESS the user checks the acknowledgement checkbox. Once checked, confirm you can submit without an validation error.
  • With an existing LKE-E cluster, go to the details page > ACL drawer > try to edit by saving 0 IP addresses and observe the validation error UNLESS the user checks the acknowledgement checkbox. Once checked, confirm you can submit without an validation error.
  • Confirm acknowledgement checkboxes show for LKE-E clusters only. There should be no changes to LKE standard.
  • Test coverage passes for lke-update.spec.ts and lke-create.spec.ts
  • Note that there's currently some existing behavior going on where the ACL drawer doesn't accept edits to the revision ID for LKE-E. This is a backend issue.
Author Checklists

As an Author, to speed up the review process, I considered 🤔

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


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@mjac0bs mjac0bs force-pushed the M3-9519-add-acknowledgement-for-acl branch from 5558ac3 to e0fe8ac Compare March 17, 2025 22:45
@mjac0bs mjac0bs marked this pull request as ready for review March 17, 2025 23:03
@mjac0bs mjac0bs requested review from a team as code owners March 17, 2025 23:03
@mjac0bs mjac0bs requested review from dmcintyr-akamai, carrillo-erik, hasyed-akamai and hana-akamai and removed request for a team March 17, 2025 23:03
Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Can we fix the padding for the Create LKE-E error notice?

@mjac0bs mjac0bs requested a review from hana-akamai March 18, 2025 20:43
@hana-akamai hana-akamai added the Add'tl Approval Needed Waiting on another approval! label Mar 19, 2025
@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 2 failing tests on test run #5 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
2 Failing538 Passing3 Skipped118m 52s

Details

Failing Tests
SpecTest
smoke-community-stackscripts.spec.tsCommunity Stackscripts integration tests » pagination works with infinite scrolling
delete-firewall.spec.tsdelete firewall » deletes a firewall

Troubleshooting

Use this command to re-run the failing tests:

pnpm cy:run -s "cypress/e2e/core/stackscripts/smoke-community-stackscripts.spec.ts,cypress/e2e/core/firewalls/delete-firewall.spec.ts"

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! Ready for Review labels Mar 19, 2025
@@ -59,8 +64,8 @@ export const ControlPlaneACLPane = (props: ControlPlaneACLProps) => {
)}
<Typography mb={1} sx={{ width: '85%' }}>
{isEnterpriseCluster
? CREATE_CLUSTER_STANDARD_TIER_ACL_COPY
: CREATE_CLUSTER_ENTERPRISE_TIER_ACL_COPY}
? CREATE_CLUSTER_ENTERPRISE_TIER_ACL_COPY
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of placing the tier name at the beginning of these constants to make them more distinguishable.
For example:
CREATE_CLUSTER_ENTERPRISE_TIER_ACL_COPY --> ENTERPRISE_TIER_CREATE_CLUSTER_ACL_COPY
CREATE_CLUSTER_STANDARD_TIER_ACL_COPY --> STANDARD_TIER_CREATE_CLUSTER_ACL_COPY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. I'm going to merge as is so this doesn't hold up a release branch cut, but we'll keep this in mind when we do some refactoring with the RHF PR that will have some clean up to do.

@mjac0bs mjac0bs merged commit 9ff0e44 into linode:develop Mar 19, 2025
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! LKE-Enterprise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants