Skip to content

[github] Allow polymorphic audit ruleset_conditions parameters via subobjects:false#20306

Merged
efd6 merged 1 commit into
elastic:mainfrom
vatsaldesai93:fix/github-audit-ruleset-conditions-subobjects
Jul 27, 2026
Merged

[github] Allow polymorphic audit ruleset_conditions parameters via subobjects:false#20306
efd6 merged 1 commit into
elastic:mainfrom
vatsaldesai93:fix/github-audit-ruleset-conditions-subobjects

Conversation

@vatsaldesai93

@vatsaldesai93 vatsaldesai93 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Proposed commit message

The audit ruleset_conditions parameters (include, exclude, repository_ids) are declared as keyword. That fits repository_name and repository_id targets, but repository_property targets send an object {name, source, property_values} in the same field, so the document is rejected to the failure store.

Setting subobjects to false on the parameters and old_parameters objects lets both forms coexist: strings stay on the keyword leaf, objects expand to dotted keyword leaves such as parameters.include.name. include, exclude and repository_ids are kept as explicit keyword declarations, so they stay statically mapped with the same names and types and existing ES|QL/KQL queries are unaffected. This is the polymorphic-parameters case that #19073 deferred.

Follow-ups from review: package-spec has no way to declare a scalar-or-object field (elastic/package-spec#1211), and elastic-package rejects an object at a keyword leaf under subobjects: false though Elasticsearch accepts it (elastic/elastic-package#3788).

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.
  • I have verified that any added dashboard complies with Kibana's Dashboard good practices (no dashboard changes).

How to test this PR locally

A keyword leaf accepts the string form but rejects the object form:

PUT gh-demo
{ "mappings": { "properties": { "params": { "type": "keyword" } } } }

PUT gh-demo/_doc/1
{ "params": ["repo-a", "repo-b"] }

PUT gh-demo/_doc/2
{ "params": { "name": "example", "source": "custom", "property_values": ["true"] } }

The second document fails with document_parsing_exception. With subobjects: false on the enclosing object both forms index cleanly. Then run elastic-package build and elastic-package test pipeline -C packages/github.

Related issues

@vatsaldesai93
vatsaldesai93 force-pushed the fix/github-audit-ruleset-conditions-subobjects branch from 5cc6e4f to c369122 Compare July 23, 2026 02:32
@elastic-vault-github-plugin-prod

Copy link
Copy Markdown
Contributor

Reviewers

Buildkite won't run for external contributors automatically; you need to add a comment:

  • /test : will kick off a build in Buildkite.

NOTE: https://github.com/elastic/integrations/blob/main/.buildkite/pull-requests.json contains all those details.

@andrewkroh andrewkroh added documentation Improvements or additions to documentation. Applied to PRs that modify *.md files. Integration:github GitHub labels Jul 23, 2026
@vatsaldesai93
vatsaldesai93 marked this pull request as ready for review July 23, 2026 15:40
@vatsaldesai93
vatsaldesai93 requested review from a team as code owners July 23, 2026 15:40
@vatsaldesai93
vatsaldesai93 force-pushed the fix/github-audit-ruleset-conditions-subobjects branch from c369122 to bbc8d3e Compare July 23, 2026 15:48
@andrewkroh andrewkroh added the Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations] label Jul 23, 2026
@infra-vault-gh-plugin-prod

Copy link
Copy Markdown

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

@vera-review-bot

Copy link
Copy Markdown

No issues across the latest commits bbc8d3e.

Since this is a community PR, a new commit triggers another review — at most once every 30 minutes. I skip the PR while it's approved or has merge conflicts.

🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills

⚠️ Automated review — verify suggestions before applying.

@efd6 efd6 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add pipeline tests for some of these.

This appears to be a breaking change for users who are using ES|QL to query ….parameters.{include,exclude,repository_ids}. I think that needs to be noted somewhere. You note that the issue could be avoided if package-spec were sufficiently strong to specify behaviours that elasticsearch is capable of. I think that is a worthwhile issue to file; we should not be limited by the configuration layer when the engine is capable.

Co-authored-by: Cursor <cursoragent@cursor.com>
@vatsaldesai93

vatsaldesai93 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @efd6, here's what I did for each. Let me know if I'm thinking about any of it wrong.

Breaking change. Rather than just note it, I kept include / exclude / repository_ids as explicit keyword fields alongside subobjects: false, so they stay mapped exactly as before, existing data and ES|QL queries should remain unaffected, and object values just add new leaves (include.name, etc.). This also corrects the PR description: explicit children are allowed under subobjects: false, okta does the same.

Pipeline tests. I kept the string form tests and tried to add one for the object form, but the elastic-package validator rejects an object at a keyword leaf (... does not match the expected field type: keyword), even though the same doc ingests fine in ES. So the validator, not the engine, is the blocker ? Filed elastic/elastic-package#3788. Does the string form coverage plus that issue suffice, or would you prefer a different approach ?

package-spec issue. Filed elastic/package-spec#1211 with no first class way to declare a scalar-or-object field and the validator gap as elastic/elastic-package#3788. Happy to reframe either.

@vatsaldesai93
vatsaldesai93 requested a review from efd6 July 26, 2026 17:52
@vera-review-bot

Copy link
Copy Markdown

⚠️ @vatsaldesai93 I couldn't complete the review for commits 7bd73531c4aab7 (20 commits) after several attempts. A maintainer can re-run it with @vera-review-bot review.

@efd6

efd6 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

/test

@efd6

efd6 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Thanks @vatsaldesai93, that all looks good, and thanks for the issues.

Since there is only one commit, can you make the commit message more informative? Normally, we would want the PR description to have a proposed commit message to be use in the commit merge (so, no Markdown, something along the lines described here).

@elastic-vault-github-plugin-prod

Copy link
Copy Markdown
Contributor

✅ All changelog entries have the correct PR link.

@elastic-vault-github-plugin-prod

Copy link
Copy Markdown
Contributor

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@infra-vault-gh-plugin-prod

Copy link
Copy Markdown

💚 Build Succeeded

@vatsaldesai93

Copy link
Copy Markdown
Contributor Author

@efd6 Added the proposed commit message to the description as requested (plain text, no Markdown). Thanks, let me know if you'd like any other tweaks.

@efd6
efd6 merged commit efa9aad into elastic:main Jul 27, 2026
4 checks passed
@elastic-vault-github-plugin-prod

Copy link
Copy Markdown
Contributor

Package github - 2.25.1 containing this change is available at https://epr.elastic.co/package/github/2.25.1/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation. Applied to PRs that modify *.md files. Integration:github GitHub Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants