On 04/23/12 20:54, Tim Foster wrote:
On 04/17/12 02:31 PM, Brock Pytlik wrote:
On 04/09/12 22:02, Tim Foster wrote:
https://cr.opensolaris.org/action/browse/pkg/timf/granular-proxies/
Thanks for the detailed review! I've got updated bits at:
https://cr.opensolaris.org/action/browse/pkg/timf/granular-proxies-webrev-v1/
comments below.
--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.
Och, I'm wrong (I was flip-flopping over how to do this originally)
-G '*' still works, it just ignores proxy arguments, which itself may
need an explicit error message along the lines of "using wildcards
with --proxy is not supported" to avoid being ambiguous.
Seems like a reasonable compromise.
# 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.
I hoped that would come along with a -o option at some point with
7133972.
Without the -o support, it feels like changing the default to include
a new column for what I suspect won't be a hugely common configuration
(using a proxy) would be overkill. The long-form "pkg publisher
<publisher>" does include the proxy used, which I thought would be
enough for now, with the expectation of getting -o soon.
I think it makes sense to include that information in the default output
since, imo, even if the text of the URL is the same, if one URL is
proxied and one isn't, they're really different entities. Even with the
-o support, I'd still want the proxy column to be there in the default
output. What I could live with is that we only show the proxy column at
all if there are any origins with proxies.
# 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?
Do you mean, have tsv format use "URI" (which can contain
"<system-repository>" values), and add "Actual URI" (or somesuch) and
"Proxy"?
Yes, I think that's roughly what I meant. I don't like the idea that the
URI column of pkg publisher contains <system-repository> while the URI
column of pkg publisher -F tsv contains proxy://http://.... (or whatever).
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.
Well, we don't document the 'proxy://http://foo' format anywhere as
far as I know, and as is, passing one of those URLs to any of the
pkg(5) tools will result in errors that are at least as bad as trying
to do
pkg set-publisher -G '<system-repository>'
That said, if others feel like we ought to wait for a major release
for this change, I can do that, but I'd really rather get rid of
proxy:// sooner if at all possible.
I'm not sure it needs to wait for a major release. It was more that I
wanted to ask the question because I'm still learning how to judge these
things, and hoped to get further comment on it.
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?)
Good point - I forgot that. There's an argument to be made that a
$http_proxy environment variable works just as well for something
that's supposed to be temporary in the first place, but I can add this
support if others feel it's worthwhile (it's a relatively large change
though, so would like to get consensus on whether we really need this
first)
I think I'd be fine with a filed RFE to do this in the future. I don't
think it has to go back as part of this wad, but I do think it's
something we should plan to have eventually.
[snip]
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?
Good question. I'm trying to be backwards compatible here, to allow
older clients to still read these images. Yes, there's a risk that
older pkg(5) bits could remove one of those origins, causing the
origin_info map to become stale, but I felt that was better than
causing an image-format bump.
Ok. Can you document it, and maybe at least hand test what happens if
the origin_info map becomes stale? Does it cause tracebacks, do we even
notice it at all, etc...
[snip]
line 325: given how url_affix_trailing_slash is implemented, I don't get
why this line is necessary
It's necessary because it copes with input urls such as
http://foo/bar//
ensuring that we canonicalize them properly to simply
htt://foo/bar/
Ah. Hrm. I wonder whether that logic really belongs in
url_affix_trailing_slash, but I don't feel strongly about it.
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.
Groan, yes I agree - I'm a little lost as to our exact policy on
trailing slashes too. In this method, I wanted whatever we use to be
canonical because they're keys to allow us to look up RepositoryURI
objects in a dictionary, and I wanted to choose one representation and
stick with it.
More generally, perhaps it's worth revisiting our approach to trailing
slashes and demystifying the code a little, but perhaps not this
changeset?
I agree, not this changeset, but I do think it's something that we
should do.
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.
Yep, I thought about that, but wasn't sure whether the effort was
worth it. I've got a comment in
pkg.client.transport.engine.__cleanup_requests, but should have a
comment pointer going in the other direction. I've added that.
I'll leave that judgment to you. I'm not certain it's worth the effort,
but I think it might be.
[snip]
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).
Good idea - I've added a function to pkg.misc that does this. I'd
like to keep the <RepositoryURI>.runtime_proxy as a property though,
since we do still want to cache the result there, but it does now use
that common function in pkg.misc.
Good point about the caching, sounds like a good solution.
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?
Not quite, v[item] = "" is us encoding 'None' values in the dictionary
as an empty string, otherwise we end up with the property:
foo = [ {"this": "bar", "other": None} ]
getting serialised as:
foo = [ {"this": "bar", "other": "None"} ]
line 626 is really checking whether we allow an empty list.
I actually had the _is_allowed() method incorrect here - it wasn't
properly checking the dictionary contents against the allowed ones
(though we actually don't use that functionality in the code)
Ok, i think I follow now. Maybe a comment explaining why checking for ""
in self.allowed is a check for whether an empty list is allowed would be
helpful.
[snip]
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?
Yes.
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.
Yes, I understand - we've had similar limitations with p5p archives
and temporary publishers and zones in the past. Our choice is to:
a) invest in Apache trying to write code to have it support
authenticating proxies
b) document the behaviour (which the code checks for) and move on
I'm not inclined to do a) unless it was something that was a highly
requested feature, and so far, this isn't - I'd rather do b) and trust
that administrators would have test systems that closely match the
environment that they use in production.
If there's consensus that this isn't a good approach, I'll remove
support for authenticating proxies altogether.
I'm torn on what to do here, but going with what you've done seems as
good of an option as anything else.
[snip]
t_pkg_install:
[snip]
5011: Why start the proxy again here?
In this part of the test, we've configured a proxied origin as well as
an unproxied one. The proxy isn't configured to access either URI
though, so I want to be sure that the transport allows us to access
the URI (trying and failing to connect via the proxy, and falling back
to the direct route)
Ah, gotcha. Maybe call that out explicitly in a comment?
[snip]
I think I've addressed all of your comments here, but Shawn had said
he was interested in having a look at the changes too, so I'll hang on
for his comments, and any follow-on questions you or others may have.
Thanks,
Brock
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
