How to integrate a custom rule into the linter?

[julioz]

While adopting AIPs in my organization, and relying on api-linter to perform the validations, we have pretty quickly hit the need of writing our own custom rules, that would extend the behavior of the linter.

This page describes how one might adopt AIPs on their orgs, but I found no reference on this repository, nor on the documentation page on what the advised approach is, or the steps to be taken.

Our current workaround is to fork this repository and maintain our own copy of the linter, but that has an embedded operational cost of distributing the binary, keeping it in sync with the upstream remote, etc.

Let's say I'd like to write the simple rule as follows:

var myCustomRule = &lint.MessageRule{
    Name: lint.NewRuleName("custom", "9001", "message-naming"),
    LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
        if !strings.HasPrefix(m.GetName(), "MyCompany") {
            return []lint.Problem{{
                Message:    "Message names must start with 'MyCompany'",
                Descriptor: m,
            }}
        }
        return nil
    },
}

[noahdietz]

Hey @julioz I don't think there is currently an easy way to integrate custom rules with the api-linter CLI.

The RuleRegistry is loaded internally for the CLI here:

api-linter/cmd/api-linter/main.go

Lines 27 to 47 in c674736

	var (
	globalRules = lint.NewRuleRegistry()
	globalConfigs = defaultConfigs()
	)
	func init() {
	if err := rules.Add(globalRules); err != nil {
	log.Fatalf("error when registering rules: %v", err)
	}
	}
	func main() {
	if err := runCLI(os.Args[1:]); err != nil {
	log.Fatalln(err)
	}
	}
	func runCLI(args []string) error {
	c := newCli(args)
	return c.lint(globalRules, globalConfigs)
	}

What would need to happen (as I'm sure you know) is the ApiLinter would need a mechanism for specifying and dynamically loading package-external rules to add them to the RuleRegistry used by the CLI to lint the source files.

One back-of-the-napkin idea would be to use the Go plugin API for this and specify some "custom rule API" like an exported func AddCustomRules(r lint.RuleRegistry) error function that the CLI invokes after loading the plugin specified by --rule_plugin="./customrules.so". This could get pretty tricky from a maintainability standpoint as it requires that caller and callee (plugin) are built with the same Go version - basically custom rule authors would be required to use the exact version of Go that ApiLinter uses.

While it's not ideal, writing your own CLI/executor and initializing your own RuleRegistry a la the above snippet, then adding your custom rules would be the "simplest" approach without changing anything in ApiLinter today. I don't know how viable this is TBH.

[julioz]

Thanks @noahdietz , I'm positively surprised by how responsive you are in this repository, it has certainly not been my experience with other Google-owned open source 😊

Since we start getting into "design-land", I wonder whether you would consider an alternative to Go's plugin library (which is essentially a wrapper on dlopen) into https://github.com/hashicorp/go-plugin.

Here's a good example of how the architecture would look like.

Of course, this would mean we'd need to apply some changes on how the api-linter is built, to allow for the flexibility, essentially making it an RPC client.
Since this is an gRPC linter anyways, I see some synergy.

Before we spend energy discussing this path, I wonder if it's even something that would be considered in this Google library.

[julioz]

To document my path for future readers, in order to write a custom lint rule, I have:

• forked this repository
• created my custom rule (code sample [1]), the aip definition [2], and an associated test file [3]
• since the custom rules live under the range 9xxx, we need to update rule_groups.go to allow for the internal aip group
• Update rules/rules.go to add your new package, and reference into the slice.
• execute tests with go test ./...
• for the sake of verification, I have also updated version.go to customize the version of the output binary
• The main binary is built from the cmd/api-linter package, so run go build ./cmd/api-linter to compile the binary in my own machine's architecture

With this, I am able to run the custom AIP rule, and also see the rule_id:

$ ../api-linter/api-linter --config api-linter.yaml assessment.proto

- file_path: assessment.proto
  problems:
    - message: Message names must start with 'Julio'
      suggestion: JulioSendAssessmentTasksRequest
      location:
        start_position:
            line_number: 12
            column_number: 1
        end_position:
            line_number: 12
            column_number: 37
        path: assessment.proto
      rule_id: internal::9000::julio-prefix
      rule_doc_uri: ""

The rule_id can also be used to supress the rule if need be in the AIP Linter config file (api-linter.yaml):

---
- disabled_rules:
    - 'internal::9000::julio-prefix'

---

[1]:

package aip9000

import (
	"strings"

	"github.com/googleapis/api-linter/lint"
	"github.com/jhump/protoreflect/desc"
)

var julioPrefix = &lint.MessageRule{
	Name: lint.NewRuleName(9000, "julio-prefix"),
	OnlyIf: func(m *desc.MessageDescriptor) bool {
		return strings.HasSuffix(m.GetName(), "Request") || strings.HasSuffix(m.GetName(), "Response")
	},
	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
		name := m.GetName()
		if !strings.HasPrefix(name, "Julio") {
			return []lint.Problem{{
				Message:    "Message names must start with 'Julio'",
				Descriptor: m,
				Suggestion: "Julio" + name,
			}}
		}
		return nil
	},
}

[2]:

// Package aip9000 contains custom rules for the organization
package aip9000

import (
	"github.com/googleapis/api-linter/lint"
)

