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

Route termination options follow public url scheme #265

Merged
merged 3 commits into from Mar 27, 2020

Conversation

hallelujah
Copy link
Contributor

@hallelujah hallelujah commented Oct 22, 2019

If public route was created with http: scheme, do not enforce secure route edge redirect

Fixes THREESCALE-3545

TODO

  • Add tests for RouteSpec
  • Test in live OpenShift

Issues/Questions

The following does not prevent the merge of this PR but we should take into account that when the environment variable KUBERNETES_ROUTE_TLS is set to 1, t or true then it will prevent creating
non secure routes, which might be an issue as Porta has something different from OpenShift

This prevents removing the :tls option

class_attribute :maintain_tls_spec,
default: ActiveModel::Type::Boolean.new.cast(ENV['KUBERNETES_ROUTE_TLS'])

It is used in

def update_resource(existing, resource)
resource.spec.delete_field(:tls) if maintain_tls_spec?
client.merge_resource(existing, resource)
rescue K8s::Error::Invalid
resource.spec.tls = existing.spec.tls if maintain_tls_spec?
client.delete_resource(existing)
client.create_resource(resource)
end

But the integration service builds all routes for both staging and production endpoint, so disabling the maintain_tls_spec for the integration service is not possible because for example, staging route can be HTTP and production route can be HTTPS

def build_proxy_routes(entry)
build_routes('zync-3scale-api-', [
RouteSpec.new(entry.data.fetch('endpoint'), 'apicast-production', 'gateway'),
RouteSpec.new(entry.data.fetch('sandbox_endpoint'), 'apicast-staging', 'gateway')
], labels: labels_for_proxy(entry), annotations: annotations_for(entry))
end

Openshift testing

Using this Build config

apiVersion: v1
kind: Template
metadata:
  name: "zync-build"
message: "3scale AMP zync builder"
objects:

- kind: "BuildConfig"
  apiVersion: "v1"
  metadata:
    name: "zync-build"
  spec:
    source:
      git:
        uri: "https://github.com/3scale/zync"
        ref: "THREESCALE-3545"
    output:
      to:
        kind: "ImageStreamTag"
        name: "amp-zync:latest"

    strategy:
      type: Docker
      dockerStrategy:

Screenshots

image

image

Caveats

Due to OpenShift router limitation, the HTTP and HTTPS ports are automatically set by OpenShift
If you use different ports than ROUTER_SERVICE_HTTP_PORT and ROUTER_SERVICE_HTTPS_PORT, they are not taken into account

@hallelujah hallelujah changed the title Route termination options follow public url scheme [WIP] Route termination options follow public url scheme Oct 22, 2019
@hallelujah hallelujah force-pushed the THREESCALE-3545 branch 4 times, most recently from c612486 to e370fd0 Compare October 28, 2019 14:10
@hallelujah hallelujah changed the title [WIP] Route termination options follow public url scheme Route termination options follow public url scheme Oct 28, 2019
Copy link
Contributor

@duduribeiro duduribeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

to: {
kind: 'Service',
name: service
}
})
}.merge(tls: tls_options))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it OK to merge tls: nil if uri.class != URI::HTTPS?

Copy link
Contributor Author

@hallelujah hallelujah Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is what k8s expect, there is a test for it

Anyway this PR is in conflict with the KUBERNETES_ROUTE_TLS env
I will think of a way to make it compatible later

EDIT: Actually you are right:
There might be other secure protocol that could be used but not supported right now. AFAIK we only accept HTTP and HTTPS

@hallelujah hallelujah changed the title Route termination options follow public url scheme [wip] Route termination options follow public url scheme Nov 6, 2019
@hallelujah hallelujah changed the title [wip] Route termination options follow public url scheme Route termination options follow public url scheme Mar 27, 2020
If public route was created with `http:` scheme, do not enforce secure route edge redirect

Fixes THREESCALE-3545
Fixing:

Error: Database is uninitialized and superuser password is not specified.
       You must specify POSTGRES_PASSWORD to a non-empty value for the
       superuser. For example, "-e POSTGRES_PASSWORD=password" on "docker run".

       You may also use "POSTGRES_HOST_AUTH_METHOD=trust" to allow all
       connections without a password. This is *not* recommended.

       See PostgreSQL documentation about "trust":
       https://www.postgresql.org/docs/current/auth-trust.html
This happens because we create a transaction, make a query then set transaction isolation level:

BEGIN
INSERT INTO ....
SET TRANSACTION ISOLATION LEVEL

To avoid that, isolate transaction aware tests into its own class

  1) Error:
Prometheus::QueStatsTest#test_readonly_transaction:
ActiveRecord::StatementInvalid: PG::ActiveSqlTransaction: ERROR:  SET TRANSACTION ISOLATION LEVEL must be called before any query
: SET TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/postgresql/database_statements.rb:75:in `exec'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/postgresql/database_statements.rb:75:in `block (2 levels) in execute'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activesupport-5.2.3/lib/active_support/dependencies/interlock.rb:48:in `block in permit_concurrent_loads'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activesupport-5.2.3/lib/active_support/concurrency/share_lock.rb:187:in `yield_shares'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activesupport-5.2.3/lib/active_support/dependencies/interlock.rb:47:in `permit_concurrent_loads'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/postgresql/database_statements.rb:74:in `block in execute'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract_adapter.rb:581:in `block (2 levels) in log'
    /usr/local/lib/ruby/2.4.0/monitor.rb:214:in `mon_synchronize'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract_adapter.rb:580:in `block in log'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activesupport-5.2.3/lib/active_support/notifications/instrumenter.rb:23:in `instrument'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract_adapter.rb:571:in `log'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/postgresql/database_statements.rb:73:in `execute'
    /home/circleci/zync/lib/prometheus/que_stats.rb:41:in `block in execute'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/database_statements.rb:267:in `block in transaction'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/transaction.rb:239:in `block in within_new_transaction'
    /usr/local/lib/ruby/2.4.0/monitor.rb:214:in `mon_synchronize'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/transaction.rb:236:in `within_new_transaction'
    /home/circleci/zync/vendor/bundle/ruby/2.4.0/gems/activerecord-5.2.3/lib/active_record/connection_adapters/abstract/database_statements.rb:267:in `transaction'
    /home/circleci/zync/lib/prometheus/que_stats.rb:39:in `execute'
    /home/circleci/zync/lib/prometheus/que_stats.rb:15:in `worker_stats'
    /home/circleci/zync/test/lib/prometheus/que_stats_test.rb:28:in `test_readonly_transaction'
Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple things seem not to belong to this PR, but OK. I guess you needed them along the process.

@hallelujah
Copy link
Contributor Author

hallelujah commented Mar 27, 2020

I need them to make it pass tests, and really opening 2 other PR is not going to happen :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants