New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider HTTP host configuration when creating notifications #1142

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@flavio

flavio commented Oct 29, 2015

Use the HTTP host value specified by the user when building notification events. This value has now a higher priority when building the "source" part of the event notifications.

This is required when running the registry behind a web server like Apache. Without this code all the notifications would have the source set to "registry.example.com:5000" or "127.0.0.1:5000" instead of the FQDN exposed by Apache (i.e. "registry.example.com"). We found that issue when working on a special deployment scenario of Portus (see here for more details).

I would like to hear your opinion about this PR. If you like the change I can look into extending the test suite.

Consider HTTP host configuration when creating notifications
Use the HTTP host value specified by the user when building
notification events. This value has now a higher priority when building
the "source" part of the event notifications.

This is required when running the registry behind a web server like
Apache. Without this code all the notifications would have the source
set to "registry.example.com:5000" or "127.0.0.1:5000" instead of the
FQDN exposed by Apache (i.e. "registry.example.com").

Signed-off-by: Flavio Castelli <fcastelli@suse.com>
@RichardScothern

This comment has been minimized.

Show comment
Hide comment
@RichardScothern

RichardScothern Oct 29, 2015

Contributor

LGTM. @stevvooe ?

Contributor

RichardScothern commented Oct 29, 2015

LGTM. @stevvooe ?

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Oct 30, 2015

Contributor

@flavio This change is not correct. Portus is using the wrong field from the notification.

The hostname field of the event source is supposed to be the actual host from which the event was generated. This allows one to identify which host generated an event in a registry cluster. This is clearly documented here.

The external facing url should be available as part of the event target. You could also use the Request portion of the event record to get the host to which the registry is bound.

Contributor

stevvooe commented Oct 30, 2015

@flavio This change is not correct. Portus is using the wrong field from the notification.

The hostname field of the event source is supposed to be the actual host from which the event was generated. This allows one to identify which host generated an event in a registry cluster. This is clearly documented here.

The external facing url should be available as part of the event target. You could also use the Request portion of the event record to get the host to which the registry is bound.

@stevvooe stevvooe closed this Oct 30, 2015

@flavio

This comment has been minimized.

Show comment
Hide comment
@flavio

flavio Oct 30, 2015

This how the event looks like:

{
  "events":[
    {
      "id":"dff135fc-b470-4039-8386-9467fc8bc4ff",
      "timestamp":"2015-10-30T21:08:40.527426104Z",
      "action":"push",
      "target":{
        "mediaType":"application/vnd.docker.distribution.manifest.v1+json",
        "size":3212,
        "digest":"sha256:04a3856fbec111bfcb41e8cef86f7a7f7069487f156050738e4d15c601d867ee",
        "length":3212,
        "repository":"flavio/busybox",
        "url":"http://docker.suse.con/v2/flavio/busybox/manifests/sha256:04a3856fbec111bfcb41e8cef86f7a7f7069487f156050738e4d15c601d867ee"
      },
      "request":{
        "id":"9fa81403-8f4f-46cb-8115-30f2cb9cf61d",
        "addr":"127.0.0.1",
        "host":"127.0.0.1:5000",
        "method":"PUT",
        "useragent":"docker/1.8.2 go/go1.4.2 git-commit/0a8c2e3 kernel/3.12.44-52.18-default os/linux arch/amd64"
      },
      "actor":{
        "name":"flavio"
      },
      "source":{
        "addr":"docker:5000",
        "instanceID":"bb56ee23-99a8-4886-b7fa-faf30a2a59c1"
      }
    }
  ]
}

As you can see source has addr set to "hostname:port". This is wrong with this deployment because the registry is accessible on port 443. If you want I can change the patch to simply not add the port number.

flavio commented Oct 30, 2015

This how the event looks like:

