You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by tbouron <gi...@git.apache.org> on 2016/10/11 15:51:50 UTC
[GitHub] brooklyn-library pull request #66: Add more sensors for main.uri, based on t...
GitHub user tbouron opened a pull request:
https://github.com/apache/brooklyn-library/pull/66
Add more sensors for main.uri, based on the context, i.e public or not
Create 2 new sensors: `main.uri.mapped.public` and `main.uri.mapped.subnet`
Requires https://github.com/apache/brooklyn-server/pull/378
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tbouron/brooklyn-library fix/main-uri
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-library/pull/66.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #66
----
commit cbae2ac0f461e10d7347bf0eda0fe68f21a7809f
Author: Thomas Bouron <th...@cloudsoftcorp.com>
Date: 2016-10-11T15:50:03Z
Add more sensors for main.uri, based on the context, i.e public or not
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-library pull request #66: Add more sensors for main.uri, based on t...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-library/pull/66
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-library pull request #66: Add more sensors for main.uri, based on t...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/66#discussion_r82864446
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/AbstractControllerImpl.java ---
@@ -317,6 +331,8 @@ protected void computePortsAndUrls() {
sensors().set(PROTOCOL, inferProtocol());
sensors().set(MAIN_URI, URI.create(inferUrl()));
+ sensors().set(MAIN_URI_MAPPED_SUBNET, URI.create("http://" + sensors().get(SUBNET_ADDRESS) + ":" + sensors().get(PROXY_HTTP_PORT) + "/"));
+ sensors().set(MAIN_URI_MAPPED_PUBLIC, URI.create(inferUrlPublic()));
--- End diff --
@neykov Will the `On{Public,Private}NetworkEnricher` returns the right value? The changes I made was an attempt to follow @ahgittin's proposal
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-library pull request #66: Add more sensors for main.uri, based on t...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/66#discussion_r82855389
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/AbstractControllerImpl.java ---
@@ -317,6 +331,8 @@ protected void computePortsAndUrls() {
sensors().set(PROTOCOL, inferProtocol());
sensors().set(MAIN_URI, URI.create(inferUrl()));
+ sensors().set(MAIN_URI_MAPPED_SUBNET, URI.create("http://" + sensors().get(SUBNET_ADDRESS) + ":" + sensors().get(PROXY_HTTP_PORT) + "/"));
+ sensors().set(MAIN_URI_MAPPED_PUBLIC, URI.create(inferUrlPublic()));
--- End diff --
Shouldn't we use `On{Public,Private}NetworkEnricher` instead?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-library issue #66: Add more sensors for main.uri, based on the cont...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-library/pull/66
Thanks @tbouron - merging now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-library pull request #66: Add more sensors for main.uri, based on t...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/66#discussion_r82832610
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/AbstractControllerImpl.java ---
@@ -317,6 +331,8 @@ protected void computePortsAndUrls() {
sensors().set(PROTOCOL, inferProtocol());
sensors().set(MAIN_URI, URI.create(inferUrl()));
+ sensors().set(MAIN_URI_MAPPED_SUBNET, URI.create(inferUrl()));
--- End diff --
I was thinking we'd add an `inferUrlSubnet()` which would use `getAddress(Attributes.SUBNET_ADDRESS)` (so a similar implementation to `inferUrlPublic()`).
But maybe that is too much duplication from `inferUrl()`.
The current code in `inferUrl(false)` calls `Machines.findSubnetHostname(entity)`. On the face of it, that seems reasonable. But the implementation of `findSubnetHostname()` is a bit strange! I'd have thought we'd just want to use the sensor `Attributes.SUBNET_ADDRESS` for this. All that other code should only be executed when we are deciding how to populate the sensor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-library pull request #66: Add more sensors for main.uri, based on t...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/66#discussion_r82875579
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/AbstractControllerImpl.java ---
@@ -317,6 +331,8 @@ protected void computePortsAndUrls() {
sensors().set(PROTOCOL, inferProtocol());
sensors().set(MAIN_URI, URI.create(inferUrl()));
+ sensors().set(MAIN_URI_MAPPED_SUBNET, URI.create("http://" + sensors().get(SUBNET_ADDRESS) + ":" + sensors().get(PROXY_HTTP_PORT) + "/"));
+ sensors().set(MAIN_URI_MAPPED_PUBLIC, URI.create(inferUrlPublic()));
--- End diff --
@neykov a very good point, but for `AbstractController` it is unfortunately a special case. It wants to use the `domain` sensor for the hostname in the URL, if set. This was added for `GeoscalingDnsService` (and is also useful for things like AWS ELB).
We'd still like to advertise `MAIN_URI_MAPPED_PUBLIC`, but not with the "normal" logic. The same goes I think for `MAIN_URI_MAPPED_SUBNET`: the machine's subnet-address doesn't make sense if it's an AWS ELB that is being used. We want `MAIN_URI_MAPPED_SUBNET` to always be populated, so that other blueprints can use this without worrying about the type of load balancer.
This is probably worth further discussion. But in the mean time I suggest we merge this PR, so we get it working (important for the tutorials/examples that show a simple web-cluster being deployed to GCE and/or Azure).
I'll also look separately at improving `OnPublicNetworkEnricher` so that it can read the public IP from a sensor (rather than only working if the public mapping is registered in `PortForwardManager`, as is done for DNAT).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-library pull request #66: Add more sensors for main.uri, based on t...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/66#discussion_r82877268
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/AbstractControllerImpl.java ---
@@ -317,6 +331,8 @@ protected void computePortsAndUrls() {
sensors().set(PROTOCOL, inferProtocol());
sensors().set(MAIN_URI, URI.create(inferUrl()));
+ sensors().set(MAIN_URI_MAPPED_SUBNET, URI.create("http://" + sensors().get(SUBNET_ADDRESS) + ":" + sensors().get(PROXY_HTTP_PORT) + "/"));
+ sensors().set(MAIN_URI_MAPPED_PUBLIC, URI.create(inferUrlPublic()));
--- End diff --
@neykov Interesting approach, I didn't know about those enrichers. I did a quick test and the `OnSubnetNetworkEnricher` works whereas `OnPublicNetworkEnricher` does as @aledsage explained.
Although, using enrichers would probably be a cleaner solution, we can go back to it later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-library issue #66: Add more sensors for main.uri, based on the cont...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-library/pull/66
@aledsage Comment addressed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-library pull request #66: Add more sensors for main.uri, based on t...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/66#discussion_r82837626
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/AbstractControllerImpl.java ---
@@ -317,6 +331,8 @@ protected void computePortsAndUrls() {
sensors().set(PROTOCOL, inferProtocol());
sensors().set(MAIN_URI, URI.create(inferUrl()));
+ sensors().set(MAIN_URI_MAPPED_SUBNET, URI.create("http://" + sensors().get(SUBNET_ADDRESS) + ":" + sensors().get(PROXY_HTTP_PORT) + "/"));
--- End diff --
Don't hard-code "http" - need to call `inferProtocol()`.
Also want to guard against `SUBNET_ADDRESS` sensor not being set (e.g. for `GeoscalingDnsService`). No strong feelings whether we'd set the sensor to null, or would use `domain` instead if that is set.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-library pull request #66: Add more sensors for main.uri, based on t...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/66#discussion_r82838964
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/AbstractControllerImpl.java ---
@@ -317,6 +331,8 @@ protected void computePortsAndUrls() {
sensors().set(PROTOCOL, inferProtocol());
sensors().set(MAIN_URI, URI.create(inferUrl()));
+ sensors().set(MAIN_URI_MAPPED_SUBNET, URI.create("http://" + sensors().get(SUBNET_ADDRESS) + ":" + sensors().get(PROXY_HTTP_PORT) + "/"));
--- End diff --
That's why I was calling `inferUrl` as it does all this
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---