Thanks Geoff for the good summary
IMO the path of least resistance that provides the best / most predictable
behaviour is allowing clients to optionally set the app id.
Off the top of my head I cant see this causing any issue, as long as we
sanitise the supplied id something like [a-zA-Z0-9-]{8,}.
Was there a particular reason this was removed in the past?
Cheers
M
On 26 July 2017 at 13:07, Duncan Grant <[email protected]>
wrote:
> Thanks all for the advice.
>
> I think Geoff's email summarises the issue nicely. I like Alex's
> suggestion but agree that limiting ourselves to deploy in the first is
> probably significantly easier.
>
> Personally I don't feel comfortable with using a tag for idempotency and I
> do like Aled's suggestion of using PUT with a path with /{id} but would be
> happy with either as I think they both fit our use case.
>
> thanks
>
> Duncan
>
> On Wed, 26 Jul 2017 at 11:00 Geoff Macartney <[email protected]
> >
> wrote:
>
> > If I understand correctly this isn't quite what Duncan/Aled are asking
> for
> > though?
> > Which is not a "request id" but an idempotency token for an application.
> It
> > would
> > need to work long term, not just cached for a short time, and it would
> need
> > to work
> > across e.g. HA failover, so it wouldn't be just a matter of caching it on
> > the server.
> >
> > For what it's worth, I'd have said this is a good use for tags but maybe
> > for ease of reading
> > it's better to have it as a config key as Aled does. As to how to supply
> > the value
> > I agree it should just be on the "deploy" operation.
> >
> > On Tue, 25 Jul 2017 at 19:56 Alex Heneveld <
> > [email protected]>
> > wrote:
> >
> > > Aled-
> > >
> > > Should this be applicable to all POST/DELETE calls? Imagine an
> > > `X-caller-request-uid` and a filter which caches them server side for a
> > > short period of time, blocking duplicates.
> > >
> > > Solves an overlapping set of problems. Your way deals with a
> > > "deploy-if-not-present" much later in time.
> > >
> > > --A
> > >
> > > On 25 July 2017 at 17:44, Aled Sage <[email protected]> wrote:
> > >
> > > > Hi all,
> > > >
> > > > I've been exploring adding support for `&deploymentUid=...` - please
> > see
> > > > my work-in-progress PR [1].
> > > >
> > > > Do people think that is a better or worse direction than supporting
> > > > `&appId=...` (which would likely be simpler code, but exposes the
> > > Brooklyn
> > > > internals more).
> > > >
> > > > For `&appId=...`, we could either revert [2] (so we could set the id
> in
> > > > the EntitySpec), or we could inject it via a different (i.e. add a
> new)
> > > > internal way so that it isn't exposedon our Java api classes.
> > > >
> > > > Thoughts?
> > > >
> > > > Aled
> > > >
> > > > [1] https://github.com/apache/brooklyn-server/pull/778
> > > >
> > > > [2] https://github.com/apache/brooklyn-server/pull/687/commits/5
> > > > 5eb11fa91e9091447d56bb45116ccc3dc6009df
> > > >
> > > >
> > > >
> > > > On 07/07/2017 18:28, Aled Sage wrote:
> > > >
> > > >> Hi,
> > > >>
> > > >> Taking a step back to justify why this kind of thing is really
> > > >> important...
> > > >>
> > > >> This has come up because we want to call Brooklyn in a robust way
> from
> > > >> another system, and to handle a whole load of failure scenarios
> (e.g.
> > > that
> > > >> Brooklyn is temporarily down, connection fails at some point during
> > the
> > > >> communication, the client in the other system goes down and another
> > > >> instance tries to pick up where it left off, etc).
> > > >>
> > > >> Those kind of thing becomes much easier if you can make certain
> > > >> assumptions such as an API call being idempotent, or it guaranteeing
> > to
> > > >> fail with a given error if that exact request has already been
> > accepted.
> > > >>
> > > >> ---
> > > >>
> > > >> I much prefer the semantics of the call failing (with a meaningful
> > > error)
> > > >> if the app already exists - that will make retry a lot easier to do
> > > safely.
> > > >>
> > > >> As for config keys on the app, in Duncan's use-case he'd much prefer
> > to
> > > >> not mess with the user's YAML (e.g. to inject another config key
> > before
> > > >> passing it to Brooklyn). It would be simpler in his case to supply
> in
> > > the
> > > >> url `?appId=...` or `?deploymentId=...`.
> > > >>
> > > >> For using `deploymentId`, we could but that feels like more work.
> We'd
> > > >> want create a lookup of applications indexed by `deploymentId` as
> well
> > > as
> > > >> `appId`, and to fail if it already exists. Also, what if someone
> also
> > > >> defines a config key called `deploymentId` - would that be
> forbidden?
> > Or
> > > >> would we name-space the config key with
> > > `org.apache.brooklyn.deploymentId`?
> > > >> Even with those concerns, I could be persuaded of the
> > > >> `org.apache.brooklyn.deploymentId` approach.
> > > >>
> > > >> For "/application's ID is not meant to be user-supplied/", that has
> > > >> historically been the case but why should we stick to that? What
> > > matters is
> > > >> that the appId is definitely unique. That will be checked when
> > creating
> > > the
> > > >> application entity. We could also include a regex check on the
> > supplied
> > > id
> > > >> to make sure it looks reasonable (in case someone is already relying
> > on
> > > app
> > > >> ids in weird ways like for filename generations, which would lead
> to a
> > > risk
> > > >> of script injection).
> > > >>
> > > >> Aled
> > > >>
> > > >>
> > > >> On 07/07/2017 17:38, Svetoslav Neykov wrote:
> > > >>
> > > >>> Hi Duncan,
> > > >>>
> > > >>> I've solved this problem before by adding a caller generated config
> > key
> > > >>> on the app (now it's also possible to tag them), then iterating
> over
> > > the
> > > >>> deployed apps, looking for the key.
> > > >>>
> > > >>> An alternative which I'd like to mention is creating an async
> deploy
> > > >>> operation which immediately returns an ID generated by Brooklyn.
> > > There's
> > > >>> still a window where the client connection could fail though,
> however
> > > small
> > > >>> it is, so it doesn't fully solve your use case.
> > > >>>
> > > >>> Your use case sounds reasonable so agree a solution to it would be
> > nice
> > > >>> to have.
> > > >>>
> > > >>> Svet.
> > > >>>
> > > >>>
> > > >>> On 7.07.2017 г., at 18:33, Duncan Grant <
> > > [email protected]>
> > > >>>> wrote:
> > > >>>>
> > > >>>> I'd like to propose adding an appId parameter to the deploy
> > endpoint.
> > > >>>> This
> > > >>>> would be optional and would presumably reject any attempt to
> start a
> > > >>>> second
> > > >>>> app with the same id. If set the appId would obviously be used in
> > > >>>> place of
> > > >>>> the generated id.
> > > >>>>
> > > >>>> This proposal would be of use in scripting deployments in a
> > > distributed
> > > >>>> environment where deployment is not the first step in a number of
> > > >>>> asynchronous jobs and would give us a way of "connecting" those
> jobs
> > > up.
> > > >>>> Hopefully it will help a lot in making things more robust for
> > > end-users.
> > > >>>> Currently, if the client’s connection to the Brooklyn server fails
> > > while
> > > >>>> waiting for a response, it’s impossible to tell if the app was
> > > >>>> provisioned
> > > >>>> (e.g. how can you tell the difference between a likely-looking
> app,
> > > and
> > > >>>> another one deployed with an identical blueprint?). This would
> make
> > it
> > > >>>> safe
> > > >>>> to either retry the deploy request, or to query for the app with
> the
> > > >>>> expected id to see if it exists.
> > > >>>>
> > > >>>> Initially I'm hoping to use this in a downstream project but I
> think
> > > >>>> this
> > > >>>> would be useful to others.
> > > >>>>
> > > >>>> If no one has objections I'll aim to implement this over the next
> > > >>>> couple of
> > > >>>> weeks. On the other hand I'm totally open to suggestions of a
> > better
> > > >>>> approach.
> > > >>>>
> > > >>>> Thanks
> > > >>>>
> > > >>>> Duncan Grant
> > > >>>>
> > > >>>
> > > >>
> > > >>
> > > >
> > >
> >
>
