Skip to content
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

information request: long polling seem's bugged on 304 http status code. #3923

Closed
floriangasc opened this issue Oct 25, 2021 · 2 comments · Fixed by #3926
Closed

information request: long polling seem's bugged on 304 http status code. #3923

floriangasc opened this issue Oct 25, 2021 · 2 comments · Fixed by #3926
Labels
distribution Issues related to the bundle plugin

Comments

@floriangasc
Copy link
Contributor

floriangasc commented Oct 25, 2021

Expected Behavior

  • long polling switch to regular polling in case of 304 http status code.
  • this behavior come from line 340 of download.go: Content-type Header is not in reponse in case of 304 (according https://datatracker.ietf.org/doc/html/rfc7232#section-4.1). so «longPoll» switch from true to false.
  • Same un case of error occured, longPoll is lost.

Actual Behavior

304 http status without content-type header should not change mode from long polling to regular polling.

Steps to Reproduce the Problem

the etag of foo.tar.gz is : "61751cfa-2c61e2"

opa polling config:

services:
  - name: open-policy-agent-registry
    url: http://127.0.0.1:8075/opa-registry-bundle/

bundles:
  authz:
    service: open-policy-agent-registry
    resource: foo-bundle.tar.gz
    polling:
      long_polling_timeout_seconds: 60

opa bundle server (i use openresty)

   lua_shared_dict revision_identifiers 10m;
    init_by_lua_block {
      ngx.shared.revision_identifiers:set("current_revision_identifier", '61751cfa-2c61e2')
    }

    server  {
    listen  8075 ;
        location ~ ^/opa-registry-bundle-revision/(?<revision_identifier>[^/]+) {
           access_by_lua_block {
              local revision_identifier = ngx.var['revision_identifier']
              local revision_identifiers = ngx.shared.revision_identifiers
              revision_identifiers:set("current_revision_identifier", revision_identifier)
           }
        }

        location /opa-registry-bundle/ {
          access_by_lua_block {
            local h = ngx.req.get_headers()
            local prefer_header = h['prefer']
            local etag = h['if-none-match']
            if prefer_header then
              local wait_in_second=string.match(prefer_header, "wait=([%d]+)")
              if wait_in_second then
                local wait_in_second_number = tonumber(wait_in_second)
                if wait_in_second_number then
                  local revision_identifiers = ngx.shared.revision_identifiers
                  local i = 0
                  while i < wait_in_second_number do
                    local revision_identifier = revision_identifiers:get("current_revision_identifier")
                    if revision_identifier == nil or  etag == nil then break end
                    local new_bundle = (revision_identifier == nil and etag == nil) or  '"'..revision_identifier..'"' ~= etag
                    if new_bundle then
                       ngx.header['Etag'] = '"'..revision_identifier..'"'
                       break;
                    end
                    ngx.sleep(2);
                    i = i + 2
                  end
                end
              end
             end
          }
           alias /tmp/FOO/;
           types { } default_type "application/vnd.openpolicyagent.bundles";
        }
    }

Additional Info

For solve issue i have

  • initialize longPoll in loop function
longPoll := false
if d.config.Polling.LongPollingTimeoutSeconds != nil {
        longPoll = true
}
  • pass longPoll var to oneShot and and download function
  • hange go longPoll: isLongPollSupported(resp.Header), to longPool : longPoll in case of 304 status in download function.

question:

  • if it's not a bug, but wanted behavior, sorry for this issue. And i have missed something for use long polling ? 
  • If it's a bug, i make a pull request (it's quick and dirty fix, and i not found yet better way)
floriangasc added a commit to floriangasc/opa that referenced this issue Oct 25, 2021
Signed-off-by: Gasc Florian <florian.gasc@gmail.com>
Fixes: open-policy-agent#3923
floriangasc added a commit to floriangasc/opa that referenced this issue Nov 21, 2021
…ling

download/download_test.go: testing about oneShot and download method

When 304 content-type should not be send. Fix the download code for switch to regular polling and keep the previous value.

Fixes: open-policy-agent#3923
Signed-off-by: Gasc Florian <florian.gasc@gmail.com>
floriangasc added a commit to floriangasc/opa that referenced this issue Nov 22, 2021
…ling

download/download_test.go: testing about oneShot and download method

Fixes: open-policy-agent#3923
Signed-off-by: Gasc Florian <florian.gasc@gmail.com>
floriangasc added a commit to floriangasc/opa that referenced this issue Nov 22, 2021
…ling

download/download_test.go: testing about oneShot and download method

When 304 content-type should not be send. Fix the download code for switch to regular polling and keep the previous value.

Fixes: open-policy-agent#3923
Signed-off-by: Gasc Florian <florian.gasc@gmail.com>
@stale
Copy link

stale bot commented Nov 24, 2021

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Nov 24, 2021
@ashutosh-narkar
Copy link
Member

PR# #3926

@stale stale bot removed the inactive label Nov 24, 2021
@tsandall tsandall added the distribution Issues related to the bundle plugin label Dec 3, 2021
srenatus pushed a commit that referenced this issue Dec 10, 2021
…TTP status (#3926)

Previously a 304 HTTP status code would make the downloader switch
from long polling to regular polling. This is not desired, as it's in line with
the HTTP spec not to have a Content-Type header for 304 responses.
The absent Content-Type header had triggered the switch back to regular
polling.

Fixes: #3923

Signed-off-by: Gasc Florian <florian.gasc@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distribution Issues related to the bundle plugin
Projects
None yet
3 participants