You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by nir-sopher <gi...@git.apache.org> on 2017/03/23 10:51:23 UTC
[GitHub] incubator-trafficcontrol pull request #402: Org tenancy - adding tenant to c...
GitHub user nir-sopher opened a pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402
Org tenancy - adding tenant to cdn
This PR adds tenant to the CDN table and adjusts the API.
It can be pulled by itself but merging it with PR-398 (adding the user tenancy) will improve the test coverage.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nir-sopher/incubator-trafficcontrol org-tenancy-cdn-table
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafficcontrol/pull/402.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 #402
----
commit 2efbc8c890d0ed4b7d86e10e521d79548a73be63
Author: nir-sopher <ni...@gmail.com>
Date: 2017-03-20T15:45:21Z
Adding tenant to the cdn table
commit d96bb8e243619d0da2e728df0de70b73f16f5e67
Author: nir-sopher <ni...@gmail.com>
Date: 2017-03-20T15:46:36Z
Cdn API: adding tenant
commit a11239852e70169dd774ccf120df1a2b2bf993b9
Author: nir-sopher <ni...@gmail.com>
Date: 2017-03-20T15:47:21Z
A tenant canot be deleted if a CDN is assigned to it
commit da2d902c9eef8beede3ade69bef89872c8943f08
Author: nir-sopher <ni...@gmail.com>
Date: 2017-03-20T15:48:43Z
CDN GUI - when adding a CDN the tenant is derived from the user
commit ec59fd724bacb8d942f1590f481dd66ba926cb96
Author: nir-sopher <ni...@gmail.com>
Date: 2017-03-20T15:49:11Z
CDN tenancy API documentation
----
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109292194
--- Diff: traffic_ops/app/lib/API/Cdn.pm ---
@@ -123,10 +126,14 @@ sub create {
return $self->alert( "a cdn with domain " . $params->{domainName} . " already exists." );
}
+ #setting tenant_id to the user's tenant if tenant is not set. TODO(nirs): remove when tenancy is no longer optional in the API
+ my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} : $self->current_user_tenant();
--- End diff --
Lets discuss it along with the comment of line 136
---
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] incubator-trafficcontrol issue #402: [TC-184] Org tenancy - adding tenant to...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on the issue:
https://github.com/apache/incubator-trafficcontrol/pull/402
In this phase we are dealing with "org-tenancy".
Initially I thought some CDN tenancy is required in this phase - as the content provider creating a DS needs to be able to view the CDNs but not edit them. Therefore, we decided back then that we add tenancy to CDNs but enforcing it only on read.
What we did not take into account is the AAA module that practically do the same thing.
I also believe that CDN tenancy needs to be part of this system. The only question is when it should get in. On the one hand we already did part of the effort, on the other there is more to be done (e.g. derived tenancy of servers/cache-groups/do_extensions) that I not sure it is smart to get into now.
What say you?
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109747255
--- Diff: traffic_ops/app/db/migrations/20170315000002_cdn_tenancy.sql ---
@@ -0,0 +1,31 @@
+/*
--- End diff --
Done
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher closed the pull request at:
https://github.com/apache/incubator-trafficcontrol/pull/402
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109292282
--- Diff: traffic_ops/app/lib/API/Cdn.pm ---
@@ -181,19 +189,25 @@ sub update {
return $self->alert( "a cdn with domain name " . $params->{domainName} . " already exists." );
}
+ #setting tenant_id to undef if tenant is not set. TODO(nirs): remove when tenancy is no longer optional in the API
+ my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} : undef;
--- End diff --
Pending for decision on a similar comment for PR 398
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109165170
--- Diff: traffic_ops/app/lib/API/Cdn.pm ---
@@ -123,10 +126,14 @@ sub create {
return $self->alert( "a cdn with domain " . $params->{domainName} . " already exists." );
}
+ #setting tenant_id to the user's tenant if tenant is not set. TODO(nirs): remove when tenancy is no longer optional in the API
+ my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} : $self->current_user_tenant();
--- End diff --
i don't think you need 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.
---
[GitHub] incubator-trafficcontrol issue #402: [TC-184] Org tenancy - adding tenant to...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on the issue:
https://github.com/apache/incubator-trafficcontrol/pull/402
Do you want me to hold off on this one? Are you rethinking it?
I still think we need tenancy at the CDN level.
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109292111
--- Diff: traffic_ops/app/db/migrations/20170315000002_cdn_tenancy.sql ---
@@ -0,0 +1,31 @@
+/*
--- End diff --
Will do once PR 398 is in (on the merge)
---
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] incubator-trafficcontrol issue #402: [TC-184] Org tenancy - adding tenant to...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on the issue:
https://github.com/apache/incubator-trafficcontrol/pull/402
Merge with PR398 was done.
This PR can get in from that aspect, but I'm starting to think we might not need a CDN tenancy after all.
Please wait with the PR approval.
---
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] incubator-trafficcontrol issue #402: [TC-184] Org tenancy - adding tenant to...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on the issue:
https://github.com/apache/incubator-trafficcontrol/pull/402
Closing for now, long way to go before we get to real CDN tenancy
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109164965
--- Diff: traffic_ops/app/db/migrations/20170315000002_cdn_tenancy.sql ---
@@ -0,0 +1,31 @@
+/*
--- End diff --
you'll need to rename this file
---
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] incubator-trafficcontrol issue #402: [TC-184] Org tenancy - adding tenant to...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on the issue:
https://github.com/apache/incubator-trafficcontrol/pull/402
@nir-sopher - i agree. let's tackle tenancy on a deliveryservice to start and then we can circle back to tenancy at the cdn level...
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109292359
--- Diff: traffic_ops/app/lib/Fixtures/Cdn.pm ---
@@ -33,6 +34,17 @@ my %definition_for = (
id => 200,
name => 'cdn2',
domain_name => 'cdn2.kabletown.net',
+ tenant_id => undef,
+ },
+ },
+ ## id => 3
--- End diff --
Ack
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109292241
--- Diff: traffic_ops/app/lib/API/Cdn.pm ---
@@ -123,10 +126,14 @@ sub create {
return $self->alert( "a cdn with domain " . $params->{domainName} . " already exists." );
}
+ #setting tenant_id to the user's tenant if tenant is not set. TODO(nirs): remove when tenancy is no longer optional in the API
+ my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} : $self->current_user_tenant();
+
my $values = {
name => $params->{name},
dnssec_enabled => $params->{dnssecEnabled},
domain_name => $params->{domainName},
+ tenant_id => $tenant_id,
--- End diff --
I'm not familiar with this syntax.
I assume it is equivalent to "defined".
The point is, that as "undef" tenant is a valid value that the user may set. Therefore I prefer to use "exists" and not "defined".
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109167349
--- Diff: traffic_ops/app/lib/Fixtures/Cdn.pm ---
@@ -33,6 +34,17 @@ my %definition_for = (
id => 200,
name => 'cdn2',
domain_name => 'cdn2.kabletown.net',
+ tenant_id => undef,
+ },
+ },
+ ## id => 3
--- End diff --
can you get rid of this comment? it's misleading.
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109166684
--- Diff: traffic_ops/app/lib/API/Cdn.pm ---
@@ -123,10 +126,14 @@ sub create {
return $self->alert( "a cdn with domain " . $params->{domainName} . " already exists." );
}
+ #setting tenant_id to the user's tenant if tenant is not set. TODO(nirs): remove when tenancy is no longer optional in the API
+ my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} : $self->current_user_tenant();
+
my $values = {
name => $params->{name},
dnssec_enabled => $params->{dnssecEnabled},
domain_name => $params->{domainName},
+ tenant_id => $tenant_id,
--- End diff --
oh, nevermind, i see what you are doing here. how about this instead?
tenant_id => $params->{tenantId} // $self->current_user_tenant();
meaning, set tenant_id to $params->{tenantId} if it's not null / undef, otherwise set tenant_id to $self->current_user_tenant()
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109322384
--- Diff: traffic_ops/app/lib/API/Cdn.pm ---
@@ -123,10 +126,14 @@ sub create {
return $self->alert( "a cdn with domain " . $params->{domainName} . " already exists." );
}
+ #setting tenant_id to the user's tenant if tenant is not set. TODO(nirs): remove when tenancy is no longer optional in the API
+ my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} : $self->current_user_tenant();
+
my $values = {
name => $params->{name},
dnssec_enabled => $params->{dnssecEnabled},
domain_name => $params->{domainName},
+ tenant_id => $tenant_id,
--- End diff --
that's fine
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109166992
--- Diff: traffic_ops/app/lib/API/Cdn.pm ---
@@ -181,19 +189,25 @@ sub update {
return $self->alert( "a cdn with domain name " . $params->{domainName} . " already exists." );
}
+ #setting tenant_id to undef if tenant is not set. TODO(nirs): remove when tenancy is no longer optional in the API
+ my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} : undef;
--- End diff --
I don't think you need this. This should suffice:
tenant_id => $params->{tenantId}
---
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] incubator-trafficcontrol pull request #402: [TC-184] Org tenancy - adding te...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/402#discussion_r109165433
--- Diff: traffic_ops/app/lib/API/Cdn.pm ---
@@ -123,10 +126,14 @@ sub create {
return $self->alert( "a cdn with domain " . $params->{domainName} . " already exists." );
}
+ #setting tenant_id to the user's tenant if tenant is not set. TODO(nirs): remove when tenancy is no longer optional in the API
+ my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} : $self->current_user_tenant();
+
my $values = {
name => $params->{name},
dnssec_enabled => $params->{dnssecEnabled},
domain_name => $params->{domainName},
+ tenant_id => $tenant_id,
--- End diff --
I would just change this to
tenant_id => $params->{tenantId},
that way if tenantId is not passed in, it gets a null / undef value, otherwise, it gets the value that was passed in.
---
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.
---