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

[Feat] liqoctl gateway template check #2791

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cheina97
Copy link
Member

@cheina97 cheina97 commented Oct 25, 2024

Description

This PR adds some checks in liqoctl (at peering time) about the gateway template used.
This checks are:

  • template existence
  • in case --server-service-nodeport flag has been specified, it checks if the template selected support static nodeports
  • in case --server-service-loadbalancerip flag has been specified, it checks if the template selected support static load balancer ips

@adamjensenbot
Copy link
Collaborator

Hi @cheina97. Thanks for your PR!

I am @adamjensenbot.
You can interact with me issuing a slash command in the first line of a comment.
Currently, I understand the following commands:

  • /rebase: Rebase this PR onto the master branch (You can add the option test=true to launch the tests
    when the rebase operation is completed)
  • /merge: Merge this PR into the master branch
  • /build Build Liqo components
  • /test Launch the E2E and Unit tests
  • /hold, /unhold Add/remove the hold label to prevent merging with /merge

Make sure this PR appears in the liqo changelog, adding one of the following labels:

  • kind/breaking: 💥 Breaking Change
  • kind/feature: 🚀 New Feature
  • kind/bug: 🐛 Bug Fix
  • kind/cleanup: 🧹 Code Refactoring
  • kind/docs: 📝 Documentation

@cheina97 cheina97 added the kind/feature New feature or request label Oct 25, 2024
@cheina97 cheina97 changed the title feat: liqoctl gateway template check [Feat] liqoctl gateway template check Oct 25, 2024
@cheina97 cheina97 marked this pull request as ready for review October 25, 2024 13:58
@cheina97 cheina97 requested review from claudiolor, aleoli and fra98 and removed request for claudiolor, aleoli and fra98 October 25, 2024 13:58
@@ -36,6 +36,7 @@ spec:
- port: "{{"{{ .Spec.Endpoint.Port }}"}}"
protocol: UDP
targetPort: "{{"{{ .Spec.Endpoint.Port }}"}}"

Copy link
Member

Choose a reason for hiding this comment

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

We should align this file to the eks one also on what concerns empty lines

Comment on lines +169 to +176
if err := cluster1.CheckTemplateGwClient(ctx, o); err != nil {
return err
}

// Check if the Templates exists and is valid on cluster 2
if err := cluster2.CheckTemplateGwServer(ctx, o); err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

We should add a flag to skip these checks


// GetNestedField returns the nested field of a map.
// Example: GetNestedField(map[string]interface{}{"a": map[string]interface{}{"b": "c"}}, "a.b") returns "c".
func GetNestedField(m map[string]interface{}, path string) (interface{}, error) {
Copy link
Member

Choose a reason for hiding this comment

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

In modern go, I think any type is identical to interface{} and more idiomatic. Not entirely sure about this


_, err = maps.GetNestedField(port, "nodePort")
if err != nil {
return fmt.Errorf("unable to get spec.template.spec.service.spec.ports[0].nodePort int the server template, " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Once we add a flag to skip this check (as suggested by aleoli), we can add a suggestion of how to skip this check. Morevoer, we might also add a documentation page explaining the concept of template and how to add a field in the template, so that we can add a link to it in the error message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants