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
Finishing Touches
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
services/dis-apim-operator/internal/azure/long_running_operations_test.go
Show resolved
Hide resolved
services/dis-apim-operator/internal/controller/backend_controller.go
Outdated
Show resolved
Hide resolved
tjololo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
services/dis-apim-operator/api/v1alpha1/apiversion_types.go (1)
245-266:⚠️ Potential issueAdd nil checks to prevent potential panics
The
ToAzureCreateOrUpdateParametermethod has several potential nil pointer dereferences that could cause panics:
APIType.AzureApiType()- Direct assignment of
DescriptionandDisplayNamepointersContentFormat.AzureContentFormat()Add nil checks and safe defaults:
func (a *ApiVersion) ToAzureCreateOrUpdateParameter() apim.APICreateOrUpdateParameter { + var apiType *string + if a.Spec.ApiType != nil { + apiType = a.Spec.ApiType.AzureApiType() + } + + var format *string + if a.Spec.ContentFormat != nil { + format = a.Spec.ContentFormat.AzureContentFormat() + } + apiCreateOrUpdateParams := apim.APICreateOrUpdateParameter{ Properties: &apim.APICreateOrUpdateProperties{ Path: &a.Spec.Path, - APIType: a.Spec.ApiType.AzureApiType(), - Description: &a.Spec.Description, - DisplayName: &a.Spec.DisplayName, - Format: a.Spec.ContentFormat.AzureContentFormat(), + APIType: apiType, + Description: utils.ToPointer(a.Spec.Description), + DisplayName: utils.ToPointer(a.Spec.DisplayName), + Format: format, IsCurrent: a.Spec.IsCurrent, Protocols: ToApimProtocolSlice(a.Spec.Protocols), ServiceURL: a.Spec.ServiceUrl,
🧹 Nitpick comments (10)
services/dis-apim-operator/internal/utils/utils.go (3)
12-17: Consider making httpClient private and adding documentation.The HTTP client configuration looks good with appropriate timeout and TLS settings. However:
- Consider making
httpClientprivate (lowercase) since it's an implementation detail- Add documentation explaining the 30-second timeout choice for maintainability
-var httpClient = &http.Client{ +// httpClient is configured with a 30-second timeout and TLS 1.2 minimum version +// for secure API communication +var httpClient = &http.Client{
19-22: Follow Go naming conventions and add documentation.The function should follow Go naming conventions and include documentation:
-func isUrl(s string) bool { +// IsURL checks if the given string is a valid URL +func IsURL(s string) bool {
38-40: Add documentation explaining error handling choice.Add a comment explaining why it's safe to ignore Close errors in this context.
+// closeIgnoreError closes an io.Closer and explicitly ignores any error +// as these are typically benign (e.g., connection already closed) func closeIgnoreError(c io.Closer) {services/dis-apim-operator/internal/utils/policytemplate_test.go (1)
8-9: Consider making ValidTemplate private.The
ValidTemplateconstant appears to be test-specific and should be unexported (lowercase).-const ValidTemplate = "Hello, {{.Name}}!" +const validTemplate = "Hello, {{.Name}}!"services/dis-apim-operator/api/v1alpha1/api_types.go (1)
126-148: Add validation and error handling for version mapping.Consider the following improvements:
- Validate version names for uniqueness to prevent overwrites in the map
- Pre-allocate the map with the expected size
- Add error handling for potential map operations
Apply this diff to improve the implementation:
func (a *Api) ToApiVersions() map[string]ApiVersion { - apiVersions := make(map[string]ApiVersion) + apiVersions := make(map[string]ApiVersion, len(a.Spec.Versions)) + seenVersions := make(map[string]bool, len(a.Spec.Versions)) for _, version := range a.Spec.Versions { versionFullName := version.GetApiVersionFullName(a.Name) + if seenVersions[version.GetApiVersionSpecifier()] { + // Handle duplicate version names + continue + } + seenVersions[version.GetApiVersionSpecifier()] = true apiVersion := ApiVersion{ TypeMeta: metav1.TypeMeta{}, ObjectMeta: metav1.ObjectMeta{ Name: versionFullName, Namespace: a.Namespace, }, Spec: ApiVersionSpec{ ApiVersionSetId: a.Status.ApiVersionSetID, ApiVersionScheme: a.Spec.VersioningScheme, Path: a.Spec.Path, ApiType: a.Spec.ApiType, ApiVersionSubSpec: version, }, } apiVersions[version.GetApiVersionSpecifier()] = apiVersion } return apiVersions }services/dis-apim-operator/internal/controller/apiversion_controller.go (1)
156-156: Fix typo in parameter name.The parameter name has a typo:
apiVesrionshould beapiVersion.Apply this diff to fix the typo:
-func (r *ApiVersionReconciler) createUpdateApimApi(ctx context.Context, apiVesrion apimv1alpha1.ApiVersion) (ctrl.Result, error) { +func (r *ApiVersionReconciler) createUpdateApimApi(ctx context.Context, apiVersion apimv1alpha1.ApiVersion) (ctrl.Result, error) {services/dis-apim-operator/internal/controller/api_controller.go (3)
132-133: Fix incorrect comment.The comment refers to CronJob instead of Api.
Apply this diff to fix the comment:
- // ...make sure it's a CronJob... + // ...make sure it's an Api...
229-276: Add error handling for map operations.The method should handle potential map operation errors when managing version states.
Consider adding checks before map operations:
func (r *ApiReconciler) reconcileVersions(ctx context.Context, api *apimv1alpha1.Api) (bool, error) { logger := log.FromContext(ctx) wantedVersions := api.ToApiVersions() inSync := true if api.Status.VersionStates == nil && len(wantedVersions) > 0 { api.Status.VersionStates = make(map[string]apimv1alpha1.ApiVersionStatus) } + if api.Status.VersionStates == nil { + return false, fmt.Errorf("failed to initialize version states map") + }
278-298: Consider batched operations for version cleanup.For better performance with large numbers of versions, consider implementing batched deletions.
Consider implementing a batched deletion strategy:
- Group versions into batches (e.g., 100 versions per batch)
- Delete versions in parallel within each batch
- Update status after each batch completes
services/dis-apim-operator/api/v1alpha1/apiversion_types.go (1)
188-194: Simplify GetApiVersionSpecifier methodThe method can be simplified by removing the intermediate variable and using a more concise condition.
func (avss *ApiVersionSubSpec) GetApiVersionSpecifier() string { - versionSpecifier := avss.Name - if versionSpecifier == nil || *versionSpecifier == "" { - versionSpecifier = utils.ToPointer("default") - } - return *versionSpecifier + if avss.Name != nil && *avss.Name != "" { + return *avss.Name + } + return "default" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
services/dis-apim-operator/api/v1alpha1/api_types.go(3 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/internal/controller/api_controller.go(2 hunks)services/dis-apim-operator/internal/controller/apiversion_controller.go(3 hunks)services/dis-apim-operator/internal/controller/backend_controller.go(1 hunks)services/dis-apim-operator/internal/utils/base64.go(1 hunks)services/dis-apim-operator/internal/utils/consts.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/test/utils/azure_apim_api_fake.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/dis-apim-operator/internal/utils/consts.go
🚧 Files skipped from review as they are similar to previous changes (6)
- services/dis-apim-operator/api/v1alpha1/backend_types.go
- services/dis-apim-operator/internal/controller/backend_controller.go
- services/dis-apim-operator/internal/utils/base64.go
- services/dis-apim-operator/internal/utils/sha_test.go
- services/dis-apim-operator/internal/utils/sha.go
- services/dis-apim-operator/test/utils/azure_apim_api_fake.go
🧰 Additional context used
📓 Learnings (1)
services/dis-apim-operator/internal/controller/apiversion_controller.go (1)
Learnt from: tjololo
PR: Altinn/altinn-platform#1175
File: services/dis-apim-operator/internal/controller/apiversion_controller.go:109-131
Timestamp: 2024-12-20T09:53:45.127Z
Learning: Once the finalizer is removed, the object is removed from the cluster, so updating the status to "Deleting" is unnecessary.
🔇 Additional comments (9)
services/dis-apim-operator/internal/utils/policytemplate_test.go (1)
10-77: Consider adding more test cases.
The test coverage is good but could be enhanced with:
- Test case for template with multiple replacements
- Test case for concurrent usage of GeneratePolicyFromTemplate
- Test case for very large templates approaching maxContentSize
Example additional test:
Context("with multiple replacements", func() {
It("should replace all placeholders", func() {
templateContent := "{{.Greeting}}, {{.Name}}!"
data := map[string]string{
"Greeting": "Hello",
"Name": "World",
}
result, err := GeneratePolicyFromTemplate(templateContent, data)
Expect(err).NotTo(HaveOccurred())
Expect(result).To(Equal("Hello, World!"))
})
})Also, consider adding package documentation explaining the relationship between these tests and the actual implementation.
services/dis-apim-operator/api/v1alpha1/api_types.go (4)
35-60: LGTM! Well-structured API specification with proper validation.
The struct is well-defined with:
- Clear field descriptions
- Proper validation annotations
- Appropriate default values
- Correct required/optional field markings
65-73: LGTM! Well-structured status fields.
The status fields are well-defined with proper documentation and validation annotations.
111-124: LGTM! Clean conversion to Azure's API version set contract.
The method properly handles nil checks and correctly maps the fields.
103-109: 🛠️ Refactor suggestion
Add Azure resource name validation.
The method should validate that the generated name complies with Azure's resource naming restrictions:
- Length between 1-80 characters
- Only alphanumeric characters and hyphens
- Cannot start or end with hyphen
Apply this diff to add validation:
func (a *Api) GetApiAzureFullName() string {
if a == nil {
return ""
}
- return fmt.Sprintf("%s-%s", a.Namespace, a.Name)
+ name := fmt.Sprintf("%s-%s", a.Namespace, a.Name)
+ // Validate length
+ if len(name) > 80 {
+ return name[:80]
+ }
+ return name
}Likely invalid or redundant comment.
services/dis-apim-operator/internal/controller/apiversion_controller.go (2)
109-131: LGTM! Proper deletion handling with finalizers.
The method correctly handles:
- API version deletion
- Policy cleanup
- Finalizer removal
222-265: LGTM! Well-structured policy management.
The method properly handles:
- Nil checks
- Policy templating
- Error handling
- Status updates
services/dis-apim-operator/internal/controller/api_controller.go (2)
65-120: LGTM! Well-structured reconciliation loop.
The method properly handles:
- Resource fetching
- Finalizer management
- Client initialization
- Error handling
150-182: LGTM! Proper deletion handling.
The method correctly handles:
- Version set verification
- Version cleanup
- Finalizer removal
- Status updates
services/dis-apim-operator/internal/controller/apiversion_controller.go
Outdated
Show resolved
Hide resolved
services/dis-apim-operator/internal/controller/apiversion_controller.go
Outdated
Show resolved
Hide resolved
services/dis-apim-operator/internal/controller/apiversion_controller.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
services/dis-apim-operator/internal/controller/apiversion_controller.go (2)
36-40: Fix constant naming conventions and typosThe constants have the following issues:
- Using underscores instead of camelCase (Go convention)
- Typo in "REQUE" (should be "REQUEUE")
Apply this diff to fix the naming:
const ( - API_VERSION_FINALIZER = "apiversion.apim.dis.altinn.cloud/finalizer" - DEFAULT_REQUE_TIME = 1 * time.Minute - WAITING_FOR_LRO_REQUE_TIME = 5 * time.Second + APIVersionFinalizer = "apiversion.apim.dis.altinn.cloud/finalizer" + DefaultRequeueTime = 1 * time.Minute + WaitingForLRORequeueTime = 5 * time.Second )
158-158: Fix typo in parameter name and remove redundant returnIssues found:
- Parameter name has a typo:
apiVesrionshould beapiVersion- The final return statement is redundant as all cases in the switch are handled
Apply these fixes:
-func (r *ApiVersionReconciler) createUpdateApimApi(ctx context.Context, apiVesrion apimv1alpha1.ApiVersion) (ctrl.Result, error) { +func (r *ApiVersionReconciler) createUpdateApimApi(ctx context.Context, apiVersion apimv1alpha1.ApiVersion) (ctrl.Result, error) {- return ctrl.Result{RequeueAfter: DEFAULT_REQUE_TIME}, nilAlso applies to: 221-222
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
services/dis-apim-operator/api/v1alpha1/apiversion_types.go(3 hunks)services/dis-apim-operator/api/v1alpha1/backend_types.go(2 hunks)services/dis-apim-operator/internal/controller/apiversion_controller.go(3 hunks)services/dis-apim-operator/internal/controller/backend_controller.go(2 hunks)services/dis-apim-operator/internal/utils/consts.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- services/dis-apim-operator/internal/utils/consts.go
- services/dis-apim-operator/internal/controller/backend_controller.go
- services/dis-apim-operator/api/v1alpha1/backend_types.go
🧰 Additional context used
📓 Learnings (1)
services/dis-apim-operator/internal/controller/apiversion_controller.go (1)
Learnt from: tjololo
PR: Altinn/altinn-platform#1175
File: services/dis-apim-operator/internal/controller/apiversion_controller.go:109-131
Timestamp: 2024-12-20T09:53:45.127Z
Learning: Once the finalizer is removed, the object is removed from the cluster, so updating the status to "Deleting" is unnecessary.
🔇 Additional comments (7)
services/dis-apim-operator/api/v1alpha1/apiversion_types.go (2)
38-38: Fix inconsistent field validation and serialization tags
Several fields are marked as Required with kubebuilder validation but also have omitempty json tags:
ApiVersionSetId(line 38)DisplayName(line 68)Content(line 84)Namein PolicyValue (line 121)Namein FromBackend (line 134)
Also applies to: 68-68, 84-84, 121-121, 134-134
229-250: Handle Nil Pointers in ToAzureCreateOrUpdateParameter
Ensure that all pointer fields are checked for nil before dereferencing to prevent nil pointer exceptions.
services/dis-apim-operator/internal/controller/apiversion_controller.go (5)
111-133: LGTM! Deletion logic is well-implemented
The deletion logic properly handles both API and policy deletion, with appropriate error handling and finalizer management.
224-267: LGTM! Policy management is well-implemented
The method properly handles policy creation/update with appropriate null checks and error handling.
269-290: LGTM! Policy templating is well-implemented
The method properly handles policy templating with appropriate error handling and backend reference resolution.
83-90: 🛠️ Refactor suggestion
Move APIM client initialization to SetupWithManager
Initializing the APIM client in every reconciliation loop is inefficient. This should be done during controller setup.
Move this initialization to SetupWithManager:
func (r *ApiVersionReconciler) SetupWithManager(mgr ctrl.Manager) error {
+ if r.apimClient == nil {
+ c, err := r.NewClient(r.ApimClientConfig)
+ if err != nil {
+ return fmt.Errorf("failed to create APIM client: %w", err)
+ }
+ r.apimClient = c
+ }
return ctrl.NewControllerManagedBy(mgr).
For(&apimv1alpha1.ApiVersion{}).
Named("apiversion").
Complete(r)
}Likely invalid or redundant comment.
149-154: 🛠️ Refactor suggestion
Simplify condition and fix potential null pointer dereference
The condition is complex and redundant. The apiVersion.Spec.Policies != nil check is already performed at line 143.
Apply this diff to simplify the condition:
- if apiVersion.Spec.Policies != nil && apiVersion.Status.LastAppliedPolicySha != lastPolicySha || azure.IsNotFoundError(policyErr) {
+ if apiVersion.Status.LastAppliedPolicySha != lastPolicySha || azure.IsNotFoundError(policyErr) {Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
services/dis-apim-operator/api/v1alpha1/apiversion_types.go (2)
49-51: Remove duplicate default value annotation.The default value for
ApiTypeis specified twice:
+kubebuilder:default:="http"+default:value:"http"Remove the redundant
+default:value:"http"line as the kubebuilder annotation is sufficient.
88-88: Fix typo in field comment.There's a typo in the comment for the SubscriptionRequired field:
-// SubscriptionRquired - Indicates if subscription is required to access the API. Default value is true. +// SubscriptionRequired - Indicates if subscription is required to access the API. Default value is true.services/dis-apim-operator/internal/utils/base64.go (1)
10-22: Consider streaming base64 encoding for large filesWhile the implementation is correct, it loads the entire file into memory. Consider using streaming for better memory efficiency.
func base64FromUrlContent(ctx context.Context, url string) (string, error) { resp, err := getContentUrl(ctx, url) if err != nil { return "", err } defer closeIgnoreError(resp.Body) - b, err := io.ReadAll(resp.Body) - if err != nil { - return "", err - } - return base64.StdEncoding.EncodeToString(b), nil + pr, pw := io.Pipe() + go func() { + encoder := base64.NewEncoder(base64.StdEncoding, pw) + _, err := io.Copy(encoder, resp.Body) + encoder.Close() + pw.CloseWithError(err) + }() + result, err := io.ReadAll(pr) + if err != nil { + return "", err + } + return string(result), nil }services/dis-apim-operator/internal/utils/sha.go (1)
26-37: Remove unnecessary empty lineThe implementation is correct, but there's an unnecessary empty line at line 31.
func Sha256FromContent(ctx context.Context, content *string) (string, error) { if content == nil { return "", nil } if isUrl(*content) { - return sha256FromUrlContent(ctx, *content) } h := sha256.New() h.Write([]byte(*content)) return fmt.Sprintf("%x", h.Sum(nil)), nil }services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apis.yaml (3)
16-24: Consider adding more printer columns for better observability.While the current printer columns (State and Age) are useful, consider adding columns for:
- DisplayName
- Path
- Current Version
This would improve visibility when usingkubectl get apis.
128-132: Remove template comment from schema.The 'name' field description contains a template comment that should be removed:
- description: |- - INSERT ADDITIONAL SPEC FIELDS - desired state of cluster - Important: Run "make" to regenerate code after modifying this file - Name - Name of the API Version. If no name is provided this will be the default version + description: Name - Name of the API Version. If no name is provided this will be the default version
214-216: Fix typo in field description.There's a typo in the field description:
- description: SubscriptionRquired - Indicates if subscription + description: SubscriptionRequired - Indicates if subscriptionservices/dis-apim-operator/internal/controller/apiversion_controller.go (2)
36-40: Follow Go naming conventions for constantsThe constant names should follow Go naming conventions (camelCase for non-exported constants).
Apply this diff to fix the naming:
const ( - API_VERSION_FINALIZER = "apiversion.apim.dis.altinn.cloud/finalizer" - DEFAULT_REQUE_TIME = 1 * time.Minute - WAITING_FOR_LRO_REQUE_TIME = 5 * time.Second + apiVersionFinalizer = "apiversion.apim.dis.altinn.cloud/finalizer" + defaultRequeueTime = 1 * time.Minute + waitingForLRORequeTime = 5 * time.Second )
149-153: Simplify complex condition and improve error handlingThe condition is complex and could be simplified for better readability. Also, consider adding more specific error context.
Apply this diff to improve the code:
- if apiVersion.Spec.Policies != nil && apiVersion.Status.LastAppliedPolicySha != lastPolicySha || azure.IsNotFoundError(policyErr) { + needsPolicyUpdate := apiVersion.Status.LastAppliedPolicySha != lastPolicySha + isPolicyMissing := azure.IsNotFoundError(policyErr) + if needsPolicyUpdate || isPolicyMissing { if err := r.createUpdatePolicy(ctx, apiVersion); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to create/update policy: %w", err) + return ctrl.Result{}, fmt.Errorf("failed to %s policy: %w", + map[bool]string{true: "create", false: "update"}[isPolicyMissing], + err) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
services/dis-apim-operator/api/v1alpha1/apiversion_types.go(3 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/internal/controller/apiversion_controller.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/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)
🚧 Files skipped from review as they are similar to previous changes (3)
- services/dis-apim-operator/internal/utils/base64_test.go
- services/dis-apim-operator/internal/utils/sha_test.go
- services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apiversions.yaml
🧰 Additional context used
📓 Learnings (2)
services/dis-apim-operator/internal/controller/apiversion_controller.go (1)
Learnt from: tjololo
PR: Altinn/altinn-platform#1175
File: services/dis-apim-operator/internal/controller/apiversion_controller.go:109-131
Timestamp: 2024-12-20T09:53:45.127Z
Learning: Once the finalizer is removed, the object is removed from the cluster, so updating the status to "Deleting" is unnecessary.
services/dis-apim-operator/api/v1alpha1/apiversion_types.go (2)
Learnt from: tjololo
PR: Altinn/altinn-platform#1175
File: services/dis-apim-operator/api/v1alpha1/apiversion_types.go:118-128
Timestamp: 2024-12-20T12:02:31.164Z
Learning: The user wants to attach two XValidation rules to the PolicyValue struct (rather than the Name field) to ensure that either Value or IdFromBackend is present—but not both—and that at least one is set.
Learnt from: tjololo
PR: Altinn/altinn-platform#1175
File: services/dis-apim-operator/api/v1alpha1/apiversion_types.go:229-250
Timestamp: 2024-12-20T12:07:31.493Z
Learning: Using // +kubebuilder:validation:MinLength=1 ensures that mandatory string fields have a minimum length of 1 character. This works well alongside // +kubebuilder:validation:Required for providing direct feedback during CRD creation.
🔇 Additional comments (12)
services/dis-apim-operator/api/v1alpha1/apiversion_types.go (2)
121-133: LGTM! XValidation rules implemented correctly.
The PolicyValue struct correctly implements the XValidation rules to ensure either Value or IdFromBackend is set (but not both) as requested in previous review comments.
137-140:
Remove omitempty tag from required field.
The Name field is marked as required but has an omitempty tag, which could lead to validation issues:
- Name string `json:"name,omitempty"`
+ Name string `json:"name"`Likely invalid or redundant comment.
services/dis-apim-operator/internal/utils/utils.go (2)
13-18: LGTM! Good security configuration.
The HTTP client is well-configured with:
- 30-second timeout to prevent hanging operations
- TLS 1.2 minimum version for security
- Connection pooling through global client
25-41: LGTM! Robust HTTP client implementation.
Good practices implemented:
- Context-aware request handling
- Status code validation
- Content size limits to prevent DoS
- Clear error messages
services/dis-apim-operator/internal/utils/base64.go (1)
24-33: LGTM! Clean implementation.
The function handles all cases well:
- Nil check
- URL detection
- Direct content encoding
services/dis-apim-operator/internal/utils/sha.go (1)
11-23: LGTM! Efficient streaming implementation.
The function efficiently streams content directly to the hasher without loading the entire file into memory.
services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apis.yaml (2)
1-15: LGTM! The CRD follows Kubernetes best practices.
The basic structure is well-defined with:
- Latest stable apiextensions.k8s.io/v1 API
- Standard naming conventions
- Appropriate namespaced scope for multi-tenant isolation
1-272: Verify consistency with other Azure operators.
Since this CRD is used for Azure API Management, it would be beneficial to verify if the schema structure aligns with similar CRDs in other Azure operators for consistency across the ecosystem.
✅ Verification successful
CRD schema is consistent with related Azure APIM resources
The CRD schema is well-aligned with the other Azure API Management CRDs in the codebase. Key observations:
- All three CRDs (Api, Backend, ApiVersion) follow consistent patterns for:
- Common fields like
provisioningState, status subresources - Azure-specific fields like resource IDs and versioning
- Shared concepts like policies, protocols, and subscription requirements
- Common fields like
- Status fields use consistent provisioning states across CRDs
- Schema validation and default values follow similar patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for similar Azure-related CRDs in the codebase
# Look for CRDs with similar Azure API Management related fields
# Search for other CRDs with Azure-specific fields
fd -e yaml -e yml . | xargs rg -l "azure|apim|apimanagement"
# Search for similar provisioning state patterns
fd -e yaml -e yml . | xargs rg -l "provisioningState.*Succeeded|provisioningState.*Failed"
Length of output: 2254
Script:
#!/bin/bash
# Let's examine the related CRDs found in the codebase for consistency
# Check the structure of backends CRD
echo "=== Backends CRD ==="
cat services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_backends.yaml
# Check the structure of apiversions CRD
echo -e "\n=== ApiVersions CRD ==="
cat services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apiversions.yaml
# Look for provisioning states in these CRDs
echo -e "\n=== Provisioning States ==="
rg "provisioningState.*description" services/dis-apim-operator/config/crd/bases/
Length of output: 13904
services/dis-apim-operator/internal/controller/apiversion_controller.go (4)
111-133: LGTM! Proper cleanup handling
The deletion logic correctly handles both API and policy cleanup, with proper error handling and finalizer removal.
224-267: LGTM! Well-structured policy management
The method has comprehensive error handling, proper nil checks, and consistent status updates.
83-90: 🛠️ Refactor suggestion
Move APIM client initialization to SetupWithManager
Creating a new client in the Reconcile method could impact performance as it runs on every reconciliation. Consider moving this to SetupWithManager.
Move the client initialization to SetupWithManager:
func (r *ApiVersionReconciler) SetupWithManager(mgr ctrl.Manager) error {
+ c, err := r.NewClient(r.ApimClientConfig)
+ if err != nil {
+ return fmt.Errorf("failed to create APIM client: %w", err)
+ }
+ r.apimClient = c
return ctrl.NewControllerManagedBy(mgr).
For(&apimv1alpha1.ApiVersion{}).
Named("apiversion").
Complete(r)
}Likely invalid or redundant comment.
269-290: 🛠️ Refactor suggestion
Use parent context for backend retrieval
The method should use the parent context when retrieving backend information to ensure proper context cancellation.
Apply this diff:
-func (r *ApiVersionReconciler) runPolicyTemplating(values []apimv1alpha1.PolicyValue, policyContent string, apiVersionNamespace string) (string, error) {
+func (r *ApiVersionReconciler) runPolicyTemplating(ctx context.Context, values []apimv1alpha1.PolicyValue, policyContent string, apiVersionNamespace string) (string, error) {
// ... rest of the method ...
var backend apimv1alpha1.Backend
- err := r.Get(context.Background(), client.ObjectKey{Name: v.IdFromBackend.Name, Namespace: namespace}, &backend)
+ err := r.Get(ctx, client.ObjectKey{Name: v.IdFromBackend.Name, Namespace: namespace}, &backend)Likely invalid or redundant comment.
services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apis.yaml
Outdated
Show resolved
Hide resolved
services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apis.yaml
Outdated
Show resolved
Hide resolved
services/dis-apim-operator/internal/controller/apiversion_controller.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
services/dis-apim-operator/api/v1alpha1/apiversion_types.go (2)
136-137: Namespace assignment logic.
The struct “FromBackend” includes optional Namespace. If not set, the code defaults to the same namespace. This is a straightforward approach, but keep an eye out for any conditional logic or extra validations needed if pointing to a different namespace.
195-201: Use versionSpecifier consistently.
“versionSpecifier” defaults to “default” when unspecified. This is straightforward and should be well-documented so that users know how default versioning is handled.services/dis-apim-operator/api/v1alpha1/api_types.go (2)
112-125: Return partial object for nil “a.”
When the ‘Api’ object is nil, ToAzureApiVersionSet returns an empty struct. Consider logging a warning if that’s unexpected.
127-149: Potential multi-version concurrency.
ToApiVersions constructs multiple ApiVersion objects (with unique names) from the same base. This is a typical approach. Ensure that each version’s concurrency is fully handled by the reconciler.services/dis-apim-operator/internal/utils/utils.go (1)
13-18: Consider additional transport configuration for better connection managementWhile the TLS and timeout settings are good, consider adding these transport configurations for better connection management:
var httpClient = &http.Client{ Timeout: 30 * time.Second, Transport: &http.Transport{ TLSClientConfig: &tls.Config{MinVersion: tls.VersionTLS12}, + MaxIdleConns: 100, + MaxIdleConnsPerHost: 100, + IdleConnTimeout: 90 * time.Second, }, }services/dis-apim-operator/internal/utils/utils_test.go (1)
31-44: Consider additional test cases for content typesWhile the basic functionality is well tested, consider adding tests for different content types and response headers.
It("should return content for a valid URL", func() { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/plain") _, _ = fmt.Fprintln(w, "test content") })) defer server.Close() resp, err := getContentUrl(context.Background(), server.URL) Expect(err).NotTo(HaveOccurred()) defer closeIgnoreError(resp.Body) body, err := io.ReadAll(resp.Body) Expect(err).NotTo(HaveOccurred()) Expect(string(body)).To(Equal("test content\n")) + Expect(resp.Header.Get("Content-Type")).To(Equal("text/plain")) }))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
services/dis-apim-operator/api/v1alpha1/api_types.go(3 hunks)services/dis-apim-operator/api/v1alpha1/apiversion_types.go(3 hunks)services/dis-apim-operator/api/v1alpha1/backend_types.go(3 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/internal/controller/apiversion_controller.go(3 hunks)services/dis-apim-operator/internal/utils/utils.go(1 hunks)services/dis-apim-operator/internal/utils/utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apiversions.yaml
- services/dis-apim-operator/config/crd/bases/apim.dis.altinn.cloud_apis.yaml
- services/dis-apim-operator/api/v1alpha1/backend_types.go
🧰 Additional context used
📓 Learnings (2)
services/dis-apim-operator/internal/controller/apiversion_controller.go (1)
Learnt from: tjololo
PR: Altinn/altinn-platform#1175
File: services/dis-apim-operator/internal/controller/apiversion_controller.go:109-131
Timestamp: 2024-12-20T09:53:45.127Z
Learning: Once the finalizer is removed, the object is removed from the cluster, so updating the status to "Deleting" is unnecessary.
services/dis-apim-operator/api/v1alpha1/apiversion_types.go (2)
Learnt from: tjololo
PR: Altinn/altinn-platform#1175
File: services/dis-apim-operator/api/v1alpha1/apiversion_types.go:118-128
Timestamp: 2024-12-20T12:02:31.164Z
Learning: The user wants to attach two XValidation rules to the PolicyValue struct (rather than the Name field) to ensure that either Value or IdFromBackend is present—but not both—and that at least one is set.
Learnt from: tjololo
PR: Altinn/altinn-platform#1175
File: services/dis-apim-operator/api/v1alpha1/apiversion_types.go:229-250
Timestamp: 2024-12-20T12:07:31.493Z
Learning: Using // +kubebuilder:validation:MinLength=1 ensures that mandatory string fields have a minimum length of 1 character. This works well alongside // +kubebuilder:validation:Required for providing direct feedback during CRD creation.
🔇 Additional comments (27)
services/dis-apim-operator/api/v1alpha1/apiversion_types.go (8)
20-23: Consider error handling for import "fmt" usage.
"reflect" and "fmt" are standard libraries, but we don’t see any advanced parse or error-handling logic for user input. This is acceptable as-is, but keep in mind to wrap or annotate errors (e.g., with %w) for clarity if thrown deeper in the stack.
36-58: Validation alignment with CRD best practices.
These fields are comprehensively annotated with kubebuilder validations. For mandatory fields (e.g., ApiVersionSetId, Path), ensuring the removal of “,omitempty” from the JSON tags aligns with the Required annotation. This is already addressed here, so the definitions look consistent.
60-104: Confirm that descriptive fields remain optional.
The optional fields (Name, Description, ServiceUrl, etc.) and their associated constraints (e.g., MaxLength=2000) look correct. The presence of explicit MinLength=1 constraints on mandatory fields is consistent with user feedback about CRD-level validation.
106-119: Check mandatory policyContent logic.
PolicyContent is marked Required. This matches typical usage cases, assuming that each policy definition must have content. Good job ensuring the field is validated.
121-134: Ensure XValidation is set on the PolicyValue struct.
The XValidation constraints to enforce exactly one of Value or IdFromBackend are well-placed at lines 122-123. This addresses potential misconfigurations accordingly.
148-163: Appropriate enumerations.
The provisioning state includes “Succeeded, Failed, Updating, Deleting.” This enumerated pattern is consistent with typical operational states. Ensure the controller logic interprets all states appropriately (especially “Deleting”).
215-235: Synchronization concerns for reflection logic.
The Matches function uses reflect.DeepEqual to compare slices (e.g., ‘Products’, ‘Protocols’) as well as pointerValueEqual for pointers. This is acceptable but keep in mind concurrency if the code is updated by multiple goroutines. For CRD usage, usually not an issue, but it’s good to be aware.
236-258: Potential for extended nil checks in ToAzureCreateOrUpdateParameter.
The existing code sets multiple fields from pointers that can be nil (e.g., a.Spec.Description, a.Spec.Content, etc.). However, because the CRD strongly validates them, accidental nil dereferences are unlikely. Just watch out for future expansions in which further optional fields might not be guaranteed.
services/dis-apim-operator/api/v1alpha1/api_types.go (5)
20-21: Optional direct imports.
“fmt” is used for name formatting; no issues here. All good.
35-60: Remove “omitempty” from required fields in future expansions.
The field “DisplayName” is Required with no “omitempty” tag, which is correct. Other mandatory fields (VersioningScheme, Path) also appear consistent. Always ensure that “omitempty” is removed from required fields to enforce the CRD validation.
65-74: ApiVersionSetID usage.
“ApiVersionSetID” might remain empty unless the CRD usage explicitly ensures it’s filled. That’s fine for optional scenarios, but confirm that the downstream logic properly handles an empty ID.
79-80: Printer columns enhance the experience.
Having a “State” and “Age” column is user-friendly. The columns appear to rely on “status.provisioningState.” This is helpful.
104-110: Avoid potential nil references.
“a == nil” check in GetApiAzureFullName prevents a trivial panic. This is a prudent defensive approach.
services/dis-apim-operator/internal/controller/apiversion_controller.go (11)
21-22: fmt + time usage.
All typical standard library imports are fine. Keep an eye on potential performance impacts with large time intervals.
24-26: Centralize azure & utilities.
Imports from “internal/azure” and “internal/utils” are well-organized. Bundling all Azure calls in a single specialized library is good.
30-30: No concerns with import "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil".
This is standard usage for finalizers.
36-40: Requeue intervals.
Declaring these time constants (DEFAULT_REQUE_TIME, WAITING_FOR_LRO_REQUE_TIME) clarifies the standard requeue intervals. This is a best practice.
45-48: Injecting APIM client dependencies.
The approach to define NewClient, ApimClientConfig, and apimClient enhances testability. Good injection pattern.
65-100: Initialize clients & finalizers carefully.
• The check for finalizers is good to ensure deletion logic.
• The dynamic creation of the “apimClient” is valid. Confirm that creating the client for each reconcile call is avoided if “apimClient” is already set.
• The “handleApiVersionUpdate” method is triggered only if not found or no error – that’s structured well.
110-133: No status update after finalizer removal.
Consistent with the user’s learning (once finalizer is removed, the object is removed from the cluster, so no need to update the status to “Deleting”). This logic matches that learning.
135-156: Hash-based detection of changes.
The logic compares the LastAppliedSpecSha with the newly computed sha256. This is an excellent approach for identifying real differences.
158-223: Long-running operation polling.
Well-structured approach with azure.StartResumeOperation. The switch statement for different statuses is well-handled. No immediate issues.
225-268: Handle policy creation.
Ensures that the CR’s policy is updated after a successful creation or update. The fallback to an error if PolicyContent is nil is correct.
270-291: Templating logic for policy placeholders.
Looks good. The code updates “data[v.Name]” either with a backend id or the literal “Value.” This approach is straightforward.
services/dis-apim-operator/internal/utils/utils.go (2)
20-26: LGTM! Secure URL validation implementation
The implementation correctly validates both the URL scheme (http/https) and ensures a non-empty host, providing good security controls.
46-48: LGTM! Simple and focused utility function
The function serves its purpose well as a cleanup helper.
services/dis-apim-operator/internal/utils/utils_test.go (1)
15-28: LGTM! Comprehensive test coverage for URL validation
The tests cover a good range of valid and invalid URL cases, including edge cases.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
services/dis-apim-operator/internal/controller/apiversion_controller.go
Outdated
Show resolved
Hide resolved
…roller.go Co-authored-by: Sebastian Duran <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
services/dis-apim-operator/internal/controller/apiversion_controller.go (2)
36-40: Follow Go naming conventions for constants.The constants should use camelCase instead of SCREAMING_SNAKE_CASE, and there's a typo in "REQUE".
- API_VERSION_FINALIZER = "apiversion.apim.dis.altinn.cloud/finalizer" - DEFAULT_REQUE_TIME = 1 * time.Minute - WAITING_FOR_LRO_REQUE_TIME = 5 * time.Second + APIVersionFinalizer = "apiversion.apim.dis.altinn.cloud/finalizer" + DefaultRequeueTime = 1 * time.Minute + WaitingForLRORequeueTime = 5 * time.Second
171-173: Improve error handling for status update.The status update error is silently ignored. Consider logging it or combining it with the main error.
- apiVersion.Status.ProvisioningState = apimv1alpha1.ProvisioningStateFailed - _ = r.Status().Update(ctx, &apiVersion) - return ctrl.Result{}, err + apiVersion.Status.ProvisioningState = apimv1alpha1.ProvisioningStateFailed + if updateErr := r.Status().Update(ctx, &apiVersion); updateErr != nil { + logger.Error(updateErr, "Failed to update status") + return ctrl.Result{}, fmt.Errorf("failed to create/update API and update status: %v, %v", err, updateErr) + } + return ctrl.Result{}, err
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/dis-apim-operator/internal/controller/apiversion_controller.go(3 hunks)
🧰 Additional context used
📓 Learnings (1)
services/dis-apim-operator/internal/controller/apiversion_controller.go (1)
Learnt from: tjololo
PR: Altinn/altinn-platform#1175
File: services/dis-apim-operator/internal/controller/apiversion_controller.go:109-131
Timestamp: 2024-12-20T09:53:45.127Z
Learning: Once the finalizer is removed, the object is removed from the cluster, so updating the status to "Deleting" is unnecessary.
🔇 Additional comments (3)
services/dis-apim-operator/internal/controller/apiversion_controller.go (3)
111-133: LGTM! Proper deletion handling.The function correctly handles the deletion of both the API and its policies, with appropriate error handling and finalizer removal.
225-268: LGTM! Well-structured policy management.The function has proper nil checks, comprehensive error handling, and appropriate status updates.
270-291: LGTM! Clean templating implementation.The function properly handles different policy value sources with good error handling and context propagation.
services/dis-apim-operator/internal/controller/apiversion_controller.go
Outdated
Show resolved
Hide resolved
|
LGTM now 🚀 🎸 |
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.Backendresource definition with additional printer columns for better visibility in Kubernetes.Bug Fixes
Tests
ApiVersioncontroller and utility functions.Backendcontroller to reflect new resource management strategies.Chores
.gitignorefile to exclude specific patterns.