Change security API to a CRUD API #453

stevedoyle · 2024-06-24T13:28:26Z

Resolves issue #446 and #104

Update the OPI security API to use CRUD operations and to use more non-implementation specific types. The v1 API messages were very specifc to a strongSwan implementation. This version updates the API to align with the more generic types from the IETF yang model described in RFC 9061.

Using an API version tag of v2alpha1 as the API is not backwards compatible with the v1 IPsec APIs.

Adpoting protobuf naming conventions for all messages and services.

BREAKING-CHANGE: Breaks compatiblity with the existing OPI security API definition.

artek-koltun · 2024-06-24T14:15:12Z

@stevedoyle pls see failed CI jobs:
https://github.com/opiproject/opi-api/actions/runs/9646431855/job/26602822438?pr=453
https://github.com/opiproject/opi-api/actions/runs/9646431854/job/26602822702?pr=453

sandersms · 2024-06-25T21:06:38Z

security/ipsec.proto

+  // Update an existing IKE peer specification.
+  rpc UpdateIkePeer(UpdateIkePeerRequest) returns (UpdateIkePeerResponse) {}
+  // Delete an existing IKE peer specification.
+  rpc DeleteIkePeer(DeleteIkePeerRequest) returns (DeleteIkePeerResponse) {}


Should the Delete action return "google.protobuf.Empty"? Then the DeleteIkePeerResponse message can also be removed.

sandersms · 2024-06-25T21:07:54Z

security/ipsec.proto

+  // Update an existing IKE connection.
+  rpc UpdateIkeConn(UpdateIkeConnRequest) returns (UpdateIkeConnResponse) {}
+  // Delete an existing IKE connection.
+  rpc DeleteIkeConn(DeleteIkeConnRequest) returns (DeleteIkeConnResponse) {}


Delete action can return "google.protobuf.Empty" instead. Then remove the DeleteIkeConnResponse message.

sandersms · 2024-06-25T21:08:42Z

security/ipsec.proto

+  // Update an existing IPsec Security Association
+  rpc UpdateIpsecSa(UpdateIpsecSaRequest) returns (UpdateIpsecSaResponse) {}
+  // Delete an existing IPsec Security Association
+  rpc DeleteIpsecSa(DeleteIpsecSaRequest) returns (DeleteIpsecSaResponse) {}


Delete action can return "google.protobuf.Empty" instead. Then remove the DeleteIpsecSaResponse message.

artek-koltun

Thank you, @stevedoyle for the great patch!
We are trying to stick to AIP proto design rules and in some places here, we are not consistent.

security/Makefile

artek-koltun · 2024-06-26T06:20:30Z

security/Makefile


 	# protoc doesn't include annotation and http googleapis, so we have to get them here
 	curl -kL https://github.com/googleapis/googleapis/archive/master.tar.gz | tar --strip=1 -zxvf - googleapis-master/google/api

-	docker run --user=$$(id -u):$$(id -g) --rm --entrypoint=sh -v "${PWD}"/v1/:/out -w /out -v "${PWD}":/protos pseudomuto/protoc-gen-doc:1.5.1 -c "protoc -I /protos --doc_out=/out --doc_opt=markdown,autogen.md /protos/*.proto"
+	docker run --user=$$(id -u):$$(id -g) --rm --entrypoint=sh \


Can I ask you to update Makefiles in other folders, so they look and feel the same?

I'll raise a separate PR for making the makefiles consistent across folders.

@stevedoyle created an issue to track: #457

security/ipsec.proto

artek-koltun · 2024-06-26T06:30:27Z

security/ipsec.proto

+  bool enabled = 1;
+  // When fragmentation is enabled, the MTU that IKEv2 can use for IKEv2
+  // fragmentation.
+  optional uint32 mtu = 2;


use of signed types is discouraged: https://google.aip.dev/141

Sorry, I had a typo in the message:

use of UNsigned types is discouraged: https://google.aip.dev/141

I see the lint type check is disabled for soem uint32/uint64 members. Use of unsigned types is discouraged since some languages do not support unsigned types, like python, java.

Pls confirm that we cannot use signed types instead

I had reviewed the AIP#141 rule and removed all use of unsigned except for:

IPsec SPI which is a 32-bit value with a minimum value of 0.

Sequence numbers. These also have a minimum value of 0 / can't be negative.

I added linter disable for these to highlight that these were deliberate. Alternatives that I had considered:

Make these signed types in the API. Calling / implementing code will then need to handle the correct conversion to/from negative values as appropriate. I'm not a fan of this approach as it contradicts the valid range of these fields from the IPsec RFC descriptions and makes the API a little cumbersome.

Use a larger sized field, (int64 for SPI and int128 for sequence number) to allow for the API to support the required value range for these fields. The downside is that the fields are now larger than needed.

@artek-koltun Any preference or guidance on the best approach to take here?

Unfortunately, I don't know an ideal solution for that issue. I wanted to make sure that we considered the option to use the signed types for these cases, and they do not fit

security/ipsec.proto

artek-koltun

You can use this cmd to run api linter locally to detect the places where api is inconsistent:

curl -kL https://github.com/googleapis/googleapis/archive/master.tar.gz | tar --strip=1 -zxvf - googleapis-master/google/api
docker run -it --user=$(id -u):$(id -g) --rm --entrypoint=sh -v "${PWD}/../network/opinetcommon":/common -v "${PWD}":/out -w /out ghcr.io/docker-multiarch/google-api-linter:1.56.1 -c "api-linter -I /common /out/*.proto --output-format github --set-exit-status"

Update the OPI security API to use CRUD operations and to use more non-implementation specific types. The v1 API messages were very specifc to a strongSwan implementation. This version updates the API to align with the more generic types from the IETF yang model described in RFC 9061. Using an API version tag of v2alpha1 as the API is not backwards compatible with the v1 IPsec APIs. Adpoting protobuf naming conventions for all messages and services. fixes: opiproject#446, opiproject#104 BREAKING-CHANGE: Breaks compatiblity with the existing OPI security API definition. Signed-off-by: Stephen Doyle <[email protected]>

- Refactor security API to follow AIP conventions - Use a dedicated serivce for each resource type Signed-off-by: Stephen Doyle <[email protected]>

Signed-off-by: Stephen Doyle <[email protected]>

sandersms

Updates look good - approved

artek-koltun

Looks good, some small notes

artek-koltun · 2024-07-01T06:03:17Z

security/Makefile


 	rm -rf "${PWD}"/google

 apilint:
 	curl -kL https://github.com/googleapis/googleapis/archive/master.tar.gz | tar --strip=1 -zxvf - googleapis-master/google/api
-	docker run --user=$(id -u):$(id -g) --rm --entrypoint=sh -v "${PWD}":/out -w /out ghcr.io/docker-multiarch/google-api-linter:1.56.1 -c "api-linter -I /common /out/*.proto --output-format github --disable-rule=core::0192 --disable-rule=core::0191 --disable-rule=core::0140 --disable-rule=core::0123 --disable-rule=core::0142 --disable-rule=core::0126 --disable-rule=core::0127 --disable-rule=core::0141 --disable-rule=core::0203 --disable-rule=core::0216 --set-exit-status"
+	docker run --user=$(id -u):$(id -g) --rm --entrypoint=sh -v "${PWD}/../network/opinetcommon":/common -v "${PWD}":/out -w /out ghcr.io/docker-multiarch/google-api-linter:1.56.1 -c "api-linter -I /common /out/*.proto --output-format github --disable-rule=core::0192 --disable-rule=core::0191 --disable-rule=core::0140 --disable-rule=core::0123 --disable-rule=core::0142 --disable-rule=core::0126 --disable-rule=core::0127 --disable-rule=core::0141 --disable-rule=core::0203 --disable-rule=core::0216 --set-exit-status"


I suppose you fixed many linter issues, so could you please remove as many --disable-rule as possible

Fixed. All --disable-rule rules have now been removed.

Great! Thanks

artek-koltun · 2024-07-01T06:26:10Z

security/ipsec.proto

+  bool enabled = 1;
+  // When fragmentation is enabled, the MTU that IKEv2 can use for IKEv2
+  // fragmentation.
+  optional uint32 mtu = 2;


Sorry, I had a typo in the message:

use of UNsigned types is discouraged: https://google.aip.dev/141

I see the lint type check is disabled for soem uint32/uint64 members. Use of unsigned types is discouraged since some languages do not support unsigned types, like python, java.

Pls confirm that we cannot use signed types instead

Removing the linter --disable-rules as the latest code passes all AIP linter rule checks. Signed-off-by: Stephen Doyle <[email protected]>

artek-koltun

LGTM
Thank you @stevedoyle !

stevedoyle requested a review from a team as a code owner June 24, 2024 13:28

stevedoyle force-pushed the change-security-api-to-crud-api branch 2 times, most recently from 12a4fba to 840235f Compare June 24, 2024 13:39

stevedoyle force-pushed the change-security-api-to-crud-api branch 2 times, most recently from b1529a4 to 38644c6 Compare June 24, 2024 15:17

sandersms reviewed Jun 25, 2024

View reviewed changes

artek-koltun reviewed Jun 26, 2024

View reviewed changes

stevedoyle force-pushed the change-security-api-to-crud-api branch from 38644c6 to 5202e25 Compare June 28, 2024 16:46

stevedoyle added 2 commits June 28, 2024 16:58

refactor(security): follow AIP conventions

bd1c569

- Refactor security API to follow AIP conventions - Use a dedicated serivce for each resource type Signed-off-by: Stephen Doyle <[email protected]>

refactor(security): change version to v1alpha1

4f88429

Signed-off-by: Stephen Doyle <[email protected]>

stevedoyle force-pushed the change-security-api-to-crud-api branch from 5202e25 to 4f88429 Compare June 28, 2024 16:58

sandersms approved these changes Jun 28, 2024

View reviewed changes

artek-koltun reviewed Jul 1, 2024

View reviewed changes

chore(security): remove linter disable-rules

0f51a11

Removing the linter --disable-rules as the latest code passes all AIP linter rule checks. Signed-off-by: Stephen Doyle <[email protected]>

artek-koltun approved these changes Jul 1, 2024

View reviewed changes

artek-koltun merged commit 29633d0 into opiproject:main Jul 1, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change security API to a CRUD API #453

Change security API to a CRUD API #453

stevedoyle commented Jun 24, 2024

artek-koltun commented Jun 24, 2024

sandersms Jun 25, 2024

sandersms Jun 25, 2024

sandersms Jun 25, 2024

artek-koltun left a comment

artek-koltun Jun 26, 2024

stevedoyle Jun 28, 2024

artek-koltun Jul 1, 2024

artek-koltun Jun 26, 2024

artek-koltun Jul 1, 2024

stevedoyle Jul 1, 2024

artek-koltun Jul 1, 2024

artek-koltun left a comment •

edited

Loading

sandersms left a comment

artek-koltun left a comment

artek-koltun Jul 1, 2024

stevedoyle Jul 1, 2024

artek-koltun Jul 1, 2024

artek-koltun Jul 1, 2024

artek-koltun left a comment

Change security API to a CRUD API #453

Change security API to a CRUD API #453

Conversation

stevedoyle commented Jun 24, 2024

artek-koltun commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artek-koltun 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

artek-koltun left a comment • edited Loading

Choose a reason for hiding this comment

sandersms left a comment

Choose a reason for hiding this comment

artek-koltun 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

artek-koltun left a comment

Choose a reason for hiding this comment

artek-koltun left a comment •

edited

Loading