You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/07/23 18:57:38 UTC
[GitHub] [trafficcontrol] shamrickus opened a new pull request #4907: Add verification for TO server ips across a profile
shamrickus opened a new pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907
<!--
************ STOP!! ************
If this Pull Request is intended to fix a security vulnerability, DO NOT submit it! Instead, contact
the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
-->
## What does this PR (Pull Request) do?
<!-- Explain the changes you made here. If this fixes an Issue, identify it by
replacing the text in the checkbox item with the Issue number e.g.
- [x] This PR fixes #9001 OR is not related to any Issue
^ This will automatically close Issue number 9001 when the Pull Request is
merged (The '#' is important).
Be sure you check the box properly, see the "The following criteria are ALL
met by this PR" section for details.
-->
- [x] This PR fixes #4888 <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this
Pull Request. Also, feel free to add the name of a tool or script that is
affected but not on the list.
Additionally, if this Pull Request does NOT affect documentation, please
explain why documentation is not required. -->
- Traffic Ops
- Traffic Portal
## What is the best way to verify this PR?
Run the TO/TP tests.
Ensure that you cannot update a server to have the same service ip address as another server with the same profile. Both when creating and updating a server.
<!-- Please include here ALL the steps necessary to test your Pull Request. If
it includes tests (and most should), outline here the steps needed to run the
tests. If not, lay out the manual testing procedure and please explain why
tests are unnecessary for this Pull Request. -->
<!-- If this PR fixes a bug, please list here all of the affected versions - to
the best of your knowledge. It's also pretty helpful to include a commit hash
of where 'master' is at the time this PR is opened (if it affects master),
because what 'master' means will change over time. For example, if this PR
fixes a bug that's present in master (at commit hash '1df853c8'), in v4.0.0,
and in the current 4.0.1 Release candidate (e.g. RC1), then this list would
look like:
- master (1df853c8)
- 4.0.0
- 4.0.1 (RC1)
If you don't know what other versions might have this bug, AND don't know how
to find the commit hash of 'master', then feel free to leave this section
blank (or, preferably, delete it entirely).
-->
## The following criteria are ALL met by this PR
<!-- Check the boxes to signify that the associated statement is true. To
"check a box", replace the space inside of the square brackets with an 'x'.
e.g.
- [ x] <- Wrong
- [x ] <- Wrong
- [] <- Wrong
- [*] <- Wrong
- [x] <- Correct!
-->
- [x] This PR includes tests OR I have explained why tests are unnecessary
- [x] This PR includes documentation OR I have explained why documentation is unnecessary
- [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
<!-- If you would like to include any additional information on the PR for
potential reviewers please put it here.
Some examples of this would be:
- Before and after screenshots/gifs of the Traffic Portal if it is affected
- Links to other dependent Pull Requests
- References to relevant context (e.g. new/updates to dependent libraries,
mailing list records, blueprints)
Feel free to leave this section blank (or, preferably, delete it entirely).
-->
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 edited a comment on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-682087066
@bdgranger - good question about 5.0 release date. As a community, we were striving for quarterly releases. 4.1 was end of Q1. 5.0 was supposed to be end of Q2 (june 30/july 1) but that didn't happen due to a couple of big features that were incomplete ([flexible topologies](https://github.com/apache/trafficcontrol/blob/master/blueprints/flexible-topologies.md) and [multi-interface support](https://github.com/apache/trafficcontrol/blob/master/blueprints/multi-interface-servers.md)) so hopefully pretty soon?? @ocket8888 is the 5.0 release manager.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
shamrickus commented on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-671545594
I see no reference to that constraint in our migrations?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
shamrickus commented on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-679242254
This change does not add new functionality but port over existing functionality to our [multi-interface initiative](https://github.com/apache/trafficcontrol/blob/master/blueprints/multi-interface-servers.md) (which will be released in ATCv5.0). There already existed a primary key on the server table across [`ip_address` and `profile`](https://github.com/apache/trafficcontrol/blob/0cf375d2b1/traffic_ops/app/db/create_tables.sql#L2415), so if it was working before it should still work now; at least in relation to this PR. That having been said I'm not sure I understand your setup, and it may be that we have made changes in multi-interface such-that your current setup won't work. What version of ATC are you currently using?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] bdgranger commented on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
bdgranger commented on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-678479987
I have a question about this change (and left a similar comment in #4888) -- we have a system where we will be running multiple ATS caches in containers on the same host node, but the traffic ops and other ATC servers aren't necessarily in containers or on the same kubernetes or docker cluster, so will access the cache containers on the same IP address of the host server. Currently traffic ops allows us to create multiple servers with the same IP address, and as long as we have unique hostnames and unique tcpPorts everything is working as expected.
Will this change preclude us from being allowed to create multiple servers on the same IP address in this scenario?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-671549121
What we had before removing `ip_address` and `ip6_address` from the `server` table (from [traffic_ops/app/db/create_tables.sql#L2412](https://github.com/apache/trafficcontrol/blob/0cf375d2b1/traffic_ops/app/db/create_tables.sql#L2412)):
```pgsql
--
-- Name: idx_89709_ip6_profile; Type: INDEX; Schema: public; Owner: traffic_ops
--
CREATE UNIQUE INDEX idx_89709_ip6_profile ON server USING btree (ip6_address, profile);
--
-- Name: idx_89709_ip_profile; Type: INDEX; Schema: public; Owner: traffic_ops
--
CREATE UNIQUE INDEX idx_89709_ip_profile ON server USING btree (ip_address, profile);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#discussion_r467985008
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -16,13 +16,19 @@ package v3
*/
import (
+
Review comment:
`gofmt` removes this line
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -26,14 +26,15 @@ import (
"encoding/json"
"errors"
"fmt"
- "github.com/apache/trafficcontrol/lib/go-rfc"
- "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims"
"net"
"net/http"
"strconv"
"strings"
"time"
+ "github.com/apache/trafficcontrol/lib/go-rfc"
+ "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims"
+
Review comment:
These newlines between ATC package imports can be removed
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -16,13 +16,19 @@ package v3
*/
import (
+
"fmt"
- "github.com/apache/trafficcontrol/lib/go-rfc"
"net/http"
"net/url"
"strconv"
"testing"
"time"
+
+ "github.com/apache/trafficcontrol/lib/go-util"
+
+ "github.com/apache/trafficcontrol/lib/go-tc"
+
Review comment:
These newlines between ATC package imports can be removed
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -485,6 +490,39 @@ func validateV3(s *tc.ServerNullable, tx *sql.Tx) (string, error) {
}
errs = append(errs, validateCommon(&s.CommonServerProperties, tx)...)
+
+ query := `
+SELECT s.ID, ip.address FROM server s
+JOIN profile p on p.Id = s.Profile
+JOIN interface i on i.server = s.ID
+JOIN ip_address ip on ip.Server = s.ID and ip.interface = i.name
+WHERE i.monitor = true
+and p.id = $1
+`
+ var rows *sql.Rows
+ var err error
+ //ProfileID already validated
+ if s.ID != nil {
+ rows, err = tx.Query(query+" and s.id != $2", *s.ProfileID, *s.ID)
+ } else {
+ rows, err = tx.Query(query, *s.ProfileID)
+ }
+ if err != nil {
+ errs = append(errs, errors.New("unable to determine service address uniqueness"))
+ } else if rows != nil {
+ defer rows.Close()
+ for rows.Next() {
+ var id int
+ var ipaddress string
+ err = rows.Scan(&id, &ipaddress)
+ if err != nil {
+ errs = append(errs, errors.New("unable to determine service address uniqueness"))
+ } else if (ipaddress == ipv4 || ipaddress == ipv6) && (s.ID == nil || *s.ID != id) {
+ errs = append(errs, errors.New("there exists a server on the same profile that has the same service address"))
Review comment:
This error should include the IP addresses and IDs being compared.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-679341492
ATCv3.1 has [a constraint that won't let you have multiple servers with the same IP and Profile](https://github.com/apache/trafficcontrol/blob/3.1.x/traffic_ops/app/db/create_tables.sql#L2131) so I'm not sure how you got that set up that way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-671453042
We used to have a database constraint for each set of server IPs and profile being unique, (lost when we moved to using the `ip_address` table for IP addresses). This PR should bring back that constraint.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#discussion_r470897651
##########
File path: traffic_ops/app/db/migrations/20200811082611_add_server_ip_profile_trigger.sql
##########
@@ -0,0 +1,124 @@
+/*
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+-- +goose Up
+-- SQL in section 'Up' is executed when this migration is applied
+-- +goose StatementBegin
+CREATE OR REPLACE FUNCTION before_server_table()
+ RETURNS TRIGGER AS
+$$
+DECLARE
+ server_count BIGINT;
+BEGIN
+ WITH server_ips AS (
+ SELECT s.id, i.name, ip.address, s.profile
+ FROM server s
+ JOIN interface i on i.server = s.ID
+ JOIN ip_address ip on ip.Server = s.ID and ip.interface = i.name
+ WHERE i.monitor = true
+ )
+ SELECT count(*)
+ INTO server_count
+ FROM server_ips sip
+ JOIN server_ips sip2 on sip.id <> sip2.id
+ WHERE sip.id = NEW.id
+ AND sip2.address = sip.address
+ AND sip2.profile = sip.profile;
+
+ IF server_count > 0 THEN
+ RAISE EXCEPTION 'Server [id:%] does not have a unique ip_address over the profile [id:%], [%] conflicts',
+ NEW.id,
+ NEW.profile,
+ server_count;
+ END IF;
+ RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+-- +goose StatementEnd
+
+-- +goose StatementBegin
+CREATE OR REPLACE FUNCTION before_ip_address_table()
+ RETURNS TRIGGER
+AS
+$$
+DECLARE
+ server_count BIGINT;
+ server_id BIGINT;
+ server_profile BIGINT;
+BEGIN
+ WITH server_ips AS (
+ SELECT s.id as sid, ip.interface, i.name, ip.address, s.profile, ip.server
+ FROM server s
+ JOIN interface i
+ on i.server = s.ID
+ JOIN ip_address ip
+ on ip.Server = s.ID and ip.interface = i.name
+ WHERE i.monitor = true
+ )
+ SELECT count(sip.sid), sip.sid, sip.profile
+ INTO server_count, server_id, server_profile
+ FROM server_ips sip
+ JOIN server_ips sip2 on sip.sid <> sip2.sid
+ WHERE (sip.server = NEW.server AND sip.address = NEW.address AND sip.interface = NEW.interface)
+ AND sip2.address = sip.address
+ AND sip2.profile = sip.profile
+ GROUP BY sip.sid, sip.profile;
+
+ IF server_count > 0 THEN
+ RAISE EXCEPTION 'ip_address is not unique accross the server [id:%] profile [id:%], [%] conflicts',
+ server_id,
+ server_profile,
+ server_count;
+ END IF;
+ RETURN NEW;
+END;
+$$ LANGUAGE PLPGSQL;
+-- +goose StatementEnd
+
+CREATE TRIGGER before_update_server_trigger
+ BEFORE UPDATE
+ ON server
+ FOR EACH ROW
+ WHEN (NEW.profile <> OLD.profile)
+EXECUTE PROCEDURE before_server_table();
+
+CREATE TRIGGER before_create_server_trigger
+ BEFORE INSERT
+ ON server
+ FOR EACH ROW
+EXECUTE PROCEDURE before_server_table();
+
+CREATE TRIGGER before_create_ip_address_trigger
+ BEFORE INSERT
+ ON ip_address
+ FOR EACH ROW
+EXECUTE PROCEDURE before_ip_address_table();
+
+CREATE TRIGGER before_update_ip_address_trigger
+ BEFORE UPDATE
+ ON ip_address
+ FOR EACH ROW
+ WHEN (NEW.address <> OLD.address)
+EXECUTE PROCEDURE before_ip_address_table();
+
+-- +goose Down
+-- SQL section 'Down' is executed when this migration is rolled back
+DROP FUNCTION IF EXISTS before_server_table();
+DROP FUNCTION IF EXISTS before_ip_address_table();
+
+DROP TRIGGER IF EXISTS before_update_server_trigger ON server;
+DROP TRIGGER IF EXISTS before_create_server_trigger ON server;
+DROP TRIGGER IF EXISTS before_update_ip_address_trigger ON ip_address;
+DROP TRIGGER IF EXISTS before_create_ip_address_trigger ON ip_address;
Review comment:
@shamrickus ^^
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 merged pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
mitchell852 merged pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#discussion_r470857930
##########
File path: traffic_ops/app/db/migrations/20200811082611_add_server_ip_profile_trigger.sql
##########
@@ -0,0 +1,124 @@
+/*
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+-- +goose Up
+-- SQL in section 'Up' is executed when this migration is applied
+-- +goose StatementBegin
+CREATE OR REPLACE FUNCTION before_server_table()
+ RETURNS TRIGGER AS
+$$
+DECLARE
+ server_count BIGINT;
+BEGIN
+ WITH server_ips AS (
+ SELECT s.id, i.name, ip.address, s.profile
+ FROM server s
+ JOIN interface i on i.server = s.ID
+ JOIN ip_address ip on ip.Server = s.ID and ip.interface = i.name
+ WHERE i.monitor = true
+ )
+ SELECT count(*)
+ INTO server_count
+ FROM server_ips sip
+ JOIN server_ips sip2 on sip.id <> sip2.id
+ WHERE sip.id = NEW.id
+ AND sip2.address = sip.address
+ AND sip2.profile = sip.profile;
+
+ IF server_count > 0 THEN
+ RAISE EXCEPTION 'Server [id:%] does not have a unique ip_address over the profile [id:%], [%] conflicts',
+ NEW.id,
+ NEW.profile,
+ server_count;
+ END IF;
+ RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+-- +goose StatementEnd
+
+-- +goose StatementBegin
+CREATE OR REPLACE FUNCTION before_ip_address_table()
+ RETURNS TRIGGER
+AS
+$$
+DECLARE
+ server_count BIGINT;
+ server_id BIGINT;
+ server_profile BIGINT;
+BEGIN
+ WITH server_ips AS (
+ SELECT s.id as sid, ip.interface, i.name, ip.address, s.profile, ip.server
+ FROM server s
+ JOIN interface i
+ on i.server = s.ID
+ JOIN ip_address ip
+ on ip.Server = s.ID and ip.interface = i.name
+ WHERE i.monitor = true
+ )
+ SELECT count(sip.sid), sip.sid, sip.profile
+ INTO server_count, server_id, server_profile
+ FROM server_ips sip
+ JOIN server_ips sip2 on sip.sid <> sip2.sid
+ WHERE (sip.server = NEW.server AND sip.address = NEW.address AND sip.interface = NEW.interface)
+ AND sip2.address = sip.address
+ AND sip2.profile = sip.profile
+ GROUP BY sip.sid, sip.profile;
+
+ IF server_count > 0 THEN
+ RAISE EXCEPTION 'ip_address is not unique accross the server [id:%] profile [id:%], [%] conflicts',
+ server_id,
+ server_profile,
+ server_count;
+ END IF;
+ RETURN NEW;
+END;
+$$ LANGUAGE PLPGSQL;
+-- +goose StatementEnd
+
+CREATE TRIGGER before_update_server_trigger
+ BEFORE UPDATE
+ ON server
+ FOR EACH ROW
+ WHEN (NEW.profile <> OLD.profile)
+EXECUTE PROCEDURE before_server_table();
+
+CREATE TRIGGER before_create_server_trigger
+ BEFORE INSERT
+ ON server
+ FOR EACH ROW
+EXECUTE PROCEDURE before_server_table();
+
+CREATE TRIGGER before_create_ip_address_trigger
+ BEFORE INSERT
+ ON ip_address
+ FOR EACH ROW
+EXECUTE PROCEDURE before_ip_address_table();
+
+CREATE TRIGGER before_update_ip_address_trigger
+ BEFORE UPDATE
+ ON ip_address
+ FOR EACH ROW
+ WHEN (NEW.address <> OLD.address)
+EXECUTE PROCEDURE before_ip_address_table();
+
+-- +goose Down
+-- SQL section 'Down' is executed when this migration is rolled back
+DROP FUNCTION IF EXISTS before_server_table();
+DROP FUNCTION IF EXISTS before_ip_address_table();
+
+DROP TRIGGER IF EXISTS before_update_server_trigger ON server;
+DROP TRIGGER IF EXISTS before_create_server_trigger ON server;
+DROP TRIGGER IF EXISTS before_update_ip_address_trigger ON ip_address;
+DROP TRIGGER IF EXISTS before_create_ip_address_trigger ON ip_address;
Review comment:
Goose down fails
```
2020/08/14 20:41:51 FAIL 20200811082611_add_server_ip_profile_trigger.sql (pq: cannot drop function before_server_table() because other objects depend on it), quitting migration.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-679338030
@bdgranger - hmm. i'm not really sure when the IP + profile unique constraint was added to the server table (i guess after 3.1 since you can apparently do it in 3.1) and many have questioned it's value. I do know this is being worked on #4940 and will hopefully be in TC 5.0.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-682087066
@bdgranger - good question about 5.0 release date. As a community, we were striving for quarterly releases. 4.1 was end of Q1. 5.0 was supposed to be end of Q2 (june 30/july 1) but that didn't happen due to a couple of big features ([flexible topologies](https://github.com/apache/trafficcontrol/blob/master/blueprints/flexible-topologies.md) and [multi-interface support](https://github.com/apache/trafficcontrol/blob/master/blueprints/multi-interface-servers.md)) so hopefully pretty soon?? @ocket8888 is the 5.0 release manager.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#discussion_r470906317
##########
File path: traffic_ops/app/db/migrations/20200811082611_add_server_ip_profile_trigger.sql
##########
@@ -0,0 +1,124 @@
+/*
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+-- +goose Up
+-- SQL in section 'Up' is executed when this migration is applied
+-- +goose StatementBegin
+CREATE OR REPLACE FUNCTION before_server_table()
+ RETURNS TRIGGER AS
+$$
+DECLARE
+ server_count BIGINT;
+BEGIN
+ WITH server_ips AS (
+ SELECT s.id, i.name, ip.address, s.profile
+ FROM server s
+ JOIN interface i on i.server = s.ID
+ JOIN ip_address ip on ip.Server = s.ID and ip.interface = i.name
+ WHERE i.monitor = true
+ )
+ SELECT count(*)
+ INTO server_count
+ FROM server_ips sip
+ JOIN server_ips sip2 on sip.id <> sip2.id
+ WHERE sip.id = NEW.id
+ AND sip2.address = sip.address
+ AND sip2.profile = sip.profile;
+
+ IF server_count > 0 THEN
+ RAISE EXCEPTION 'Server [id:%] does not have a unique ip_address over the profile [id:%], [%] conflicts',
+ NEW.id,
+ NEW.profile,
+ server_count;
+ END IF;
+ RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+-- +goose StatementEnd
+
+-- +goose StatementBegin
+CREATE OR REPLACE FUNCTION before_ip_address_table()
+ RETURNS TRIGGER
+AS
+$$
+DECLARE
+ server_count BIGINT;
+ server_id BIGINT;
+ server_profile BIGINT;
+BEGIN
+ WITH server_ips AS (
+ SELECT s.id as sid, ip.interface, i.name, ip.address, s.profile, ip.server
+ FROM server s
+ JOIN interface i
+ on i.server = s.ID
+ JOIN ip_address ip
+ on ip.Server = s.ID and ip.interface = i.name
+ WHERE i.monitor = true
+ )
+ SELECT count(sip.sid), sip.sid, sip.profile
+ INTO server_count, server_id, server_profile
+ FROM server_ips sip
+ JOIN server_ips sip2 on sip.sid <> sip2.sid
+ WHERE (sip.server = NEW.server AND sip.address = NEW.address AND sip.interface = NEW.interface)
+ AND sip2.address = sip.address
+ AND sip2.profile = sip.profile
+ GROUP BY sip.sid, sip.profile;
+
+ IF server_count > 0 THEN
+ RAISE EXCEPTION 'ip_address is not unique accross the server [id:%] profile [id:%], [%] conflicts',
+ server_id,
+ server_profile,
+ server_count;
+ END IF;
+ RETURN NEW;
+END;
+$$ LANGUAGE PLPGSQL;
+-- +goose StatementEnd
+
+CREATE TRIGGER before_update_server_trigger
+ BEFORE UPDATE
+ ON server
+ FOR EACH ROW
+ WHEN (NEW.profile <> OLD.profile)
+EXECUTE PROCEDURE before_server_table();
+
+CREATE TRIGGER before_create_server_trigger
+ BEFORE INSERT
+ ON server
+ FOR EACH ROW
+EXECUTE PROCEDURE before_server_table();
+
+CREATE TRIGGER before_create_ip_address_trigger
+ BEFORE INSERT
+ ON ip_address
+ FOR EACH ROW
+EXECUTE PROCEDURE before_ip_address_table();
+
+CREATE TRIGGER before_update_ip_address_trigger
+ BEFORE UPDATE
+ ON ip_address
+ FOR EACH ROW
+ WHEN (NEW.address <> OLD.address)
+EXECUTE PROCEDURE before_ip_address_table();
+
+-- +goose Down
+-- SQL section 'Down' is executed when this migration is rolled back
+DROP FUNCTION IF EXISTS before_server_table();
+DROP FUNCTION IF EXISTS before_ip_address_table();
+
+DROP TRIGGER IF EXISTS before_update_server_trigger ON server;
+DROP TRIGGER IF EXISTS before_create_server_trigger ON server;
+DROP TRIGGER IF EXISTS before_update_ip_address_trigger ON ip_address;
+DROP TRIGGER IF EXISTS before_create_ip_address_trigger ON ip_address;
Review comment:
Will fix
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
shamrickus commented on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-673709275
Should be good to go now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] bdgranger commented on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
bdgranger commented on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-679358897
@ocket8888 @mitchell852 - The system was originally running ATC2.2 and that was where I started this work and was able to create the multiple servers with the same IP and profile. However, when I upgraded to ATC 3.1, it didn't complain about the multiple servers with the same IP address, and even now on the ATC3.1 system (which was upgraded from 2.2) I am still able to create new servers with the same IP address and profile as existing servers. I wonder if the upgrade somehow didn't apply the constraint change? Not that I'm complaining, since I'm relying on that behavior currently :-) I guess I'll have to try a "clean" ATC3.1 install and see what happens.
Looks like #4940 is indeed what I really want, if any type of constraint is to be enforced. Is there any estimated date for the TC 5.0 release yet?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] bdgranger commented on pull request #4907: Add verification for TO server ips across a profile
Posted by GitBox <gi...@apache.org>.
bdgranger commented on pull request #4907:
URL: https://github.com/apache/trafficcontrol/pull/4907#issuecomment-679331661
We are currently running ATC 3.1.0 but plan to upgrade in the near future. In our current setup, we are able to create multiple servers in the traffic ops UI, all of which use the same IP address and the same profile, but have different hostnames and different tcpPort and httpsPort, so they can be individually communicated with, even though they are running on the same docker node and share the same external IP address.
I was only concerned since the entire point of #4888 seemed to be to prevent multiple servers with the same IP address.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org