-
Notifications
You must be signed in to change notification settings - Fork 6
feat(oaslint): add oaslint #371
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
base: main
Are you sure you want to change the base?
Changes from 35 commits
a143620
2cdaa1e
53d8c7a
0422636
24648b8
9a1bb96
843e7da
0fe4516
799f51e
0d63d9b
8e4df39
ed60c71
e1b5bde
3104ad0
f8e9e06
db2f31e
4db1eb6
427a979
ae6b0b1
6082d9a
82df404
20fad69
4edc142
9f6f6c2
941910d
e51d49d
d573338
72197cb
05c4c4e
cee8de3
32deef7
55f5f80
f967e1d
a94205f
a8119a6
557cec7
3ca4e9d
06e6d5c
71a10f5
abdcefb
7a51dfb
32b811e
091da41
0ad7014
02e32c2
f0e8817
732c272
5c6ecea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ package config | |
|
|
||
| import ( | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/pkg/errors" | ||
| "github.com/spf13/viper" | ||
|
|
@@ -16,6 +17,15 @@ type ServerConfig struct { | |
| Security SecurityConfig `json:"security"` | ||
| Log LogConfig `json:"log"` | ||
| FileManager FileManagerConfig `json:"fileManager"` | ||
| OasLinting OasLintingConfig `json:"oasLinting"` | ||
| } | ||
|
|
||
| type OasLintingConfig struct { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could use some comment, e.g. what is the meaning of ErrorMessage and DashboardURL? |
||
| ErrorMessage string `json:"errorMessage"` | ||
| Timeout time.Duration `json:"timeout"` | ||
| URL string `json:"url"` | ||
| DashboardURL string `json:"dashboardURL"` | ||
| SkipTLS bool `json:"skipTLS"` | ||
| } | ||
|
|
||
| type SecurityConfig struct { | ||
|
|
@@ -72,6 +82,13 @@ func setDefaults() { | |
| // FileManager | ||
| viper.SetDefault("fileManager.skipTLS", true) | ||
|
|
||
| // OAS Linting | ||
| viper.SetDefault("oasLinting.errorMessage", "Linter scan result contains errors. Please visit the linter UI for details on the RULESET_NAME_PLACEHOLDER ruleset.") | ||
| viper.SetDefault("oasLinting.timeout", 0) // 0 means block indefinitely until linter responds | ||
|
stefan-ctrl marked this conversation as resolved.
Outdated
|
||
| viper.SetDefault("oasLinting.url", "") | ||
| viper.SetDefault("oasLinting.dashboardURL", "") | ||
| viper.SetDefault("oasLinting.skipTLS", false) | ||
|
|
||
| // Database | ||
| viper.SetDefault("database.filepath", "") // empty string means in-memory only | ||
| viper.SetDefault("database.reduceMemory", false) // see common-server docs | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,154 @@ | ||
| // Copyright 2025 Deutsche Telekom IT GmbH | ||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package controller | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/go-logr/logr" | ||
| apiv1 "github.com/telekom/controlplane/api/api/v1" | ||
| commonclient "github.com/telekom/controlplane/common-server/pkg/client" | ||
| "github.com/telekom/controlplane/rover-server/internal/config" | ||
| "github.com/telekom/controlplane/rover-server/internal/oaslint" | ||
| roverv1 "github.com/telekom/controlplane/rover/api/v1" | ||
| ) | ||
|
|
||
| // LintOutcome describes how linting completed. | ||
| type LintOutcome int | ||
|
|
||
| const ( | ||
| // LintSkipped means no linting was needed (no config, whitelisted, or hash dedup). | ||
| LintSkipped LintOutcome = iota | ||
| // LintCompleted means linting ran synchronously and the result is on apiSpec.Spec.Lint. | ||
| LintCompleted | ||
| // LintBlocked means linting ran, the spec failed, and the category mode is Block. | ||
| LintBlocked | ||
| ) | ||
|
|
||
| // ApiLinter abstracts the full OAS linting lifecycle: config lookup, | ||
| // whitelists, and execution and should populate apiSpec.Spec.Lint with the result if linting was performed. | ||
| type ApiLinter interface { | ||
| // Lint performs the full linting lifecycle for an ApiSpecification. | ||
| // It looks up the linting config from the category list, checks whitelists, | ||
| // and runs the linter synchronously. | ||
| Lint(ctx context.Context, apiSpec *roverv1.ApiSpecification, category *apiv1.ApiCategory, specBytes []byte) (LintOutcome, error) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Normally you would pass []byte via io.Reader interfaces. It a bit nicer
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Implemented in 0ad7014 |
||
| } | ||
|
|
||
| // apiLinterImpl is the production implementation of ApiLinter. | ||
| type apiLinterImpl struct { | ||
| errorMessage string | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On first look this confused me a bit, but this basically is the errorMessage returned if the lint fails? Maybe name it errorMessageTemplate or add a comment
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| url string | ||
| dashboardURL string | ||
| httpClient oaslint.HTTPDoer | ||
| } | ||
|
|
||
| // NewApiLinter creates an ApiLinter from the given linting configuration. | ||
| func NewApiLinter(lintCfg config.OasLintingConfig) ApiLinter { | ||
| return &apiLinterImpl{ | ||
| errorMessage: lintCfg.ErrorMessage, | ||
| url: lintCfg.URL, | ||
| dashboardURL: lintCfg.DashboardURL, | ||
| httpClient: commonclient.NewHttpClientOrDie( | ||
| commonclient.WithClientName("oaslint"), | ||
| commonclient.WithClientTimeout(lintCfg.Timeout), | ||
| commonclient.WithSkipTlsVerify(lintCfg.SkipTLS), | ||
| ), | ||
| } | ||
| } | ||
|
|
||
| func (l *apiLinterImpl) Lint(ctx context.Context, apiSpec *roverv1.ApiSpecification, category *apiv1.ApiCategory, specBytes []byte) (LintOutcome, error) { | ||
| log := logr.FromContextOrDiscard(ctx) | ||
| log.V(1).Info("Looking up linting config", "namespace", apiSpec.Namespace, "name", apiSpec.Name, | ||
| "category", apiSpec.Spec.Category, "basepath", apiSpec.Spec.BasePath) | ||
|
|
||
| if category == nil { | ||
| log.V(1).Info("No category provided, skipping linting", "namespace", apiSpec.Namespace, "name", apiSpec.Name) | ||
| return LintSkipped, nil | ||
| } | ||
|
|
||
| lintCfg := category.Spec.Linting | ||
| if lintCfg == nil || l.url == "" || lintCfg.Mode == apiv1.LintingModeNone { | ||
| log.V(1).Info("No linting config or no URL, skipping linting", "namespace", apiSpec.Namespace, "name", apiSpec.Name) | ||
| return LintSkipped, nil | ||
| } | ||
|
|
||
| log.V(1).Info("Linting config found, checking whitelists", "namespace", apiSpec.Namespace, "name", apiSpec.Name) | ||
| if !l.prepareLinting(lintCfg, apiSpec) { | ||
| log.V(1).Info("Linting skipped (whitelisted)", "namespace", apiSpec.Namespace, "name", apiSpec.Name) | ||
| return LintSkipped, nil | ||
| } | ||
|
|
||
| if err := l.runLint(ctx, apiSpec, lintCfg.Ruleset, specBytes); err != nil { | ||
| return LintCompleted, err | ||
| } | ||
|
|
||
| if lintCfg.Mode == apiv1.LintingModeBlock && apiSpec.Spec.Lint != nil && !apiSpec.Spec.Lint.Passed { | ||
| return LintBlocked, fmt.Errorf("linting failed in block mode: %s", apiSpec.Spec.Lint.Message) | ||
| } | ||
|
|
||
| return LintCompleted, nil | ||
| } | ||
|
|
||
| func (l *apiLinterImpl) prepareLinting(lintCfg *apiv1.LintingConfig, apiSpec *roverv1.ApiSpecification) bool { | ||
| if isBasepathWhitelisted(lintCfg, apiSpec.Spec.BasePath) { | ||
| apiSpec.Spec.Lint = &roverv1.LintResult{Passed: true, Message: fmt.Sprintf("The basepath %q is whitelisted", apiSpec.Spec.BasePath)} | ||
| return false | ||
| } | ||
| apiSpec.Spec.Lint = nil | ||
| return true | ||
| } | ||
|
|
||
| func (l *apiLinterImpl) runLint(ctx context.Context, apiSpec *roverv1.ApiSpecification, ruleset string, specBytes []byte) error { | ||
| log := logr.FromContextOrDiscard(ctx).WithName("linting") | ||
|
|
||
| var opts []oaslint.ExternalLinterOption | ||
| if ruleset != "" { | ||
| opts = append(opts, oaslint.WithRuleset(ruleset)) | ||
| } | ||
| opts = append(opts, oaslint.WithHTTPClient(l.httpClient)) | ||
| linter := oaslint.NewExternalLinter(l.url, opts...) | ||
|
|
||
| result, err := linter.Lint(ctx, specBytes) | ||
| if err != nil { | ||
| apiSpec.Spec.Lint = &roverv1.LintResult{ | ||
| Passed: false, | ||
| Message: fmt.Sprintf("linter API error: %s", err), | ||
| } | ||
| log.Error(err, "OAS linting failed", "namespace", apiSpec.Namespace, "name", apiSpec.Name) | ||
| return fmt.Errorf("linter API error: %w", err) | ||
| } | ||
|
|
||
| apiSpec.Spec.Lint = l.buildLintResult(result) | ||
| if !apiSpec.Spec.Lint.Passed { | ||
| log.Info("Linting failed", "namespace", apiSpec.Namespace, "name", apiSpec.Name, "message", apiSpec.Spec.Lint.Message) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (l *apiLinterImpl) buildLintResult(result *oaslint.LintResult) *roverv1.LintResult { | ||
| lintResult := &roverv1.LintResult{ | ||
| Passed: result.Passed, | ||
| Message: result.Reason, | ||
| } | ||
| if l.dashboardURL != "" && result.LinterId != "" { | ||
| lintResult.DashboardURL = fmt.Sprintf("%s/scans/%s", strings.TrimRight(l.dashboardURL, "/"), result.LinterId) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this not handled the same way as the errorMessage template? "/scans" could be part of the dashboardURL or not? |
||
| } | ||
| if !result.Passed { | ||
| lintResult.Message = strings.ReplaceAll(l.errorMessage, "RULESET_NAME_PLACEHOLDER", result.Ruleset) | ||
| } | ||
| return lintResult | ||
| } | ||
|
|
||
| // isBasepathWhitelisted checks whether the given basepath is in the category's whitelist. | ||
| func isBasepathWhitelisted(cfg *apiv1.LintingConfig, basepath string) bool { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this not a method on LintingConfig? [nitpick]: Could use strings.ContainsFunc(...) |
||
| for _, wp := range cfg.WhitelistedBasepaths { | ||
| if strings.EqualFold(wp, basepath) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] These should be above the enum type