Re: Proposal: Add appId optional paramater to deploy api

Wed, 26 Jul 2017 23:24:59 -0700 

Hi Alex,
I explored setting a config key in my PR. The downsides of that compared to setting the app-id: 


1. Code is more complicated - in particular, there are very low-level 
changes to check that the uid is not already in use.
2. Config keys (and tags) are mutable - we can't enforce the proposed 
semantics of the deploymentUid.
3. You can always rely on looking up an entity's id, but config is more 
complicated and can be "resolved" - leads to more complicated code (e.g. 
my PR currently has test-failures for hot-standby).
4. Appropriate search not supported in the REST api (would require 
additional changes - i.e. more work).
5. Server-side searching would either be inefficient or require a new 
index to be exposed (e.g. low-level changes in `EntityManager`).

6. Adds another concept - a new kind of id.

---

By "camp.id", do you mean "camp.plan.id"? (there is also 
"camp.template.id", but I can't find a "camp.id".)



I don't think we should mix this up with that existing concept, which is 
set and used in a completely different way:
* One can already set a camp id on the top-level app, and reference it 
with `$brooklyn:entity(...)` - we don't want to break that.
* The camp id does not have to be unique - changing that would break 
backwards compatibility.
* Existing catalog items can set the camp id, which are used by the app 
instance - e.g. see example in the appendix.


---

What are your main concerns about allowing the id to be set (with the 
regex validation that Mark suggested)? The reason that the is used by 
other internal parts of Brooklyn seems too vague to me. For example, is it:
1. Security (e.g. if we didn't validate, then it would risk allowing 
code injection).
2. Makes future changes harder (but the sorts of changes I envisage I 
can also see how we could handle).

3. Point of principle to keep internal things internal wherever possible.

4. Risks breaking places that we use the id in strange ways (e.g. if an 
entity uses the id to generate a dns name, then it implies 
case-insensitivity for the uniqueness check).



That last concern is real - I recall that Andrew Kennedy changed our ids 
to be lower case for that reason!



However, I don't think we should have given such "undocumented 
guarantees" on the internal id format. But given some entities rely on 
it, we can be stricter with the validation regex of user-supplied ids.


Aled

======= Appendix =======

Example of setting a `camp.plan.id` - we don't want to subtly change the 
semantics of this.


Add this to the catalog:
```
brooklyn.catalog:
  id: app-with-camp-plan-id
  version: 0.0.0-SNAPSHOT
  itemType: template
  item:
    services:
      - type: org.apache.brooklyn.entity.stock.BasicApplication
        id: myAppCampPlanId
```

Deploy this app:
```
services:
  - type: app-with-camp-plan-id
```


The resulting app instance will have config `camp.plan.id` with value 
`myAppCampPlanId`.




On 27/07/2017 00:40, Alex Heneveld wrote:

The core `id` is a low-level part of `BrooklynObject` used by all adjuncts
and entities and persistence.  It feels wrong and risky making this
something that is user- or client- settable.  I gave one example but there
are others.

What's wrong with a new config key or reusing `camp.id`?  We already use
the latter one if there is a user-specified ID on an entity so it feels
natural to use it, give it special meaning for apps (blocking repeat
deployments), and add support for searching for it.  (Apologies if this was
explained and I missed it.)

--A


On 26 July 2017 at 22:42, Aled Sage <[email protected]> wrote:


Hi Alex,

Other things get a lot simpler for us if we can just supply the app-id
(e.g. subsequently searching for the app, or ensuring that a duplicate app
is not deployed). It also means we're not adding another concept that we
need to explain to users.

To me, that simplicity is very compelling.

If we want to support conformance to external id requirements, we could
have a config key for a predicate or regex that the supplied id must
satisfy. A given user could thus enforce their id standards in the future.
I'd favour deferring that until there is a need to support it (e.g. we
could add it at the same time as adding support for a pluggable id
generator, if we ever do that).

Aled



On 26/07/2017 15:42, Alex Heneveld wrote:


2 feels compelling to me. I want us to have the ability easily to change
the ID generation eg to conform with external reqs such as timestamp or
source.

Go with deploymentUid or similar? Or camp.id?

Best
Alex

On 26 Jul 2017 15:00, "Aled Sage" <[email protected]> wrote:

Hi Mark,

We removed from EntitySpec the ability to set the id for two reasons:

1. there was no use-case at that time; simplifying the code by deleting it
was therefore sensible - we'd deprecated it for several releases.

2. allowing all uids to be generated/managed internally is simpler to
reason about, and gives greatest freedom for future refactoring.


I don't see (2) as a compelling reason.  And we now have a use-case, so
that changes (1).

I'd still be tempted to treat this as an internal part of the api, rather
than it going on the public `EntitySpec`, but need to look at that more to
see how feasible it is.

Aled



On 26/07/2017 13:36, Mark Mc Kenna wrote:

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































					Reply via email to