-
Notifications
You must be signed in to change notification settings - Fork 24
RFC: Multi-file Autoinstallation Support #109
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
base: master
Are you sure you want to change the base?
RFC: Multi-file Autoinstallation Support #109
Conversation
Remove duplicated question and rephrasings
b619470 to
bcb4a41
Compare
| - Number of files per type? | ||
| - File size? | ||
|
|
||
| - Should list the allowed file types (in the UI)? |
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.
If so, what are the allowed file types we want to support?
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.
These were be the ones I had in mind to go for in this initial stage:
- Kickstart / Redhad based, INI-like (.ks) and plain text
- AutoYaST: XML
- Agama: YAML
- Combustion: shell scripts
- Ignition: JSON
- Cloud-config: Cloud-init YAML
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.
Agama profile is not YAML, but Jsonnet (see https://agama-project.github.io/docs/user/reference/profile )
I think this list should be part of the initial RFC
| # Alternatives | ||
| [alternatives]: #alternatives | ||
|
|
||
| - Keep only one file per profile - too restrictive for complex setups. |
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.
Could make sense to allow either a single normal file, either a compressed tgz file?
So, if we upload a compressed file, we will decompress it when running the profile to be used during the autoinstallation and we will store it in database as a single file too.
We can then avoid thinking about allowed file types and so on, being more flexible.
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 can see how supporting a compressed file upload could be convenient. But I’d still prefer to keep the individual file contents. The major reason being that one of the future steps we outlined in the document is turning this into a templating mechanism. And for that we need to be able to edit, parameterize, and validate each file. Do you agree @aaannz ?
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.
These files currently are not stored on the SMLM side at all. They are transferred to the cobbler upon creation.
So we would need to make cobbler to support those tgz files and then we are not in any business serving those files.
Adding support for compressed files will IMO be much bigger task particularly because of dealing with cobbler.
rjmateus
left a comment
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.
Overall looks good, just a few improvements and clarification that I think would help the overall understanding.
| Files will be stored locally (in the database), making the system the primary source of truth, rather than relying on external services like Cobbler. | ||
|
|
||
| ## Validation | ||
| Ensure that uploaded file types are compatible with the selected OS or distribution (eg, Agama files for SLE15SP6). |
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 think agama will be part of sles16 only, and not sles15sp6
|  | ||
|
|
||
| ## Validations | ||
| - Ensure OS / distribution compatibility (e.g., Agama not allowed on SLE15SP6). |
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 would agama not allowed in sle15 (all SP's)
| ## Database | ||
| - New tables: | ||
| - suseAutoInstallFileType — id, type, description, timestamp | ||
| - suseAutoInstallFile — id, type FK, content, timestamp |
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.
Should the file be linked to a profile also? If that is the case, I think we should add it here.
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.
What @rjmateus said. Plus we have two timestamps - creation and modification time.
Additionally, where will we add the format and OS compatibility. Will this business logic be in the Java code or we'll have a reference table in a database?
Generally I think that support for new OS will not need additional code, just assigning correct format with particular OS, adding it to the database data is better. However currently AFAIK we do not have OS information available in the database during the build time, only once synced with SCC and/or common channels.
| - Deleting a profile deletes all associated files. | ||
| - Deleting an individual file does not affect the rest of the profile. | ||
|
|
||
| # Drawbacks |
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.
Are you planning on migrating the existing profiles from cobbler to this mechanism? I think it would be a good approach to have a single mechanism for all the files, instead of keeping to ways of working (even for a single file profile)
|
|
||
| - Should we limit: | ||
| - Number of files in a profile? | ||
| - Number of files per type? |
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 not sure in this one, but we should follow what the OS's supports. Does sle16 support more than one agama file? Does it support more than one ignition, and so on.
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.
As far as I know currently two files are for ignition/combustion combo. But that are two different types. So for now I think it is just one file per type, but allowing multiple types.
If we want to have a limit, then the suseAutoInstallFileType database schema should be extended to cover this information.
| - Should we limit: | ||
| - Number of files in a profile? | ||
| - Number of files per type? | ||
| - File size? |
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.
We must have some limit for it. These are text files normally, so should not be to big.
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 think this is for us to arbitrarily decide. I did not see any maximum size validation anywhere and those autoinstallation files can contain binary data as well.
From security POV, I think this is fairly irrelevant because as soon as attacker can provide their own file, it is trivial to generate OOM condition if that is the goal here even with small sizes.
| - UI/UX design for multi-file edit/upload - the provided screen update is a suggestion. UI team should review it so we make sure were following FE rules and usability constraints are met. | ||
|
|
||
| ## Future steps | ||
| - Should files be validated for content according to type? |
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.
In my opinion yes, but that would be a future improvement, as you said.
|
|
||
| ## Future steps | ||
| - Should files be validated for content according to type? | ||
| - Templating/snippet support should be covered by another RFC? |
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.
That depends on big it would be. We can have it in this RFC, but the implementation can be split into multiple issues or even Epics.
@admd any opinion on this one?
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.
Depends if we want powerful templating like cheetah in the cobbler, or will plain string replacement suffice, maybe with some trivial tests like if not set use default.
| - Number of files per type? | ||
| - File size? | ||
|
|
||
| - Should list the allowed file types (in the UI)? |
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.
Agama profile is not YAML, but Jsonnet (see https://agama-project.github.io/docs/user/reference/profile )
I think this list should be part of the initial RFC
| # Summary | ||
| [summary]: #summary | ||
|
|
||
| This RFC proposes extending the autoinstallation profile mechanism to support multiple files per profile and storing them in the database (instead of relying on Cobbler). |
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.
Let's drop the 'This RFC proposes' and handle this as an architectural overview document.
Goal is to be able to source this into the AI for potential help.
E.g.
| This RFC proposes extending the autoinstallation profile mechanism to support multiple files per profile and storing them in the database (instead of relying on Cobbler). | |
| Extend autoinstallation profile mechanism to support multiple files per profile and store them in the database instead of relying on additional independent software like Cobbler. |
| Ensure that uploaded file types are compatible with the selected OS or distribution (eg, Agama files for SLE15SP6). | ||
|
|
||
| ## Benefits | ||
| To sum it, supporting multiple files would: |
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.
| To sum it, supporting multiple files would: |
| ## Database | ||
| - New tables: | ||
| - suseAutoInstallFileType — id, type, description, timestamp | ||
| - suseAutoInstallFile — id, type FK, content, timestamp |
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.
What @rjmateus said. Plus we have two timestamps - creation and modification time.
Additionally, where will we add the format and OS compatibility. Will this business logic be in the Java code or we'll have a reference table in a database?
Generally I think that support for new OS will not need additional code, just assigning correct format with particular OS, adding it to the database data is better. However currently AFAIK we do not have OS information available in the database during the build time, only once synced with SCC and/or common channels.
| # Alternatives | ||
| [alternatives]: #alternatives | ||
|
|
||
| - Keep only one file per profile - too restrictive for complex setups. |
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.
These files currently are not stored on the SMLM side at all. They are transferred to the cobbler upon creation.
So we would need to make cobbler to support those tgz files and then we are not in any business serving those files.
Adding support for compressed files will IMO be much bigger task particularly because of dealing with cobbler.
|
|
||
| - Should we limit: | ||
| - Number of files in a profile? | ||
| - Number of files per type? |
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.
As far as I know currently two files are for ignition/combustion combo. But that are two different types. So for now I think it is just one file per type, but allowing multiple types.
If we want to have a limit, then the suseAutoInstallFileType database schema should be extended to cover this information.
| - Should we limit: | ||
| - Number of files in a profile? | ||
| - Number of files per type? | ||
| - File size? |
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 think this is for us to arbitrarily decide. I did not see any maximum size validation anywhere and those autoinstallation files can contain binary data as well.
From security POV, I think this is fairly irrelevant because as soon as attacker can provide their own file, it is trivial to generate OOM condition if that is the goal here even with small sizes.
|
|
||
| ## Future steps | ||
| - Should files be validated for content according to type? | ||
| - Templating/snippet support should be covered by another RFC? |
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.
Depends if we want powerful templating like cheetah in the cobbler, or will plain string replacement suffice, maybe with some trivial tests like if not set use default.
This RFC proposes extending the autoinstallation profile mechanism to support multiple files per profile and storing them in the database (instead of relying on Cobbler).
See the rendered version.