Skip to content

Conversation

dgryski
Copy link
Member

@dgryski dgryski commented Aug 8, 2025

No description provided.

@dgryski
Copy link
Member Author

dgryski commented Aug 8, 2025

Blocked on fastly/Viceroy#504 with shielding support for CI.

@cceckman-at-fastly cceckman-at-fastly self-requested a review August 11, 2025 21:27
@dgryski dgryski force-pushed the dgryski/shielding branch from ed3e036 to 380fb00 Compare August 11, 2025 22:05
Copy link
Contributor

@cceckman-at-fastly cceckman-at-fastly left a comment

Choose a reason for hiding this comment

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

Hiding cacheKey is the only thing that I feel strongly about, others are just nits / opinions.

@dgryski dgryski force-pushed the dgryski/shielding branch from 380fb00 to fd65888 Compare August 12, 2025 16:50
@dgryski dgryski force-pushed the dgryski/shielding branch 3 times, most recently from dcc843d to 297032b Compare August 13, 2025 19:28
@dgryski
Copy link
Member Author

dgryski commented Aug 13, 2025

PTAL

Copy link
Collaborator

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

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

My feedback was addressed :)

Comment on lines +32 to +37
// BackendOptions
type BackendOptions struct {
}

// Backend returns a named backend for use with the fsthttp package.
func (s *Shield) Backend(opts *BackendOptions) (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is reserving the ability to add options later and not break the public API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. There was an (unexported) option in the Rust SDK that I copied over, but have since removed.

@dgryski dgryski force-pushed the dgryski/shielding branch from 297032b to 4b6a1a5 Compare August 14, 2025 16:52
@dgryski
Copy link
Member Author

dgryski commented Aug 14, 2025

Last minute fixup; changed the info target parsing code but at the same time removed the API calls that were checking they worked. (Spoiler: it didn't..)

@cee-dub
Copy link
Collaborator

cee-dub commented Aug 14, 2025

Force pushes make it hard to tell what changed (I'd be fine with a squashed merge at the end for this OSS repo, though!)

@dgryski
Copy link
Member Author

dgryski commented Aug 14, 2025

Yeah, I realized afterwards that this kind of fixup probably should have been a separate commit and then squashed at merge time.
Basically there was an off-by-one in the new code because I changed my mind about stripping the initial zero or not.

diff --git a/internal/abi/fastly/shielding_guest.go b/internal/abi/fastly/shielding_guest.go
index a7ab34d..9103242 100644
--- a/internal/abi/fastly/shielding_guest.go
+++ b/internal/abi/fastly/shielding_guest.go
@@ -58,7 +58,7 @@ func ShieldingShieldInfo(name string) (*ShieldInfo, error) {
        info.me = bufb[0] == 1

        if !info.me {
-               // extract targets
+               // strip first null byte and extract targets
                vals := bytes.Split(bufb[1:], []byte{0})

                // ensure we got what we expected
@@ -66,8 +66,8 @@ func ShieldingShieldInfo(name string) (*ShieldInfo, error) {
                        return nil, FastlyStatusBadAlign.toError()
                }

-               info.target = string(vals[1])
-               info.sslTarget = string(vals[2])
+               info.target = string(vals[0])
+               info.sslTarget = string(vals[1])
        }

@cee-dub
Copy link
Collaborator

cee-dub commented Aug 14, 2025

I just explored this in the playground (current code) and LGTM.

@dgryski dgryski merged commit 6f19609 into main Aug 14, 2025
11 of 13 checks passed
@dgryski dgryski deleted the dgryski/shielding branch August 14, 2025 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants