[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4015: Rewrite /federations to Go - POST/PUT/DELETE
mhoppa commented on a change in pull request #4015: Rewrite /federations to Go
- POST/PUT/DELETE
URL: https://github.com/apache/trafficcontrol/pull/4015#discussion_r339822218
##
File path: traffic_ops/traffic_ops_golang/api/api.go
##
@@ -86,7 +86,7 @@ func WriteRespRaw(w http.ResponseWriter, r *http.Request, v
interface{}) {
return
}
w.Header().Set("Content-Type", "application/json")
- w.Write(bts)
+ w.Write(append(bts, '\n'))
Review comment:
I believe this is causing a unit test to fail in this package ->
```
--- FAIL: TestReadHandler (0.00s)
/Users/mhoppa509/go/src/github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api/shared_handlers_test.go:187:
Expected body {"response":[{"ID":1}]} got {"response":[{"ID":1}]}
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4015: Rewrite /federations to Go - POST/PUT/DELETE
mhoppa commented on a change in pull request #4015: Rewrite /federations to Go
- POST/PUT/DELETE
URL: https://github.com/apache/trafficcontrol/pull/4015#discussion_r339808566
##
File path: traffic_ops/traffic_ops_golang/federations/federations.go
##
@@ -167,3 +206,296 @@ ORDER BY
}
return feds, nil
}
+
+// AddFederationResorverMappingsForCurrentUser is the handler for a POST
request to /federations.
+// Confusingly, it does not create a federation, but is instead used to
manipulate the resolvers
+// used by one or more particular Delivery Services for one or more particular
Federations.
+func AddFederationResolverMappingsForCurrentUser(w http.ResponseWriter, r
*http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil)
+ tx := inf.Tx.Tx
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ mappings, userErr, sysErr := getMappingsFromRequestBody(*inf.Version,
r.Body)
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, userErr, sysErr)
+ return
+ }
+
+ if err := mappings.Validate(tx); err != nil {
+ errCode = http.StatusBadRequest
+ userErr = fmt.Errorf("validating request: %v", err)
+ api.HandleErr(w, r, tx, errCode, userErr, nil)
+ return
+ }
+
+ userErr, sysErr, errCode =
addFederationResolverMappingsForCurrentUser(inf.User, tx, mappings)
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
+ return
+ }
+
+ msg := fmt.Sprintf("%s successfully created federation resolvers.",
inf.User.UserName)
+ if inf.Version.Minor <= 3 {
+ api.WriteResp(w, r, msg)
+ } else {
+ api.WriteRespAlertObj(w, r, tc.SuccessLevel, msg, msg)
+ }
+}
+
+// handles the main logic of the POST handler, separated out for convenience
+func addFederationResolverMappingsForCurrentUser(u *auth.CurrentUser, tx
*sql.Tx, mappings []tc.DeliveryServiceFederationResolverMapping) (error, error,
int) {
+ for _, fed := range mappings {
+ dsTenant, ok, err := dbhelpers.GetDSTenantIDFromXMLID(tx,
fed.DeliveryService)
+ if err != nil {
+ return nil, err, http.StatusInternalServerError
+ } else if !ok {
+ return fmt.Errorf("'%s' - no such Delivery Service",
fed.DeliveryService), nil, http.StatusConflict
+ }
+
+ if ok, err = tenant.IsResourceAuthorizedToUserTx(dsTenant, u,
tx); err != nil {
+ err = fmt.Errorf("Checking user #%d tenancy permissions
on DS '%s' (tenant #%d)", u.ID, fed.DeliveryService, dsTenant)
+ return nil, err, http.StatusInternalServerError
+ } else if !ok {
+ userErr := fmt.Errorf("'%s' - no such Delivery
Service", fed.DeliveryService)
+ sysErr := fmt.Errorf("User '%s' requested unauthorized
federation resolver mapping modification on the '%s' Delivery Service",
u.UserName, fed.DeliveryService)
+ return userErr, sysErr, http.StatusConflict
+ }
+
+ fedID, ok, err := dbhelpers.GetFederationIDForUserIDByXMLID(tx,
u.ID, fed.DeliveryService)
+ if err != nil {
+ return nil, fmt.Errorf("Getting Federation ID: %v",
err), http.StatusInternalServerError
+ } else if !ok {
+ err = fmt.Errorf("No federation(s) found for user %s on
delivery service '%s'.", u.UserName, fed.DeliveryService)
+ return err, nil, http.StatusConflict
+ }
+
+ inserted, err :=
addFederationResolverMappingsToFederation(fed.Mappings, fed.DeliveryService,
fedID, tx)
+ if err != nil {
+ err = fmt.Errorf("Adding federation resolver mapping(s)
to federation: %v", err)
+ return nil, err, http.StatusInternalServerError
+ }
+
+ changelogMsg := "FEDERATION DELIVERY SERVICE: %s, ID: %d,
ACTION: User %s successfully added federation resolvers [ %s ]"
+ changelogMsg = fmt.Sprintf(changelogMsg, fed.DeliveryService,
fedID, u.UserName, inserted)
+ api.CreateChangeLogRawTx(api.ApiChange, changelogMsg, u, tx)
+ }
+ return nil, nil, http.StatusOK
+}
+
+// adds federation resolver mappings for a particular delivery service to a
given federation, creating said resolvers if
+// they don't already exist.
+func addFederationResolverMappingsToFederation(res tc.ResolverMapping, xmlid
string, fed uint, tx *sql.Tx) (string, error) {
+ var resp string
+ if len(res.Resolve4) > 0 {
+
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4015: Rewrite /federations to Go - POST/PUT/DELETE
mhoppa commented on a change in pull request #4015: Rewrite /federations to Go
- POST/PUT/DELETE
URL: https://github.com/apache/trafficcontrol/pull/4015#discussion_r339600297
##
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##
@@ -346,3 +430,17 @@ func GetCacheGroupNameFromID(tx *sql.Tx, id int64)
(tc.CacheGroupName, bool, err
}
return tc.CacheGroupName(name), true, nil
}
+
+// GetFederationIDForUserIDByXMLID retrieves the ID of the Federation assigned
to the user defined by
+// userID on the Delivery Service identified by xmlid. If no such federation
exists, the boolean
+// returned will be 'false', while the error indicates unexpected errors that
occurred when querying.
+func GetFederationIDForUserIDByXMLID(tx *sql.Tx, userID int, xmlid string)
(uint, bool, error) {
+ var id uint
Review comment:
I am curious why a uint here?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4015: Rewrite /federations to Go - POST/PUT/DELETE
mhoppa commented on a change in pull request #4015: Rewrite /federations to Go
- POST/PUT/DELETE
URL: https://github.com/apache/trafficcontrol/pull/4015#discussion_r339566176
##
File path: lib/go-tc/federation.go
##
@@ -67,36 +80,85 @@ type FederationMapping struct {
TTL int`json:"ttl"`
}
-// AllFederation is the JSON object returned by /api/1.x/federations?all
-type AllFederation struct {
- Mappings[]AllFederationMapping `json:"mappings"`
- DeliveryService DeliveryServiceName`json:"deliveryService"`
+// AllDeliveryServiceFederationsMapping is a structure that contains
identifying information for a
+// Delivery Service as well as any and all Federation Resolver mapping
assigned to it (or all those
+// getting assigned to it).
+type AllDeliveryServiceFederationsMapping struct {
+ Mappings[]FederationResolverMapping `json:"mappings"`
+ DeliveryService DeliveryServiceName `json:"deliveryService"`
}
-func (a AllFederation) IsAllFederations() bool { return true }
+// IsAllFederations implements the IAllFederation interface. Always returns
true.
+func (a AllDeliveryServiceFederationsMapping) IsAllFederations() bool { return
true }
// AllFederation is the JSON object returned by
/api/1.x/federations?all&cdnName=my-cdn-name
type AllFederationCDN struct {
CDNName *CDNName `json:"cdnName"`
}
+// IsAllFederations implements the IAllFederation interface. Always returns
true.
func (a AllFederationCDN) IsAllFederations() bool { return true }
-type AllFederationMapping struct {
- TTL *int `json:"ttl"`
- CName*string `json:"cname"`
+type ResolverMapping struct {
Resolve4 []string `json:"resolve4,omitempty"`
Resolve6 []string `json:"resolve6,omitempty"`
}
+func (r *ResolverMapping) Validate(tx *sql.Tx) error {
+ errs := []error{}
+ for _, res := range r.Resolve4 {
Review comment:
This looks good but anyway we could leverage
https://github.com/go-ozzo/ozzo-validation#built-in-validation-rules to
accomplish this? I see they have validation for ipv4/ipv6.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4015: Rewrite /federations to Go - POST/PUT/DELETE
mhoppa commented on a change in pull request #4015: Rewrite /federations to Go - POST/PUT/DELETE URL: https://github.com/apache/trafficcontrol/pull/4015#discussion_r339599291 ## File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go ## @@ -42,6 +42,75 @@ const BaseOrderBy = "\nORDER BY" const BaseLimit = "\nLIMIT" const BaseOffset = "\nOFFSET" +const UserIDHasAccessToDeliveryServiceXMLIDQuery = ` Review comment: is this used anywhere? 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: us...@infra.apache.org With regards, Apache Git Services
