Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Add service category feature to TO and TP#4518

Merged
mattjackson220 merged 44 commits intoapache:masterfrom
jehunte:serv-type-col
Aug 11, 2020
Merged

Add service category feature to TO and TP#4518
mattjackson220 merged 44 commits intoapache:masterfrom
jehunte:serv-type-col

Conversation

@jehunte
Copy link
Copy Markdown
Contributor

@jehunte jehunte commented Mar 18, 2020

What does this PR (Pull Request) do?

  • This PR is not associated with any existing issue.

This PR enables the user to associate a delivery service with a specific service category. The user can create, add, update, delete categories as necessary and the value will be used in combination with the xmlid to set the HTTP header "X-CDN-SVC"

Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Control Client Go
  • Traffic Ops
  • Traffic Ops ORT - atstccfg
  • Traffic Portal

What is the best way to verify this PR?

  1. Validate database changes
    - New table service category with columns (id, name, last_updated)
    - service_category column added to delivery service table
  2. Verify in TP that you can:
    - create a new delivery service with a service category
    - Add, update, delete, get service categories
  3. Run the ORT script and verify that the header is being set properly
    X-CDN-SVC "{xmlid}|{service-category}"

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR ensures that database migration sequence is correct OR this PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

Screen Shot 2020-03-18 at 9 41 41 AM

Screen Shot 2020-03-18 at 1 19 57 PM

Screen Shot 2020-03-20 at 3 09 57 PM

Screen Shot 2020-03-20 at 3 10 07 PM

Screen Shot 2020-03-18 at 1 21 07 PM

@mitchell852
Copy link
Copy Markdown
Member

Something to think about. Should "service categories" be restricted to certain tenants? For example, imagine the following tenant tree:

- root
-- tenant 1
--- tenant 1.1
-- tenant 2
--- tenant 2.1

If I am a user in tenant 2.1 and i create a Foo service category, should users in root, tenant 1, 1.1, 2 be able to attach the Foo service category to their delivery services? Maybe that's not a problem as the tenant 2.1 user will still only see delivery services scoped to tenant 2.1 in that category. However, this could potentially result in information leakage across tenants. I.e. if i'm in the company A tenant and i create a company-a-service-category-1, everybody across all tenants can see/use that service category.

The more I think about this, service categories should be scoped to tenancy. most tenants don't want to be concerned with the service categories created by other tenants.

@rob05c
Copy link
Copy Markdown
Member

rob05c commented Mar 18, 2020

If I am a user in tenant 2.1 and i create a Foo service category

I think the question for anything is, "Will delivery service customers who are not CDN operators be able to create this?" If so, it needs to be under tenancy, and invisible to other tenants.

@mitchell852
Copy link
Copy Markdown
Member

I think the question for anything is, "Will delivery service customers who are not CDN operators be able to create this?" If so, it needs to be under tenancy, and invisible to other tenants.

Well i'm assuming only operators (role=operations or above) can create service categories but those service categories will show up as an option of EVERY delivery service across all tenants which seems problematic.

@rob05c
Copy link
Copy Markdown
Member

rob05c commented Mar 18, 2020

Do we expect Service Categories to be created by DS customers? Or is it something only Operators will create?

@mitchell852
Copy link
Copy Markdown
Member

mitchell852 commented Mar 18, 2020

Do we expect Service Categories to be created by tenants? Or is it something only Operators will create?

everybody is scoped to a tenant. operators and non operators. all users are tenant scoped. for example, if i am an operator in the 2.1 tenant, i should probably only be able to create service categories that apply to the 2.1 delivery services.

@rob05c
Copy link
Copy Markdown
Member

rob05c commented Mar 18, 2020

Right. But for example, Delivery Service Types aren't "scoped" to tenants, because they're created by operators of the system, not customers of the system. I think it's a valid distinction, to decide if something applies across the entire CDN, or only to the Tenant who created it.

@rob05c
Copy link
Copy Markdown
Member

rob05c commented Mar 18, 2020

@mitchell852 I think I agree with you. It's possible a TC Operator will want to make a hard list of categories, e.g. "Linear," "VOD," "Images." But it's also possible the TC Operator will want to allow any DS owner to make any categories they want.

If Service Categories are scoped to Tenants, and under Roles&Capabilities, both are possible.
For the former, the Operator just doesn't give any tenants the Create Service Category Capability, and they make the hard-coded categories on the Root tenant.
For the latter, they give everyone the Capability.

Keeping YAGNI in mind, IMO it's sufficiently likely someone will need the latter, to go ahead and support it from the start.

@mitchell852
Copy link
Copy Markdown
Member

mitchell852 commented Mar 18, 2020

IMO it's sufficiently likely someone will need the latter, to go ahead and support it from the start.

Yeah, it's harder to add tenancy after the fact imo. It seems to me if i were to create a service category i might want to specify that it applies to all ds's (tenant=root) or only applies to ds's of the 2.1 tenant (tenant=2.1) or like you said grant users the ability to create service categories ONLY in there tenancy.

@mitchell852
Copy link
Copy Markdown
Member

mitchell852 commented Mar 18, 2020

also, those 2 sql scripts under additional info seem like something very specific to a certain company's use of edge header rewrites. probably not something needed as part of this PR. also, @jehunte can you make sure your screenshots are generic? i.e. no company-specific info.

@jehunte
Copy link
Copy Markdown
Contributor Author

jehunte commented Mar 18, 2020

@rob05c @mitchell852 These are great points. I will require a tenant to be associated with the service categories so tenants can only see the service categories they've created. I've also replaced the screenshots and removed the sql queries.

@mitchell852 mitchell852 added Traffic Portal v1 related to Traffic Portal version 1 new feature A new feature, capability or behavior documentation related to documentation Traffic Ops related to Traffic Ops database relating to setup/installation/structure of the Traffic Ops database Traffic Ops ORT *DEPRECATED* related to the traffic_ops_ort.pl script cache-config Cache config generation and removed database relating to setup/installation/structure of the Traffic Ops database cache-config Cache config generation labels Mar 18, 2020
Comment thread docs/source/api/v2/deliveryservices.rst Outdated
Comment thread docs/source/api/v2/service_categories.rst Outdated
Comment thread docs/source/api/v2/service_categories.rst Outdated
Comment thread docs/source/api/v2/service_categories.rst Outdated
Comment thread docs/source/api/v2/service_categories.rst Outdated
Comment thread traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go Outdated
Comment thread traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go Outdated
Comment thread traffic_ops/traffic_ops_golang/routing/routes.go Outdated
Comment thread traffic_ops/traffic_ops_golang/servicecategory/servicecategories.go Outdated
Comment thread traffic_ops/traffic_ops_golang/servicecategory/servicecategories.go Outdated
Comment thread traffic_ops/traffic_ops_golang/routing/routes.go Outdated
Comment thread traffic_portal/app/src/common/api/ServiceCategoryService.js Outdated
@mattjackson220
Copy link
Copy Markdown
Contributor

one small comment then this looks good! might depend on PR4633 to get TP set up to use API v3

Comment thread lib/go-atscfg/headerrewritedotconfig.go Outdated
Copy link
Copy Markdown
Member

@rob05c rob05c left a comment

Choose a reason for hiding this comment

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

I'm approving with respect to the ORT code changes. I haven't actually tested, but I believe the code will work, from what I know of ORT. I had previously raised concerns that it wouldn't, because ORT wasn't using the latest API (if it exists) to get DeliveryServices. It now is, so it should work correctly.

Also, w.r.t. my Tenancy comments: I'll leave it up to @mitchell852 as the Tenacy expert.

Comment thread traffic_portal/app/src/common/api/ServiceCategoryService.js Outdated
@mattjackson220 mattjackson220 merged commit 7c4a124 into apache:master Aug 11, 2020
@jehunte jehunte deleted the serv-type-col branch August 11, 2020 16:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation related to documentation new feature A new feature, capability or behavior Traffic Ops ORT *DEPRECATED* related to the traffic_ops_ort.pl script Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants