Add provider_permissions field for declaring integration permission requirements#1180
Add provider_permissions field for declaring integration permission requirements#1180jeniawhite wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a provider_permissions JSON schema definition, wires it into package, policy_template, input, and data_stream specs, and supplies validator tests plus good/bad package fixtures exercising acceptance and rejection of the new shape. ChangesProvider Permissions Schema and Test Coverage
Estimated code review effort: Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/changelog.yml`:
- Line 13: Update the placeholder link value for the changelog entry by
replacing the current "https://github.com/elastic/package-spec/pull/0000" with
the actual GitHub PR URL for this change or the literal string "TBD" if the PR
does not yet exist; locate the YAML entry using the link key in the changelog
record and ensure the value is a full GitHub PR URL format
(https://github.com/{owner}/{repo}/pull/{number}) or "TBD".
In `@spec/integration/manifest.spec.yml`:
- Around line 1082-1089: Reorder the JSON Patch ops so all removals of property
references occur before removing the definition; specifically, move the three
remove ops for "/properties/provider_permissions",
"/properties/policy_templates/items/properties/provider_permissions", and
"/properties/policy_templates/items/properties/inputs/items/properties/provider_permissions"
ahead of the remove op for "/definitions/provider_permissions" so references are
deleted before the "/definitions/provider_permissions" entry is removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ce53ca47-2ebc-460d-89cb-145c66c83415
📒 Files selected for processing (34)
code/go/pkg/validator/validator_test.gospec/changelog.ymlspec/input/manifest.spec.ymlspec/integration/data_stream/manifest.spec.ymlspec/integration/manifest.spec.ymltest/packages/bad_provider_permissions/LICENSE.txttest/packages/bad_provider_permissions/changelog.ymltest/packages/bad_provider_permissions/docs/README.mdtest/packages/bad_provider_permissions/manifest.ymltest/packages/bad_provider_permissions_extra_field/LICENSE.txttest/packages/bad_provider_permissions_extra_field/changelog.ymltest/packages/bad_provider_permissions_extra_field/docs/README.mdtest/packages/bad_provider_permissions_extra_field/manifest.ymltest/packages/bad_provider_permissions_missing_name/LICENSE.txttest/packages/bad_provider_permissions_missing_name/changelog.ymltest/packages/bad_provider_permissions_missing_name/docs/README.mdtest/packages/bad_provider_permissions_missing_name/manifest.ymltest/packages/bad_provider_permissions_old_version/LICENSE.txttest/packages/bad_provider_permissions_old_version/changelog.ymltest/packages/bad_provider_permissions_old_version/docs/README.mdtest/packages/bad_provider_permissions_old_version/manifest.ymltest/packages/good_provider_permissions/LICENSE.txttest/packages/good_provider_permissions/changelog.ymltest/packages/good_provider_permissions/data_stream/ec2_metrics/agent/stream/stream.yml.hbstest/packages/good_provider_permissions/data_stream/ec2_metrics/fields/fields.ymltest/packages/good_provider_permissions/data_stream/ec2_metrics/manifest.ymltest/packages/good_provider_permissions/docs/README.mdtest/packages/good_provider_permissions/manifest.ymltest/packages/good_provider_permissions_input/LICENSE.txttest/packages/good_provider_permissions_input/agent/input/input.yml.hbstest/packages/good_provider_permissions_input/changelog.ymltest/packages/good_provider_permissions_input/docs/README.mdtest/packages/good_provider_permissions_input/fields/base-fields.ymltest/packages/good_provider_permissions_input/manifest.yml
| - description: Add support for semantic_text field definition. | ||
| type: enhancement | ||
| link: https://github.com/elastic/package-spec/pull/807 | ||
| - description: Add provider_permissions field to package, policy_template, input, and data_stream levels for declaring provider-specific permissions. |
There was a problem hiding this comment.
is there any change required on the kibana side for this to be released?
There was a problem hiding this comment.
we will use the provider_permissions from kibana to generate a list of required permissions in the onboarding flow: https://github.com/elastic/ingest-dev/issues/7927
there should be no dependency on kibana, the new fields are additive and won't be used on old kibana versions (should be smoke tested)
There was a problem hiding this comment.
As this doesn't affect existing versions of Kibana we can release this in 3.6.4, and use it on any package.
| provider_permissions: | ||
| description: > | ||
| Permissions and roles this integration unit requires from the named provider. | ||
| May be declared at package, policy_template, input, and data_stream levels. |
There was a problem hiding this comment.
Could we document how different levels of declaring this permissions work? what if i declare it at package and then at an input? does it override? Are there any cases where we might need to check semantically the meaning of this permissions across levels? Is there any case we want to avoid? or validate?
| provider: | ||
| description: > | ||
| Identifier of the provider these permissions apply to | ||
| (e.g. "aws", "gcp", "azure", "kubernetes", "okta"). |
There was a problem hiding this comment.
| (e.g. "aws", "gcp", "azure", "kubernetes", "okta"). |
as we have them as example on the examples i think this might be redundant. Else, we can keep them here and perhaps add a single example (reducing the lines)
jsoriano
left a comment
jsoriano
left a comment
There was a problem hiding this comment.
FTR, as discussed over slack, as these new settings won't affect current versions of kibana, we can release this in next 3.6 version, and we can remove the version patches for 3.7.0.
This way this can be earlier used in packages without needing to bump their kibana or format versions.
| - before: 3.7.0 | ||
| patch: | ||
| - op: remove | ||
| path: "/properties/provider_permissions" | ||
| - op: remove | ||
| path: "/properties/policy_templates/items/properties/provider_permissions" |
There was a problem hiding this comment.
JSON patches would not be needed if this new setting doesn't affect existing versions of Fleet.
| - before: 3.7.0 | |
| patch: | |
| - op: remove | |
| path: "/properties/provider_permissions" | |
| - op: remove | |
| path: "/properties/policy_templates/items/properties/provider_permissions" |
| - before: 3.7.0 | ||
| patch: | ||
| # provider_permissions field for provider-specific permission declarations. | ||
| - op: remove | ||
| path: "/properties/provider_permissions" |
There was a problem hiding this comment.
JSON patches would not be needed if this new setting doesn't affect existing versions of Fleet.
| - before: 3.7.0 | |
| patch: | |
| # provider_permissions field for provider-specific permission declarations. | |
| - op: remove | |
| path: "/properties/provider_permissions" |
| - before: 3.7.0 | ||
| patch: | ||
| # provider_permissions field for provider-specific permission declarations. | ||
| - op: remove | ||
| path: "/properties/provider_permissions" | ||
| - op: remove | ||
| path: "/properties/policy_templates/items/properties/provider_permissions" | ||
| - op: remove | ||
| path: "/properties/policy_templates/items/properties/inputs/items/properties/provider_permissions" | ||
| - op: remove | ||
| path: "/definitions/provider_permissions" |
There was a problem hiding this comment.
| - before: 3.7.0 | |
| patch: | |
| # provider_permissions field for provider-specific permission declarations. | |
| - op: remove | |
| path: "/properties/provider_permissions" | |
| - op: remove | |
| path: "/properties/policy_templates/items/properties/provider_permissions" | |
| - op: remove | |
| path: "/properties/policy_templates/items/properties/inputs/items/properties/provider_permissions" | |
| - op: remove | |
| path: "/definitions/provider_permissions" |
| - description: Add support for semantic_text field definition. | ||
| type: enhancement | ||
| link: https://github.com/elastic/package-spec/pull/807 | ||
| - description: Add provider_permissions field to package, policy_template, input, and data_stream levels for declaring provider-specific permissions. |
There was a problem hiding this comment.
As this doesn't affect existing versions of Kibana we can release this in 3.6.4, and use it on any package.
|
@teresaromero @jsoriano |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/integration/manifest.spec.yml (1)
1077-1080:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a version-gating patch for
provider_permissionsto preserve spec-version contracts.Line 1077 starts the
versionslist, but there is nobeforepatch removingprovider_permissionsfor olderformat_versionvalues. As written, versions before the feature’s introduction can still accept this field.Suggested patch shape
versions: + - before: 3.6.4 + patch: + - op: remove + path: "/properties/provider_permissions" + - op: remove + path: "/properties/policy_templates/items/properties/provider_permissions" + - op: remove + path: "/properties/policy_templates/items/properties/inputs/items/properties/provider_permissions" + - op: remove + path: "/definitions/provider_permissions" - before: 3.6.0 patch:Based on learnings: “In
.spec.ymlfiles that support multiple format versions, addversions.beforeremove patches for newpropertiesanddefinitionswhen olderformat_versionvalues should not include the new schema.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/integration/manifest.spec.yml` around lines 1077 - 1080, Add a version-gating "before" patch to the versions list that removes the new provider_permissions schema for older format_version values so pre-introduction manifests cannot accept that field; locate the versions block (the existing "versions:" list starting at the current entry) and add a patch entry that targets and removes the provider_permissions property/definition (referencing "provider_permissions") under the appropriate "before" key so older versions are stripped of that property while newer patches keep it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/changelog.yml`:
- Around line 11-15: The changelog entry was added as a new released version
block ("version: 3.6.4") but must be appended to the in-development section
("version: 3.7.0-next"); move the entire changes item (the list entry with
description about provider_permissions and link to PR `#1180`) out of the newly
created 3.6.4 block and insert it at the bottom of the changes list under the
existing "version: 3.7.0-next" section so it becomes the last item in that list.
---
Outside diff comments:
In `@spec/integration/manifest.spec.yml`:
- Around line 1077-1080: Add a version-gating "before" patch to the versions
list that removes the new provider_permissions schema for older format_version
values so pre-introduction manifests cannot accept that field; locate the
versions block (the existing "versions:" list starting at the current entry) and
add a patch entry that targets and removes the provider_permissions
property/definition (referencing "provider_permissions") under the appropriate
"before" key so older versions are stripped of that property while newer patches
keep it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: da7a3b24-9f9e-413e-b87e-3f763c858556
📒 Files selected for processing (10)
code/go/pkg/validator/validator_test.gospec/changelog.ymlspec/input/manifest.spec.ymlspec/integration/data_stream/manifest.spec.ymlspec/integration/manifest.spec.ymltest/packages/bad_provider_permissions/manifest.ymltest/packages/bad_provider_permissions_extra_field/manifest.ymltest/packages/bad_provider_permissions_missing_name/manifest.ymltest/packages/good_provider_permissions/manifest.ymltest/packages/good_provider_permissions_input/manifest.yml
💤 Files with no reviewable changes (3)
- code/go/pkg/validator/validator_test.go
- spec/input/manifest.spec.yml
- spec/integration/data_stream/manifest.spec.yml
| - version: 3.6.4 | ||
| changes: | ||
| - description: Add provider_permissions field to package, policy_template, input, and data_stream levels for declaring provider-specific permissions. | ||
| type: enhancement | ||
| link: https://github.com/elastic/package-spec/pull/1180 |
There was a problem hiding this comment.
Move this changelog item under 3.7.0-next instead of creating a new 3.6.4 block.
Line 11 introduces a new released-version section, but changelog policy here requires adding new PR entries to the bottom of the in-development -next version’s changes list.
Suggested edit
- version: 3.7.0-next
changes:
# Pending on https://github.com/elastic/kibana/issues/220294
- description: Add support for semantic_text field definition.
type: enhancement
link: https://github.com/elastic/package-spec/pull/807
+ - description: Add provider_permissions field to package, policy_template, input, and data_stream levels for declaring provider-specific permissions.
+ type: enhancement
+ link: https://github.com/elastic/package-spec/pull/1180
-- version: 3.6.4
- changes:
- - description: Add provider_permissions field to package, policy_template, input, and data_stream levels for declaring provider-specific permissions.
- type: enhancement
- link: https://github.com/elastic/package-spec/pull/1180As per coding guidelines: “Changelog entries must be added at the bottom of the changes list of the in-development version (the one with -next suffix).”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/changelog.yml` around lines 11 - 15, The changelog entry was added as a
new released version block ("version: 3.6.4") but must be appended to the
in-development section ("version: 3.7.0-next"); move the entire changes item
(the list entry with description about provider_permissions and link to PR
`#1180`) out of the newly created 3.6.4 block and insert it at the bottom of the
changes list under the existing "version: 3.7.0-next" section so it becomes the
last item in that list.
There was a problem hiding this comment.
@jsoriano
Since we want to release as part of 3.6.4, I assume that pushing this to 3.7.0-next is not the intended change. Please correct me if I'm wrong.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Co-authored-by: Tere <romero.teresa@protonmail.com>
💚 Build Succeeded
History
|
5 participants
What does this PR do?
Adds a new optional provider_permissions field to the package manifest schema for both integration and input package types. The field can be declared at four levels - package, policy_template, input, and data_stream - allowing permission requirements to be expressed at the right granularity. Each entry in the array targets a specific provider (e.g. aws, gcp, azure, kubernetes) and carries two optional sub-fields:
The field is introduced in spec version 3.7.0 and is removed via version patches for older format_version values.
Why is it important?
Integration packages currently have no machine-readable way to declare what provider permissions they require. This means permission requirements live only in documentation, drift over time, and cannot be used by tooling. With provider_permissions in the spec, tooling can read exactly which permissions each enabled integration needs and generate appropriately scoped infrastructure-as-code (IaC) artifacts - rather than granting a static kitchen-sink policy. The permission declarations are versioned alongside the package, making them auditable and kept in sync with the data streams they support.
Checklist
test/packagesthat prove my change is effective.spec/changelog.yml.Related issues
Summary by CodeRabbit
New Features
Documentation
Tests