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-9357] - Enable ACL by default for LKE-E clusters #11746

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Feb 26, 2025

Description 📝

LKE-E clusters must have ACL enabled to be able to access the cluster.

Currently, the cluster create flow allows the user to submit an LKE-E cluster without ACL enabled by default, and ACL enablement by default is not enforced on the backend, though that will be handled in LKE-6468.

The UI should still make the ACL requirement clear to users. We will force ACL enablement in the cluster create flow and ACL drawer, containing updated copy to explain this and validate that they provide at least one IP address or CIDR to the ACL, or they cannot access their cluster API server. By default, it is preventing all access.

Changes 🔄

In both the ACL section of the create flow and the ACL drawer in the cluster details flow...

  • Removed the toggle that allowed ACL to be disabled when the cluster tier is enterprise
    • Enabled ACL when the enterprise tier is selected on the create flow
  • Made copy conditional on the cluster tier so it is accurate
  • Enforced validation of at least one required IP field
  • Updated test coverage

TODO:

  • Debug Kubernetes version is required. error
  • Clear the IP error once the cluster tier is switched?

Target release date 🗓️

3/11

Preview 📷

💡 Use <video src="" /> tag when including recordings in table.

Before After
Screenshot 2025-02-27 at 7 18 49 AM x
Screenshot 2025-02-27 at 7 19 38 AM x

How to test 🧪

Prerequisites

(How to setup test environment)

  • ...
  • ...

Reproduction steps

(How to reproduce the issue, if applicable)

  • ...
  • ...

Verification steps

(How to verify changes)

  • ...
  • ...
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 self-assigned this Feb 26, 2025
@mjac0bs mjac0bs added the UX/UI Changes for UI/UX to review label Feb 26, 2025
Comment on lines +1298 to +1301
cy.findByText(
'At least one IP address or CIDR range is required for LKE Enterprise.',
{ exact: false }
).should('be.visible');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't figure out why this is causing a failure. The error is present in the UI.

I tried other ways of finding the text (cy.contains() and cy.get('[data-qa-error="true"]').contains()), and those didn't work either. No issues with the lke-update.spec.ts, which looks for the same error and finds it.

SDET eyes would be appreciated. 🙏🏼 I'll also take another look tomorrow.

Screenshot 2025-02-26 at 3 24 06 PM

Comment on lines +54 to +56
{selectedTier === 'enterprise'
? 'An access control list (ACL) is enabled by default on LKE Enterprise clusters. All traffic to the control plane is restricted except from IP addresses listed in the ACL. Add at least one IP address or CIDR range.'
: 'Enable an access control list (ACL) on your LKE cluster to restrict access to your cluster’s control plane. Only the IP addresses and ranges specified in the ACL can connect to the control plane.'}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds copy for when the selected tier is enterprise and updates the existing copy (courtesy of Tech Docs).

Comment on lines +90 to +97
export const createKubeClusterSchema = object().shape({
label: clusterLabelSchema,
region: string().required('Region is required.'),
k8s_version: string().required('Kubernetes version is required.'),
node_pools: array()
.of(nodePoolSchema)
.min(1, 'Please add at least one node pool.'),
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

createKubeClusterSchema was pre-existing. Just moved it to make it easier to compare to the enterprise cluster schema.

Comment on lines +266 to +268
{errors.acl?.root && (
<Notice spacingBottom={12} spacingTop={8} variant="error">
{errors.acl.message}
{errors.acl?.root.message}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wrong chaining seemed to be happening on this error object, resulting in the validation message not being surfaceable until this changed.

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 1 failing test on test run #3 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
1 Failing530 Passing3 Skipped106m 48s

Details

Failing Tests
SpecTest
lke-create.spec.tsshows the LKE-E flow with the feature flag on » creates an LKE-E cluster with the account capability

Troubleshooting

Use this command to re-run the failing tests:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LKE-Enterprise UX/UI Changes for UI/UX to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants