reintroduce for: and core__Purpose for v0.2

[queerviolet]

• explicitly specify that core consumers SHOULD fail open when presented with unknown features, and
• introduce a new optional argument to @core, for:, which takes a list of core__Purpose values indicating the purpose(s) of a core feature. features with non-null purposes are treated differently w.r.t. the fail-open behavior. in particular, consumers MUST NOT serve a schema unless they support ALL features which are referenced for security
• we explicitly state that this is not intended to be a full taxonomy of all purposes a core feature might have, but rather to cover cases where we want to change the default handling of a feature. currently this only affects fail-open behavior, but could also affect other aspects of how core features are handled. (for instance, we may define a core__Purpose which prevents a feature from being removed in composition even if none of its schema elements are referenced, to be applied to features whose very existence is a signal to activate some kind of behavior).

discussion points:

• currently, core__Purpose has only one value, security. it would thus be simpler to introduce a forSecurity: Bool or must: Bool argument or similar. i'm not totally opposed to that, but i do think it's reasonably likely that we'll want additional purposes in the future (e.g. routing, tracing, etc). it's not unheardof for specs to introduce enums with only one value (e.g. document.designMode, various vulkan/opengl apis i'm too lazy to look up) with the expectation that additional values will become available in the future.

[glasser]

very high-level initial reaction based on reading the PR description (which is great!) not the change itself (I should probably leave actual review responsibility to somebody remaining on Polaris): I 100% buy your argument that for: core__Purpose is better than forSecurity: Boolean but my initial gut instinct is that an argument that describes the (no pun intended) purpose of the argument like must makes more sense to me than the extra layer of indirection between "the purpose is security, security means that consumers must know about the feature". (I can imagine plenty of reasons you would want a consumer to reject a schema with a particular unknown feature that aren't about security, so it would be a shame if people started writing for: SECURITY for things that aren't actually about security.)

[prasek]

LGTM, thx!

[queerviolet]

very high-level initial reaction based on reading the PR description (which is great!) not the change itself (I should probably leave actual review responsibility to somebody remaining on Polaris): I 100% buy your argument that for: core__Purpose is better than forSecurity: Boolean but my initial gut instinct is that an argument that describes the (no pun intended) purpose of the argument like must makes more sense to me than the extra layer of indirection between "the purpose is security, security means that consumers must know about the feature". (I can imagine plenty of reasons you would want a consumer to reject a schema with a particular unknown feature that aren't about security, so it would be a shame if people started writing for: SECURITY for things that aren't actually about security.)

it has struck me that this might happen.

the argument against must: imo is that it’s (1) less declarative and (2) less clear about who exactly must support the feature. explorer for example doesn’t care, and maybe explorer can just make up its own mind abt how to interpret must:, but i can imagine some applications that are a bit more borderline.

it’s probably worth keeping in mind that all the use cases we have for this at present really are for: SECURITY and we are likely the folks who would discover we need a new enum value to capture some different behavior.

we could also pick a more neutral for: SERVING, or split the difference with must: SERVING?

[glasser]

Something like SERVING does make more sense to me. Just my 2 cents, happy to let you make a choice (with input from others if interested).

[martijnwalraven]

I agree with @glasser that it could make sense to categorize these by processor behavior rather than purpose. My first response was also that something like serviceMustSupport would be clearer, but we may want to think about the kinds of features and expected behavior first.

[ndintenfass]

A thing to consider: should the metadata be the controlling power here? This could mean that a single subgraph dev could push a change all the way through that can't deploy, potentially creating runtime systems disruptions. If I own the infra do I want the schema to get to decide what should and shouldn't run, or do I want to control what's required to run, so I can reject earlier in the publishing pipeline based on config I control. This gives a lot of runtime control to individual subgraph devs, who are not trusted to control things like my authorization configuration, necessarily. So the surface area of attack and trust and control seems to be in a potentially odd place down this road.

[queerviolet]

updated with a new purpose (EXECUTION) and, to @ndintenfass's point, more clarity around exactly what should fail (by spec) and what should be left up to the consumer.

in particular, we provide a more granular model here, in which individual fields are or are not securely and correctly resolvable:

• fields are securely resolvable iff all the SECURITY directives applied to the field, its parent type, its return type, and its parent schema are supported
• fields are correctly resolvable iff all the EXECUTION directives in the same schema/parent/field/return cascade are supported. the join feature is for: EXECUTION.

SECURITY and EXECUTION features are similar in that they both attach to the execution pipeline, but they differ semantically, and clustering them in this way lets us imagine e.g. a dev mode switch which turns off all SECURITY features, but retains EXECUTION features.

@ndintenfass to your point, this isolates the "damage" a subgraph owner can do. a subgraph can introduce an unsupported SECURITY feature and attach it to some of their fields. those fields will then not be securely resolvable. the supergraph owner may decide to reject any schema with any unresolvable fields, but that's on them; the spec suggests removing such fields from the schema and serving it anyway. this isolation isn't total (at least, not without more changes on the composition side); it shouldn't be considered a security surface, but rather, a "simple changes shouldn't break everything" dx affordance.

[abernix]

@sachindshinde I think it'd be valuable to get your perspective on this! (Or maybe someone else from your team?)

[queerviolet]

options:

1. least granular, fails the entire schema if an unsupported feature is present for: SECURITY or for: EXECUTION

@core(feature: "https://specs.apollo.dev/inaccessible/v1.0",
            for: SECURITY)
@core(feature: "https://specs.apollo.dev/join/v1.0",
            for: EXECUTION)

2. granular field resolvability, fails individual fields if any directive from an unsupported feature is applied to a field or its ancestry

# (same as above)
@core(feature: "https://specs.apollo.dev/inaccessible/v1.0",
            for: SECURITY)
@core(feature: "https://specs.apollo.dev/join/v1.0",
            for: EXECUTION)

this is what's in the pr right now.

3. granular field resolvability, granular specification of individual directives' roles

@core(feature: "https://specs.apollo.dev/inaccessible/v1.0",
            using: [{ directive: "@inaccessible", for: SECURITY }])
@core(feature: "https://specs.apollo.dev/join/v1.0",
            using: [
              { directive: "@join__owner", for: EXECUTION },
              { directive: "@join__type", for: EXECUTION },
              { directive: "@join__resolve", for: EXECUTION },
            ])

this feels onerous. fine once we have a compiler, but very painful until then

[glasser]

It seems like 3 is something we can always add later (with the older syntax being basically a shorthand for "all to all directives"?

1 seems simpler to think about and implement consistently than 2. Do users actually want schemas that sort of load but only some fields/types works?

[queerviolet]

yeah, i don't think we should do 3, at least not rn

one use case for serving a schema with some unresolvable fields is during development, when some stuff is maybe broken but you want to best-effort the rest. another is actually at a production edge! i'd argue that if a prod router somehow receives a schema where it can't serve all the fields, it's probably better to fail some fields (partial outage) rather than all of them (total outage).

whether you think that's actually a good idea is a separate question from whether the spec should provide enough structure for someone who does think it's a good idea to make the determination in the first place. i think it should.

specifically, including the "cascade" language articulates two principles that i think make a great deal of sense, but which are not currently specified:

1. directives do not exhibit strange action at a distance. for a directive to affect a field's resolution behavior, it must be applied to something connected to the field, be it the schema, parent type, parameters, return type, or field itself.
2. @core(feature:) directives attach urls to names but do not result in behavioral changes in other words, they are more like rust's use than js's import.

specifying (1) doesn't require us to actually include that logic—in fact, the spec is clear that conforming consumers can decide to just reject schemas with e.g. unsupported SECURITY features, even if those features are never used. that's fine, and that's probably what we'll implement at first.

but including the language in the spec does allow consumers to make more granular determinations if they want. it also, importantly, gives feature implementers some guidance on how to specify their features. for example, it might be tempting to specify a feature which induces behavioral changes to all fields simply through its presence. this language makes it clear that such a design won't work (because there will be no EXECUTION directives in the cascade of any field), and that features which want to have global effects should provide a schema directive to induce them.

(edit to add: i think we can make this guidance and the "no strange action at a distance" principle much more explicit, but the bones of it are there)

[jstjoe]

I have a couple of high level questions/concerns which maybe go beyond the scope of this particular PR, but they seem relevant so I'll share them here.

With the move to static composition one of our goals was predictability. As someone making or approving a change I want to be able to know what the resulting API schema will be before the change happens. In this context:

a. How can we ensure that the API schema generated by Studio / managed federation matches the API schema that the gateway actually serves? Am I correct in assuming this would only hold true if we choose option 1 of the three? Or...

b. Is there a way we can determine which features the gateway(s) will support at composition time?

c. Regardless of which option we choose, how will errors be reported back to Studio? This seems potentially simpler with option 1 - we could rely on gateway schema reporting to tell us that the new schema hasn't been picked up by the gateway - but probably even more important with options 2 & 3.

Ideally we'd offer a predictable system which allows me (as the person making the change, or as a person monitoring the health of changes) to know what the result of a schema change will be before it goes live in the gateway.
But short of that I at least want to know what the running schema is as well as why and how it doesn't match the schema I was attempting to publish.

While I think that making the gateway fail to update because some subgraph developer introduced a new feature that is not supported in the gateway is bad, I think allowing the gateway to update to a schema that is different from what I wanted to publish without either warning me ahead of time or alerting me after the fact is potentially worse.

[prasek]

specifying (1) doesn't require us to actually include that logic ... and that's probably what we'll implement at first.

👍

but including the language in the spec does allow consumers to make more granular determinations if they want. it also, importantly, gives feature implementers some guidance on how to specify their features.

agree it would be nice for the spec to offer a choice of (1) or (2) and hold back (3) for later.

Is there a way we can determine which features the gateway(s) will support at composition time?

don't think that's knowable given that different gateways in a fleet may be running different versions with different directive support -- and lots can change between composition time and the gateway loading a new schema at runtime.

this can be mitigated with immutable deploys (e.g. Packer AMIs, docker images, etc.), blue/green deploys with pre-cutover analysis tests, and declarative config management (Terraform or k8s) where new configs (images plus schema) must undergo admission control (pass some smoke tests) before being admitted to an environments config repo similar to the supergraph-demo-k8s-graphops (see simple smoke tests) or in the Analysis step of a BlueGreen deploy prior to cutover.

One additional smoke test could be an API Schema Check that compares the expected vs. actual api schema sha (compose-time vs. run-time) for a given supergraph schema sha running on the exact Gateway image that will be deployed with a given supergraph schema.

This would catch Gateway vs. supergraph schema mismatches before they were deployed live into an environment (assuming you did immutable image + schema deploys). However in the event that a supergraph schema did get pushed into a live environment, at that point would favor (2) with a partial outage vs. a total outage.

[prasek]

With the move to static composition one of our goals was predictability. As someone making or approving a change I want to be able to know what the resulting API schema will be before the change happens.

Agree this is a property we should strive to uphold. The supergraph schema and API schema should be an intrinsically linked pair, as much as possible, so they could be reasoned about as two sides of one thing.

In this context:
a. How can we ensure that the API schema generated by Studio / managed federation matches the API schema that the gateway actually serves? Am I correct in assuming this would only hold true if we choose option 1 of the three? Or...

Yes, option (1) fail fast - reject the entire schema is the only way to enforce this property vs. (2) gracefully degrading by omitting portions of the API schema at runtime.

If doing BlueGreen deploys, you probably want (1) anyways to simplify the smoke tests for admission control for config repo PRs or in the Analysis step of a BlueGreen deploy prior pointing the active service to point at the new deployment.

However in a dev environment, you might want (2) to enable free experimentation without breaking composition as a DX affordance.

You might also want (2) in an traditional managed federation update-in-place mode (polling the Uplink) to avoid prod outages when gateways don't have a previous version of a supergraph schema in memory and are running an older Gateway version that doesn't support some directives (e.g. when scaling replica counts, when k8s nodes are updated which evicts and restarts pods, etc.).

Having a way to pick the Gateway runtime behavior (1) or (2) that is suitable for different Gateway deployments seems both important and something that is best configured as part of the Gateway deployment itself, given that a single supergraph schema artifact on a given variant may be deployed into different Gateway environments.

If I own the infra do I want the schema to get to decide what should and shouldn't run, or do I want to control what's required to run

Agree with @ndintenfass that this needs more thought -- for seems more like a runtime config concern for a given Gateway deployment and something that graph operators would likely want to enforce for: security globally vs. relying on individual subgraph authors to consistently decorate subgraph directives. Imports may help, but defining the allowed/enforced of directives (e.g. dev mode switch) seems like more of a runtime config thing than a schema thing.

[prasek]

Some takeaways from discussion with @queerviolet and @martijnwalraven:

• Failure modes: (1) fast total failure and (2) graceful degradation from ☝️
  • Both are useful in different scenarios, so the spec should define both - @queerviolet suggest the spec requires (1) at a minimum with (2) being optional:
    • (1) fast total failure - simplifies smoke testing for blue/green, config repo admission control, and to ensure a supergraph has a predictable, single API schema that is served per @jstjoe's comments above ☝️.
    • (2) graceful degradation - for dev environments that need to more permissive, or for prod environments with loose deployment practices (i.e. no immutable deployment configs w/ admission control, no blue/green deploys, etc.)
  • graph operators should be able to pick between (1) or (2) for a given Gateway deployment, based on the use case for a given Gateway deployment - i.e. the schema should not inform the selection of the failure-mode, but rather should the failure-mode should be specified in separate runtime config.
• for: - to mark directives as required for security, execution, etc.
  • Useful to allow subgraph authors to define the core directives they need vs. relying solely on separate runtime allowlist config.
  • Composition can enforce the for: "security" on @inaccessible and other well known directives (and possibly populate for: automatically on the composed supergraph schema if omitted in the subgraph schemas)
  • An import mechanism should ease the burden of consistent core directive usage across subgraph schemas.
  • Separate from the core spec:
    • Higher-level governance could restrict/require core directives on a per-graph basis - providing graph operator control at composition-time
    • Runtime implementations could restrict/require core directives - providing graph operator control at runtime

[abernix]

@pcmanus Even with your current focus, I think this could stand to benefit from your review and your collaboration with @queerviolet on it as I believe we need to make considerations for how this would work in the context of authoring subgraph schemas and the changes to composition you're looking at: e.g., how does this flow through the pipeline, namespacing, differing for values across different subgraphs being composed. I haven't reviewed the above, but could you take a look?

[pcmanus]

I believe we need to make considerations for how this would work in the context of authoring subgraph

To be honest, I'm not sure I'm up to date on our story regarding core schema and subgraph authoring.

My most recent understanding is that the end goal would be that subgraph authors do not use @core directly, but instead use another "import mechanism" which is then "compiled" to @core declarations.

I'm not completely clear on whether the intent is for said "import mechanism" to expose the "purposes" introduced by this PR to subgraph authors, or instead to have said "purposes" associated to the feature and automatically imported by the "compiler". I seem however to have understood it was the latter (which makes more sense to me), in which case, there is no real authoring consideration. At least at that end goal.

That said, I also do not know if we intend a temporary state where we'd ask subgraph authors to manually author @core while we finish discussing, designing and implementing that "import mechanism". I personally feel we should avoid such temporary state though fwiw.

differing for values across different subgraphs being composed

This is a bit related to my previous points. If "purposes" are essentially hard-coded to a feature (through some kind of module definition), then there is no reason to get differing values (for a given feature) in practice and we can just simply error out if that's not the case, which is easy enough to implement.

In fact, even if subgraph authors manually author those "purposes", it feels erroring out on values discrepancies between subgraphs is the most reasonable option anyway.

[queerviolet]

yeah, i think in practice the composer can fail if different subgraphs have different purposes for a given feature. if it's some very well-known feature (i.e. something we have a list of in the composer) we could provide a better error or even just fix it. there isn't currently a "composing" section either here or in the join spec, but if there ever is, that would be a good place to discuss such things.

we do want to provide graph modules as a frontend which doesn't expose these details quite so hard (nor require users to correctly input metadata and definitions). i don't know if that will ship before subgraph core schemas. my suspicion is no, so there will be an intermediate period where folks are writing these by hand, but it really depends on release schedule.

it'll also always be possible for people to write core schemas by hand or with a tool other than our compiler—and for that matter, it is possible for our compiler to contain bugs—so having some kind of handling for these cases will always be germane

[lennyburdette]

Are directives for documentation and informational purposes relevant in this discussion? Between customers, there's a hankering for directives to flow from subgraph schemas to the API schema (and the Studio UI, ideally) without any affect on execution or security. Some examples:

• team/contact information
• api maturity levels
• references to examples/other docs

[queerviolet]

@lennyburdette such directives are in fact the default assumption! if for: is not specified, then processors and servers are expected to fail open, simply ignoring or passing through directives and metadata associated with unknown features. this is, i believe, the desired behavior for directives whose purposes are documentation, contact info, and so on—the server shouldn't fail because it doesn't understand @contact, it should just ignore it.

purposes explicitly specified with for: are laid out to cover cases where that behavior is undesirable—i.e. you pretty much never want to try to serve a field touched by unsupported EXECUTION directives, because you may not end up with the right data, and you definitely don't want to serve fields touched by unsupported SECURITY directives in production, because you may be creating a security hole. but apart from those (and perhaps other behaviorally-meaningful purposes we can enumerate), we're not trying to lay out a total taxonomy of everything you might want to use directives for, in part because i don't think we have enough information to do it properly, but also because it adds authoring burden for no real value.

[queerviolet]

(i should note that the gateway doesn't currently work this way, instead failing closed in the presence of any unsupported core features. although that's still allowable under this version of the spec, it's more clear that such behavior isn't generally desirable.)

[martijnwalraven]

Happy to see this land! As discussed in the comments, we need to figure out how this affects subgraph authoring. Avoiding manually specifying the purpose for each feature may be another argument for getting to modules/imports sooner rather than later.