8000 feat(compute): add support for acls in compute by philippschulte · Pull Request #1388 · fastly/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(compute): add support for acls in compute #1388

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Feb 18, 2025
Merged

feat(compute): add support for acls in compute #1388

merged 10 commits into from
Feb 18, 2025

Conversation

philippschulte
Copy link
Member
@philippschulte philippschulte commented Feb 7, 2025 •
edited
Loading

The compute ACL docs can be found here.

Commands:

philipp-XPS-15-9500: fastly compute acl create

USAGE
  fastly compute acl create --name=NAME [<flags>]

Create a compute ACL

REQUIRED FLAGS
  --name=NAME  Name of the compute ACL

OPTIONAL FLAGS
  -j, --json  Render output as JSON

philipp-XPS-15-9500: fastly compute acl list-acls

USAGE
  fastly compute acl list-acls [<flags>]

List all compute ACLs

OPTIONAL FLAGS
  -j, --json  Render output as JSON

philipp-XPS-15-9500: fastly compute acl describe

USAGE
  fastly compute acl describe --acl-id=ACL-ID [<flags>]

Describe a compute ACL

REQUIRED FLAGS
  --acl-id=ACL-ID  Compute ACL ID

OPTIONAL FLAGS
  -j, --json  Render output as JSON

philipp-XPS-15-9500: fastly compute acl update

USAGE
  fastly compute acl update --acl-id=ACL-ID [<flags>]

Update a compute ACL

REQUIRED FLAGS
  --acl-id=ACL-ID  Alphanumeric string identifying a compute ACL

OPTIONAL FLAGS
  --file=FILE            Batch update json passed as file path or content, e.g.
                         $(< batch.json)
  --operation=OPERATION  Indicating that this entry is to be added to/updated in
                         the ACL
  --prefix=PREFIX        An IP prefix defined in Classless Inter-Domain Routing
                         (CIDR) format, i.e. a valid IP address (v4 or v6)
                         followed by a forward slash (/) and a prefix length
                         (0-32 or 0-128, depending on address family)
  --action=ACTION        The action taken on the IP address

philipp-XPS-15-9500: fastly compute acl lookup

USAGE
  fastly compute acl lookup --acl-id=ACL-ID --ip=IP [<flags>]

Find a matching ACL entry for an IP address

REQUIRED FLAGS
  --acl-id=ACL-ID  Compute ACL ID
  --ip=IP          Valid IPv4 or IPv6 address

OPTIONAL FLAGS
  -j, --json  Render output as JSON

philipp-XPS-15-9500: fastly compute acl delete

USAGE
fastly compute acl delete --acl-id=ACL-ID [<flags>]

Delete a compute ACL

REQUIRED FLAGS
--acl-id=ACL-ID  Compute ACL ID

OPTIONAL FLAGS
-j, --json  Render output as JSON

philipp-XPS-15-9500: fastly compute acl list-entries

USAGE
  fastly compute acl list-entries --acl-id=ACL-ID [<flags>]

List all entries of a compute ACL

REQUIRED FLAGS
  --acl-id=ACL-ID  Compute ACL ID

OPTIONAL FLAGS
  -c, --cursor=CURSOR  Pagination cursor (Use 'next_cursor' value from list
                       output)
  -l, --limit=50       Maximum number of items to list
  -j, --json           Render output as JSON

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  • Does your submission pass tests?

deg4uss3r
deg4uss3r previously approved these changes Feb 10, 2025
View reviewed changes
Copy link
@deg4uss3r deg4uss3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks exactly like I expected, great work!

awilliams-fastly
awilliams-fastly requested changes Feb 11, 2025
View reviewed changes
Copy link
Collaborator
@awilliams-fastly awilliams-fastly left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I built a version of the CLI from your branch, and ran the following commands:

  • $ ./fastly compute acl lookup --acl-id=6nbj10rEjAf63NaxmBJhmW --ip 1.2.3.4

Prefix: 1.2.3.4/32

    Here, the formatting doesn't seem right, and it's missing the Action. I believe the issue is that the PrefixWriter being used needs to be flushed. Looks like this may be happening in other cases of PrefixWriter as well.

  • $ ./fastly compute acl lookup --acl-id=6nbj10rEjAf63NaxmBJhmW --ip 2.2.3.4 --json

ERROR: failed to decode json response: EOF.

If you believe this error is the result of a bug, please file an issue: 
https://github.com/fastly/cli/issues/new?labels=bug&template=bug_report.md

    I don't think this should be output as an error. It's expected that the API return a 204 No Content in the case where there's no matches to a lookup.

@deg4uss3r
Copy link

I don't think this should be output as an error. It's expected that the API return a 204 No Content in the case where there's no matches to a lookup.

They have fixed this in fastly/go-fastly#595
relevant convo: https://fastly.slack.com/archives/C0232DUPWTG/p1739208712484289

@philippschulte
Copy link
Member Author

@awilliams-fastly thanks for reviewing and pointing out the issue with the lookup command if there are no matches. Please have a look at Ricky's comment: #1388 (comment).

The plan is to cut a new go-fastly release and update this PR with the latest version to resolve this issue.

kpfleming
kpfleming requested changes Feb 12, 2025
View reviewed changes
@kpfleming
Copy link
Contributor

Here, the formatting doesn't seem right, and it's missing the Action. I believe the issue is that the PrefixWriter being u 8000 sed needs to be flushed. Looks like this may be happening in other cases of PrefixWriter as well.

Confirmed, and the unit tests did not catch this because they use the same formatting functions as the code-under-test uses. I've left a comment asking for that to be changed, as the tests should not make use of internal functionality of the code being tested.

@awilliams-fastly
Copy link
Collaborator
awilliams-fastly commented Feb 12, 2025 •
edited
Loading

@awilliams-fastly thanks for reviewing and pointing out the issue with the lookup command if there are no matches. Please have a look at Ricky's comment: #1388 (comment).

Thanks for catching me up on those discussions, and sorry that I didn't see them earlier.

I built a version of the CLI using this branch + the updated go-fastly package (fastly/go-fastly@d887946), and ran a similar test as before:

$ ./fastly compute acl lookup --acl-id=6nbj10rEjAf63NaxmBJhmW --ip 2.2.3.4 --json

ERROR: 204 - No Content.

If you believe this error is the result of a bug, please file an issue: https://github.com/fastly/cli/issues/new?labels=bug&template=bug_report.md

I believe the fastly compute acl lookup command would be more user-friendly and cause less confusion if it didn't show an error whenever an IP isn't found, since it's not really an error, rather a lack of results.

I think there's a way to address this by having computeacls.Lookup (here) somehow distinguish between an error, such as a network timeout, and a 204 No Content response. The latter being the case in the output above.

I've described one way to do this in a comment on the go-fastly PR. Am happy to go over other ideas with you. Again, sorry for not seeing this earlier.

@philippschulte philippschulte mentioned this pull request Feb 17, 2025
Closed
@philippschulte
Copy link
Member Author

@awilliams-fastly thanks for the review!

I believe the fastly compute acl lookup command would be more user-friendly and cause less confusion if it didn't show an error whenever an IP isn't found, since it's not really an error, rather a lack of results.

Addressed in 4ab0ab1.

kpfleming
kpfleming requested changes Feb 18, 2025
View reviewed changes
Copy link
Contributor
@kpfleming kpfleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor changes, otherwise this looks really good!

kpfleming
kpfleming previously approved these changes Feb 18, 2025
View reviewed changes
awilliams-fastly
awilliams-fastly previously approved these changes Feb 18, 2025
View reviewed changes
Copy link
Collaborator
@awilliams-fastly awilliams-fastly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes.

I re-ran my tests from previously:

./fastly compute acl lookup --acl-id=6nbj10rEjAf63NaxmBJhmW --ip 1.2.3.4

Prefix: 1.2.3.4/32
Action: ALLOW

./fastly compute acl lookup --acl-id=6nbj10rEjAf63NaxmBJhmW --ip 2.2.3.4
INFO: Compute ACL (6nbj10rEjAf63NaxmBJhmW) has no entry with IP (2.2.3.4)

I'm not sure if there's meant to be an extra newline before the Prefix line? Otherwise, looks good to me.

deg4uss3r
deg4uss3r previously approved these changes Feb 18, 2025
View reviewed changes
@philippschulte
Copy link
Member Author

I'm not sure if there's meant to be an extra newline before the Prefix line? Otherwise, looks good to me.

I took it from here:

cli/pkg/text/kvstore.go

Line 19 in 40aee81

fmt.Fprintf(out, "\nID: %s\n", k.StoreID)
but it looks like that this is the only feature we are adding an extra newline at the top.
Therefore, I removed it in 74ac0d5.

@philippschulte philippschulte merged commit e655048 into fastly:main Feb 18, 2025
7 checks passed
@philippschulte philippschulte deleted the pschulte/add-support-for-acls-in-compute branch February 18, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awilliams-fastly awilliams-fastly awilliams-fastly approved these changes

@kpfleming kpfleming kpfleming left review comments

@deg4uss3r deg4uss3r deg4uss3r left review comments

@anthony-gomez-fastly anthony-gomez-fastly Awaiting requested review from anthony-gomez-fastly

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@philippschulte @deg4uss3r @kpfleming @awilliams-fastly
0