feat(apim): crds and controller for configuring apis in apim #1175
Conversation
Still some work to be done with the tests. Committing to not loose work
add base64-encoded string of api spec (content) to status. refactor internal/utils after new methods are added. fix bug in long_running_operations checker
📝 WalkthroughWalkthroughThe pull request introduces several significant changes to the Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Outside diff range and nitpick comments (40)
services/dis-apim-operator/internal/utils/sha_test.go (3)
52-58: Enhance URL error testing.The invalid URL test case could be more comprehensive:
- Test with malformed URLs (e.g., "not-a-url")
- Be more specific about the expected error type
Context("with an invalid URL", func() { - It("should return an error", func() { - invalidUrl := "http://invalid-url" - _, err := Sha256FromContent(&invalidUrl) - Expect(err).To(HaveOccurred()) + DescribeTable("should return appropriate errors", + func(url string, expectedError string) { + _, err := Sha256FromContent(&url) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(expectedError)) + }, + Entry("invalid hostname", "http://invalid-url", "no such host"), + Entry("malformed URL", "not-a-url", "unsupported protocol"), + )
43-43: Fix typo in test description.There's a double space in the test description.
- It("should return correct SHA256 when empty string content", func() { + It("should return correct SHA256 when empty string content", func() {
19-20: Consider using constants for test data.Test values and their expected hashes are repeated throughout the tests. Consider extracting them to constants for better maintainability.
+const ( + testContent = "test content" + testContentHash = "6ae8a75555209fd6c44157c0aed8016e763ff435a19cf186f76863140143ff72" + emptyContentHash = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" +)Also applies to: 29-30, 44-46
services/dis-apim-operator/internal/utils/sha.go (1)
25-26: Improve function documentation with parameter and return value descriptionsThe current documentation could be more descriptive about the parameters and return values.
Consider updating the documentation:
-// Sha256FromContent returns the SHA256 hash of the given content. If the content is a URL, it will fetch the content and return the SHA256 hash. +// Sha256FromContent computes the SHA256 hash of the provided content. +// Parameters: +// content: Pointer to a string containing either direct content or a URL. If nil, returns empty string. +// Returns: +// string: Hexadecimal representation of the SHA256 hash, or empty string if content is nil +// error: Any error encountered during content retrieval or hashingservices/dis-apim-operator/internal/utils/utils.go (1)
1-6: Consider a more specific package name and separation of concerns.The package name 'utils' is quite generic. Consider:
- Renaming to something more specific like
ioutilsorurlutilsbased on primary purpose- Splitting URL and I/O utilities into separate packages to follow single responsibility principle
services/dis-apim-operator/internal/controller/backend_controller_test.go (2)
61-63: Remove duplicate assertion ofProvisioningStateThe assertion on line 63 is redundant as it repeats the check already performed on line 61. Removing the duplicate assertion will make the test more concise.
Apply this diff to remove the duplicate assertion:
g.Expect(err).NotTo(HaveOccurred()) g.Expect(updatedBackend.Status.ProvisioningState).To(Equal(apimv1alpha1.BackendProvisioningStateSucceeded)) g.Expect(updatedBackend.Status.BackendID).To(Equal("/subscriptions/fake-subscription/resourceGroups/fake-resource-group/providers/APIM/Backend/" + updatedBackend.GetAzureResourceName())) -g.Expect(updatedBackend.Status.ProvisioningState).To(Equal(apimv1alpha1.BackendProvisioningStateSucceeded)) g.Expect(fakeApim.Backends).To(HaveLen(1))
68-72: Simplify resource update by removing unnecessaryEventuallyblockUsing
Eventuallyto perform a resource update is unnecessary since updates are immediate operations. You can perform the update directly without wrapping it inEventually.Apply this diff to simplify the code:
-Eventually(func(g Gomega) { updatedBackend.Spec.Url = "https://updated.example.com" err := k8sClient.Update(ctx, updatedBackend) - Expect(err).NotTo(HaveOccurred()) -}, timeout, interval).Should(Succeed()) +updatedBackend.Spec.Url = "https://updated.example.com" +err := k8sClient.Update(ctx, updatedBackend) +Expect(err).NotTo(HaveOccurred())services/dis-apim-operator/api/v1alpha1/api_types.go (1)
65-73: Use pointers for optional fields inApiStatusTo accurately represent the presence or absence of optional fields, it's better to use pointers in the
ApiStatusstruct.Consider updating the types as follows:
- ProvisioningState ProvisioningState `json:"provisioningState,omitempty"` + ProvisioningState *ProvisioningState `json:"provisioningState,omitempty"` - ApiVersionSetID string `json:"apiVersionSetID,omitempty"` + ApiVersionSetID *string `json:"apiVersionSetID,omitempty"`services/dis-apim-operator/api/v1alpha1/apiversion_enums.go (4)
14-33: Consider grouping related constantsFor better readability and maintainability, group related constants using iota or logical grouping.
Example:
const ( // OpenAPI formats ContentFormatOpenapi ContentFormat = "openapi" ContentFormatOpenapiJSON ContentFormat = "openapi+json" ContentFormatOpenapiLink ContentFormat = "openapi-link" ContentFormatOpenapiJSONLink ContentFormat = "openapi+json-link" // Swagger formats ContentFormatSwaggerJSON ContentFormat = "swagger-json" ContentFormatSwaggerLinkJSON ContentFormat = "swagger-link-json" // Other formats ContentFormatGraphqlLink ContentFormat = "graphql-link" ContentFormatWadlLinkJSON ContentFormat = "wadl-link-json" ContentFormatWadlXML ContentFormat = "wadl-xml" )
40-49: Add validation forAPIContactInformationfieldsConsider adding validation annotations to ensure the fields match the expected formats, such as email and URL.
Example:
// +kubebuilder:validation:Format=email Email *string `json:"email,omitempty"` // +kubebuilder:validation:Format=uri URL *string `json:"url,omitempty"`
62-71: Use consistent naming for versioning scheme constantsEnsure that the constants for
APIVersionSchemematch the naming conventions and consider simplifying the constant names.Example:
const ( VersioningSchemeHeader APIVersionScheme = "Header" VersioningSchemeQuery APIVersionScheme = "Query" VersioningSchemeSegment APIVersionScheme = "Segment" )
89-97: Avoid unnecessary pointer receiversIf
Protocolis a string type, methods likeAzureProtocol()might not need a pointer receiver unless mutation is required.Change the method receiver to value type if mutation is not necessary:
-func (p *Protocol) AzureProtocol() *apim.Protocol { +func (p Protocol) AzureProtocol() *apim.Protocol {services/dis-apim-operator/internal/controller/apiversion_controller.go (2)
156-220: Consolidate error handling increateUpdateApimApiThe method has multiple error checks and status updates. Consolidate error handling to reduce repetition and potential mistakes.
Example:
- Use a defer function to handle status updates based on the error variable.
- Ensure that all exit points update the status appropriately.
264-285: Optimize therunPolicyTemplatingmethodConsider handling errors more gracefully if a backend resource is not found, and provide clearer error messages.
Example:
err := r.Get(context.Background(), client.ObjectKey{Name: v.IdFromBackend.Name, Namespace: namespace}, &backend) if err != nil { if errors.IsNotFound(err) { return "", fmt.Errorf("backend %s/%s not found", namespace, v.IdFromBackend.Name) } return "", fmt.Errorf("failed to get backend %s/%s: %w", namespace, v.IdFromBackend.Name, err) }services/dis-apim-operator/internal/controller/api_controller.go (2)
45-48: Consider Dependency Injection for the APIM ClientThe
ApiReconcilerstruct now includesNewClient,ApimClientConfig, andapimClientfields. To enhance testability and adherence to the Dependency Injection principle, consider injecting the APIM client dependencies via the constructor or setup method rather than instantiating them within the reconciler.
179-179: Log Errors When Updating Api StatusWhen updating the
Apistatus usingr.Status().Update(ctx, api), if an error occurs, it's returned but not logged. For better observability and debugging, it's advisable to log the error.Modify the code to log the error before returning:
if err := r.Status().Update(ctx, api); err != nil { logger.Error(err, "Failed to update Api status") return ctrl.Result{}, err } return ctrl.Result{}, nilservices/dis-apim-operator/test/utils/azure_apim_api_fake.go (1)
46-51: Unused Error FlagsThe fields
createUpdateServerError,deleteServerError, andgetServerErrorare initialized but not used in the fake servers.If these flags are intended for simulating errors, ensure they are correctly implemented in the methods. Otherwise, consider removing them to simplify the code.
services/dis-apim-operator/api/v1alpha1/apiversion_types.go (3)
65-65: Clarify the Usage ofNameFieldThe
Namefield inApiVersionSubSpecis optional and defaults to "default" if not provided. However, the comment mentions that it will be the default version if no name is provided. This might be confusing.Clarify the comment to explain the default behavior more explicitly.
188-194: Avoid Hardcoding Default Version SpecifierIn
GetApiVersionSpecifier, the default value "default" is hardcoded. This could lead to conflicts if "default" is a valid version name. Consider making the default value configurable or validate that "default" is acceptable.
264-272: Improve Pointer Comparison UtilityThe function
pointerValueEqualdoes not handle the case where the underlying values are pointers to complex types. IfTis a struct or slice, this comparison may not work as intended.Consider using
reflect.DeepEqualfor more robust comparison of complex types.services/dis-apim-operator/internal/utils/policytemplate.go (1)
10-10: Consider adding template security optionsThe template parsing is using default options which might allow unsafe template operations. Consider using
template.ParseFS()with a restricted file system or adding security restrictions to prevent template injection attacks.services/dis-apim-operator/internal/utils/utils_suite_test.go (2)
17-21: Consider implementing the setup placeholderThe BeforeSuite block contains a TODO comment but no actual setup. Consider what test environment setup might be needed (e.g., test fixtures, mock configurations).
23-26: Consider implementing the teardown placeholderThe AfterSuite block contains a TODO comment but no actual teardown. Consider what cleanup might be needed (e.g., removing test data, closing connections).
services/dis-apim-operator/config/samples/apim_v1alpha1_api.yaml (2)
31-39: Enhance API policy configurationThe policy configuration is basic and could benefit from additional security and performance policies.
policies: policyContent: | <policies> <inbound> + <check-header name="Authorization" failed-check-httpcode="401" /> <set-backend-service base-url="http://example.com/api/4/" /> + <cache-lookup vary-by-developer="false" vary-by-developer-groups="false" downstream-caching-type="none" /> <base /> </inbound> + <backend> + <retry condition="@(context.Response.StatusCode >= 500)" count="3" interval="10" max-interval="30" delta="5" /> + <base /> + </backend> + <outbound> + <cache-store duration="3600" /> + <base /> + </outbound> </policies>
52-52: Add newline at end of fileAdd a newline character at the end of the file to comply with YAML standards.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 52-52: no new line character at the end of file
(new-line-at-end-of-file)
services/dis-apim-operator/internal/utils/policytemplate_test.go (1)
8-54: Enhance test coverage with additional scenariosWhile the current tests cover basic functionality and error cases, consider adding the following test scenarios for more comprehensive coverage:
- Complex nested templates with multiple levels
- Templates with multiple variables
- Special characters and HTML content that might need escaping
- Large template strings to verify performance
Example test case for multiple variables:
Context("with multiple template variables", func() { It("should generate the correct policy", func() { expected := "Hello, John Doe from Example Corp!" templateContent := "Hello, {{.FirstName}} {{.LastName}} from {{.Company}}!" data := map[string]string{ "FirstName": "John", "LastName": "Doe", "Company": "Example Corp", } result, err := GeneratePolicyFromTemplate(templateContent, data) Expect(err).NotTo(HaveOccurred()) Expect(result).To(Equal(expected)) }) })services/dis-apim-operator/internal/utils/base64_test.go (3)
15-17: Don't ignore error handling in test serverThe test server ignores potential write errors which could mask issues. Consider handling or at least logging the error.
- _, _ = fmt.Fprintln(w, "test content") + _, err := fmt.Fprintln(w, "test content") + if err != nil { + t.Logf("Error writing response: %v", err) + }
52-58: Improve invalid URL test reliabilityThe current invalid URL test might be unreliable as "invalid-url" could potentially resolve. Consider using a guaranteed invalid URL scheme or domain.
- invalidUrl := "http://invalid-url" + invalidUrl := "invalid-scheme://localhost"
12-59: Add test cases for additional scenariosConsider adding tests for:
- Large content handling to verify memory efficiency
- Network timeouts
- Non-200 HTTP responses
- Different content types (JSON, XML, binary)
Example test for non-200 response:
Context("with non-200 HTTP response", func() { It("should return an error", func() { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusInternalServerError) })) defer server.Close() _, err := Base64FromContent(&server.URL) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("500")) }) })services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_backends.yaml (1)
17-24: Consider adding shortNames and categoriesWhile the printer columns addition improves observability, the CRD could benefit from additional metadata to improve CLI usability.
Add shortNames and categories to the CRD spec:
names: kind: Backend listKind: BackendList plural: backends singular: backend + shortNames: + - bk + - apimbackend + categories: + - apim + - allservices/dis-apim-operator/internal/azure/long_running_operations_test.go (1)
65-80: Consider adding more in-progress scenarios.While the basic in-progress case is covered, consider adding tests for:
- Multiple in-progress states before success
- Timeout scenarios
- State transitions
services/dis-apim-operator/internal/controller/backend_controller.go (2)
Line range hint
89-117: Consider adding retry mechanism for APIM client operations.The reconciliation logic could benefit from a retry mechanism when dealing with transient Azure API Management failures:
if !backend.MatchesActualState(&azBackend) { logger.Info("Backend does not match actual state, updating") - err := r.handleCreateUpdate(ctx, &backend) + err := utils.RetryWithExponentialBackoff(ctx, func() error { + return r.handleCreateUpdate(ctx, &backend) + }, 3, time.Second) if err != nil { logger.Error(err, "Failed to update backend") return ctrl.Result{}, err } logger.Info("Backend updated") return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil }
Line range hint
156-186: Enhance error handling in handleDeletion.The deletion logic could be improved to handle various edge cases:
func (r *BackendReconciler) handleDeletion(ctx context.Context, backend *apimv1alpha1.Backend) error { logger := log.FromContext(ctx) + // Add timeout context + ctx, cancel := context.WithTimeout(ctx, 5*time.Minute) + defer cancel() + azureBackend, err := r.apimClient.GetBackend(ctx, backend.GetAzureResourceName(), nil) if err != nil { if azure.IsNotFoundError(err) { controllerutil.RemoveFinalizer(backend, BACKEND_FINALIZER) if err := r.Update(ctx, backend); err != nil { + // Add specific error type for client update failures + return fmt.Errorf("failed to remove finalizer after backend not found: %w", err) - logger.Error(err, "Failed to remove finalizer") - return err } return nil } - logger.Error(err, "Failed to get backend for deletion") - return err + return fmt.Errorf("failed to get backend for deletion: %w", err) } + + // Add validation for required fields + if azureBackend.ETag == nil { + return fmt.Errorf("backend ETag is nil") + } + resp, err := r.apimClient.DeleteBackend(ctx, backend.GetAzureResourceName(), *azureBackend.ETag, nil) if err != nil { - logger.Error(err, fmt.Sprintf("Failed to delete backend. backend: %#v", azureBackend)) - return err + return fmt.Errorf("failed to delete backend %s: %w", backend.GetAzureResourceName(), err) } logger.Info("Backend deleted", "response", resp) + controllerutil.RemoveFinalizer(backend, BACKEND_FINALIZER) if err := r.Update(ctx, backend); err != nil { - logger.Error(err, "Failed to remove finalizer") - return err + return fmt.Errorf("failed to remove finalizer after successful deletion: %w", err) } return nil }services/dis-apim-operator/internal/controller/api_controller_test.go (3)
51-89: Consider adding error scenario test casesWhile the happy path is well tested, consider adding test cases for:
- Invalid API specifications
- Version set creation failures
- Concurrent version updates
91-103: Enhance test readability with state transition descriptionsConsider adding more descriptive messages in the Eventually blocks to clarify the expected state transitions. This will make test failures more informative.
Eventually(func(g Gomega) { g.Expect(fakeApim.APIMVersionSets).To(HaveLen(1)) g.Expect(k8sClient.List(ctx, &apiVersionList)).Should(Succeed()) g.Expect(apiVersionList.Items).To(HaveLen(1)) - g.Expect(*apiVersionList.Items[0].Spec.Content).To(Equal(`{"openapi": "3.0.0","info": {"title": "Minimal API v1","version": "1.0.0"},""paths": {"test": "test"}}`)) + g.Expect(*apiVersionList.Items[0].Spec.Content).To(Equal(`{"openapi": "3.0.0","info": {"title": "Minimal API v1","version": "1.0.0"},""paths": {"test": "test"}}`), + "API version content should be updated with new paths")
140-146: Consider parameterizing timeout in helper functionThe helper function could be more flexible by accepting a custom timeout parameter, allowing different wait times for different scenarios.
-func getUpdatedApi(ctx context.Context, typeNamespacedName types.NamespacedName) apimv1alpha1.Api { +func getUpdatedApi(ctx context.Context, typeNamespacedName types.NamespacedName, customTimeout time.Duration) apimv1alpha1.Api { resource := apimv1alpha1.Api{} Eventually(func(g Gomega) { g.Expect(k8sClient.Get(ctx, typeNamespacedName, &resource)).To(Succeed()) - }, timeout, interval).Should(Succeed()) + }, customTimeout, interval).Should(Succeed()) return resource }services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apiversions.yaml (2)
46-192: Consider adding additional field validationsWhile the schema is comprehensive, consider adding:
- Pattern validation for
pathfield to ensure valid URL paths- Maximum length validation for
descriptionanddisplayName- Enum validation for
contentFormatpath: description: Path - API prefix. The value is combined with the API version to form the URL of the API endpoint. + pattern: ^/[a-zA-Z0-9-_/]*$ + maxLength: 255 type: string
193-215: Add enum values for provisioningStateConsider adding an enum for the provisioningState field to ensure valid state transitions and improve validation.
provisioningState: description: 'ProvisioningState - The provisioning state of the API. Possible values are: Creating, Succeeded, Failed, Updating, Deleting, and Deleted.' + enum: + - Creating + - Succeeded + - Failed + - Updating + - Deleting + - Deleted type: stringservices/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apis.yaml (1)
46-218: Consider enhancing field validationsThe spec structure is well-defined, but could benefit from additional validations:
- Add pattern validation for
pathfield to ensure valid URL paths- Add maxLength validation for
displayName- Add enum validation for allowed protocol values
path: description: Path - API prefix. The value is combined with the API version to form the URL of the API endpoint. type: string + pattern: '^[a-zA-Z0-9-_/]+$' + maxLength: 255 displayName: description: DisplayName - The display name of the API. This name is used by the developer portal as the API name. type: string + maxLength: 128 protocols: default: - https description: Protocols - Describes protocols over which API is made available. Default value is https. items: type: string + enum: + - http + - httpsservices/dis-apim-operator/api/v1alpha1/zz_generated.deepcopy.go (1)
Line range hint
1-519: LGTM! Auto-generated deepcopy implementationsThe generated code correctly implements deep copying for all types, with proper handling of nil pointers, maps, and slices.
Remember to run
make generatewhen making changes to the API types to keep this file in sync.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
services/dis-apim-operator/.gitignore(1 hunks)services/dis-apim-operator/api/v1alpha1/api_types.go(3 hunks)services/dis-apim-operator/api/v1alpha1/apiversion_enums.go(1 hunks)services/dis-apim-operator/api/v1alpha1/apiversion_types.go(3 hunks)services/dis-apim-operator/api/v1alpha1/backend_types.go(1 hunks)services/dis-apim-operator/api/v1alpha1/zz_generated.deepcopy.go(7 hunks)services/dis-apim-operator/cmd/main.go(1 hunks)services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apis.yaml(1 hunks)services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apiversions.yaml(1 hunks)services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_backends.yaml(1 hunks)services/dis-apim-operator/config/rbac/role.yaml(2 hunks)services/dis-apim-operator/config/samples/apim_v1alpha1_api.yaml(1 hunks)services/dis-apim-operator/config/samples/apim_v1alpha1_backend.ignore.yaml(0 hunks)services/dis-apim-operator/config/samples/apim_v1alpha1_backend.yaml(1 hunks)services/dis-apim-operator/internal/azure/long_running_operations.go(1 hunks)services/dis-apim-operator/internal/azure/long_running_operations_test.go(1 hunks)services/dis-apim-operator/internal/controller/api_controller.go(2 hunks)services/dis-apim-operator/internal/controller/api_controller_test.go(1 hunks)services/dis-apim-operator/internal/controller/apiversion_controller.go(3 hunks)services/dis-apim-operator/internal/controller/apiversion_controller_test.go(0 hunks)services/dis-apim-operator/internal/controller/backend_controller.go(1 hunks)services/dis-apim-operator/internal/controller/backend_controller_test.go(2 hunks)services/dis-apim-operator/internal/controller/suite_test.go(3 hunks)services/dis-apim-operator/internal/utils/base64.go(1 hunks)services/dis-apim-operator/internal/utils/base64_test.go(1 hunks)services/dis-apim-operator/internal/utils/policytemplate.go(1 hunks)services/dis-apim-operator/internal/utils/policytemplate_test.go(1 hunks)services/dis-apim-operator/internal/utils/sha.go(1 hunks)services/dis-apim-operator/internal/utils/sha_test.go(1 hunks)services/dis-apim-operator/internal/utils/utils.go(1 hunks)services/dis-apim-operator/internal/utils/utils_suite_test.go(1 hunks)services/dis-apim-operator/test/utils/azure_apim_api_fake.go(1 hunks)services/dis-apim-operator/test/utils/azure_fake.go(0 hunks)
💤 Files with no reviewable changes (3)
- services/dis-apim-operator/config/samples/apim_v1alpha1_backend.ignore.yaml
- services/dis-apim-operator/internal/controller/apiversion_controller_test.go
- services/dis-apim-operator/test/utils/azure_fake.go
✅ Files skipped from review due to trivial changes (1)
- services/dis-apim-operator/.gitignore
🧰 Additional context used
🪛 yamllint (1.35.1)
services/dis-apim-operator/config/samples/apim_v1alpha1_api.yaml
[error] 52-52: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (23)
services/dis-apim-operator/internal/utils/sha_test.go (2)
1-10: LGTM! Package and imports are well-structured.
The imports are appropriate and follow the standard testing patterns for Ginkgo/Gomega.
12-59: Well-structured test suite with comprehensive coverage.
The test suite is well-organized with good coverage of various scenarios including:
- URL content hashing
- String content hashing
- Edge cases (nil, empty string)
- Error handling
The use of Ginkgo's BDD-style testing provides clear test organization and readability.
services/dis-apim-operator/internal/utils/sha.go (1)
30-33: Verify URL validation implementation
Let's verify the implementation of the isUrl helper function to ensure proper URL validation.
✅ Verification successful
URL validation implementation is secure and follows Go standards
The isUrl function uses Go's standard url.ParseRequestURI function, which provides robust URL validation. This implementation:
- Properly validates URL syntax according to RFC 3986
- Returns a boolean indicating if the string is a valid URL
- Uses the standard library instead of custom regex or string manipulation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of isUrl function
# Expected: Find the implementation and verify it handles various URL formats correctly
# Search for isUrl function implementation
ast-grep --pattern 'func isUrl($$$) $$$'
# Backup: Use ripgrep if ast-grep doesn't find it
rg -A 10 'func isUrl'
Length of output: 909
services/dis-apim-operator/internal/utils/utils.go (2)
13-15: 🛠️ Refactor suggestion
Document error handling strategy and consider logging.
While ignoring close errors can be appropriate in some cases, it's important to:
- Add documentation explaining when to use this function vs handling errors
- Consider adding debug logging for troubleshooting
- Evaluate if some errors should not be ignored (e.g., during critical operations)
+// closeIgnoreError closes an io.Closer and intentionally ignores any errors.
+// Use this only for cleanup operations where failure to close is acceptable,
+// such as closing response bodies after reading.
func closeIgnoreError(c io.Closer) {
- _ = c.Close()
+ if err := c.Close(); err != nil {
+ // TODO: Consider adding debug level logging here
+ }
}Let's check the usage context to understand the risk level of ignoring errors:
8-11: 🛠️ Refactor suggestion
Enhance URL validation function implementation.
Several improvements are recommended:
- Rename to follow Go conventions:
isUrl→isURL - Consider exporting if used by other packages
- Add documentation explaining validation criteria
- Consider additional validation for specific URL requirements (e.g., HTTPS only, valid hostname)
-func isUrl(s string) bool {
+// isURL checks if the given string is a valid URL.
+// It uses url.ParseRequestURI for basic URL validation.
+func isURL(s string) bool {
_, err := url.ParseRequestURI(s)
return err == nil
}Let's check if this function is used elsewhere to determine if it should be exported:
✅ Verification successful
Function should remain unexported but follow Go naming conventions
The isUrl function is only used internally within the utils package by two exported functions: Base64FromContent and Sha256FromContent. While the package itself is imported by other packages, the isUrl function is not directly used outside its package. Therefore:
- The function should remain unexported
- Rename to follow Go conventions:
isUrl→isURL - Add documentation as suggested
-func isUrl(s string) bool {
+// isURL checks if the given string is a valid URL using url.ParseRequestURI
+func isURL(s string) bool {
_, err := url.ParseRequestURI(s)
return err == nil
}Note: The current URL validation is sufficient for its use case in Base64FromContent and Sha256FromContent, where it's only used to determine if content should be fetched from a URL.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for isUrl usage across the codebase
rg "isUrl\(" --type go
Length of output: 248
Script:
#!/bin/bash
# Let's check the usage context in these files
echo "=== base64.go ==="
rg -B 5 -A 5 "isUrl\(" services/dis-apim-operator/internal/utils/base64.go
echo -e "\n=== sha.go ==="
rg -B 5 -A 5 "isUrl\(" services/dis-apim-operator/internal/utils/sha.go
# Check if this package is imported by other packages
echo -e "\n=== Package imports ==="
rg "\".*\/internal\/utils\"" --type go
Length of output: 1175
Script:
#!/bin/bash
# Check if this package is imported by other packages (fixing the regex)
echo "=== Package imports ==="
rg "internal/utils" --type go | grep -v "^services/dis-apim-operator/internal/utils"
# Let's also check if there are any exported functions in this package that use isUrl
echo -e "\n=== Exported functions using isUrl ==="
ast-grep --pattern 'func $FUNC($$$) $$$' services/dis-apim-operator/internal/utils/*.go | grep -B2 -A2 "isUrl"
Length of output: 3371
services/dis-apim-operator/internal/controller/suite_test.go (8)
26-31: Imports are appropriate and necessary
The added imports are relevant for the new functionalities introduced, including Azure SDK components and test utilities.
52-52: Initialization of new variables is correct
The declaration of k8sManager and fakeApim is appropriate and follows Go conventions.
Also applies to: 56-56
92-92: Proper initialization of fakeApim
The fakeApim variable is correctly instantiated using testutils.NewFakeAPIMClientStruct(), facilitating mock Azure API Management interactions for testing.
97-102: Kubernetes manager setup is correctly implemented
The k8sManager is initialized without errors, and necessary checks ensure it's not nil.
103-109: Configuration of serverFactoryTransport is sound
The setup of serverFactoryTransport with the fake Azure API Management server factory is appropriate for simulating Azure interactions.
110-120: APIM client configuration is correctly defined
The apimClientConfig is properly constructed with mock Azure details and client options, ensuring compatibility with the fake Azure environment.
121-144: Reconcilers are set up correctly with the manager
The ApiVersionReconciler, BackendReconciler, and ApiReconciler are correctly initialized and registered with the Kubernetes manager, using the fake APIM client.
145-149: Manager start function handles errors appropriately
Starting the k8sManager within a goroutine includes error handling with GinkgoRecover() and checks to ensure the manager starts without issues.
services/dis-apim-operator/config/samples/apim_v1alpha1_backend.yaml (1)
9-11: Update Sample Backend Configuration
The addition of title, description, and url fields enhances the sample backend configuration. Ensure that this sample aligns with the latest CRD specifications and serves as a proper example for users.
services/dis-apim-operator/internal/utils/policytemplate.go (1)
15-23: LGTM! Efficient implementation using strings.Builder
The implementation correctly uses strings.Builder for memory-efficient string concatenation and properly handles template execution errors.
services/dis-apim-operator/config/rbac/role.yaml (1)
10-11: LGTM! Well-structured RBAC permissions
The new resources follow Kubernetes RBAC best practices with appropriate verb restrictions for different resource types (core resources, finalizers, and status subresources).
Let's verify that these permissions match the controller requirements:
Also applies to: 24-25, 32-33
✅ Verification successful
RBAC permissions correctly match controller requirements
The RBAC permissions in role.yaml perfectly align with the controller implementations:
-
API controller has kubebuilder RBAC markers that match the role:
apisresource with full CRUD permissionsapis/statuswith get, update, patchapis/finalizerswith update
-
APIVersion controller has matching RBAC markers for:
apiversionsresource with full CRUDapiversions/statuswith get, update, patchapiversions/finalizerswith update
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify controller uses these permissions
# Look for reconciler code that uses these new resources
# Check for API resource usage
rg -A 5 "apis|apiversions" services/dis-apim-operator/internal/controller/
# Check for finalizer usage
ast-grep --pattern 'SetupWithManager($_) {
$$$
Owns(&$_)
$$$
}'
Length of output: 4276
services/dis-apim-operator/internal/utils/utils_suite_test.go (1)
12-15: LGTM! Well-structured test suite setup
The test suite follows Ginkgo best practices with proper initialization, logger configuration, and lifecycle hooks.
Also applies to: 17-21, 23-26
services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_backends.yaml (1)
17-24: LGTM on printer columns addition
The addition of State and Age columns will improve resource monitoring and troubleshooting capabilities.
services/dis-apim-operator/internal/azure/long_running_operations_test.go (1)
15-64: LGTM! Well-structured success case tests.
The test cases effectively cover different HTTP status codes (200, 201, 202) for successful operations using BDD style testing.
services/dis-apim-operator/internal/controller/api_controller_test.go (1)
34-37: LGTM: Test setup with appropriate timeout values
The timeout (60s) and interval (250ms) constants are well-chosen for integration tests, providing sufficient time for API operations while maintaining reasonable test execution times.
services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apiversions.yaml (1)
1-24: LGTM: Well-structured CRD definition
The CRD follows Kubernetes best practices with proper metadata, versioning, and scope definition. The additional printer columns for State and Age improve resource visibility.
services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apis.yaml (2)
1-24: LGTM! Well-structured CRD definition.
The CRD follows Kubernetes best practices with proper printer columns for status and age.
219-259: Review security implications of storing policies in status
The status field stores base64-encoded policies which might contain sensitive information. Consider:
- Implementing encryption for sensitive policy content
- Adding RBAC rules to restrict access to status subresource
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| _, _ = fmt.Fprintln(w, "test content") | ||
| })) |
There was a problem hiding this comment.
Fix inconsistent test server response.
The fmt.Fprintln adds a newline to the response, which affects the hash calculation. This makes the test brittle and potentially platform-dependent.
- _, _ = fmt.Fprintln(w, "test content")
+ _, _ = fmt.Fprint(w, "test content")📝 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.
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| _, _ = fmt.Fprintln(w, "test content") | |
| })) | |
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| _, _ = fmt.Fprint(w, "test content") | |
| })) |
| func sha256FromUrlContent(url string) (string, error) { | ||
| resp, err := http.Get(url) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| defer closeIgnoreError(resp.Body) | ||
|
|
||
| h := sha256.New() | ||
| if _, err := io.Copy(h, resp.Body); err != nil { | ||
| return "", err | ||
| } | ||
| return fmt.Sprintf("%x", h.Sum(nil)), nil | ||
| } |
There was a problem hiding this comment.
Add timeout and improve error handling in URL content retrieval
The current implementation has several potential issues that should be addressed:
- No timeout on HTTP requests could lead to hanging operations
- HTTP response status codes aren't validated
- TLS verification settings aren't explicitly configured
Consider implementing these improvements:
+var httpClient = &http.Client{
+ Timeout: 30 * time.Second,
+ Transport: &http.Transport{
+ TLSClientConfig: &tls.Config{MinVersion: tls.VersionTLS12},
+ },
+}
+
func sha256FromUrlContent(url string) (string, error) {
- resp, err := http.Get(url)
+ resp, err := httpClient.Get(url)
if err != nil {
return "", err
}
defer closeIgnoreError(resp.Body)
+ if resp.StatusCode != http.StatusOK {
+ return "", fmt.Errorf("unexpected status code: %d", resp.StatusCode)
+ }
+
h := sha256.New()
if _, err := io.Copy(h, resp.Body); err != nil {
return "", err
}
return fmt.Sprintf("%x", h.Sum(nil)), nil
}Don't forget to add these imports:
import (
"crypto/tls"
"time"
)| func Sha256FromContent(content *string) (string, error) { | ||
| if content == nil { | ||
| return "", nil | ||
| } | ||
| if isUrl(*content) { | ||
|
|
||
| return sha256FromUrlContent(*content) | ||
| } | ||
| h := sha256.New() | ||
| h.Write([]byte(*content)) | ||
| return fmt.Sprintf("%x", h.Sum(nil)), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add content size validation to prevent memory exhaustion
The function should implement size limits for both direct content and URL responses to prevent potential memory exhaustion.
Consider implementing size limits:
+const maxContentSize = 50 * 1024 * 1024 // 50MB limit
+
func Sha256FromContent(content *string) (string, error) {
if content == nil {
return "", nil
}
+ if len(*content) > maxContentSize {
+ return "", fmt.Errorf("content size exceeds maximum allowed size of %d bytes", maxContentSize)
+ }
if isUrl(*content) {
return sha256FromUrlContent(*content)
}
h := sha256.New()
h.Write([]byte(*content))
return fmt.Sprintf("%x", h.Sum(nil)), nil
}Committable suggestion skipped: line range outside the PR's diff.
| // GetApiAzureFullName returns the name of the Azure resource. | ||
| func (a *Api) GetApiAzureFullName() string { | ||
| return fmt.Sprintf("%s-%s", a.Namespace, a.Name) | ||
| } | ||
|
|
||
| // ToAzureApiVersionSet returns an APIVersionSetContract object. | ||
| func (a *Api) ToAzureApiVersionSet() apim.APIVersionSetContract { | ||
| return apim.APIVersionSetContract{ | ||
| Properties: &apim.APIVersionSetContractProperties{ | ||
| DisplayName: &a.Spec.DisplayName, | ||
| VersioningScheme: a.Spec.VersioningScheme.AzureAPIVersionScheme(), | ||
| Description: a.Spec.Description, | ||
| }, | ||
| Name: utils.ToPointer(a.GetApiAzureFullName()), | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle possible nil values in methods
In the methods ToAzureApiVersionSet and GetApiAzureFullName, ensure that all accessed fields are properly checked for nil to prevent runtime panics.
Review and update the methods to include nil checks where necessary.
| // DisplayName - The display name of the API. This name is used by the developer portal as the API name. | ||
| // +kubebuilder:validation:Required | ||
| DisplayName string `json:"displayName,omitempty"` | ||
| // Description - Description of the API. May include its purpose, where to get more information, and other relevant information. | ||
| // +kubebuilder:validation:Optional | ||
| Description *string `json:"description,omitempty"` | ||
| // VersioningScheme - Indicates the versioning scheme used for the API. Possible values include, but are not limited to, "Segment", "Query", "Header". Default value is "Segment". | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:default:="Segment" | ||
| // +kubebuilder:validation:Enum:=Header;Query;Segment | ||
| VersioningScheme APIVersionScheme `json:"versioningScheme,omitempty"` | ||
| // Path - API prefix. The value is combined with the API version to form the URL of the API endpoint. | ||
| // +kubebuilder:validation:Required | ||
| Path string `json:"path,omitempty"` | ||
| // ApiType - Type of API. | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:default:="http" | ||
| // +default:value:"http" | ||
| // +kubebuilder:validation:Enum:=graphql;http;websocket | ||
| ApiType *APIType `json:"apiType,omitempty"` | ||
| // Contact - Contact details of the API owner. | ||
| // +kubebuilder:validation:Optional | ||
| Contact *APIContactInformation `json:"contact,omitempty"` | ||
| // Versions - A list of API versions associated with the API. If the API is specified using the OpenAPI definition, then the API version is set by the version field of the OpenAPI definition. | ||
| // +kubebuilder:validation:Required | ||
| Versions []ApiVersionSubSpec `json:"versions,omitempty"` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure required fields are validated appropriately
The new fields added to ApiSpec include proper validation annotations. However, consider the following:
- Fields marked with
+kubebuilder:validation:Requiredshould omit theomitemptytag in the JSON struct tag to enforce validation.
Apply this diff to ensure required fields are correctly validated:
- DisplayName string `json:"displayName,omitempty"`
+ DisplayName string `json:"displayName"`Repeat this change for other required fields like Path and Versions.
Committable suggestion skipped: line range outside the PR's diff.
| - name: v2 | ||
| displayName: Version 2 | ||
| description: First version of the API | ||
| serviceUrl: https://example.com/v2 | ||
| products: | ||
| - product1 | ||
| - product2 | ||
| protocols: | ||
| - https | ||
| subscriptionRequired: true | ||
| contentFormat: swagger-link-json | ||
| content: https://primary-test-aca-vga.gentleground-884783d5.norwayeast.azurecontainerapps.io/swagger/doc.json | ||
| isCurrent: true |
There was a problem hiding this comment.
Fix inconsistencies in API version configuration
Several issues in the API version configuration need attention:
- Version 2 description is incorrect (states "First version of the API")
- Both versions are marked as
isCurrent: true, which could cause confusion - The content URL appears to be an environment-specific URL
- name: v2
displayName: Version 2
- description: First version of the API
+ description: Second version of the API
serviceUrl: https://example.com/v2
products:
- product1
- product2
protocols:
- https
subscriptionRequired: true
contentFormat: swagger-link-json
- content: https://primary-test-aca-vga.gentleground-884783d5.norwayeast.azurecontainerapps.io/swagger/doc.json
+ content: https://example.com/v2/swagger/doc.json
- isCurrent: true
+ isCurrent: false📝 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.
| - name: v2 | |
| displayName: Version 2 | |
| description: First version of the API | |
| serviceUrl: https://example.com/v2 | |
| products: | |
| - product1 | |
| - product2 | |
| protocols: | |
| - https | |
| subscriptionRequired: true | |
| contentFormat: swagger-link-json | |
| content: https://primary-test-aca-vga.gentleground-884783d5.norwayeast.azurecontainerapps.io/swagger/doc.json | |
| isCurrent: true | |
| - name: v2 | |
| displayName: Version 2 | |
| description: Second version of the API | |
| serviceUrl: https://example.com/v2 | |
| products: | |
| - product1 | |
| - product2 | |
| protocols: | |
| - https | |
| subscriptionRequired: true | |
| contentFormat: swagger-link-json | |
| content: https://example.com/v2/swagger/doc.json | |
| isCurrent: false |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 52-52: no new line character at the end of file
(new-line-at-end-of-file)
| type MockPoller[T any] struct { | ||
| IsDone bool | ||
| PollResult http.Response | ||
| PollError error | ||
| FakeResultError error | ||
| } | ||
|
|
||
| func (p MockPoller[T]) Poll(_ context.Context) (*http.Response, error) { | ||
| return &p.PollResult, p.PollError | ||
| } | ||
|
|
||
| func (p MockPoller[T]) Done() bool { | ||
| return p.IsDone | ||
| } | ||
|
|
||
| func (p MockPoller[T]) Result(_ context.Context, _ *T) error { | ||
| if p.FakeResultError != nil { | ||
| return p.FakeResultError | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance MockPoller implementation.
The current MockPoller implementation could be more robust:
type MockPoller[T any] struct {
IsDone bool
PollResult http.Response
PollError error
FakeResultError error
+ // Add fields for more complex scenarios
+ PollCount int // Track number of polls
+ MaxPolls int // Maximum number of polls before success
+ StateSequence []int // Sequence of status codes to return
}
func (p MockPoller[T]) Poll(_ context.Context) (*http.Response, error) {
- return &p.PollResult, p.PollError
+ if p.StateSequence != nil && p.PollCount < len(p.StateSequence) {
+ result := http.Response{StatusCode: p.StateSequence[p.PollCount]}
+ p.PollCount++
+ return &result, nil
+ }
+ if p.MaxPolls > 0 && p.PollCount >= p.MaxPolls {
+ return &p.PollResult, p.PollError
+ }
+ p.PollCount++
+ return &p.PollResult, p.PollError
}📝 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.
| type MockPoller[T any] struct { | |
| IsDone bool | |
| PollResult http.Response | |
| PollError error | |
| FakeResultError error | |
| } | |
| func (p MockPoller[T]) Poll(_ context.Context) (*http.Response, error) { | |
| return &p.PollResult, p.PollError | |
| } | |
| func (p MockPoller[T]) Done() bool { | |
| return p.IsDone | |
| } | |
| func (p MockPoller[T]) Result(_ context.Context, _ *T) error { | |
| if p.FakeResultError != nil { | |
| return p.FakeResultError | |
| } | |
| return nil | |
| } | |
| type MockPoller[T any] struct { | |
| IsDone bool | |
| PollResult http.Response | |
| PollError error | |
| FakeResultError error | |
| // Add fields for more complex scenarios | |
| PollCount int // Track number of polls | |
| MaxPolls int // Maximum number of polls before success | |
| StateSequence []int // Sequence of status codes to return | |
| } | |
| func (p MockPoller[T]) Poll(_ context.Context) (*http.Response, error) { | |
| if p.StateSequence != nil && p.PollCount < len(p.StateSequence) { | |
| result := http.Response{StatusCode: p.StateSequence[p.PollCount]} | |
| p.PollCount++ | |
| return &result, nil | |
| } | |
| if p.MaxPolls > 0 && p.PollCount >= p.MaxPolls { | |
| return &p.PollResult, p.PollError | |
| } | |
| p.PollCount++ | |
| return &p.PollResult, p.PollError | |
| } | |
| func (p MockPoller[T]) Done() bool { | |
| return p.IsDone | |
| } | |
| func (p MockPoller[T]) Result(_ context.Context, _ *T) error { | |
| if p.FakeResultError != nil { | |
| return p.FakeResultError | |
| } | |
| return nil | |
| } |
| // +kubebuilder:printcolumn:name="State",type=string,JSONPath=`.status.provisioningState` | ||
| // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" |
There was a problem hiding this comment.
Fix JSONPath syntax in print column definition.
The JSONPath for the State column uses backticks instead of single quotes, which might cause issues:
-// +kubebuilder:printcolumn:name="State",type=string,JSONPath=`.status.provisioningState`
+// +kubebuilder:printcolumn:name="State",type=string,JSONPath='.status.provisioningState'📝 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.
| // +kubebuilder:printcolumn:name="State",type=string,JSONPath=`.status.provisioningState` | |
| // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" | |
| // +kubebuilder:printcolumn:name="State",type=string,JSONPath='.status.provisioningState' | |
| // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" |
| @@ -32,7 +32,7 @@ import ( | |||
| apimv1alpha1 "github.com/Altinn/altinn-platform/services/dis-apim-operator/api/v1alpha1" | |||
| ) | |||
|
|
|||
| const BACKEND_FINALIZER = "finalizers.apim.dis.altinn.cloud/backend" | |||
| const BACKEND_FINALIZER = "backend.apim.dis.altinn.cloud/finalzer" | |||
There was a problem hiding this comment.
Fix typo in finalizer name.
The finalizer constant contains a typo in the word "finalzer":
-const BACKEND_FINALIZER = "backend.apim.dis.altinn.cloud/finalzer"
+const BACKEND_FINALIZER = "backend.apim.dis.altinn.cloud/finalizer"📝 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.
| const BACKEND_FINALIZER = "backend.apim.dis.altinn.cloud/finalzer" | |
| const BACKEND_FINALIZER = "backend.apim.dis.altinn.cloud/finalizer" |
| Client: mgr.GetClient(), | ||
| Scheme: mgr.GetScheme(), | ||
| NewClient: azure.NewAPIMClient, | ||
| ApimClientConfig: &azure.ApimClientConfig{ | ||
| AzureConfig: *operatorConfig, | ||
| }, | ||
| }).SetupWithManager(mgr); err != nil { | ||
| setupLog.Error(err, "unable to create controller", "controller", "Api") | ||
| os.Exit(1) | ||
| } | ||
| if err = (&controller.ApiVersionReconciler{ | ||
| Client: mgr.GetClient(), | ||
| Scheme: mgr.GetScheme(), | ||
| Client: mgr.GetClient(), | ||
| Scheme: mgr.GetScheme(), | ||
| NewClient: azure.NewAPIMClient, | ||
| ApimClientConfig: &azure.ApimClientConfig{ | ||
| AzureConfig: *operatorConfig, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for APIM client initialization
Consider adding validation and error handling for the APIM client configuration before initializing the controllers.
+func validateApimConfig(config *azure.ApimClientConfig) error {
+ if config == nil || config.AzureConfig == nil {
+ return fmt.Errorf("invalid APIM client configuration")
+ }
+ return nil
+}
if err = (&controller.ApiReconciler{
Client: mgr.GetClient(),
Scheme: mgr.GetScheme(),
NewClient: azure.NewAPIMClient,
ApimClientConfig: &azure.ApimClientConfig{
AzureConfig: *operatorConfig,
},
+}).SetupWithManager(mgr); err != nil {
-}).SetupWithManager(mgr); err != nil {
+ if err := validateApimConfig(apimClientConfig); err != nil {
+ setupLog.Error(err, "invalid APIM configuration")
+ os.Exit(1)
+ }
setupLog.Error(err, "unable to create controller", "controller", "Api")
os.Exit(1)
}Committable suggestion skipped: line range outside the PR's diff.
tjololo
changed the title
Sorry, yet another HUGE pull request. Once the first version is out the size will hopefully change!
Description
CRD and Controller for managing API specific resources.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Backendresource, includingtitle,description, andurl.Bug Fixes
Tests
Chores
.gitignorefile to exclude specific patterns.