new: Add support for LKE Control Plane ACLs by lgarber-akamai · Pull Request #406 · linode/linode_api4-python

lgarber-akamai · 2024-05-14T15:08:41Z

📝 Description

This pull request adds support for configuring and viewing the ACL configuration for an LKE cluster's control plane.

NOTE: This PR does NOT include the PUT LKE cluster changes because the acl configuration is not returned in the GET LKE cluster response.

New Endpoint Methods

control_plane_acl - GET /lke/clusters/{cluster_id}/control_plane_acl
control_plane_acl_update(...) - PUT /lke/clusters/{cluster_id}/control_plane_acl
control_plane_acl_delete() - POST /lke/clusters/{cluster_id}/control_plane_acl

Updated Endpoint Methods

LKEGroup.cluster_create(...) - Add control_plane field to method arguments

Misc Changes

Added data classes for LKE cluster control plane and all substructures
Added logic to JSONObject to remove Optional values from the generated dict if None
- Added a new always_include class var used to designate optional values that should always be included in the generated Dict
Updated test fixture framework to support underscores in path

✔️ How to Test

The following test steps assume you have pulled down this PR locally and run make install.

Unit Testing

make testunit

Integration Testing

make TEST_COMMAND=models/lke/test_lke.py testint

Manual Testing

In a Python SDK sandbox environment (e.g. dx-devenv), run the following:

import os

from linode_api4 import (
    LinodeClient,
    LKEClusterControlPlaneOptions,
    LKEClusterControlPlaneACLOptions,
    LKEClusterControlPlaneACLAddressesOptions,
)

client = LinodeClient(token=os.getenv("LINODE_TOKEN"))

cluster = client.lke.cluster_create(
    "us-mia",
    "test-cluster",
    client.lke.node_pool("g6-standard-1", 1),
    "1.29",
    control_plane=LKEClusterControlPlaneOptions(
        acl=LKEClusterControlPlaneACLOptions(
            enabled=True,
            addresses=LKEClusterControlPlaneACLAddressesOptions(
                ipv4=["10.0.0.1/32"], ipv6=["1234::5678"]
            ),
        )
    ),
)

print("Original ACL:", cluster.control_plane_acl)

cluster.control_plane_acl_update(
    LKEClusterControlPlaneACLOptions(
        addresses=LKEClusterControlPlaneACLAddressesOptions(ipv4=["10.0.0.2/32"])
    )
)

print("Updated ACL:", cluster.control_plane_acl)

cluster.control_plane_acl_delete()

print("Deleted ACL:", cluster.control_plane_acl)

Ensure the script runs successfully and the output matches the following:

Original ACL: LKEClusterControlPlaneACL(enabled=True, addresses=LKEClusterControlPlaneACLAddresses(ipv4=['10.0.0.1/32'], ipv6=['1234::5678/128']))
Updated ACL: LKEClusterControlPlaneACL(enabled=True, addresses=LKEClusterControlPlaneACLAddresses(ipv4=['10.0.0.2/32'], ipv6=None))
Deleted ACL: LKEClusterControlPlaneACL(enabled=False, addresses=None)

lgarber-akamai · 2024-05-14T16:23:25Z


+# This regex is useful for finding individual underscore characters,
+# which is necessary to allow us to use underscores in URL paths.
+PATH_REPLACEMENT_REGEX = re.compile(r"(?<!_)_(?!_)")


This is necessary for lke/clusters/18881/control_plane_acl fixture since the _s were being replaced with /s.

lgarber-akamai · 2024-05-14T16:24:26Z

            "{}/servicetoken".format(LKECluster.api_endpoint), model=self
        )
