-
Notifications
You must be signed in to change notification settings - Fork 344
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.
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
Fixed lastUpdated timestamp format in api v5 federation resolvers #7596
Changes from 11 commits
daec738
7e28bcf
d02c96b
4870a84
5171f6c
57cdedc
e2384b2
a0412cd
d9c2ef6
98a9ca5
65094f0
a9d345c
47337a6
c2bde3f
c7e5f6d
e6a2eac
f027940
c911558
2d86f48
6a3e0f1
ef6a13f
8e923f0
98bd9a5
7aea476
d3f17ac
80001fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ import ( | |
"database/sql" | ||
"errors" | ||
"net" | ||
"time" | ||
|
||
validation "github.com/go-ozzo/ozzo-validation" | ||
) | ||
|
@@ -50,6 +51,36 @@ type FederationResolver struct { | |
TypeID *uint `json:"typeId,omitempty" db:"type"` | ||
} | ||
|
||
// [V5] FederationResolverV5 is an alias for the Federal Resolver struct response used for the latest minor version associated with api major version 5. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The GoDoc comment should be gin with the name of the function/ struct that it applies to. |
||
type FederationResolverV5 = FederationResolverV50 | ||
|
||
// [V50] FederationResolverV50 is used for RFC3339 format timestamp in FederationResolver which represents a resolver record for a CDN Federation. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here about godocs. |
||
type FederationResolverV50 struct { | ||
ID *uint `json:"id" db:"id"` | ||
IPAddress *string `json:"ipAddress" db:"ip_address"` | ||
LastUpdated *time.Time `json:"lastUpdated,omitempty" db:"last_updated"` | ||
Type *string `json:"type"` | ||
TypeID *uint `json:"typeId,omitempty" db:"type"` | ||
} | ||
|
||
// [V5] FederationResolversV5Response is an alias for the FederationResolvers struct response used for the latest minor version associated with api major version 5. | ||
type FederationResolversV5Response = FederationResolversV50Response | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't look like this comment was addressed :) |
||
|
||
// [V50] GET request to its /federation_resolvers endpoint for APIv5. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here |
||
type FederationResolversV50Response struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. V50 should be at the end of the name of the struct here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one as well. |
||
Alerts | ||
Response []FederationResolverV5 `json:"response"` | ||
} | ||
|
||
// [V5] FederationResolverV5Response represents struct response used for the latest minor version associated with api major version 5. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here |
||
type FederationResolverV5Response = FederationResolverV50Response | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. V5 should be at the end of the names of the structs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
|
||
// [V50] POST request to its /federation_resolvers endpoint APIv5. | ||
type FederationResolverV50Response struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, following the conventions in our repo, the struct name should end with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And here. |
||
Alerts | ||
Response FederationResolverV5 `json:"response"` | ||
} | ||
|
||
// Validate implements the github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api.ParseValidator | ||
// interface. | ||
func (fr *FederationResolver) Validate(tx *sql.Tx) error { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -217,3 +217,14 @@ func StringIsValidFloat() *validation.StringRule { | |
return err == nil && !math.IsNaN(validated) | ||
}, "must be a valid float") | ||
} | ||
|
||
// IsValidIPorCIDR returns whether the input is a valid IP address or CIDR notation subnet | ||
func IsValidIPorCIDR(input string) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: The name should be |
||
ip := net.ParseIP(input) | ||
if ip != nil { | ||
return true // Valid IP address | ||
} | ||
|
||
_, _, err := net.ParseCIDR(input) | ||
return err == nil // Valid CIDR notation subnet | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we return the error from here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now removed as i removed post method itself. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,19 +23,24 @@ package federation_resolvers | |
|
||
import ( | ||
"database/sql" | ||
"encoding/json" | ||
"errors" | ||
"fmt" | ||
"net/http" | ||
"time" | ||
|
||
"github.com/apache/trafficcontrol/lib/go-log" | ||
"github.com/apache/trafficcontrol/lib/go-rfc" | ||
"github.com/apache/trafficcontrol/lib/go-tc" | ||
"github.com/apache/trafficcontrol/lib/go-tc/tovalidate" | ||
"github.com/apache/trafficcontrol/lib/go-util" | ||
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api" | ||
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers" | ||
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims" | ||
|
||
"github.com/jmoiron/sqlx" | ||
|
||
validation "github.com/go-ozzo/ozzo-validation" | ||
) | ||
|
||
const insertFederationResolverQuery = ` | ||
|
@@ -301,3 +306,153 @@ func deleteFederationResolver(inf *api.APIInfo) (tc.Alert, tc.FederationResolver | |
|
||
return alert, result, userErr, sysErr, statusCode | ||
} | ||
|
||
// GetFederationResolvers [V5] - get list of federation resolver for APIv5 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since federation_resolvers is not using the CRUDer interface, it will be much easier (and would involve lesser lines of code) if we could just check the api version in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice, i would do this srijeet |
||
func GetFederationResolvers(w http.ResponseWriter, r *http.Request) { | ||
var maxTime time.Time | ||
var runSecond bool | ||
inf, sysErr, userErr, errCode := api.NewInfo(r, nil, nil) | ||
tx := inf.Tx | ||
if sysErr != nil || userErr != nil { | ||
api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr) | ||
return | ||
} | ||
defer inf.Close() | ||
|
||
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{ | ||
"id": dbhelpers.WhereColumnInfo{Column: "federation_resolver.id", Checker: api.IsInt}, | ||
"ipAddress": dbhelpers.WhereColumnInfo{Column: "federation_resolver.ip_address"}, | ||
"type": dbhelpers.WhereColumnInfo{Column: "type.name"}, | ||
} | ||
if _, ok := inf.Params["orderby"]; !ok { | ||
inf.Params["orderby"] = "id" | ||
} | ||
|
||
where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params, queryParamsToQueryCols) | ||
if len(errs) > 0 { | ||
sysErr = util.JoinErrs(errs) | ||
errCode = http.StatusBadRequest | ||
api.HandleErr(w, r, tx.Tx, errCode, nil, sysErr) | ||
return | ||
} | ||
|
||
query := readQuery + where + orderBy + pagination | ||
useIMS := false | ||
config, e := api.GetConfig(r.Context()) | ||
if e == nil && config != nil { | ||
useIMS = config.UseIMS | ||
} else { | ||
log.Warnf("Couldn't get config %v", e) | ||
} | ||
if useIMS { | ||
runSecond, maxTime = ims.TryIfModifiedSinceQuery(tx, r.Header, queryValues, SelectMaxLastUpdatedQuery(where, "federation_resolver")) | ||
if !runSecond { | ||
log.Debugln("IMS HIT") | ||
api.AddLastModifiedHdr(w, maxTime) | ||
w.WriteHeader(http.StatusNotModified) | ||
api.WriteResp(w, r, []tc.FederationResolverV5{}) | ||
return | ||
} | ||
log.Debugln("IMS MISS") | ||
} else { | ||
log.Debugln("Non IMS request") | ||
} | ||
|
||
rows, err := inf.Tx.NamedQuery(query, queryValues) | ||
|
||
if err != nil { | ||
userErr, sysErr, errCode = api.ParseDBError(err) | ||
if sysErr != nil { | ||
sysErr = fmt.Errorf("federation_resolver read query: %v", sysErr) | ||
} | ||
|
||
api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr) | ||
return | ||
} | ||
defer rows.Close() | ||
|
||
var resolvers = []tc.FederationResolverV5{} | ||
for rows.Next() { | ||
var resolver tc.FederationResolverV5 | ||
if err := rows.Scan(&resolver.ID, &resolver.IPAddress, &resolver.LastUpdated, &resolver.Type); err != nil { | ||
userErr, sysErr, errCode = api.ParseDBError(err) | ||
if sysErr != nil { | ||
sysErr = fmt.Errorf("federation_resolver scanning: %v", sysErr) | ||
} | ||
api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr) | ||
return | ||
} | ||
|
||
resolvers = append(resolvers, resolver) | ||
} | ||
|
||
api.WriteResp(w, r, resolvers) | ||
} | ||
|
||
// CreateFederationResolver [V5] - creates the federation resolver with given data for APIv5 | ||
func CreateFederationResolver(w http.ResponseWriter, r *http.Request) { | ||
|
||
inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil) | ||
if userErr != nil || sysErr != nil { | ||
api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) | ||
return | ||
} | ||
defer inf.Close() | ||
tx := inf.Tx.Tx | ||
|
||
fr, readValErr := readAndValidateJsonStructV5(r) | ||
if readValErr != nil { | ||
api.HandleErr(w, r, tx, http.StatusBadRequest, readValErr, nil) | ||
return | ||
} | ||
|
||
// check if type already exists | ||
var exists bool | ||
checkQuery := fmt.Sprintf("SELECT EXISTS(%s WHERE ip_address = $1)", readQuery) | ||
err := tx.QueryRow(checkQuery, fr.IPAddress).Scan(&exists) | ||
|
||
if err != nil { | ||
api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("error: %w, when checking if federation_resolver with ip_address %s exists", err, fr.IPAddress)) | ||
return | ||
} | ||
if exists { | ||
api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("federation_resolver ip_address '%s' already exists", *fr.IPAddress), nil) | ||
return | ||
} | ||
|
||
// create type | ||
err = tx.QueryRow(insertFederationResolverQuery, fr.IPAddress, fr.TypeID).Scan(&fr.ID, &fr.IPAddress, &fr.Type, &fr.TypeID) | ||
if err != nil { | ||
if errors.Is(err, sql.ErrNoRows) { | ||
api.HandleErr(w, r, tx, http.StatusInternalServerError, fmt.Errorf("error: %w in creating federation_resolver ip_address with ip_address: %s", err, fr.IPAddress), nil) | ||
return | ||
} | ||
usrErr, sysErr, code := api.ParseDBError(err) | ||
api.HandleErr(w, r, tx, code, usrErr, sysErr) | ||
return | ||
} | ||
message := fmt.Sprintf("Federation Resolver created [ IP = %s ] with id: %d", *fr.IPAddress, *fr.ID) | ||
alerts := tc.CreateAlerts(tc.SuccessLevel, message) | ||
w.Header().Set("Location", fmt.Sprintf("/api/%d.%d/federation_resolvers?id=%s", inf.Version.Major, inf.Version.Minor, fr.ID)) | ||
api.WriteAlertsObj(w, r, http.StatusCreated, alerts, fr) | ||
return | ||
} | ||
|
||
// readAndValidateJsonStructV5 [V5] - validates the JSON object passed for APIv5 | ||
func readAndValidateJsonStructV5(r *http.Request) (tc.FederationResolverV5, error) { | ||
var fr tc.FederationResolverV5 | ||
if err := json.NewDecoder(r.Body).Decode(&fr); err != nil { | ||
userErr := fmt.Errorf("error decoding POST request body into FederationResolverV5 struct %w", err) | ||
return fr, userErr | ||
} | ||
|
||
// validate JSON body | ||
rule := validation.NewStringRule(tovalidate.IsValidIPorCIDR, "invalid network IP or CIDR-notation subnet.") | ||
errs := tovalidate.ToErrors(validation.Errors{ | ||
"ip_address": validation.Validate(fr.IPAddress, validation.Required, rule), | ||
}) | ||
if len(errs) > 0 { | ||
userErr := util.JoinErrs(errs) | ||
return fr, userErr | ||
} | ||
return fr, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -466,8 +466,8 @@ func Routes(d ServerData) ([]Route, http.Handler, error) { | |
{Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodDelete, Path: `federations/{id}/deliveryservices/{dsID}/?$`, Handler: api.DeleteHandler(&federations.TOFedDSes{}), RequiredPrivLevel: auth.PrivLevelAdmin, RequiredPermissions: []string{"FEDERATION:UPDATE", "DELIVERY-SERVICE:UPDATE", "FEDERATION:READ", "DELIVERY-SERVICE:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 441740257031}, | ||
|
||
// Federation Resolvers | ||
{Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodPost, Path: `federation_resolvers/?$`, Handler: federation_resolvers.Create, RequiredPrivLevel: auth.PrivLevelAdmin, RequiredPermissions: []string{"FEDERATION-RESOLVER:CREATE", "TYPE:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 413437366131}, | ||
{Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodGet, Path: `federation_resolvers/?$`, Handler: federation_resolvers.Read, RequiredPrivLevel: auth.PrivLevelReadOnly, RequiredPermissions: []string{"FEDERATION-RESOLVER:READ", "TYPE:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 45660875931}, | ||
{Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodPost, Path: `federation_resolvers/?$`, Handler: federation_resolvers.CreateFederationResolver, RequiredPrivLevel: auth.PrivLevelAdmin, RequiredPermissions: []string{"FEDERATION-RESOLVER:CREATE", "TYPE:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 413437366131}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you just used the same functions with the api version checks in there, you wouldn't need to change anything in this file. |
||
{Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodGet, Path: `federation_resolvers/?$`, Handler: federation_resolvers.GetFederationResolvers, RequiredPrivLevel: auth.PrivLevelReadOnly, RequiredPermissions: []string{"FEDERATION-RESOLVER:READ", "TYPE:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 45660875931}, | ||
{Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodPost, Path: `federations/{id}/federation_resolvers/?$`, Handler: federations.AssignFederationResolversToFederationHandler, RequiredPrivLevel: auth.PrivLevelAdmin, RequiredPermissions: []string{"FEDERATION:UPDATE", "FEDERATION:READ", "FEDERATION-RESOLVER:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 45660876031}, | ||
{Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodGet, Path: `federations/{id}/federation_resolvers/?$`, Handler: federations.GetFederationFederationResolversHandler, RequiredPrivLevel: auth.PrivLevelReadOnly, RequiredPermissions: []string{"FEDERATION:READ", "FEDERATION-RESOLVER:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 45660876131}, | ||
{Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodDelete, Path: `federation_resolvers/?$`, Handler: federation_resolvers.Delete, RequiredPrivLevel: auth.PrivLevelAdmin, RequiredPermissions: []string{"FEDERATION-RESOLVER:DELETE", "TYPE:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 400131}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a part of the "fixed" section.