⭐ Expand OCI provider with new resources for CIS benchmark coverage#6623
Open
⭐ Expand OCI provider with new resources for CIS benchmark coverage#6623
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
This comment has been minimized.
This comment has been minimized.
tas50
force-pushed
the
tas50/oci_mega_add
branch
3 times, most recently
from
5f643f6 to
57ec08a
Compare
This comment has been minimized.
This comment has been minimized.
tas50
force-pushed
the
tas50/oci_mega_add
branch
7 times, most recently
from
69d88fb to
96ee221
Compare
tas50
force-pushed
the
tas50/oci_mega_add
branch
from
96ee221 to
4490054
Compare
![mondoo-code-review[bot]](https://avatars.githubusercontent.com/in/2834902?s=60&v=4){kind=small}
There was a problem hiding this comment.
{
"summary": "Good overall structure and consistent patterns, but boot volume listing will fail at runtime due to a missing required API parameter, and two schema inconsistencies need addressing.",
"verdict": "REQUEST_CHANGES",
"findings": [
{
"path": "providers/oci/resources/compute.go",
"line": 478,
"endLine": 481,
"severity": "critical",
"body": "`getBootVolumesForRegion` constructs `ListBootVolumesRequest` without `AvailabilityDomain`, which is a **required** parameter in the OCI REST API (unlike `ListVolumes` for block volumes, which only needs `compartmentId`). This will produce a 400 error at runtime.\n\nCompare with `filestorage.go:51-76`, which correctly iterates availability domains per region:\n```go\nadResponse, err := identityClient.ListAvailabilityDomains(ctx, identity.ListAvailabilityDomainsRequest{\n CompartmentId: common.String(conn.TenantID()),\n})\n// ... then per AD:\nrequest := filestorage.ListFileSystemsRequest{\n CompartmentId: common.String(compartmentID),\n AvailabilityDomain: common.String(*ad.Name),\n Page: page,\n}\n```\n`getBootVolumesForRegion` needs the same AD-iteration pattern."
},
{
"path": "providers/oci/resources/oci.lr",
"line": 274,
"endLine": 300,
"severity": "warning",
"body": "`oci.compute.blockVolume` and `oci.compute.bootVolume` define `compartmentID string` instead of `compartment oci.compartment`, inconsistent with the existing `oci.compute.instance` and `oci.compute.image` resources which use a typed resource reference. Per the project guidelines: *"Prefer typed resource references over raw ID strings."*\n\nChange to:\n```\ncompartment oci.compartment\n```\nand populate it the same way `oci.compute.instance` does (via `CreateResource(\"oci.compartment\", ...)`). The same inconsistency exists on `oci.network.networkSecurityGroup.vcnId string` (line 387) — that should be `vcn oci.network.vcn`."
},
{
"path": "providers/oci/resources/oci.lr",
"line": 430,
"endLine": 447,
"severity": "warning",
"body": "`oci.logging.log` is missing a `created time` field. Every other new resource in this PR exposes creation time (`logGroup`, `kmsVault`, `kmsKey`, `eventsRule`, `fileSystem`, `blockVolume`, `bootVolume`, etc.), and `logging.LogSummary.TimeCreated` is available in the SDK.\n\nAdd to the schema:\n```\n// Log creation time\ncreated time\n```\nAnd in `logging.go`'s `logs()` method, populate it the same way `logGroups()` does with `llx.TimeDataPtr(created)`."
}
]
}
Member
Author
|
/review |
tas50
force-pushed
the
tas50/oci_mega_add
branch
from
4490054 to
3958399
Compare
New resource services: - Cloud Guard: oci.cloudGuard, targets, and detector recipes - Events: oci.events and event rules - File Storage: oci.fileStorage and file systems - KMS: oci.kms, vaults, and keys - Logging: oci.logging, log groups, and logs - ONS (Notifications): oci.ons, topics, and subscriptions - Network Security Groups: oci.network.networkSecurityGroup with ingress/egress security rules Enhancements to existing resources: - Compute: block volumes, boot volumes, and new instance fields (platformConfig, launchOptions, instanceOptions, shapeConfig, sourceDetails, metadata) - Buckets: additional fields - Networking: NSG support with lazy-loaded security rules Signed-off-by: Tim Smith <tsmith84@gmail.com>
Signed-off-by: Tim Smith <tsmith84@gmail.com>
tas50
force-pushed
the
tas50/oci_mega_add
branch
from
3958399 to
e1018d0
Compare
There was a problem hiding this comment.
New OCI resources for CIS benchmarks are well-structured but missing version entries for all new resources and fields
Additional findings (file/line not in diff):
- 🔵
providers/oci/resources/oci.lr:501—oci.logging.loghasretentionDuration intbut the OCI SDK'sRetentionDurationfield is*int32. Whilellx.IntDataPtrhandles*int32via generics, double-check that the OCI SDK type for this field is numeric and not a duration enum. If it is indeed*int32, the current code is correct.
- Fix stale cnquery/v12 imports in 6 new resource files (cloudguard, events, filestorage, kms, logging, ons) and regenerated oci.lr.go - Add nil checks for ObjectEventsEnabled and ReplicationEnabled pointer dereferences in buckets.go to prevent runtime panics - Remove unused parseTime helper and its tests - Add region tracking to KMS vault resources for future extensibility - Add clarifying comment on CloudGuard's home-region-only strategy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tas50
force-pushed
the
tas50/oci_mega_add
branch
from
e1018d0 to
d229cda
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- oci.kms.key: isAutoRotationEnabled - oci.compute.instance: timeMaintenanceRebootDue - oci.logging.log: created, timeLastModified Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lt secrets New service resources: - oci.audit: tenancy-level audit log retention configuration - oci.bastion: bastion instances with typed VCN references - oci.monitoring: alarm definitions with severity, query, destinations - oci.vault: secrets with rotation status, typed KMS key/vault references Also converts existing ID fields to typed resource references: - securityList.vcnId → securityList.vcn() oci.network.vcn - networkSecurityGroup.vcnId → networkSecurityGroup.vcn() oci.network.vcn - vault.secret.vaultId → vault.secret.kmsVault() oci.kms.vault - vault.secret.keyId → vault.secret.kmsKey() oci.kms.key Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
New resource: - oci.network.subnet: subnets with VCN reference, CIDR, DNS, public IP/ingress controls Converted ID fields to typed resource references: - bastion.instance.targetSubnetId → targetSubnet() oci.network.subnet - blockVolume.kmsKeyId → kmsKey() oci.kms.key - bootVolume.kmsKeyId → kmsKey() oci.kms.key - fileStorage.fileSystem.kmsKeyId → kmsKey() oci.kms.key - ons.subscription.topicId → topic() oci.ons.topic Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
- Keep vcnId on securityList and NSG as deprecated (use vcn() instead) - Add computeagent, Vnic to spelling expect.txt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… needed) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…gging.log Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
New service resources: - oci.loadBalancer: load balancers with lazy-loaded listeners (SSL/TLS config, protocols, cipher suites) and backend sets - oci.networkFirewall: firewalls with typed subnet/policy references, and firewall policies - oci.oke: OKE clusters with K8s version, public/private endpoint config, image policy, pod security policy, typed VCN/KMS references, and lazy-loaded node pools Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- oke: Remove hardcoded zero time for nodePool.created (not on summary) - networkfirewall: Lazy-load policy description/attachedFirewallCount via GetNetworkFirewallPolicy instead of hardcoding defaults - network: Rename DestType → DestinationType for clarity - network: Add sync.Mutex double-check locking to NSG fetchSecurityRules - cloudguard: Add comment about HomeRegionKey short key format Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
/review |
Comment on lines
+462
to
+468
| } | ||
| mqlKey, err := NewResource(o.MqlRuntime, "oci.kms.key", map[string]*llx.RawData{ | ||
| "id": llx.StringData(o.cacheKmsKeyId), | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🟡 warning — NewResource("oci.kms.key", {"id": ...}) creates a stub with only the id field populated. There is no initOciKmsKey function to lazily resolve the remaining fields (name, algorithm, protectionMode, state, etc.) from the API. Any MQL query like blockVolume.kmsKey.name will return an empty string.
The same pattern affects bootVolume.kmsKey(), fileSystem.kmsKey(), secret.kmsKey(), and cluster.kmsKey(). Consider either:
- Adding an
initOciKmsKeythat fetches the key detail (requires caching the vault's management endpoint and region), or - Documenting that
.kmsKeyis currently ID-only and deferring full resolution to a follow-up.
Comment on lines
+24
to
+37
| func int64Value(i *int64) int64 { | ||
| if i == nil { | ||
| return 0 | ||
| } | ||
| return *i | ||
| } | ||
|
|
||
| func intValue(i *int) int64 { | ||
| if i == nil { | ||
| return 0 | ||
| } | ||
| return int64(*i) | ||
| } | ||
|
|
There was a problem hiding this comment.
🔵 suggestion — int64Value and intValue are added but have no callers outside their unit tests. If they're for future use that's fine, but if not, consider removing dead code to keep the surface small.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Major expansion of the OCI provider with new resources for CIS benchmark coverage and security auditing.
New resource services
oci.audit— tenancy-level audit log retention configurationoci.bastion— bastion instances with typed VCN and subnet referencesoci.cloudGuard— targets and detector recipesoci.events— event rules with actionsoci.fileStorage— file systems with KMS key referencesoci.kms— vaults and keysoci.loadBalancer— load balancers with listeners (SSL/TLS protocols, cipher suites, peer cert verification) and backend setsoci.logging— log groups and logs with typed log group referencesoci.monitoring— alarm definitions with severity, query, and notification destinationsoci.networkFirewall— firewalls with typed subnet/policy references, and policies with lazy-loaded detailsoci.ons— topics and subscriptions with typed topic referencesoci.oke— clusters with K8s version, public/private endpoint config, image policy, pod security policy, typed VCN/KMS references, and lazy-loaded node poolsoci.vault— secrets with rotation status and typed KMS key/vault referencesoci.network.subnet— subnets with VCN references, CIDR, DNS, public IP/ingress controlsoci.network.networkSecurityGroup— NSGs with ingress/egress security rules and VCN referencesEnhancements to existing resources
platformConfig,launchOptions,instanceOptions,shapeConfig,sourceDetails,metadata,timeMaintenanceRebootDue)objectLifecyclePolicyEtag)Typed resource references (instead of raw ID strings)
oci.network.securityListvcn()oci.network.vcnoci.network.networkSecurityGroupvcn()oci.network.vcnoci.network.subnetvcn()oci.network.vcnoci.bastion.instancetargetVcn()oci.network.vcnoci.bastion.instancetargetSubnet()oci.network.subnetoci.compute.blockVolumekmsKey()oci.kms.keyoci.compute.bootVolumekmsKey()oci.kms.keyoci.fileStorage.fileSystemkmsKey()oci.kms.keyoci.vault.secretkmsKey()oci.kms.keyoci.vault.secretkmsVault()oci.kms.vaultoci.ons.subscriptiontopic()oci.ons.topicoci.logging.loglogGroup()oci.logging.logGroupoci.networkFirewall.firewallsubnet()oci.network.subnetoci.networkFirewall.firewallpolicy()oci.networkFirewall.policyoci.oke.clustervcn()oci.network.vcnoci.oke.clusterkmsKey()oci.kms.keyNew fields on existing resources
oci.kms.key:isAutoRotationEnabledoci.compute.instance:timeMaintenanceRebootDueoci.logging.log:created,timeLastModifiedAlso fixed
DestTyperenamed toDestinationTypefor claritydescription/attachedFirewallCountlazy-loaded instead of hardcodedcomputeagent,Vnicto spelling expect.txtFixes
Test plan
make test/lintpassesmake providers/build/oci && make providers/install/ocimql shell oci:oci.audit.retentionPeriodDaysoci.bastion.bastions { name targetVcn targetSubnet state }oci.monitoring.alarms { name isEnabled severity query destinations }oci.vault.secrets { name kmsVault kmsKey rotationStatus state }oci.network.subnets { name vcn cidrBlock prohibitPublicIpOnVnic }oci.cloudGuard.targets { name state recipeCount }oci.events.rules { name isEnabled state }oci.kms.vaults { name keys { name isAutoRotationEnabled } }oci.logging.logGroups { logs { name logGroup created timeLastModified } }oci.ons.topics { name subscriptions { topic protocol } }oci.compute.blockVolumes { name kmsKey state }oci.compute.bootVolumes { name kmsKey state }oci.fileStorage.fileSystems { name kmsKey state }oci.loadBalancer.loadBalancers { name isPrivate listeners { protocol sslProtocols } }oci.networkFirewall.firewalls { name subnet policy state }oci.networkFirewall.policies { name description attachedFirewallCount }oci.oke.clusters { name kubernetesVersion isPublicEndpointEnabled isImagePolicyEnabled kmsKey }oci.oke.clusters { nodePools { name kubernetesVersion nodeShape sshPublicKey } }🤖 Generated with Claude Code