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.

Sign up for GitHub

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

Jump to bottom

Adding check to make sure that you cannot add two servers with identical content #5415

Merged
merged 7 commits into from
Jan 13, 2021
Merged

Adding check to make sure that you cannot add two servers with identical content #5415

merged 7 commits into from
Jan 13, 2021

Conversation

srijeet0406
Copy link
Contributor

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

  • Traffic Ops
  • CI tests

What is the best way to verify this PR?

Using API 1.x, create a server using the POST API call.
Now, try to add the same server again using the same POST API call -> You should see an error saying that another server exists with the same profile/ IP address combination.

If this is a bug fix, what versions of Traffic Control are affected?

  • master

The following criteria are ALL met by this PR

  • This PR includes tests
  • This PR does not include documentation
  • This PR does not include an update to CHANGELOG.md
  • This PR includes any and all required license headers
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

@mitchell852
Copy link
Member

mitchell852 commented Jan 8, 2021 •
edited
Loading

it's weird that you could do this before because I thought there was a database constraint to enforce unique IP/profile so at the very least you'd get a 500.

@mitchell852 mitchell852 added Traffic Ops related to Traffic Ops bug something isn't working as intended high impact impacts the basic function, deployment, or operation of a CDN labels Jan 8, 2021
@srijeet0406
Copy link
Contributor Author

it's weird that you could do this before because I thought there was a database constraint to enforce unique IP/profile so at the very least you'd get a 500.

I think that was for API 3.x onwards.

@rawlinp
Copy link
Contributor

rawlinp commented Jan 8, 2021

No, there was definitely a constraint before API 3.x, but then 3.x moved things around so the constraint was replaced with these triggers: traffic_ops/app/db/migrations/2020081108261100_add_server_ip_profile_trigger.sql. So, I'm curious why those triggers aren't preventing this and if they can be fixed instead of adding the check to the API.

@srijeet0406 srijeet0406 marked this pull request as draft January 11, 2021 17:55
@srijeet0406 srijeet0406 marked this pull request as ready for review January 12, 2021 06:11
@mattjackson220 mattjackson220 self-requested a review January 12, 2021 16:40
@mitchell852 mitchell852 added this to the 5.0.0 milestone Jan 12, 2021
mattjackson220
mattjackson220 reviewed Jan 13, 2021
View reviewed changes
Copy link
Contributor

@mattjackson220 mattjackson220 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! Unit and API tests pass, goose up/down work, tested manually and works as expected

@mattjackson220 mattjackson220 merged commit 468065c into apache:master Jan 13, 2021
ocket8888 pushed a commit that referenced this pull request Jan 13, 2021
@srijeet0406 @ocket8888
Adding check to make sure that you cannot add two servers with identi…
6c32df7 
…cal content (#5415)

* Adding check to make sure that you cannot add two servers with identical content

* wip

* move the modifications from the code to migrations

* changelog entry

* Code review fixes

* code review fixes

(cherry picked from commit 468065c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattjackson220 mattjackson220 mattjackson220 approved these changes

Assignees
No one assigned
Labels
bug something isn't working as intended high impact impacts the basic function, deployment, or operation of a CDN Traffic Ops related to Traffic Ops
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

1.x API permits duplicate servers
4 participants
@srijeet0406 @mitchell852 @rawlinp @mattjackson220