// AddRules adds all of the custom rules to the provided registry.
func AddRules(r lint.RuleRegistry) error {
	return r.Register(
		9000,
		julioPrefix,
	)
}

[3]:

package aip9000

import (
	"testing"

	"github.com/googleapis/api-linter/rules/internal/testutils"
)

func TestJulioPrefix(t *testing.T) {
	tests := []struct {
		name     string
		Message  string
		problems testutils.Problems
	}{
		{
			name: "Valid",
			Message: `
                message JulioGetBookRequest {
                    string name = 1;
                }
            `,
			problems: nil,
		},
		{
			name: "Invalid",
			Message: `
                message GetBookRequest {
                    string name = 1;
                }
            `,
			problems: testutils.Problems{{
				Message: "Message names must start with 'Julio'",
			}},
		},
	}

	for _, test := range tests {
		t.Run(test.name, func(t *testing.T) {
			file := testutils.ParseProto3String(t, test.Message)
			message := file.GetMessageTypes()[0]
			problems := julioPrefix.Lint(file)
			if diff := test.problems.SetDescriptor(message).Diff(problems); diff != "" {
				t.Error(diff)
			}
		})
	}
}

[julioz]

@noahdietz So, looking at the steps identified on the above, we can have a sense of the required integration points, if we were to extend the library to allow for plug-ins.

[noahdietz]

I'm positively surprised by how responsive you are in this repository, it has certainly not been my experience with other Google-owned open source 😊

You spoke too soon - I've ignored this for a few days now 😆 but seriously, thank you, I appreciate you saying that. I try my best.

Here's a good example of how the architecture would look like.

Oh, yeah, that's a very useful write up. After reading it I think both approaches have merit, and I'm not 100% sure of either...my core priorities would be as follows:

• dead simple to implement
• dead simple to maintain (for both parties)
• minimize complexity, but make sure we build in enough flexibility for future extensibility
• do not try to boil the ocean (e.g. supporting non-Go based implementations)

Those have me still lean more towards shared-library/Go plugin model (we could implement it in a few days with minimal friction for all involved).

To document my path for future readers, in order to write a custom lint rule...looking at the steps identified on the above, we can have a sense of the required integration points

Thank you very much for taking the time to include this. The AddRules portion is certainly what seems to be the primary API for integration in my mind (at least in a shared-library plugin model). This is how we register internal-only rules internally, by providing more AddRules functions for registration, hence why my mind went there first.

Before we spend energy discussing this path, I wonder if it's even something that would be considered in this Google library.

This is the question isn't it! At the moment, I don't think this would be a priority. My main priority is to begin planning for migration off of jhump/protoreflect, which will require a new major version (😿) . This might one of those things that we look at whilst planning, especially because adding it now would expand the surface that is taking a major version :) Adding it afterwards means we and you have less to worry about/migrate.

[julioz]

At the moment, I don't think this would be a priority. My main priority is to begin planning for migration off of jhump/protoreflect, which will require a new major version (😿) . This might one of those things that we look at whilst planning, especially because adding it now would expand the surface that is taking a major version :) Adding it afterwards means we and you have less to worry about/migrate.

This is great to know. If a major bump is in the near future outlook, this is a great time to start designing what a plugin API would look like. Technically speaking, for the long run I am more inclined to the hashicorp/go-plugin solution, exactly due to the stronger decoupling it introduces in comparison to goplugin. It would be lovely if you could point me into a "public roadmap" for this repository (do you have something public like e.g this?); as my org is currently pretty keen in investing in this area, it's going to help us focus our attention on where to invest time.

That said, I am also curious to verify what goplugin would mean in terms of implementation effort, and especially how/if it could (really) end up becoming a burden on plugin authors. I will set some time aside in the next days to invest a bit in implementing that on my fork and can write here when I have news.

[julioz]

Hey @noahdietz,

I've implemented a working solution for custom rule plugins and submitted it as PR #1487.

I went with the Go plugin approach you mentioned in your initial comment. The implementation includes:

1. A --rule-plugin flag to specify one or more .so plugin files
2. A plugin loader that expects an exported AddRules(r lint.RuleRegistry) error function
3. Build scripts to address the Go plugin version compatibility requirements
4. A working example plugin that enforces message names starting with "Julio"

As you mentioned, Go's plugin system does have strict version requirements. To address this, I included:

• A build-all.sh script that builds both the api-linter and plugin in the same environment
• An improved plugin build script with version detection

The PR has a detailed description with usage examples and test commands. This approach should be relatively easy to maintain, as it follows the existing pattern of using AddRules functions for registration.

I understand that with the upcoming major version (moving away from jhump/protoreflect), this might not be the long-term solution. If accepted, it could provide immediate value to users while a more robust RPC-based solution could be considered for the next major version. There are definitely parts we need to iterate there, and I don't expect it to be a direct-merge solution since it's supposed to only be an illustration on how a custom plugin could be implemented. Nevertheless, I'm interested in your review for the overall architecture.

Thanks again for your guidance on this.

[jpeach]

FWIW, I'm also interested in this. Right now, we have custom validations in a protoc plugin, but api-linter has a much better user experience. In the short term, I'll probably vendor a hacked-up copy of cmd/api-linter, but would be interested in adopting an official approach.