feat: support port ranges and endpoints#2366
feat: support port ranges and endpoints#2366james-garner-canonical wants to merge 34 commits intocanonical:mainfrom
Conversation
ops/model.py
Outdated
| ('tcp', p, None, ()) | ||
| if isinstance(p, int) | ||
| # If a user specifies a Port with endpoints=None (default) we normalise to endpoints=() | ||
| # but maybe this is too simple -- if the user didn't specify endpoints then do we | ||
| # actually want to take action if the port is already open but with some endpoints? | ||
| # In terms of backwards compatibility, probably not, since previously the endpoints | ||
| # would have been ignored ... | ||
| # FIXME: we might need to drop this nice set logic and do an explicit loop instead | ||
| else (p.protocol, p.port, p.to_port, p.endpoints or ()) | ||
| for p in ports |
There was a problem hiding this comment.
In terms of backwards compatibility, this problem applies equally to to_port, since previously we'd treat a request for Port(port=80) to be satisfied if the open port was Port(port=80, to_port=90, endpoints=('foo', 'bar')).
There was a problem hiding this comment.
I think this partially goes away with a separate ops.Port type that has clearer defaults for endpoint, though backwards compatibly behaviour when requesting set_ports(1) if 1-1000 is open would still be a problem and might be desirable ... on the other hand, it's kind of a bugfix, but a very longstanding bug. On the other other hand, unlikely to be a problem since who would have opened the port range?
tonyandrewmeyer
left a comment
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
Some initial notes on the code - I haven't looked at the tests at all.
One thing I think that is missing here is that as far as I can see, this only changes ModelBackend so it doesn't actually expose the functionality to the user for setting, only getting (which is the most important part, to be fair).
I can see an argument that this PR should only do that and there should be a follow-up that provides the ability to set as well, which has some additional design (for example, do you set ports for an endpoint via Model or Relation?).
I haven't checked what the behaviour is if you try to close a port that is inside a range or don't provide the endpoint but it was opened with an endpoint and things like that. If we're adding setting as well, then we should make sure we know what happens and probably have it in the docstring (maybe it's just a regular ModelError?). Also how set_ports works in those sorts of cases (possibly it's fine, since it's just closing exactly everything that isn't in the desired list, and opening the others).
This is a malformed port range expression. If we don't error here, the current code silently ignores the malformed port range entirely, which seems like a bug.
| return self._backend.opened_ports() | ||
|
|
||
| def set_ports(self, *ports: int | Port) -> None: | ||
| def set_ports(self, *ports: int | tuple[int, int | None] | Port) -> None: |
There was a problem hiding this comment.
Per discussion, I'd be inclined to drop this change, and they can pass in Port with to_port if they need it.
| to_port: int | None = None, | ||
| endpoints: str | Iterable[str] | None = None, | ||
| ) -> None: ... | ||
| ) -> str | None: ... |
There was a problem hiding this comment.
I think this is a bug in Juju, or at least a wart. I don't think we should return stdout from hookcmds, but detect the error (from stdout) and raise an exception here. We should also open a bug on Juju to return a non-zero exit code for this case.
| protocol: typing.Literal['tcp', 'udp', 'icmp'], | ||
| port: int | tuple[int, int | None] | None = None, | ||
| *, | ||
| endpoints: Sequence[str] = '*', |
There was a problem hiding this comment.
I think it'd be better / more Pythonic for this input parameter, so endpoints: Sequence[str] | None = None.
3 participants
Resolves #2035
TODO: Update Harness backend port methods with new arguments (that's why DB charm tests are failing).
TODO: Update Scenario port methods and classes.