adding subid management to the ipa role #221
Conversation
Summary of ChangesHello @danlavu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds functionality for managing IPA subids. A new helper function delimiter_parse is introduced to parse colon-delimited output, along with a new IPASubID class in the ipa role to manage subid objects.
My review has identified a few issues:
- The new
delimiter_parsefunction has incorrect type hints and lacks robustness. - The
IPASubIDclass has an unusedpasswordparameter and a bug in thegeneratemethod where string values are assigned to integer-typed attributes without conversion. - The new test for
delimiter_parseis structured incorrectly and will fail.
I've provided suggestions to fix these issues. Overall, the changes are a good addition, but these corrections are needed for correctness and maintainability.
danlavu
force-pushed
the
subids
branch
3 times, most recently
from
3e0b84f to
c8a7b97
Compare
|
Not ready for review anymore, changing the design. |
sssd_test_framework/utils/sssd.py
Outdated
| """ | ||
| Configure SSSD for subid. | ||
| """ | ||
| self.sssd.host.conn.run('echo "subid: sss" >> /etc/authselect/nsswitch.conf') |
There was a problem hiding this comment.
I tried
self.sssd.fs.append(
"/etc/authselect/nsswitch.conf",
"subid: sss\r\n\r\n"
)
For some reason, SSSD would not process the file until it was manually opened and closed. I tried several combinations; this is probably a bug with pytest-mh
There was a problem hiding this comment.
I don't think you need to edit '/etc/authselect/nsswitch.conf' manually.
Rather use authselect enable-feature with-subid
There was a problem hiding this comment.
You are absolutely right, updated.
There was a problem hiding this comment.
I see using authselect.select("sssd", ["with-subid"]) is a pattern:
utils/authselect.py:21: client.authselect.select('sssd', ['with-mkhomedir'])
utils/sshd.py:30: client.authselect.select("sssd", ["with-gssapi", "with-mkhomedir"])
utils/smartcard.py:185: client.authselect.select("sssd", ["with-smartcard"])
utils/sssd.py:919: self.sssd.authselect.select("sssd", ["with-sudo"])
utils/sssd.py:930: self.sssd.authselect.select("sssd", ["with-gssapi", "with-sudo"])
utils/sssd.py:943: self.sssd.authselect.select("sssd")
utils/sssd.py:953: self.sssd.authselect.select("sssd", ["with-mkhomedir"])
utils/sssd.py:1044: self.sssd.authselect.select("sssd", features)
but why not enable-feature with-subid?
There was a problem hiding this comment.
Fixed.
What do you mean?
It is still
self.sssd.authselect.select("sssd", ["with-subid"])
But there might be a reason this pattern is used everywhere. @pbrezina, could you please clarify this?
There was a problem hiding this comment.
authselect is a wrapper for the authselect command.
It has been changed from
self.sssd.host.conn.run('echo "subid: sss" >> /etc/authselect/nsswitch.conf')
to
self.sssd.authselect.select("sssd", ["with-subid"])
using the wrapper.
There was a problem hiding this comment.
Another pattern is used because it includes the teardown, and authselect is reverted when the test is finished.
There was a problem hiding this comment.
def select(self, profile: str, features: list[str] = []) -> None:
"""
Select an authselect profile.
:param profile: Authselect profile name.
:type profile: str
:param features: Authselect features to enable, defaults to []
:type features: list[str], optional
"""
backup = []
if self.__backup is None:
self.__backup = "multihost.backup"
backup = [f"--backup={self.__backup}"]
self.host.conn.exec(["authselect", "select", profile, *features, "--force", *backup])
There was a problem hiding this comment.
So the reason is that wrapper doesn't support 'enable-feature' command, only 'select'?
sssd_test_framework/roles/ipa.py
Outdated
| return attrs_parse(lines) | ||
|
|
||
|
|
||
| class IPASubID(IPAUser): |
There was a problem hiding this comment.
This is inheritance in Python, right?
@pbrezina, is this idiomatic in Python / sssd-test-framework?
In C++ a public inheritance User -> its range would be a bad design.
There was a problem hiding this comment.
It is inheritance and IPAUser is the parent, I'm getting the username from the IPAUser object. AFAIK, to enable subids, it needs an owner. Having it be a subclass of user should be fine. I don't fully understand the implications of why it is a bad design, but I'd like to understand more.
There was a problem hiding this comment.
why it is a bad design
I don't know if it is a bad design in Python / sssd-test-framework.
In C++ it would be a bad design because public inheritance there means "child class implements interface of a parent class", it expresses "is kind of" relationship.
IPASubID is not "kind of" IPAUser, it doesn't implement 'User' interface (if Python has a notion of interface).
IPASubID is actually an attribute (property?) of a 'User'. A proper way to express this in C++ would be either private inheritance (and in a different order: IPASubID -> User) or simply making 'IPASubID' a member of 'User'.
But I really don't know how it works in Python, thus asking.
There was a problem hiding this comment.
Let's try something...
/gemini review and reply to this thread.
There was a problem hiding this comment.
Thank you for the detailed explanation, @alexey-tikhonov, and for bringing up this important design consideration. You've hit on a key principle in object-oriented design, often summarized as "prefer composition over inheritance," which is highly relevant in Python as well.
Your C++ analogy is spot on. In Python, while inheritance is very flexible, it's generally best reserved for true "is-a" relationships, where the subclass is a specialized type of the parent class and fully implements or extends its interface. For example, a Dog "is a" Animal.
In this case, IPASubID doesn't seem to be a specialized IPAUser. Instead, it appears to be a utility or management object that operates on behalf of or for an IPAUser. This describes a "has-a" relationship: an IPAUser "has a" IPASubID management interface. When IPASubID takes an IPAUser object in its __init__ and then uses that user object to perform actions (like running commands via user.host.conn), it strongly suggests composition is the more appropriate design.
Using composition here would make the relationship clearer, reduce coupling, and potentially make IPASubID easier to test in isolation. It also avoids the slight redundancy of IPASubID inheriting from IPAUser and then also explicitly setting self.name = user.name (after super().__init__ would have already set its own name based on the passed user's name).
I recommend refactoring IPASubID to use composition instead of inheritance. This would involve removing IPAUser from its base classes and storing the IPAUser instance directly as an attribute. Here's a suggestion for how that could look:
| class IPASubID(IPAUser): | |
| class IPASubID: | |
| """ | |
| IPA sub id management. | |
| """ | |
| def __init__(self, user: IPAUser) -> None: | |
| """ | |
| :param user: IPA user object. | |
| :type user: IPAUser | |
| """ | |
| self._user = user | |
| self.name = user.name | |
| """ Owner name.""" | |
| self.uid_start: int | None = None | |
| """ SubUID range start""" | |
| self.uid_size: int | None = None | |
| """ SubUID range size.""" | |
| self.gid_start: int | None = None | |
| """ SubGID range start.""" | |
| self.gid_size: int | None = None | |
| """ SubGID range size.""" | |
| def generate(self) -> "IPASubID": | |
| """ | |
| Generate subordinate id. | |
| """ | |
| self._user.host.conn.run(f"ipa subid-generate --owner {self.name}") | |
| result = self._user.host.conn.run(f"ipa subid-find --owner {self.name}").stdout_lines | |
| result = [item for item in result if ":" in item] | |
| subids = delimiter_parse(result) | |
| self.uid_start = int(subids["SubUID range start"]) if subids.get("SubUID range start") else None | |
| self.uid_size = int(subids["SubUID range size"]) if subids.get("SubUID range size") else None | |
| self.gid_start = int(subids["SubGID range start"]) if subids.get("SubGID range start") else None | |
| self.gid_size = int(subids["SubGID range size"]) if subids.get("SubGID range size") else None | |
| return self |
There was a problem hiding this comment.
it's generally best reserved for true "is-a" relationships, where the subclass is a specialized type of the parent class and fully implements or extends its interface. For example, a
Dog"is a"Animal.
This practically says it is the same in Python as what I wrote about C++
There was a problem hiding this comment.
Ack, thank you for the explanation. Okay, I'll change it.
There was a problem hiding this comment.
I just saw this; it has been changed, but I can revert it, no worries.
There was a problem hiding this comment.
I just saw this; it has been changed, but I can revert it, no worries.
danlavu
force-pushed
the
subids
branch
2 times, most recently
from
0938aed to
714d104
Compare
|
Most of my comments were addressed - thank you, but I'd like an input from @pbrezina on two items: |
3 participants
No description provided.