-
Notifications
You must be signed in to change notification settings - Fork 73
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
new: Add support for LKE Control Plane ACLs #406
Conversation
import sys | ||
|
||
FIXTURES_DIR = sys.path[0] + "/test/fixtures" | ||
|
||
# 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"(?<!_)_(?!_)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary for lke/clusters/18881/control_plane_acl
fixture since the _
s were being replaced with /
s.
@@ -335,3 +407,40 @@ def service_token_delete(self): | |||
self._client.delete( | |||
"{}/servicetoken".format(LKECluster.api_endpoint), model=self | |||
) | |||
|
|||
def control_plane_acl_update( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -100,7 +100,7 @@ def subnet_create( | |||
return d | |||
|
|||
@property | |||
def ips(self, *filters) -> PaginatedList: | |||
def ips(self) -> PaginatedList: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 馃憤
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. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that in the most of the case we just want to omit empty values.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
linode_api4/objects/lke.py
Outdated
@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 | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and everything passed locally, nice work!
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. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that in the most of the case we just want to omit empty values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tests pass locally
Also, thanks for fixing the lint error 馃憤 |
馃摑 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 argumentsMisc Changes
Optional
values from the generated dict if Nonealways_include
class var used to designate optional values that should always be included in the generated Dict鉁旓笍 How to Test
The following test steps assume you have pulled down this PR locally and run
make install
.Unit Testing
Integration Testing
Manual Testing
In a Python SDK sandbox environment (e.g. dx-devenv), run the following: