Ovh dns provider #696
Ovh dns provider #696
Conversation
WalkthroughAdds a new OVH provider implementing DNS resource collection, registers it in the inventory, updates module dependencies to include the OVH client, and documents provider configuration in PROVIDERS.md. Changes
Sequence Diagram(s)sequenceDiagram
participant Inv as Inventory
participant OVH as ovh.Provider
participant DNS as dnsProvider
participant API as OVH API
Inv->>OVH: New(options)
OVH->>API: ovh.NewClient(endpoint, appKey, appSecret, consumerKey)
Inv->>OVH: Resources(ctx)
alt dns service enabled
OVH->>DNS: instantiate with shared client
DNS->>API: GET /domain/zone
loop for each zone
DNS->>API: GET /domain/zone/{zone}/record?fieldType=A|AAAA|CNAME
loop for each record ID
DNS->>API: GET /domain/zone/{zone}/record/{id}
DNS-->>OVH: append DNSName / IP resources
end
end
end
OVH-->>Inv: Aggregated Resources
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
PROVIDERS.md (1)
485-513: Fix bare URLs in the references section.The static analysis tools correctly identified bare URLs that should be wrapped in angle brackets for proper Markdown formatting.
Apply this diff to fix the markdown formatting:
References - -1. https://eu.api.ovh.com/console/?section=%2Fdomain&branch=v1 -2. https://api.ovh.com/createToken/ -3. https://help.ovhcloud.com/csm/en-gb-api-getting-started-ovhcloud-api +1. <https://eu.api.ovh.com/console/?section=%2Fdomain&branch=v1> +2. <https://api.ovh.com/createToken/> +3. <https://help.ovhcloud.com/csm/en-gb-api-getting-started-ovhcloud-api>pkg/providers/ovh/dns.go (2)
41-43: Consider logging errors for debugging purposes.When fetching record types fails, the error is silently ignored with
continue. Consider logging these errors for better observability during debugging.Consider adding error logging to help with debugging:
if err := d.client.GetWithContext(ctx, path, &ids); err != nil { + // Log error for debugging: unable to fetch record type %s for zone %s: %v continue }
46-48: Consider logging errors for debugging purposes.When fetching individual records fails, the error is silently ignored with
continue. Consider logging these errors for better observability during debugging.Consider adding error logging to help with debugging:
if err := d.client.GetWithContext(ctx, fmt.Sprintf("/domain/zone/%s/record/%d", zone, id), &rec); err != nil { + // Log error for debugging: unable to fetch record %d for zone %s: %v continue }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
PROVIDERS.md(1 hunks)go.mod(2 hunks)pkg/inventory/inventory.go(3 hunks)pkg/providers/ovh/dns.go(1 hunks)pkg/providers/ovh/ovh.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/inventory/inventory.go (1)
pkg/providers/ovh/ovh.go (2)
Services(15-15)New(24-78)
pkg/providers/ovh/ovh.go (2)
pkg/schema/schema.go (5)
ServiceMap(251-251)OptionBlock(192-192)ErrNoSuchKey(163-165)Resources(39-42)NewResources(45-50)pkg/inventory/inventory.go (1)
New(37-52)
pkg/providers/ovh/dns.go (3)
pkg/schema/schema.go (3)
Resources(39-42)NewResources(45-50)Resource(141-160)pkg/providers/ovh/ovh.go (1)
Provider(17-22)pkg/schema/validate/validate.go (3)
DNSName(72-72)PublicIPv4(73-73)PublicIPv6(74-74)
🪛 markdownlint-cli2 (0.17.2)
PROVIDERS.md
510-510: Bare URL used
(MD034, no-bare-urls)
511-511: Bare URL used
(MD034, no-bare-urls)
512-512: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (7)
go.mod (2)
168-168: LGTM!The
gopkg.in/ini.v1dependency upgrade from v1.66.6 to v1.67.0 is a minor version update that should be backward compatible.
217-217: LGTM!The addition of
github.com/ovh/go-ovh v1.9.0as an indirect dependency is appropriate for the new OVH provider implementation.pkg/inventory/inventory.go (3)
24-24: LGTM!The import of the OVH provider package is correctly placed and follows the existing import structure.
74-74: LGTM!The OVH provider is correctly registered in the Providers map with the appropriate services mapping.
130-131: LGTM!The OVH provider case is correctly added to the nameToProvider switch statement, following the established pattern.
pkg/providers/ovh/ovh.go (2)
24-78: Well-structured provider initialization with proper error handling.The
Newfunction correctly validates all required credentials, handles optional configuration with sensible defaults, and properly initializes the OVH client with appropriate timeout settings.
84-95: Clean resource collection implementation.The
Resourcesmethod correctly handles service-specific resource collection and properly propagates errors from the DNS provider.
| // For A/AAAA also append IP resource; skip CNAME target to avoid duplicates | ||
| res := &schema.Resource{ | ||
| Public: true, | ||
| Provider: providerName, | ||
| ID: d.id, | ||
| Service: d.name(), | ||
| } | ||
|
|
||
| if strings.ToUpper(rec.FieldType) == "A" { | ||
| res.PublicIPv4 = rec.Target | ||
| } else if strings.ToUpper(rec.FieldType) == "AAAA" { | ||
| res.PublicIPv6 = rec.Target | ||
| } | ||
|
|
||
| list.Append(res) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid appending empty IP resources for CNAME records.
The code currently appends an IP resource even for CNAME records, resulting in an empty resource with no IP addresses set. This creates unnecessary entries in the resource list.
Apply this diff to skip appending empty IP resources:
- // For A/AAAA also append IP resource; skip CNAME target to avoid duplicates
- res := &schema.Resource{
- Public: true,
- Provider: providerName,
- ID: d.id,
- Service: d.name(),
- }
-
- if strings.ToUpper(rec.FieldType) == "A" {
- res.PublicIPv4 = rec.Target
- } else if strings.ToUpper(rec.FieldType) == "AAAA" {
- res.PublicIPv6 = rec.Target
- }
-
- list.Append(res)
+ // For A/AAAA records, append IP resource
+ if strings.ToUpper(rec.FieldType) == "A" || strings.ToUpper(rec.FieldType) == "AAAA" {
+ res := &schema.Resource{
+ Public: true,
+ Provider: providerName,
+ ID: d.id,
+ Service: d.name(),
+ }
+
+ if strings.ToUpper(rec.FieldType) == "A" {
+ res.PublicIPv4 = rec.Target
+ } else {
+ res.PublicIPv6 = rec.Target
+ }
+
+ list.Append(res)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // For A/AAAA also append IP resource; skip CNAME target to avoid duplicates | |
| res := &schema.Resource{ | |
| Public: true, | |
| Provider: providerName, | |
| ID: d.id, | |
| Service: d.name(), | |
| } | |
| if strings.ToUpper(rec.FieldType) == "A" { | |
| res.PublicIPv4 = rec.Target | |
| } else if strings.ToUpper(rec.FieldType) == "AAAA" { | |
| res.PublicIPv6 = rec.Target | |
| } | |
| list.Append(res) | |
| } | |
| // For A/AAAA records, append IP resource | |
| if strings.ToUpper(rec.FieldType) == "A" || strings.ToUpper(rec.FieldType) == "AAAA" { | |
| res := &schema.Resource{ | |
| Public: true, | |
| Provider: providerName, | |
| ID: d.id, | |
| Service: d.name(), | |
| } | |
| if strings.ToUpper(rec.FieldType) == "A" { | |
| res.PublicIPv4 = rec.Target | |
| } else { | |
| res.PublicIPv6 = rec.Target | |
| } | |
| list.Append(res) | |
| } |
🤖 Prompt for AI Agents
In pkg/providers/ovh/dns.go around lines 67 to 82, the code constructs and
appends an IP resource for every record including CNAMEs, producing empty
resources; change the logic so you only create and append the resource when the
record is an A or AAAA (or when PublicIPv4/PublicIPv6 would be set). Concretely,
either move the res := &schema.Resource{...} creation inside the A/AAAA branches
or add a guard after setting PublicIPv4/PublicIPv6 that skips appending (e.g.,
continue or return) when neither IP field is populated.
2 participants
This PR adds a new provider for OVH, enabling users to enumerate DNS records from their OVH accounts via the OVH API.
Issue#2 - Add Support for OVH DNS provider
Changes
Example
provider-config.yaml:Usage / Testing
cloudlist -pc ovh.yaml -p ovhSummary by CodeRabbit
New Features
Documentation
Chores