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

fix: tilt local development 2 #348

Merged
merged 1 commit into from
Dec 31, 2024
Merged

fix: tilt local development 2 #348

merged 1 commit into from
Dec 31, 2024

Conversation

Greyeye
Copy link
Collaborator

@Greyeye Greyeye commented Dec 31, 2024

  1. KUBECHECKS_ARGOCD_REPOSITORY_ENDPOINT set, as default is not valid for the localdev.
  2. argocd-repo-server-network-policy updated to allow access to the argocd-repo-server from kubechecks pod.

1. KUBECHECKS_ARGOCD_REPOSITORY_ENDPOINT set
2. argocd-repo-server-network-policy updated to allow access to the argocd-repo-server from kubechecks pod.
@zapier-sre-bot
Copy link
Collaborator

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of localdev/argocd/manifests/base/repo-server/argocd-repo-server-network-policy.yaml

@@ -19,6 +19,9 @@ spec:
       - podSelector:
           matchLabels:
             app.kubernetes.io/name: argocd-notifications-controller
+      - podSelector:
+          matchLabels:
+            app.kubernetes.io/name: kubechecks
       ports:
         - protocol: TCP
           port: 8081

Feedback & Suggestions:

  1. Security Consideration: 🛡️ Adding a new podSelector for kubechecks without specifying the exact purpose or ensuring it is necessary could potentially open up unintended access. Ensure that the kubechecks pod needs access to the argocd-repo-server and that this access is secure and justified.

  2. Documentation: 📚 It would be beneficial to document the reason for allowing kubechecks access in the comments or related documentation. This will help future maintainers understand the rationale behind this change.

  3. Testing: 🧪 After implementing this change, ensure thorough testing is conducted to verify that the kubechecks pod can communicate as expected and that no unintended access is granted.


😼 Mergecat review of localdev/kubechecks/values.yaml

@@ -3,6 +3,7 @@ configMap:
   env:
     GRPC_ENFORCE_ALPN_ENABLED: false
     KUBECHECKS_ADDITIONAL_APPS_NAMESPACES: "*"
+    KUBECHECKS_ARGOCD_REPOSITORY_ENDPOINT: argocd-repo-server.kubechecks:8081
     KUBECHECKS_LOG_LEVEL: debug
     KUBECHECKS_ENABLE_WEBHOOK_CONTROLLER: "false"
     KUBECHECKS_ARGOCD_API_INSECURE: "true"

Feedback & Suggestions:

  1. Security Concern: Ensure that the KUBECHECKS_ARGOCD_REPOSITORY_ENDPOINT does not expose sensitive information or allow unauthorized access. Consider using environment variables or secrets management tools to handle sensitive endpoints securely. 🔒

  2. Documentation: It would be helpful to add a comment or documentation about the purpose of KUBECHECKS_ARGOCD_REPOSITORY_ENDPOINT and how it should be configured. This will aid future developers in understanding its role and configuration requirements. 📚

  3. Validation: Consider adding validation checks to ensure that the endpoint is correctly formatted and reachable. This can prevent runtime errors and improve reliability. ✅



Dependency Review

Click to read mergecats review!

No suggestions found

Copy link
Collaborator

@abhi-kapoor abhi-kapoor left a comment

Choose a reason for hiding this comment

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

Thanks for pushing the change.

Copy link

github-actions bot commented Dec 31, 2024

Temporary image deleted.

@Greyeye Greyeye merged commit 38a515c into main Dec 31, 2024
3 checks passed
@Greyeye Greyeye deleted the fix-local-dev-2 branch December 31, 2024 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants