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

Fixes #28349 - refactors importer comparison #8455

Merged
merged 5 commits into from
Nov 26, 2019

Conversation

jjeffers
Copy link
Member

@jjeffers jjeffers commented Nov 25, 2019

This PR addresses a pulp2 bug where the proxy password is returned as a masked string.

>> in the pulp2 db

> db.repo_importers.find({})
{ "_id" : ObjectId("5dd6ce3506c71a7bd3d7623f"), "repo_id" : "5bc050e0-88d7-4f8d-8a75-ab2516b53b74", "importer_type_id" : "yum_importer", "config" : { "feed" : "https://repos.fedorapeople.org/pulp/pulp/fixtures/rpm-unsigned/", "remove_missing" : true, "proxy_host" : "", "ssl_validation" : true, "proxy_password" : "", "download_policy" : "immediate", "proxy_username" : "" }, "last_updated" : ISODate("2019-11-22T14:52:10.174Z"), "last_override_config" : { "num_threads" : 4, "validate" : true }, "_ns" : "repo_importers", "scratchpad" : { "repomd_revision" : 1573300917, "repomd_checksum" : "734ef2769824aeb357165a7959b0c942fad7ce0ba5d8bde12c145d7f4fa0c231" }, "last_sync" : "2019-11-21T17:50:14Z" }

>> Check repos details using pulp-admin

[vagrant@centos7-katello-devel-stable foreman]$ pulp-admin -u admin -p admin repo list --details --repo-id 5bc050e0-88d7-4f8d-8a75-ab2516b53b74
+----------------------------------------------------------------------+
                              Repositories
+----------------------------------------------------------------------+

Id:                   5bc050e0-88d7-4f8d-8a75-ab2516b53b74
Display Name:         test
Description:          None
Content Unit Counts:  
  Erratum:           4
  Package Category:  1
  Package Group:     2
  Package Langpacks: 1
  Rpm:               35
Notes:                
Scratchpad:           
  Checksum Type: sha256
Importers:            
  Config:               
    Download Policy: immediate
    Feed:            https://repos.fedorapeople.org/pulp/pulp/fixtures/rpm-unsig
                     ned/
    Proxy Host:      
    Proxy Password:  *****
    Proxy Username:  
   [...]

This condition triggers repo refreshes unnecessarily.

@theforeman-bot
Copy link

Issues: #28349

@jturel
Copy link
Member

jturel commented Nov 25, 2019

🔥 🔥 🔥 🔥 🔥

@theforeman-bot
Copy link

There were the following issues with the commit message:

  • length of the first commit message line for ece0687 exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

if generated_config['proxy_password'] == "" && capsule_config['proxy_password'] == "*****"
generated_config.delete('proxy_password')
capsule_config.delete('proxy_password')
end
Copy link
Member

Choose a reason for hiding this comment

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

to clarify, if generated_config['proxy_password'] is set, it will always refresh the importer. Is that what we want?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is your suggestion to always ignore proxy_password in the comparison? Since pulp2 will always return that masked string (if set previously to any value), it seems like any comparison will fail.

Copy link
Member

Choose a reason for hiding this comment

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

possibly, but i was more opening up debate :)
I think this is the safter thing to do is the way you have it, although it makes #8415 not really work if you're using an http proxy, although thats not really a problem introduced by this pr. So i guess yes lets leave it the way you have it 👍 but we should probably open a bug around this to discuss how to properly address it (especially with pulp3)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully this isn't an issue in pulp3.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could ask pulp2 to be fixed to resolve this. That's another option.

Copy link
Member

Choose a reason for hiding this comment

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

I checked and pulp3 returns the full proxy url:

{"pulp_href":"/pulp/api/v3/remotes/file/file/c6688395-58f8-461a-bd3e-145083741543/","pulp_created":"2019-11-26T19:11:49.606085Z","name":"proxytest-137322","url":"http://foo.com/bar/PULP_MANIFEST","ca_cert":null,"client_cert":null,"client_key":null,"tls_validation":true,"proxy_url":"http://boobar:boobar@foo.com","pulp_last_updated":"2019-11-26T19:12:43.872594Z","download_concurrency":20,"policy":"immediate"}

however i suspect that might be a bug ;)

I think this is fine for now!

@ianballou
Copy link
Member

I gave this a test by calling refresh_if_needed directly on the repository's backend_service. Without the PR I could see the unnecessary actions, and with it applied the actions went away.

Copy link
Member

@jlsherrill jlsherrill left a comment

Choose a reason for hiding this comment

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

ACK based on @ianballou 's testing

@jlsherrill
Copy link
Member

don't forget to squash!

@jjeffers jjeffers merged commit 398feda into Katello:master Nov 26, 2019
@jjeffers jjeffers deleted the 28349_impoter_comarison branch February 20, 2020 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants