Fixed lastUpdated timestamp format in api v5 federation resolvers #7596

gbkannan89 · 2023-06-23T09:04:27Z

Related: #5911

Fixes federation_resolver v5 apis to respond with RFC3339 date/time Format.

Which Traffic Control components are affected by this PR?

Documentation
Traffic Ops

What is the best way to verify this PR?

Make Api calls to types 5.0

Ferderation Resolver to Use RFC3339 Format

  --url https://localhost:8443/api/5.0/federation_resolvers


{ 
  "response": [
       {
		"id": 1,
		"ipAddress": "::1/1",
		"lastUpdated": "2019-11-06T15:18:14.952814+05:30",
		"type": "RESOLVE6"
	}
  ]
}

If this is a bugfix, which Traffic Control versions contained the bug?

-7.0.1

PR submission checklist

This PR has tests
This PR has documentation
This PR has a CHANGELOG.md entry
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

codecov · 2023-06-26T19:47:20Z

Codecov Report

Merging #7596 (80001fe) into master (fabdce4) will increase coverage by 0.00%.
The diff coverage is 15.38%.

@@            Coverage Diff            @@
##             master    #7596   +/-   ##
=========================================
  Coverage     30.13%   30.13%           
  Complexity       98       98           
=========================================
  Files           794      794           
  Lines         84078    84054   -24     
  Branches        907      907           
=========================================
  Hits          25333    25333           
+ Misses        56615    56591   -24     
  Partials       2130     2130

Flag	Coverage Δ
golib_unit	`48.26% <0.00%> (-0.04%)`	⬇️
grove_unit	`4.60% <ø> (ø)`
t3c_unit	`5.28% <ø> (ø)`
traffic_monitor_unit	`21.30% <ø> (ø)`
traffic_ops_integration	`69.42% <100.00%> (ø)`
traffic_ops_unit	`22.97% <0.00%> (+0.01%)`	⬆️
traffic_portal_v2	`73.78% <ø> (ø)`
traffic_stats_unit	`10.14% <ø> (ø)`
unit_tests	`27.26% <0.00%> (+<0.01%)`	⬆️
v3	`57.79% <ø> (ø)`
v4	`79.18% <ø> (ø)`
v5	`78.63% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/go-tc/federation_resolver.go	`45.45% <0.00%> (-31.47%)`	⬇️
...olang/federation_resolvers/federation_resolvers.go	`0.00% <0.00%> (ø)`
traffic_ops/v5-client/federation_resolver.go	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go

srijeet0406 · 2023-07-05T20:42:21Z

CHANGELOG.md

@@ -59,6 +59,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - [#7469](https://github.com/apache/trafficcontrol/pull/7469) *Traffic Ops* Changed logic to not report empty or missing cookies into TO error.log.
 - [#7586](https://github.com/apache/trafficcontrol/pull/7586) *Traffic Ops* Add permission to Operations Role to read from dnsseckeys endpoint.
 - [#7600](https://github.com/apache/trafficcontrol/pull/7600) *t3c* changed default go-direct command line arg to be old to avoid unexpected config changes upon upgrade.
+- [#7596](https://github.com/apache/trafficcontrol/pull/7596) *Traffic Ops* Fixes `federation_resolvers` v5 apis to respond with `RFC3339` date/time Format.


This should be a part of the "fixed" section.

srijeet0406 · 2023-07-05T20:44:16Z

lib/go-tc/federation_resolver.go

@@ -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.


The GoDoc comment should be gin with the name of the function/ struct that it applies to.

srijeet0406 · 2023-07-05T20:44:33Z

lib/go-tc/federation_resolver.go

+// [V5] FederationResolverV5 is an alias for the Federal Resolver struct response used for the latest minor version associated with api major version 5.
+type FederationResolverV5 = FederationResolverV50
+
+// [V50] FederationResolverV50 is used for RFC3339 format timestamp in FederationResolver which represents a resolver record for a CDN Federation.


Same comment here about godocs.

srijeet0406 · 2023-07-05T20:45:51Z

lib/go-tc/federation_resolver.go

+}
+
+// [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


Same comment here

Doesn't look like this comment was addressed :)

srijeet0406 · 2023-07-05T20:45:58Z

lib/go-tc/federation_resolver.go

+// [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
+
+// [V50] GET request to its /federation_resolvers endpoint for APIv5.


Same comment here

srijeet0406 · 2023-07-05T20:49:01Z

lib/go-tc/federation_resolver.go

+type FederationResolversV5Response = FederationResolversV50Response
+
+// [V50] GET request to its /federation_resolvers endpoint for APIv5.
+type FederationResolversV50Response struct {


V50 should be at the end of the name of the struct here

This one as well.

srijeet0406 · 2023-07-05T20:50:23Z

lib/go-tc/tovalidate/rules.go

@@ -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 {


nit: The name should be IsValidIPOrCIDR, instead of IsValidIPorCIDR
Note that the O needs to be capitalized

srijeet0406 · 2023-07-05T20:51:05Z

lib/go-tc/tovalidate/rules.go

+	}
+
+	_, _, err := net.ParseCIDR(input)
+	return err == nil // Valid CIDR notation subnet


Should we return the error from here?

now removed as i removed post method itself.

srijeet0406 · 2023-07-05T20:58:38Z

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go

@@ -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


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 Get and Create methods of federation_resolvers, and return the appropriate structs.
You can take a look at this PR for reference: #7547

nice, i would do this srijeet

srijeet0406 · 2023-07-05T20:59:13Z

traffic_ops/traffic_ops_golang/routing/routes.go

@@ -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},


If you just used the same functions with the api version checks in there, you wouldn't need to change anything in this file.

srijeet0406 · 2023-07-05T21:00:24Z

Also, could you please change the title of the PR to be something like
Fixed lastUpdated timestamp format in api v5 federation resolvers

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go

srijeet0406 · 2023-07-10T17:24:41Z

lib/go-tc/federation_resolver.go

@@ -50,6 +51,36 @@ type FederationResolver struct {
 	TypeID      *uint      `json:"typeId,omitempty" db:"type"`
 }

+// FederationResolverV5 [V5] - is an alias for the Federal Resolver struct response used for the latest minor version associated with APIv5.


nit: Do we need the text within the brackets?

srijeet0406 · 2023-07-10T17:24:48Z

lib/go-tc/federation_resolver.go

+// FederationResolverV5 [V5] - is an alias for the Federal Resolver struct response used for the latest minor version associated with APIv5.
+type FederationResolverV5 = FederationResolverV50
+
+// FederationResolverV50 [V50]- is used for RFC3339 format timestamp in FederationResolver which represents a resolver record for a CDN Federation for APIv50.


Same comment here

srijeet0406 · 2023-07-10T17:25:33Z

lib/go-tc/federation_resolver.go

+}
+
+// [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


Doesn't look like this comment was addressed :)

srijeet0406 · 2023-07-10T17:25:45Z

lib/go-tc/federation_resolver.go

+type FederationResolversV5Response = FederationResolversV50Response
+
+// [V50] GET request to its /federation_resolvers endpoint for APIv5.
+type FederationResolversV50Response struct {


This one as well.

srijeet0406 · 2023-07-10T17:25:54Z

lib/go-tc/federation_resolver.go

+}
+
+// [V5] FederationResolverV5Response represents struct response used for the latest minor version associated with api major version 5.
+type FederationResolverV5Response = FederationResolverV50Response


srijeet0406 · 2023-07-10T17:27:54Z

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go

@@ -103,6 +101,7 @@ func Create(w http.ResponseWriter, r *http.Request) {
 }

 // Read is the handler for GET requests to /federation_resolvers (and /federation_resolvers/{{ID}}).
+// here based on request V5 is identified to get list of federation resolver for APIv5


I don't think you need this godoc comment here.

srijeet0406 · 2023-07-10T17:28:50Z

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go

+
+	// Based on version we load types - for version 5 and above we use FederationResolverV5
+	var resolvers []interface{}
+	if inf.Version != nil && inf.Version.Major >= 5 && inf.Version.Minor >= 0 {


This is fine, but there is a utility function that can be used in place of this if statement like this:
if inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 5, Minor: 0})

srijeet0406 · 2023-07-10T17:34:22Z

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go

+		for _, item := range frData {
+			resolvers = append(resolvers, item)
+		}
+	}


This entire code block is not needed, because, on lines 142 and 147, you're declaring a new variable (array), and on the subsequent lines, you're traversing over that array. There are 0 entries in the array, so traversing through it is needless.

srijeet0406 · 2023-07-10T17:35:37Z

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go

-			if sysErr != nil {
-				sysErr = fmt.Errorf("federation_resolver scanning: %v", sysErr)
+		// Based on version we load types - for version 5 and above we use FederationResolverV5
+		if inf.Version != nil && inf.Version.Major >= 5 && inf.Version.Minor >= 0 {


You could use the pre defined utility function here, but it's your call.

srijeet0406 · 2023-07-10T17:37:13Z

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go

 			}
-			api.HandleErr(w, r, tx, errCode, userErr, sysErr)
-			return
+			resolvers = append(resolvers, resolver)


Instead of repeating this block twice, here's what I'd do:
Scan the struct into a v4 struct as it was being done earlier. Then, check for the version, and if it's >= 5.0, then convert the v4 struct into a v5 struct before appending it to the interface array resolvers.

srijeet0406 · 2023-07-11T16:33:07Z

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go

+			var v5Resolver tc.FederationResolverV5
+
+			// Convert FederationResolver fields to FederationResolverV5 fields
+			v5Resolver.ID = resolver.ID


This looks good. Do you mind making it into a utility function please, so that future development can make use of it and not have to repeat these lines everywhere?

srijeet0406 · 2023-07-11T16:33:42Z

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go

+			v5Resolver.LastUpdated = &lastUpdated
+			v5Resolver.Type = resolver.Type
+
+			resolvers = append(resolvers, &v5Resolver)


Should this be appending the reference of v5Resolver?

here we are trying to append value of v5Resolver to resolvers, is that done wrongly ?

It needs to append the value of v5Resolver, not the reference (address), which is what the & does.
So, it should just be

resolvers = append(resolvers, v5Resolver)

srijeet0406

Looks great! Manual testing also passes.

…ache#7596) * fr changes * fr changes * changelog added * type import correction * fr codefix * fr code changes * comment correction * fixes * comments addressed * changelog correction * fr changed to give rfc format res * removed unused function * version revert for deletion * space added * comments addressed * comments addressed * reference mistake correction

gbkannan89 added 4 commits June 22, 2023 19:42

fr changes

daec738

Merge branch 'master' into federation-resolvers-v5

7e28bcf

fr changes

d02c96b

changelog added

4870a84

ocket8888 added Traffic Ops related to Traffic Ops documentation related to documentation low impact affects only a small portion of a CDN, and cannot itself break one tech debt rework due to choosing easy/limited solution labels Jun 23, 2023

type import correction

5171f6c

gbkannan89 marked this pull request as ready for review June 26, 2023 08:11

github-advanced-security bot found potential problems Jun 26, 2023

View reviewed changes

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go Fixed Show fixed Hide fixed

gbkannan89 added 6 commits July 4, 2023 11:23

Merge branch 'master' into federation-resolvers-v5

57cdedc

fr codefix

e2384b2

fr code changes

a0412cd

comment correction

d9c2ef6

fixes

98a9ca5

Merge branch 'master' into federation-resolvers-v5

65094f0

srijeet0406 requested changes Jul 5, 2023

View reviewed changes

github-advanced-security bot found potential problems Jul 5, 2023

View reviewed changes

traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go Fixed Show fixed Hide fixed

Merge branch 'master' into federation-resolvers-v5

a9d345c

gbkannan89 changed the title ~~Federation resolvers v5~~ Fixed lastUpdated timestamp format in api v5 federation resolvers Jul 6, 2023

gbkannan89 added 4 commits July 6, 2023 12:28

comments addressed

47337a6

Merge branch 'master' into federation-resolvers-v5

c2bde3f

changelog correction

c7e5f6d

fr changed to give rfc format res

e6a2eac

gbkannan89 requested a review from srijeet0406 July 7, 2023 08:22

gbkannan89 added 2 commits July 7, 2023 13:54

removed unused function

f027940

version revert for deletion

c911558

gbkannan89 added 2 commits July 7, 2023 14:17

space added

2d86f48

Merge branch 'master' into federation-resolvers-v5

6a3e0f1

srijeet0406 requested changes Jul 10, 2023

View reviewed changes

gbkannan89 added 2 commits July 11, 2023 20:55

Merge branch 'master' into federation-resolvers-v5

ef6a13f

comments addressed

8e923f0

gbkannan89 requested a review from srijeet0406 July 11, 2023 16:17

srijeet0406 reviewed Jul 11, 2023

View reviewed changes

gbkannan89 added 3 commits July 12, 2023 17:45

Merge branch 'master' into federation-resolvers-v5

98bd9a5

Merge branch 'master' into federation-resolvers-v5

7aea476

comments addressed

d3f17ac

gbkannan89 requested a review from srijeet0406 July 13, 2023 07:51

reference mistake correction

80001fe

srijeet0406 approved these changes Jul 14, 2023

View reviewed changes

srijeet0406 merged commit db67a94 into apache:master Jul 14, 2023

rimashah25 added this to the 8.0.0 milestone Jul 17, 2023

gbkannan89 deleted the federation-resolvers-v5 branch August 28, 2023 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed lastUpdated timestamp format in api v5 federation resolvers #7596

Fixed lastUpdated timestamp format in api v5 federation resolvers #7596

gbkannan89 commented Jun 23, 2023 •

edited by ocket8888

Loading

codecov bot commented Jun 26, 2023 •

edited

Loading

srijeet0406 Jul 5, 2023

srijeet0406 Jul 5, 2023

srijeet0406 Jul 5, 2023

srijeet0406 Jul 5, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 5, 2023

srijeet0406 Jul 5, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 5, 2023

srijeet0406 Jul 5, 2023

gbkannan89 Jul 7, 2023

srijeet0406 Jul 5, 2023

gbkannan89 Jul 6, 2023

srijeet0406 Jul 5, 2023

srijeet0406 commented Jul 5, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 10, 2023

srijeet0406 Jul 11, 2023

srijeet0406 Jul 11, 2023

gbkannan89 Jul 12, 2023

srijeet0406 Jul 12, 2023 •

edited

Loading

srijeet0406 left a comment

Fixed lastUpdated timestamp format in api v5 federation resolvers #7596

Fixed lastUpdated timestamp format in api v5 federation resolvers #7596

Conversation

gbkannan89 commented Jun 23, 2023 • edited by ocket8888 Loading

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

If this is a bugfix, which Traffic Control versions contained the bug?

PR submission checklist

codecov bot commented Jun 26, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijeet0406 commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijeet0406 Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

srijeet0406 left a comment

Choose a reason for hiding this comment

gbkannan89 commented Jun 23, 2023 •

edited by ocket8888

Loading

codecov bot commented Jun 26, 2023 •

edited

Loading

srijeet0406 Jul 12, 2023 •

edited

Loading