If we can remove these support, I would like this approach too. Current implementation has too many inconsistencies.
On Mon, Dec 30, 2019 at 9:34 AM Kirk Lund <kl...@apache.org> wrote: > My vote is for #2 in Jinmei's proposal with the following correction: > > After removing --host/--port, you still have two ways to get "status > locator": > > a) Use --dir option: > > gfsh>status locator --dir=/locator-dir > > b) Use connect command first: > > gfsh>connect locator <etc> > gfsh>status locator --name=locator1 > > > On Mon, Dec 30, 2019 at 9:30 AM Kirk Lund <kl...@apache.org> wrote: > > > Long before we open-sourced as Geode, GemFire engineering planned to > > remove support for: > > > > gfsh>status locator --host=fooish --port=10334 > > > > The reasoning at the time was (1) this method for getting details about > > the locator was wide open (security hole), and (2) it's inconsistent with > > "status server" which has no such counterpart. > > > > I prefer: remove --host and --port from status locator to be more > > consistent with status server and avoid having to add something that no > > user has requested (as far as I know) > > > > If we need to add SSL support for --host and --port to status locator, > > then we should include adding --host and --port to status server as well > to > > improve consistency and usability -- I use the term "usability" because > > users expect "status server --host=xxx --port=xxx" to also work IF we > > continue to provide "status locator --host=xxx --port=xxx". We need to > get > > rid of and avoid these weird quirks and inconsistencies. > > > > On Mon, Dec 23, 2019 at 11:24 AM Jinmei Liao <jil...@pivotal.io> wrote: > > > >> So, looks like need to do either one of the following: > >> 1. add --security-property-file option for the user to specify the ssl > >> connection properties > >> 2. remove --host/--port combination and remove the capability to report > >> the > >> status of the cluster configuration, this would only allow "status > >> locator" > >> command to report the locally started locator's status > >> > >> I would vote for option 1, given that option 2 greatly changes behavior. > >> > >> If no other objections, I will go ahead with option 1. > >> > >> Jinmei > >> > >> On Fri, Dec 20, 2019 at 4:20 PM Jens Deppe <jde...@pivotal.io> wrote: > >> > >> > I think Jinmei is also referring to the situation where you might > *not* > >> > already > >> > be connected but want to execute something like this: > >> > > >> > gfsh>status locator --host=fooish --port=10334 > >> > > >> > > >> > In this case it would fail as well. Seems like we'd either need to > >> remove > >> > those options so you *always* need to 'connect' if you want to access > a > >> > remote system, or we need to add an *optional* > >> --security-properties-file > >> > option that would only be necessary if the shell isn't already > >> connected. > >> > > >> > --Jens > >> > > >> > > >> > > >> > > >> > On Fri, Dec 20, 2019 at 3:15 PM Ernest Burghardt < > eburgha...@pivotal.io > >> > > >> > wrote: > >> > > >> > > I like the approach Jens is suggesting, seems intuitive to me > >> > > > >> > > On Thu, Dec 19, 2019 at 6:31 PM Jens Deppe <jde...@pivotal.io> > wrote: > >> > > > >> > > > So it seems that the situation is something like this where I'm > >> able to > >> > > > make the initial connection but then retrieving status fails: > >> > > > > >> > > > gfsh>connect --security-properties-file=../security.properties > >> > > > > >> > > > gfsh>status locator --name=locator1 > >> > > > Server version response invalid: This could be the result of > trying > >> to > >> > > > connect a non-SSL-enabled client to an SSL-enabled locator. > >> > > > > >> > > > > >> > > > It would seem very weird if I have to provide additional > connection > >> > > params > >> > > > to the 'status' command if I've already provided them as part of > the > >> > > > 'connect'. Could we not stash the connection properties (in the > Gfsh > >> > > > instance object) for subsequent usage? > >> > > > > >> > > > --Jens > >> > > > > >> > > > On Wed, Dec 18, 2019 at 9:04 AM Jinmei Liao <jil...@pivotal.io> > >> wrote: > >> > > > > >> > > > > "status locator" command is broken on ssl enabled locators ever > >> since > >> > > we > >> > > > > fixed a bug that leaked the connection properties from one tcp > >> socket > >> > > > > connection to another. Before that it would just magically work > >> if we > >> > > > have > >> > > > > previously made a successfully tcp connection to that same > >> locator, > >> > now > >> > > > we > >> > > > > are either required to find a way to specify the ssl properties > >> when > >> > > > > running the `status locator` command or change what we want to > >> report > >> > > > back > >> > > > > to the user. > >> > > > > > >> > > > > Here is what's happening now when we issue the command: > >> > > > > > >> > > > > 1. it needs to retrieve two sets of info from locator: > general > >> > info > >> > > > like > >> > > > > (pid, working dir, status, jvm args etc) and whether cluster > >> > > > > configuration > >> > > > > service is running or not. > >> > > > > 2. when locator’s ssl is on, the retrieval of the cluster > >> > > > configuration > >> > > > > info will always fail since it’s using a tcp connection to > get > >> > that > >> > > > info > >> > > > > and we currently don’t have the ssl security properties to > >> > connect. > >> > > > > 3. when locator’s ssl is on, the retrieval of the general > info > >> > will > >> > > > > mostly succeed except when user is only providing a host and > >> port, > >> > > > > there we > >> > > > > would also need the ssl security properties in order to > create > >> an > >> > > ssl > >> > > > > socket. > >> > > > > > >> > > > > > >> > > > > So we wither need to add an option for user to specify a > >> > > > > `--security-properties-file` option to include all the ssl > >> connection > >> > > > > properties or change the behavior not to report cluster config > >> status > >> > > or > >> > > > > not allowing host/port combination. (I am not here long enough > to > >> > > > > understand what users would use this command for, so this is > just > >> a > >> > > > random > >> > > > > suggestions). > >> > > > > > >> > > > > Comments/thoughts? > >> > > > > > >> > > > > -- > >> > > > > Cheers > >> > > > > > >> > > > > Jinmei > >> > > > > > >> > > > > >> > > > >> > > >> > >> > >> -- > >> Cheers > >> > >> Jinmei > >> > > > -- Cheers Jinmei
