Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Refactor istio pkg #1342

Merged

Conversation

c-pius
Copy link
Contributor

@c-pius c-pius commented Feb 23, 2024

Description

Changes proposed in this pull request:

  • refactors the construction of the VirtualService object
    • moves the object construction out of the istio client
    • removes the dependency on the client for fetching the gateways and instead receives the gateways as an argument
    • => we have a pure function where output is fully dependent on input that is easily unit testable
    • added some guard clauses / sanity checks
  • adds unit tests for the VirtualService construction
  • refactors the construction of the HttpRoute object
    • added some guard clauses / sanity checks
  • adds unit tests for the HttpRoute construction
  • aligns methods in istio#client.go wrt. method name, argument name, return values
  • moves the istio package from pkg to internal

Related issue(s)

Examples of VirtualService objects

Before and After look equal ✅

Before

apiVersion: networking.istio.io/v1beta1                     
kind: VirtualService                                        
metadata:                                                   
  creationTimestamp: "2024-02-23T10:56:53Z"                 
  generation: 1                                             
  name: klm-kyma-watcher                                    
  namespace: kcp-system                                     
  resourceVersion: "86048"                                  
  uid: de1e8e9e-358e-46ad-a994-8795276ae55d                 
spec:                                                       
  gateways:                                                 
  - kcp-system/klm-watcher-gateway                          
  hosts:                                                    
  - host.k3d.internal                                       
  http:                                                     
  - match:                                                  
    - uri:                                                  
        prefix: /v1/lifecycle-manager/event                 
    name: kcp-system/klm-kyma-watcher                       
    route:                                                  
    - destination:                                          
        host: klm-event-service.kcp-system.svc.cluster.local
        port:                                               
          number: 8082                                      
                                             

After

apiVersion: networking.istio.io/v1beta1                     
kind: VirtualService                                        
metadata:                                                   
  creationTimestamp: "2024-02-23T10:54:20Z"                 
  generation: 1                                             
  name: klm-kyma-watcher                                    
  namespace: kcp-system                                     
  resourceVersion: "85740"                                  
  uid: 6abd22b4-bcce-4c99-848b-d022db6a2299                 
spec:                                                       
  gateways:                                                 
  - kcp-system/klm-watcher-gateway                          
  hosts:                                                    
  - host.k3d.internal                                       
  http:                                                     
  - match:                                                  
    - uri:                                                  
        prefix: /v1/lifecycle-manager/event                 
    name: kcp-system/klm-kyma-watcher                       
    route:                                                  
    - destination:                                          
        host: klm-event-service.kcp-system.svc.cluster.local
        port:                                               
          number: 8082       

@c-pius c-pius requested a review from a team as a code owner February 23, 2024 10:41
@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 23, 2024
@c-pius c-pius force-pushed the chore/refactor-virtual-service-creation branch from 194c706 to 380822d Compare February 23, 2024 10:58
@nesmabadr nesmabadr self-assigned this Feb 23, 2024
@c-pius c-pius force-pushed the chore/refactor-virtual-service-creation branch from 380822d to f93b02d Compare February 26, 2024 13:11
@kyma-bot kyma-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 26, 2024
@c-pius c-pius changed the title chore: Refactor VirtualService construction chore: Refactor istio pkg Feb 26, 2024
@@ -142,7 +142,7 @@ func (r *WatcherReconciler) SetupWithManager(mgr ctrl.Manager, options ctrlrunti
return errRestConfigIsNotSet
}
var err error
r.IstioClient, err = istio.NewVersionedIstioClient(r.RestConfig, r.EventRecorder,
r.IstioClient, err = istio.NewIstioClient(r.RestConfig, r.EventRecorder,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed versioned as the constructor doesn't include any special semantics with regards to version.

internal/controller/watcher_controller.go Show resolved Hide resolved
internal/istio/client.go Show resolved Hide resolved
internal/istio/client.go Show resolved Hide resolved
@c-pius c-pius requested a review from nesmabadr February 26, 2024 13:36
@c-pius c-pius force-pushed the chore/refactor-virtual-service-creation branch from 48c38af to f780202 Compare February 27, 2024 09:57
@c-pius c-pius requested a review from nesmabadr February 27, 2024 10:14
@kyma-bot kyma-bot added the lgtm Looks good to me! label Feb 27, 2024
@kyma-bot kyma-bot merged commit b2e06b6 into kyma-project:main Feb 27, 2024
33 checks passed
@c-pius c-pius deleted the chore/refactor-virtual-service-creation branch February 28, 2024 09:23
@c-pius c-pius assigned c-pius and unassigned nesmabadr Feb 28, 2024
@c-pius
Copy link
Contributor Author

c-pius commented Mar 1, 2024

Deleted the VirtualService on DEV and it got re-created as expected. Therefore moving it to "To Accept"

@c-pius c-pius added the area/quality Related to all activites around quality label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/quality Related to all activites around quality cla: yes Indicates the PR's author has signed the CLA. lgtm Looks good to me! size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants