Skip to content
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.

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

Enforce visibility struct tags #89

Merged
merged 3 commits into from
May 7, 2024
Merged

Conversation

mbarnes
Copy link
Collaborator

@mbarnes mbarnes commented May 2, 2024

What this PR does

This enforces property visibility when creating or updating ARO-HCP resource types to comply with the OpenAPI spec. The implementation encodes property visibility as struct tags.

The autorest-generated API models have the useful characteristic of all struct fields being pointers. This allows us to identify which properties were explicitly provided in a create or update request.

However the generated models do not have property metadata encoded into struct tags, and we can't (or shouldn't) modify the generated models to add such information.

To get around this limitation, struct tags for property visibiility are encoded into the internal API models, which are not generated. The internal API models are of different Go types than the generated API models and are therefore not compatible, however the field names in each set of models are identical where property visibility is explicit (as opposed to implicitly inherited from parent types).

We can therefore build a lookup table of visibility struct tags by field name to use when evaluating create and update requests using the generated API models. We call this table a StructTagMap:

type StructTagMap map[string]reflect.StructTag

where the map keys are dotted struct field names (e.g. "A.B.C.D").

Then, while recursing over a generated API model type, we keep track of struct field names in order to build lookup keys for the table. This effectively allows struct tags from one Go type to be grafted on to another similar but incompatible Go type.

Jira: ARO-6347 : Validate metadata from api structs
Link to demo recording:

Special notes for your reviewer

Supporting documentation:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

  • PR: The PR description is expressive enough and will help future contributors
  • Code: Write code that humans can understand and Keep it simple
  • Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
  • Upgrade: Impact of this change on upgrade flows was considered and addressed if required
  • Deployment: The deployment process was considered and addressed if required
  • Testing: New code requires new unit tests.
  • Documentation: Is the documentation updated? Either in the doc located in focus area, in the README or in the code itself.
  • Customers: Is this change affecting customers? Is the release plan considered?

@mbarnes mbarnes changed the title api: Enforce visibility struct tags Enforce visibility struct tags May 2, 2024
This enforces property visibility when creating or updating ARO-HCP
resource types to comply with the OpenAPI spec.  The implementation
encodes property visibility as struct tags.

The autorest-generated API models have the useful characteristic of
all struct fields being pointers.  This allows us to identify which
properties were explicitly provided in a create or update request.

However the generated models do not have property metadata encoded
into struct tags, and we can't (or shouldn't) modify the generated
models to add such information.

To get around this limitation, struct tags for property visibiility
are encoded into the INTERNAL API models, which are not generated.
The internal API models are of different Go types than the generated
API models and are therefore not compatible, however the field names
in each set of models are identical where property visibility is
explicit (as opposed to implicitly inherited from parent types).

We can therefore build a lookup table of visibility struct tags by
field name to use when evaluating create and update requests using
the generated API models.  We call this table a StructTagMap:

type StructTagMap map[string]reflect.StructTag

where the map keys are dotted struct field names (e.g. "A.B.C.D").

Then, while recursing over a generated API model type, we keep
track of struct field names in order to build lookup keys for the
table.  This effectively allows struct tags from one Go type to be
grafted on to another similar but incompatible Go type.
@mbarnes mbarnes force-pushed the enforce-visibility branch from 1c2e992 to 8307aaf Compare May 2, 2024 18:41
@mbarnes
Copy link
Collaborator Author

mbarnes commented May 2, 2024

@bennerv asked me (on Slack) about how we'd handle a potential field visibility change in future APIs out of concern that the metadata is on the internal structs. I wanted to address that here.

It's worth noting this type of change has never occurred on ARO Classic, so it's likely to be rare here too. But we can't discount it.

Suppose API versions v1-v4 (simplifying the version IDs for the sake of discussion) all had some field "foo" that was tagged @visibility("read", "create") in the OpenAPI spec.

Then v5 comes along and changes field "foo" to be @visibility("read", "create", "update").

Here's how I would handle that:

  • Update the struct tag for field "foo" in the internal API to be visibility:"read create update". Visibility is now correct for v5 and subsequent versions.

  • Each of the API packages for v1-v4 will have their own StructTagMap instances.

package v1 (or v2 or v3 or v4)

var clusterStructTagMap = api.NewStructTagMap[api.HCPOpenShiftCluster]()
  • We would need to override the clusterStructTagMap for field "foo" in each of these older API packages to restore the original visibility for field "foo".
func init() {
        ...

        // This field was changed in v5 to be "read create update".
        clusterStructTagMap["Properties.Spec.Foo"] = reflect.StructTag("visibility:\"read create\"")
}

mjlshen
mjlshen previously approved these changes May 6, 2024
Copy link
Contributor

@mjlshen mjlshen left a comment

Choose a reason for hiding this comment

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

The proposed mitigation for api visibility changes makes sense to me as well.

s-amann
s-amann previously approved these changes May 7, 2024
internal/api/visibility.go Outdated Show resolved Hide resolved
@mbarnes mbarnes dismissed stale reviews from s-amann and mjlshen via f6b8f3f May 7, 2024 16:31
Too many single-letter variables make for poor readability, turns out.
@mbarnes mbarnes force-pushed the enforce-visibility branch from f6b8f3f to cf19516 Compare May 7, 2024 16:35
@mbarnes mbarnes merged commit 5354076 into Azure:main May 7, 2024
5 checks passed
@mbarnes mbarnes deleted the enforce-visibility branch May 7, 2024 18:00
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