{
  "events":[
    {
      "id":"dff135fc-b470-4039-8386-9467fc8bc4ff",
      "timestamp":"2015-10-30T21:08:40.527426104Z",
      "action":"push",
      "target":{
        "mediaType":"application/vnd.docker.distribution.manifest.v1+json",
        "size":3212,
        "digest":"sha256:04a3856fbec111bfcb41e8cef86f7a7f7069487f156050738e4d15c601d867ee",
        "length":3212,
        "repository":"flavio/busybox",
        "url":"http://docker.suse.con/v2/flavio/busybox/manifests/sha256:04a3856fbec111bfcb41e8cef86f7a7f7069487f156050738e4d15c601d867ee"
      },
      "request":{
        "id":"9fa81403-8f4f-46cb-8115-30f2cb9cf61d",
        "addr":"127.0.0.1",
        "host":"127.0.0.1:5000",
        "method":"PUT",
        "useragent":"docker/1.8.2 go/go1.4.2 git-commit/0a8c2e3 kernel/3.12.44-52.18-default os/linux arch/amd64"
      },
      "actor":{
        "name":"flavio"
      },
      "source":{
        "addr":"docker:5000",
        "instanceID":"bb56ee23-99a8-4886-b7fa-faf30a2a59c1"
      }
    }
  ]
}

As you can see source has addr set to "hostname:port". This is wrong with this deployment because the registry is accessible on port 443. If you want I can change the patch to simply not add the port number.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Oct 30, 2015

Contributor

@flavio It is not wrong. Please read the documentation a few more times. The registry instance is running on port 5000 on a host called "docker", as returned by os.Hostname(). That is the definition of the source.addr field.

The bug here is actually that request.host is not being populated with the values from configuration.HTTP.Host or using the proxy values X-Forwarded-For.

Contributor

stevvooe commented Oct 30, 2015

@flavio It is not wrong. Please read the documentation a few more times. The registry instance is running on port 5000 on a host called "docker", as returned by os.Hostname(). That is the definition of the source.addr field.

The bug here is actually that request.host is not being populated with the values from configuration.HTTP.Host or using the proxy values X-Forwarded-For.

@rennhak

This comment has been minimized.

Show comment
Hide comment
@rennhak

rennhak commented Apr 5, 2016

+1

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Apr 6, 2016

Contributor

@rennhak What do you intend to achieve by adding "+1" to a six month old closed issue?

Contributor

stevvooe commented Apr 6, 2016

@rennhak What do you intend to achieve by adding "+1" to a six month old closed issue?

@rennhak

This comment has been minimized.

Show comment
Hide comment
@rennhak

rennhak Apr 6, 2016

@stevvooe sorry, had no time when I made that entry. Apologies for the hassle.

Issue is that we encountered the exactly same problem as @flavio pointed out when trying to get Portus up and running with FQDN setup. The developer here (sergio) who was working on this has given me the same comment as @flavio pointed out. So, I'm not sure actually where the problem lies (docker vs portus). He commented also that he thinks its in docker as well.

Will ask him to comment here to see if we can pinpoint where exactly the issue lies.

rennhak commented Apr 6, 2016

@stevvooe sorry, had no time when I made that entry. Apologies for the hassle.

Issue is that we encountered the exactly same problem as @flavio pointed out when trying to get Portus up and running with FQDN setup. The developer here (sergio) who was working on this has given me the same comment as @flavio pointed out. So, I'm not sure actually where the problem lies (docker vs portus). He commented also that he thinks its in docker as well.

Will ask him to comment here to see if we can pinpoint where exactly the issue lies.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Apr 6, 2016

Contributor

@rennhak No worries. I just wanted to make sure whatever problem you're experiencing gets resolved.

We have done some fixes in the detection of request values. Perhaps, we need another issue to ensure that the right values are finding their way into request. For the example above, the value of host should probably be "docker.suse.con".

Contributor

stevvooe commented Apr 6, 2016

@rennhak No worries. I just wanted to make sure whatever problem you're experiencing gets resolved.

We have done some fixes in the detection of request values. Perhaps, we need another issue to ensure that the right values are finding their way into request. For the example above, the value of host should probably be "docker.suse.con".

@sergioasantiago

This comment has been minimized.

