You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by "ocket8888 (via GitHub)" <gi...@apache.org> on 2023/07/03 18:30:02 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request, #7614: admin tool no seed on upgrade
ocket8888 opened a new pull request, #7614:
URL: https://github.com/apache/trafficcontrol/pull/7614
In PR #7343 I said that adding a Permission to the operations Role would be privilege escalation. I guess that's technically true, but it was already a much bigger problem - we run seeding on upgrade. Which means if you take away *any* Permission from **any** Role in the seed data the upgrade process will escalate privilege.
This PR aims to fix that by removing automatic seeding from the `up`/`upgrade` action. This does mean that during installs, seeding must now be a manual step - but if users use `postinstall` as the docs instruct, then that's already being done, anyway.
<hr/>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
- Traffic Ops (database initialization/upgrade)
## What is the best way to verify this PR?
If the postinstall tests still work, and CiaB still works, then I think we're good. I believe that covers both the case of installing and upgrading the db.
## If this is a bugfix, which Traffic Control versions contained the bug?
- all
## PR submission checklist
- [x] This PR uses existing tests
- [x] This PR has documentation
- [x] This PR has a CHANGELOG.md entry
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY**
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] admin tool no seed on upgrade [trafficcontrol]
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on code in PR #7614:
URL: https://github.com/apache/trafficcontrol/pull/7614#discussion_r1343142097
##########
cache-config/testing/docker/traffic_ops/run.sh:
##########
@@ -149,6 +149,7 @@ cd /opt/traffic_ops/app
(
db/admin --env=production reset
db/admin --env=production upgrade
+db/admin --env=production seed
Review Comment:
> but it does seem to me to be more appropriate to copy the way `postinstall` does it
The `postinstall` *did* it. Nothing in postinstall runs seeds anymore except for explicitly seeding. That could mess up people who don't use the postinstall script, it makes the process more manual
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7614: admin tool no seed on upgrade
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on code in PR #7614:
URL: https://github.com/apache/trafficcontrol/pull/7614#discussion_r1337918777
##########
traffic_ops/app/db/admin.go:
##########
@@ -353,7 +353,6 @@ func reset() {
func upgrade() {
runMigrations()
if !trafficVault {
- seed()
Review Comment:
Need the TC Health Client integration tests to seed explicitly now
https://github.com/apache/trafficcontrol/blob/91cd4eacc9271b515a285c05c952cd9244e0f8b3/tc-health-client/testing/docker/traffic_ops/run.sh#L105
##########
traffic_ops/app/db/admin.go:
##########
@@ -353,7 +353,6 @@ func reset() {
func upgrade() {
runMigrations()
if !trafficVault {
- seed()
Review Comment:
Need t3c integration tests to seed explicitly now
https://github.com/apache/trafficcontrol/blob/91cd4eacc9271b515a285c05c952cd9244e0f8b3/cache-config/testing/docker/traffic_ops/run.sh#L150-L153
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] admin tool no seed on upgrade [trafficcontrol]
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on code in PR #7614:
URL: https://github.com/apache/trafficcontrol/pull/7614#discussion_r1343142097
##########
cache-config/testing/docker/traffic_ops/run.sh:
##########
@@ -149,6 +149,7 @@ cd /opt/traffic_ops/app
(
db/admin --env=production reset
db/admin --env=production upgrade
+db/admin --env=production seed
Review Comment:
> but it does seem to me to be more appropriate to copy the way `postinstall` does it
The way the `postinstall` *did* it. Nothing in postinstall runs seeds anymore except for explicitly seeding. That could mess up people who don't use the postinstall script, it makes the process more manual
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] admin tool no seed on upgrade [trafficcontrol]
Posted by "ocket8888 (via GitHub)" <gi...@apache.org>.
ocket8888 commented on code in PR #7614:
URL: https://github.com/apache/trafficcontrol/pull/7614#discussion_r1343207057
##########
cache-config/testing/docker/traffic_ops/run.sh:
##########
@@ -149,6 +149,7 @@ cd /opt/traffic_ops/app
(
db/admin --env=production reset
db/admin --env=production upgrade
+db/admin --env=production seed
Review Comment:
Yes, that's correct, the postinstall script seeds manually. Which is fortunate because after this change that's the correct way to invoke it. Not sure what you mean by "did" - that is still the current default behavior.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] admin tool no seed on upgrade [trafficcontrol]
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on code in PR #7614:
URL: https://github.com/apache/trafficcontrol/pull/7614#discussion_r1343119416
##########
cache-config/testing/docker/traffic_ops/run.sh:
##########
@@ -149,6 +149,7 @@ cd /opt/traffic_ops/app
(
db/admin --env=production reset
db/admin --env=production upgrade
+db/admin --env=production seed
Review Comment:
Why should seeding happen after running migrations? Isn't the whole point of no longer seeding when running migrations that `seeds.sql` only changes when the DB is squashed?
If that's the case, it should run before 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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] admin tool no seed on upgrade [trafficcontrol]
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman merged PR #7614:
URL: https://github.com/apache/trafficcontrol/pull/7614
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] codecov[bot] commented on pull request #7614: admin tool no seed on upgrade
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7614:
URL: https://github.com/apache/trafficcontrol/pull/7614#issuecomment-1618986826
## [Codecov](https://app.codecov.io/gh/apache/trafficcontrol/pull/7614?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#7614](https://app.codecov.io/gh/apache/trafficcontrol/pull/7614?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (640c2ca) into [master](https://app.codecov.io/gh/apache/trafficcontrol/commit/2e995e734d4eeade97616cd486e000cae2038035?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2e995e7) will **increase** coverage by `9.63%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #7614 +/- ##
============================================
+ Coverage 27.74% 37.37% +9.63%
Complexity 98 98
============================================
Files 686 250 -436
Lines 78128 12676 -65452
Branches 90 90
============================================
- Hits 21675 4738 -16937
+ Misses 54396 7620 -46776
+ Partials 2057 318 -1739
```
| Flag | Coverage Δ | |
|---|---|---|
| golib_unit | `?` | |
| grove_unit | `4.60% <ø> (ø)` | |
| t3c_unit | `?` | |
| traffic_monitor_unit | `?` | |
| traffic_ops_unit | `?` | |
| traffic_stats_unit | `10.14% <ø> (ø)` | |
| unit_tests | `6.00% <ø> (-18.41%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/trafficcontrol/pull/7614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [cache-config/t3c-generate/cfgfile/wrappers.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2FjaGUtY29uZmlnL3QzYy1nZW5lcmF0ZS9jZmdmaWxlL3dyYXBwZXJzLmdv) | `49.57% <ø> (+0.12%)` | :arrow_up: |
| [cache-config/t3c-generate/config/config.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2FjaGUtY29uZmlnL3QzYy1nZW5lcmF0ZS9jb25maWcvY29uZmlnLmdv) | `0.89% <ø> (ø)` | |
| [traffic\_ops/v5-client/asn.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dHJhZmZpY19vcHMvdjUtY2xpZW50L2Fzbi5nbw==) | `100.00% <100.00%> (ø)` | |
| [traffic\_ops/v5-client/coordinate.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dHJhZmZpY19vcHMvdjUtY2xpZW50L2Nvb3JkaW5hdGUuZ28=) | `100.00% <100.00%> (ø)` | |
| [...\_ops/v5-client/deliveryservice\_request\_comments.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dHJhZmZpY19vcHMvdjUtY2xpZW50L2RlbGl2ZXJ5c2VydmljZV9yZXF1ZXN0X2NvbW1lbnRzLmdv) | `100.00% <100.00%> (ø)` | |
| [traffic\_ops/v5-client/origin.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dHJhZmZpY19vcHMvdjUtY2xpZW50L29yaWdpbi5nbw==) | `59.37% <100.00%> (ø)` | |
... and [436 files with indirect coverage changes](https://app.codecov.io/gh/apache/trafficcontrol/pull/7614/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] admin tool no seed on upgrade [trafficcontrol]
Posted by "ocket8888 (via GitHub)" <gi...@apache.org>.
ocket8888 commented on code in PR #7614:
URL: https://github.com/apache/trafficcontrol/pull/7614#discussion_r1343126420
##########
cache-config/testing/docker/traffic_ops/run.sh:
##########
@@ -149,6 +149,7 @@ cd /opt/traffic_ops/app
(
db/admin --env=production reset
db/admin --env=production upgrade
+db/admin --env=production seed
Review Comment:
No, the point is to not overwrite changes users have made to their system's data. I don't think it really matters which order it's done in, but it does seem to me to be more appropriate to copy the way `postinstall` does it, which is
1. load_schema
2. migrate
3. seed
4. patch
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] admin tool no seed on upgrade [trafficcontrol]
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on PR #7614:
URL: https://github.com/apache/trafficcontrol/pull/7614#issuecomment-1745598349
> I'm still worried that people will forget to have seeded ever.
Example: CDN in a Box for Developers breaks after #7614 (fixed in #7827).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org