On 04/09/12 22:02, Tim Foster wrote:
hi all,
I've got a webrev here for the granular-proxy wad, which gives "pkg
set-publisher" a new option "--proxy" that applies to -g, -G, -m, -M and
-p arguments.
This wad also changes the "pkg publisher <pub>", "pkg publisher -F
tsv" and, for zones, "pkg publisher" command outputs. Examples of
each are at the end of this mail, with a quick summary of the changes.
https://cr.opensolaris.org/action/browse/pkg/timf/granular-proxies/
Comments would be most welcome. Apologies for the weight of this
webrev, I didn't think it'd end up being quite as hefty as this.
cheers,
tim
--proxy option
--------------
We add a --proxy option, where the combination of -g/G/m/M/p and the
--proxy flag now indicate a unique origin or mirror. Wildcards like
'-G * --proxy foo' will only work if every origin to be removed has
the same proxy value 'foo'.
Does this mean that -G '*' (without the --proxy) will no longer
unconfigure every origin for a publisher? That would be troubling.
# pkg image-create /tmp/a
# pkg -R /tmp/a set-publisher -g http://ipkg.us.oracle.com/on-extra
on-extra
# pkg -R /tmp/a set-publisher -G http://ipkg.us.oracle.com/on-extra
--proxy http://kura:9091 on-extra
pkg set-publisher: Unknown repository origin
'http://ipkg.us.oracle.com/on-extra/' with proxy 'http://kura:9091'
# pkg -R /tmp/a set-publisher -G http://ipkg.us.oracle.com/on-extra
on-extra
# pkg -R /tmp/a set-publisher -g http://ipkg.us.oracle.com/on-extra
--proxy http://kura:9091 on-extra
# pkg -R /tmp/a publisher
PUBLISHER TYPE STATUS URI
on-extra origin online
http://ipkg.us.oracle.com/on-extra/
I thought there was going some indication that this uri was being
proxied? I'm uncomfortable with this being the entire output of pkg
publisher as I think it's actively misleading to people if they're
trying to figure out why they can't reach a publisher. I think there may
need to be an additional column (perhaps Proxied?) which has true/false
values in it. Other display suggestions are definitely welcome.
# pkg -R /tmp/a publisher on-extra
Publisher: on-extra
Alias:
Origin URI: http://ipkg.us.oracle.com/on-extra/
Proxy: http://kura:9091
SSL Key: None
SSL Cert: None
Client UUID: a8cd40a8-8227-11e1-8664-d4bed990482b
Catalog Updated: April 9, 2012 09:38:59 AM
Enabled: Yes
Zones pkg publisher changes
---------------------------
We get rid of the "proxy://<uri>" resources for the short-form pkg
publisher output, instead using the more explanatory
"<system-repository>" value, but provide the full URI in both the
long-form pkg publisher output, as well as the --format tsv output.
# pkg -R /zones/kahurangi/root publisher
PUBLISHER TYPE STATUS URI
solaris (syspub) origin online <system-repository>
punchin (syspub) origin online <system-repository>
extra (syspub) origin online <system-repository>
internal-only (syspub) origin online <system-repository>
solarisstudio (syspub) origin online <system-repository>
on-extra (syspub) origin online <system-repository>
userland (syspub) origin online
http://localhost:1008/userland/614bdd284706b4be9743fa5021c76a968192c904/
I don't think that changing the output based on whether the format's tsv
or not makes much sense. Perhaps the tsv version should have another
column which has the full uri and the proxy info?
While I'm very enthusiastic about the change to the <system-repository>
display, I'm a little concerned about it taking place now, as opposed to
across a larger change boundary.
The rest of the output looked good to me.
[snip]
client.py:
I noticed changes only on set-publisher, do we need to allow --proxy on
commands which can take a temporary repository? ('pkg list -g <mumble>
--proxy' for example?)
imageconfig.py:
line 627: nit: couldn't you just put a set() around the list comprehension?
line 627: Could we get a comment added about why origins and mirrors
should be stored as sets, when other properties aren't?
596-627: I'm having a little trouble understanding why, given the new
info stored in lines 596-619, why we also need to store the origins and
mirrors separately?
Lines 643-646: can't this be removed given the change to line 631?
773-774: this comment seems a little awkward. Perhaps:
"For now, the only property tracked on a per-origin/mirror basis is
which proxy (if any) is used to access it."
836-863: This could be condensed into a single loop (like you've done
elsewhere with the info_map dictionary for example). Not sure if that
would make things simpler or not.
publisher.py:
line 213: does it make sense to have this do url_ffix_trailing_slash as
well (or conversely, to strip the /)?
line 325: given how url_affix_trailing_slash is implemented, I don't get
why this line is necessary
381-385: why are we stripping the slashes here? as a general comment,
I'd think it'd make sense to decide either "uri/proxy's always have
trailing slashes" or "uri/proxy's never have trailing slashes" and store
and display them in the canonical form rather than stripping and adding
the slashes in various locations. Maybe there's a real need to have
both, which the code suggests might be the case, but even looking at the
code, I have no idea WHY it would be the case.
engine.py:
224: it might be overkill, but I think I can imagine a way to write this
such that the values it uses for a key are automatically updated when
the RepoUri's key method is changed. If nothing else, there probably
needs to be a comment on the key method mentioning that this needs to
change if it's changed. Let me know if you think the automatic way would
be worth doing, and we can discuss offlist how we might achieve it.
repo.py:
2242-2244: two questions: First, does it still make sense to get the
scheme from the repostats object and not the repouri object? if so,
could we add a comment as to why? Second, assuming it does, could we
collapse 2242-2244 into one line of code since origin_url and url_tuple
don't seem to be used anywhere else anymore?
2265-2267: same comments as 2242-2244
stats.py:
62-76: This looks an awful lot like the code from
publisher.__get_runtime_proxy. I'm wondering if instead of making
runtime_proxy a property, we should make get_runtime_proxy a function
(since you can't set a runtime_proxy) and allowing it to take a
parameter indicating whether expand_proxy_envvars is used or not (which
I think is what would introduce the cleartext passwords).
config.py:
626: a comment about what we care about here would be nice. Because of
setting v[item] = "" above, I initially thought that's why we're
checking for "" in self.allowed, but after looking closely at the code,
I think that's not the case. What we're checking for is whether the list
itself is allowed to be empty, not whether any of the items within the
list are allowed to be empty. Is that correct?
795: do we not care if the proxy is a valid uri at least? (or that it
starts with http?)
misc.py:
1396-1397: completely optional alternative way of expressing this:
if not all(item.startswith("$") for item in user_passwd):
sysrepo.py
278-284: If I'm reading this correctly, what this means is that if you
have a repository accessed through a proxy where you need to provide a
userid and password, you can't use that repository from the global zone
if you have any non-global zones, is that right? If that's the case, I'm
wondering about the wisdom of allowing the use of those kinds of proxies
at all. I agree it's probably the right decision to go with what you
have, but I am concerned that someone might develop and test their
publisher configuration on a machine that has no zones, then be
unpleasantly surprised when they go to roll out their setup on
production machines and find that it's broken because the production
machines have zones.
t_misc.py:
112-116: Might it be worth testing what happens to something like this:
http://$user$password@host";? I'm not sure what should happen, but it
seems like we should define that behavior and test for it since it's an
easy typo to make.
t_pkg_install:
4964-4967: Just something that popped into my head when I was looking at
this. Would it make sense to have self.sysrepo() return some of that
info as part of its return value?
5011: Why start the proxy again here?
test_19_granular_proxy: Just to put a bow on things, it might be worth
testing that once the unproxied origin for test2 is removed, that an
install of a package from test2 fails.
t_sysrepo:
Can you verify for me, at least using a manual test, that changing the
proxy on an origin induces a system-repository refresh?
Thanks,
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