Show comment
Hide comment
@sergioasantiago

sergioasantiago Apr 6, 2016

hey @stevvooe I've just exposed our case in Portus issue since as you said, it seems not to be an issue in docker registry. So let's see if they agree with my statements and example, and wait for their feedback.
Thanks in advance for your promptly support.

sergioasantiago commented Apr 6, 2016

hey @stevvooe I've just exposed our case in Portus issue since as you said, it seems not to be an issue in docker registry. So let's see if they agree with my statements and example, and wait for their feedback.
Thanks in advance for your promptly support.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Apr 6, 2016

Contributor

@sergioasantiago It looks like an issue with resolving the http host between the two systems. This could be a configuration issue or a bug. Keep us informed.

If we still think there is a bug with the request.host field, let's open up another issue to get that resolved.

Contributor

stevvooe commented Apr 6, 2016

@sergioasantiago It looks like an issue with resolving the http host between the two systems. This could be a configuration issue or a bug. Keep us informed.

If we still think there is a bug with the request.host field, let's open up another issue to get that resolved.

@santosh0705

This comment has been minimized.

Show comment
Hide comment
@santosh0705

santosh0705 Apr 22, 2016

The issue @flavio reported is valid and seems like its a necessity for registry management app like Portus which is designed to manage more than one docker registry.

Suppose we have 2 registry clusters with n numbers of registry servers in each cluster and having load balancer and ssl reverse proxy. The clusters are accessible on registery1.xyz.com and registry2.xyz.com without any port number (registry servers in each cluster may be running on custom ports). For both the clusters a single endpoint is configured to receive the notifications from all the registry servers. Here the endpoint cannot distinguish the received notification belongs to which cluster by simply reading the hostname port, same hostname port or ip port can be used by servers in both the cluster.

There should me some mechanism in the notifications to distinguish for which cluster it belongs to. For example in token based authentication we use service name. Similarly we can have some tag in the notifications too.

santosh0705 commented Apr 22, 2016

The issue @flavio reported is valid and seems like its a necessity for registry management app like Portus which is designed to manage more than one docker registry.

Suppose we have 2 registry clusters with n numbers of registry servers in each cluster and having load balancer and ssl reverse proxy. The clusters are accessible on registery1.xyz.com and registry2.xyz.com without any port number (registry servers in each cluster may be running on custom ports). For both the clusters a single endpoint is configured to receive the notifications from all the registry servers. Here the endpoint cannot distinguish the received notification belongs to which cluster by simply reading the hostname port, same hostname port or ip port can be used by servers in both the cluster.

There should me some mechanism in the notifications to distinguish for which cluster it belongs to. For example in token based authentication we use service name. Similarly we can have some tag in the notifications too.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Apr 27, 2016

Contributor

@santosh0705 I covered why this was closed in #1142 (comment). As I said before:

The bug here is actually that request.host is not being populated with the values from configuration.HTTP.Host or using the proxy values X-Forwarded-For.

While the behavior may be a valid issue, the fix proposed here is not. The source.addr field has a specific definition and should not be changed. If you're curious about the roles of the fields, the documentation for the fields are available in notifications.Event.

If this is still affecting you, please submit an issue or PR with the correct fix.

Contributor

stevvooe commented Apr 27, 2016

@santosh0705 I covered why this was closed in #1142 (comment). As I said before:

The bug here is actually that request.host is not being populated with the values from configuration.HTTP.Host or using the proxy values X-Forwarded-For.

While the behavior may be a valid issue, the fix proposed here is not. The source.addr field has a specific definition and should not be changed. If you're curious about the roles of the fields, the documentation for the fields are available in notifications.Event.

If this is still affecting you, please submit an issue or PR with the correct fix.

@flavio

This comment has been minimized.

Show comment
Hide comment
@flavio

flavio Apr 28, 2016

Sorry, I have been pretty busy with other stuff. I'll update the PR in the next days.

flavio commented Apr 28, 2016

Sorry, I have been pretty busy with other stuff. I'll update the PR in the next days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment