Skip to content
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

Added proxy setup service #696

Merged
merged 11 commits into from
Aug 21, 2023
Merged

Added proxy setup service #696

merged 11 commits into from
Aug 21, 2023

Conversation

teclator
Copy link
Contributor

@teclator teclator commented Aug 9, 2023

Problem

Currently Agama does not have support for setting up a proxy

In case of a network installation dracut url lib uses the one provided by the kernel command line option:

https://github.com/openSUSE/dracut/blob/5e324584a14377f52425be589ab53299609f066b/modules.d/45url-lib/url-lib.sh#L62

We should write a service that continue writing the /etc/sysconfig/proxy and use the microos-tools proxy setup service to make it available to the systemd units if we wanted, or own systemd service to do that before running agama.

The service should read the /proc/cmdline in case it was given as a kernel commandline option or check the configuration written by dracut when asking for more kernel paramaters

Solution

An agama-proxy-setup service and executable will take care of parsing the /proc/cmdline writing the information to /etc/sysconfig/proxy

The setup-systemd-proxy-env path provided by microos-tools is enabled in order to write the information as environment variables to systemd units.

See https://build.opensuse.org/package/rdiff/home:teclator:branches:systemsmanagement:Agama:Devel/agama-live?opackage=agama-live&oproject=systemsmanagement%3AAgama%3ADevel&rev=5

Testing

  • Tested manually

ProxySetup

@coveralls
Copy link

coveralls commented Aug 9, 2023

Coverage Status

coverage: 72.057% (+0.04%) from 72.013% when pulling d8bdfea on proxy_setup into d305be9 on master.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it looks good. However, I miss better documentation and tests.

service/bin/agama-proxy-setup Outdated Show resolved Hide resolved
service/bin/agama-proxy-setup Outdated Show resolved Hide resolved
@teclator
Copy link
Contributor Author

doc/boot_arguments.md Outdated Show resolved Hide resolved
service/lib/agama/proxy_setup.rb Outdated Show resolved Hide resolved
service/lib/agama/proxy_setup.rb Outdated Show resolved Hide resolved
service/lib/agama/proxy_setup.rb Show resolved Hide resolved
def write
return unless proxy

Proxy.Read
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that the tool will merge with any existing proxy configuration?
We should document it if it's intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it follows the same approach we have for YaST based installers (https://github.com/yast/yast-network/blob/master/src/lib/network/install_inf_convertor.rb#L80).

So, it merges the configuration but in most of the cases it should be empty. I guess by now we could just write what is defined instead of merging it as we are not doing any upgrade or something like that where an existent config could be useful at all.

doc/boot_arguments.md Outdated Show resolved Hide resolved
@mvidner
Copy link
Contributor

mvidner commented Aug 16, 2023

I wonder why CI fails (at building nokogiri gem) here but not at #703 ...

Ah that's because Imo has figured out AWK is needed for building it: 5aaac0e

teclator and others added 5 commits August 16, 2023 15:05
Applied fixes suggested from code review

Co-authored-by: Martin Vidner <mvidner@suse.cz>
Co-authored-by: Martin Vidner <mvidner@suse.cz>
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good. However I am not sure if we should keep the indentation in the setup-service.sh script.

doc/boot_arguments.md Outdated Show resolved Hide resolved
doc/boot_arguments.md Outdated Show resolved Hide resolved
service/lib/agama/proxy_setup.rb Outdated Show resolved Hide resolved
service/lib/agama/proxy_setup.rb Outdated Show resolved Hide resolved
setup-service.sh Outdated Show resolved Hide resolved
teclator and others added 3 commits August 17, 2023 23:58
Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I forgot about the changes file. Please, add an entry and do not forget the bsc and/or github references.

@teclator teclator merged commit 40e46fa into master Aug 21, 2023
11 checks passed
@teclator teclator deleted the proxy_setup branch August 21, 2023 06:50
@teclator teclator mentioned this pull request Aug 22, 2023
This was referenced Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants