Add support for Get/Patch API Shield API Discovery Operations #1413

djhworld · 2023-09-29T17:14:22Z

This change adds support for the following API Shield related endpoints related to API Discovery:

Retrieve discovered operations on a zone (https://developers.cloudflare.com/api/operations/api-shield-api-discovery-retrieve-discovered-operations-on-a-zone)
Patch discovered operations (https://developers.cloudflare.com/api/operations/api-shield-api-patch-discovered-operations)
Patch discovered operation (https://developers.cloudflare.com/api/operations/api-shield-api-patch-discovered-operation)

Description

This change adds support for API Shield endpoints related to API Shield API Discovery. This is so they can be used from the library and will enable this feature to be added to terraform in the future.

Has your change been tested?

Unit tests
Local testing

Types of changes

What sort of change does your code introduce/modify?

New feature (non-breaking change which adds functionality)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
This change is using publicly documented in cloudflare/api-schemas
and relies on stable APIs. (see API Shield API Discovery)

github-actions · 2023-09-29T17:16:02Z

changelog detected ✅

…ions This change adds support for the following API Shield related endpoints related to API Discovery: - Retrieve discovered operations on a zone - Patch discovered operations - Patch discovered operation

djhworld · 2023-10-03T16:41:41Z

PR now ready for review after documentation updated: https://developers.cloudflare.com/api/operations/api-shield-api-discovery-retrieve-discovered-operations-on-a-zone

.changelog/1413.txt

api_shield_api_discovery.go

jacobbednarz · 2023-10-03T22:13:17Z

api_shield_api_discovery.go

+// PatchAPIShieldDiscoveryOperation updates certain fields on a discovered operation
+//
+// API Documentation: https://developers.cloudflare.com/api/operations/api-shield-api-patch-discovered-operation
+func (api *API) PatchAPIShieldDiscoveryOperation(ctx context.Context, rc *ResourceContainer, params PatchAPIShieldDiscoveryOperationParams) (*PatchAPIShieldDiscoveryOperation, error) {


instead of Patch, we prefer to use Update unless the HTTP method is important context to have for the caller. more details in https://github.com/cloudflare/cloudflare-go/blob/master/docs/experimental.md#consistent-crud-method-signatures

makes sense to me - thanks!

api_shield_api_discovery.go

jacobbednarz · 2023-10-03T22:55:00Z

i went ahead and updated this to match the library conventions from the code review. feel free to take a look and let me know if anything doesn't make sense. otherwise, i'll merge this one in.

janrueth

Looks good to me. I'd provide consts for enum like filter conditions that are modeled here as strings

janrueth · 2023-10-04T08:48:15Z

api_shield_api_discovery.go

+	// Origin represents the API discovery engine(s) that discovered this operation
+	Origin []string `json:"origin"`
+	// State represents the state of operation in API Discovery
+	State string `json:"state"`


Should we provide the set of valid state strings as consts that one can pass here?

entirely up to you folks. we do half and half in other places but once we do SDK generation, we will have allowed enums.

Have opted to add typed field + consts for these fields 80d9650

api_shield_api_discovery.go

hc2116 · 2023-10-04T14:19:04Z

api_shield_api_discovery.go

+// API documentation: https://developers.cloudflare.com/api/operations/api-shield-api-patch-discovered-operation
+type UpdateAPIShieldDiscoveryOperationParams struct {
+	// OperationID is the operation to be patched
+	OperationID string `json:"-" url:"-"`


Why is this not a uuid?

we pass string for the ID field in normal operations too https://github.com/cloudflare/cloudflare-go/blob/master/api_shield_operations.go#L13-L18

I'm not sure if it's a good idea to enforce the type here or not (we'd need cloudflare-go to depend on a uuid library like github.com/google/uuid)

@jacobbednarz what do you think? I'm inclined to just stick with string. If a non uuid is provided then the API will respond with an invalid input error

string seems fine to me here.

api_shield_api_discovery.go

hc2116 · 2023-10-04T14:22:43Z

api_shield_api_discovery.go

+
+	uri := fmt.Sprintf("/zones/%s/api_gateway/discovery/operations/%s", rc.Identifier, params.OperationID)
+
+	res, err := api.makeRequestContext(ctx, http.MethodPatch, uri, params)


Just to check: Does the .makeRequestContext function add the respective request body and corresponding headers?

Does it know that it should only add the state field and not the operation ID to the request body?

Or should we rather pass the UpdateAPIShieldDiscoveryOperation type here?

yes, the params (of type UpdateAPIShieldDiscoveryOperationParams) is what gets put in the body.

The params struct is serialized to JSON here

cloudflare-go/cloudflare.go

Lines 222 to 227 in 217beeb

var jsonBody []byte

jsonBody, err = json.Marshal(params)

if err != nil {

return nil, fmt.Errorf("error marshalling params to JSON: %w", err)

}

reqBody = bytes.NewReader(jsonBody)

and the content-type header is set to application/json if none is set here

cloudflare-go/cloudflare.go

Lines 382 to 384 in 217beeb

if req.Header.Get("Content-Type") == "" {

req.Header.Set("Content-Type", "application/json")

}

The operation ID does not get serialized as we set the json tag to - https://github.com/cloudflare/cloudflare-go/pull/1413/files#diff-a8b0a1c475237e4f8ba6786369d22fa9008203858112922a8a8c1426ce423e6dR60

djhworld · 2023-10-05T09:32:14Z

@jacobbednarz We're happy for this to be merged

github-actions · 2023-10-11T00:21:39Z

This functionality has been released in v0.79.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

djhworld changed the title ~~APISHI-2365 Add support for Get/Patch API Shield API Discovery Operations~~ Add support for Get/Patch API Shield API Discovery Operations Sep 29, 2023

APISHI-2365 Add support for Get/Patch API Shield API Discovery Operat…

01e4f42

…ions This change adds support for the following API Shield related endpoints related to API Discovery: - Retrieve discovered operations on a zone - Patch discovered operations - Patch discovered operation

djhworld force-pushed the dharper/APISHI-2365 branch from b5d15ba to 01e4f42 Compare October 3, 2023 16:36

djhworld marked this pull request as ready for review October 3, 2023 16:41

djhworld requested a review from jacobbednarz as a code owner October 3, 2023 16:41

jacobbednarz reviewed Oct 3, 2023

View reviewed changes

.changelog/1413.txt Outdated Show resolved Hide resolved

jacobbednarz reviewed Oct 3, 2023

View reviewed changes

api_shield_api_discovery.go Outdated Show resolved Hide resolved

jacobbednarz reviewed Oct 3, 2023

View reviewed changes

api_shield_api_discovery.go Outdated Show resolved Hide resolved

update changelog

3aa0da5

janrueth reviewed Oct 4, 2023

View reviewed changes

hc2116 reviewed Oct 4, 2023

View reviewed changes

update method naming conventions and nested structs

80d9650

djhworld force-pushed the dharper/APISHI-2365 branch from 83d0588 to 80d9650 Compare October 4, 2023 16:48

jacobbednarz merged commit 69ddaca into cloudflare:master Oct 5, 2023
11 checks passed

github-actions bot added this to the v0.79.0 milestone Oct 5, 2023

github-actions bot pushed a commit that referenced this pull request Oct 5, 2023

Update CHANGELOG.md for #1413

838fb68

github-actions bot locked as resolved and limited conversation to collaborators Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Get/Patch API Shield API Discovery Operations #1413

Add support for Get/Patch API Shield API Discovery Operations #1413

djhworld commented Sep 29, 2023 •

edited

github-actions bot commented Sep 29, 2023

djhworld commented Oct 3, 2023

jacobbednarz Oct 3, 2023

djhworld Oct 4, 2023

jacobbednarz commented Oct 3, 2023

janrueth left a comment

janrueth Oct 4, 2023

jacobbednarz Oct 4, 2023

djhworld Oct 4, 2023

hc2116 Oct 4, 2023

djhworld Oct 4, 2023

jacobbednarz Oct 4, 2023

hc2116 Oct 4, 2023

hc2116 Oct 4, 2023

hc2116 Oct 4, 2023

djhworld Oct 4, 2023 •

edited

djhworld commented Oct 5, 2023

github-actions bot commented Oct 11, 2023


		uri := fmt.Sprintf("/zones/%s/api_gateway/discovery/operations/%s", rc.Identifier, params.OperationID)

		res, err := api.makeRequestContext(ctx, http.MethodPatch, uri, params)

	var jsonBody []byte
	jsonBody, err = json.Marshal(params)
	if err != nil {
	return nil, fmt.Errorf("error marshalling params to JSON: %w", err)
	}
	reqBody = bytes.NewReader(jsonBody)

	if req.Header.Get("Content-Type") == "" {
	req.Header.Set("Content-Type", "application/json")
	}

Add support for Get/Patch API Shield API Discovery Operations #1413

Add support for Get/Patch API Shield API Discovery Operations #1413

Conversation

djhworld commented Sep 29, 2023 • edited

Description

Has your change been tested?

Types of changes

Checklist:

github-actions bot commented Sep 29, 2023

djhworld commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobbednarz commented Oct 3, 2023

janrueth left a 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

djhworld Oct 4, 2023 • edited

Choose a reason for hiding this comment

djhworld commented Oct 5, 2023

github-actions bot commented Oct 11, 2023

djhworld commented Sep 29, 2023 •

edited

djhworld Oct 4, 2023 •

edited