+
+    def control_plane_acl_update(


This pattern is not consistent with the rest of the codebase, but unfortunately we cannot implement it using the traditional properties because ACL is not returned from the LKE cluster GET endpoint.

lgarber-akamai · 2024-05-14T16:33:01Z


    @property
-    def ips(self, *filters) -> PaginatedList:
+    def ips(self) -> PaginatedList:


This isn't directly related to this change but it was causing the linter to fail. This shouldn't be a breaking change since arguments could already not be passed into ips 👍

lgarber-akamai · 2024-05-15T21:25:01Z

+    always_include: ClassVar[Set[str]] = {}
+    """
+    A set of keys corresponding to fields that should always be
+    included in the generated output regardless of whether their values
+    are None.
+    """


I was considering implementing something more similar to omitempty in Go, but in this case I think there are far fewer instances where we would want to include null values rather than excluding them. I'd definitely appreciate some feedback on this!

Yes, I agree that in the most of the case we just want to omit empty values.

lgarber-akamai · 2024-05-16T14:55:47Z

+        cls = type(self)
+        type_hints = get_type_hints(cls)
+
+        def attempt_serialize(value: Any) -> Any:
+            """
+            Attempts to serialize the given value, else returns the value unchanged.
+            """
+            if issubclass(type(value), JSONObject):
+                return value._serialize()
+
+            return value
+
+        def should_include(key: str, value: Any) -> bool:
+            """
+            Returns whether the given key/value pair should be included in the resulting dict.
+            """
+
+            if key in cls.always_include:
+                return True
+
+            hint = type_hints.get(key)
+
+            # We want to exclude any Optional values that are None
+            # NOTE: We need to check for Union here because Optional is an alias of Union.
+            if (
+                hint is None
+                or get_origin(hint) is not Union
+                or type(None) not in get_args(hint)
+            ):
+                return True
+
+            return value is not None
+
+        result = {}
+
+        for k, v in vars(self).items():
+            if not should_include(k, v):
+                continue
+
+            if isinstance(v, List):
+                v = [attempt_serialize(j) for j in v]
+            elif isinstance(v, Dict):
+                v = {k: attempt_serialize(j) for k, j in v.items()}
+            else:
+                v = attempt_serialize(v)
+
+            result[k] = v
+
+        return result


Unfortunately we have to reinvent the wheel here because we don't have access to type hints or always_include for nested JSONObjects in the asdict dict_factory method.

Ideally I'd want to break this out into a separate PR but excluding optional fields from generated request bodies is a requirement for LKEClusterControlPlaneACL.

lgarber-akamai · 2024-05-16T15:21:54Z

+@dataclass
+class LKEClusterControlPlaneACLAddressesOptions(JSONObject):
+    """
+    LKEClusterControlPlaneACLAddressesOptions are options used to configure
+    IP ranges that are explicitly allowed to access an LKE cluster's control plane.
+    """
+
+    ipv4: Optional[List[str]] = None
+    ipv6: Optional[List[str]] = None
+
+
+@dataclass
+class LKEClusterControlPlaneACLOptions(JSONObject):
+    """
+    LKEClusterControlPlaneACLOptions is used to set
+    the ACL configuration of an LKE cluster's control plane.
+    """
+
+    enabled: Optional[bool] = None
+    addresses: Optional[LKEClusterControlPlaneACLAddressesOptions] = None
+
+
+@dataclass
+class LKEClusterControlPlaneOptions(JSONObject):
+    """
+    LKEClusterControlPlaneOptions is used to configure
+    the control plane of an LKE cluster during its creation.
+    """
+
+    high_availability: Optional[bool] = None
+    acl: Optional[LKEClusterControlPlaneACLOptions] = None
+
+
+@dataclass
+class LKEClusterControlPlaneACLAddresses(JSONObject):
+    """
+    LKEClusterControlPlaneACLAddresses describes IP ranges that are explicitly allowed
+    to access an LKE cluster's control plane.
+    """
+
+    ipv4: List[str] = None
+    ipv6: List[str] = None
+
+
+@dataclass
+class LKEClusterControlPlaneACL(JSONObject):
+    """
+    LKEClusterControlPlaneACL describes the ACL configuration of an LKE cluster's
+    control plane.
+    """
+
+    enabled: bool = False
+    addresses: LKEClusterControlPlaneACLAddressesOptions = None
+
+


This is more of a linodego pattern but I think it's necessary given we're starting to see more diverging API request & response structures

I'm wondering if we should separate them in ansible and tf as well. One of the use case that I can think of is in terraform framework, nested block can't be both optional and computed. It's a bit annoying when doing the migration. We did the customized object type, but Hashicorp once suggested that we could have two separated block attributes, one is optional and one is computed to implement that.

One of the use case that I can think of is in terraform framework, nested block can't be both optional and computed. It's a bit annoying when doing the migration. We did the customized object type, but Hashicorp once suggested that we could have two separated block attributes, one is optional and one is computed to implement that.

That's a good call out! I think in the long term we'll need to split apart user-defined and computed structures anyway because of the limitations in TF framework you mentioned

yec-akamai

Tested and everything passed locally, nice work!

yec-akamai · 2024-05-21T19:23:29Z

+    always_include: ClassVar[Set[str]] = {}
+    """
+    A set of keys corresponding to fields that should always be
+    included in the generated output regardless of whether their values
+    are None.
+    """


Yes, I agree that in the most of the case we just want to omit empty values.

ykim-akamai

LGTM, tests pass locally

ykim-akamai · 2024-05-23T15:17:24Z

Also, thanks for fixing the lint error 👍

lgarber-akamai added 5 commits May 13, 2024 16:55

WIP ACL

ed0357a

Support optional values in serializable class

2749f20

comments

73f321c

Add OmitOptional type

c05a193

Repurpose new type to Nullable

3703faa

lgarber-akamai commented May 14, 2024

View reviewed changes

lgarber-akamai added 3 commits May 14, 2024 12:27

Use nullable

dbd22a4

oops

5716469

Respond to unrelated linter changes

832d506

lgarber-akamai commented May 14, 2024

View reviewed changes

lgarber-akamai added 4 commits May 14, 2024 12:33

revert

94bb114

Fix optional check

abd0659

oops

2fe52cb

Remove Nullable; add always_include

8c4c59f

lgarber-akamai commented May 15, 2024

View reviewed changes

lgarber-akamai commented May 16, 2024

View reviewed changes

Add integration tests and different dataclasses for request/response

c355ff7

lgarber-akamai commented May 16, 2024

View reviewed changes

oops

c823fef

lgarber-akamai added the new-feature for new features in the changelog. label May 16, 2024

lgarber-akamai marked this pull request as ready for review May 16, 2024 15:32

lgarber-akamai requested a review from a team as a code owner May 16, 2024 15:32

lgarber-akamai requested review from yec-akamai and ykim-akamai and removed request for a team May 16, 2024 15:32

Support dict on PUT

deec4d7

yec-akamai reviewed May 21, 2024

View reviewed changes

Comment thread linode_api4/objects/lke.py

yec-akamai approved these changes May 21, 2024

View reviewed changes

Update docs to include ACL deletion note

33b7f16

ykim-akamai approved these changes May 23, 2024

View reviewed changes

lgarber-akamai merged commit 5f1c1df into linode:dev May 23, 2024

Uh oh!

Conversation

lgarber-akamai commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Description

New Endpoint Methods

Updated Endpoint Methods

Misc Changes

✔️ How to Test

Unit Testing

Integration Testing

Manual Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgarber-akamai May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yec-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykim-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

ykim-akamai commented May 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lgarber-akamai commented May 14, 2024 •

edited

Loading

lgarber-akamai May 15, 2024 •

edited

Loading