-
Notifications
You must be signed in to change notification settings - Fork 5
feat(ingress): automatic host and path discovery #49
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
Conversation
Tests are broken, need fixing and update. |
k8s.checklyhq.com/enabled: "true" | ||
k8s.checklyhq.com/path: "/baz" | ||
# k8s.checklyhq.com/endpoint: "foo.baaz" - Default read from spec.rules[0].host | ||
# k8s.checklyhq.com/success: "200" - Default "200" | ||
# k8s.checklyhq.com/endpoint: "foo.baaz" - Default read from spec.rules[*].host | ||
k8s.checklyhq.com/group: "group-sample" | ||
# k8s.checklyhq.com/muted: "false" # If not set, default "true" | ||
# k8s.checklyhq.com/path: "/baz" - Default read from spec.rules[*].http.paths[*].path | ||
# k8s.checklyhq.com/success: "200" - Default "200" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that it seems we can add only once custom endpoint+path+status
. I'd suggest to have one annotation that can define multiple targets. E.g.:
annotations:
k8s.checklyhq.com/api_monitor: "200|https://foo.baaz/baz,301|http://redirect.demo"
Even better, maybe include JSON in the value that is directly tied to API.
JSON for flexibility, simple syntax for UX. Should be decided, but I'd move from ability to set only one endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% agree that we need something, not sure what the best path forward is, I'd hate to manage JSON inside YAML..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we need to see if we can find best simple abstraction of what we want to achieve here and not be slapped in the face because it does not support some basic functionality. I'd still have API reference in mind here to make sure we cover basics.
What do you propose?
Also, what would be the minimum (original) API JSON request for hostname+path? Just to see verbosity. Benefit here is the ability to support those kind of annotations even in Service resources and documentation is already available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest here we write some requirement and improve it.
We can also end up with a new CRD where we can specify specific options for each resource, including auth and etc.
Seems like the tests are failing still. |
6f1d0cc
to
96bb2e4
Compare
Tests are passing now. |
d48aa93
to
dfad553
Compare
continue | ||
} | ||
|
||
logger.Info("ApiCheck resource deleted successfully.", apiCheckResource.Name, "Namespace:", apiCheckResource.Namespace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nitpicky but it would be nice to use a consistent format for log messages. Either always end in punctuation or never do. Also I don't think it's necessary to include the :
in any log key names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in latest commit.
labels := make(map[string]*string) | ||
labels["ingress-controller"] = &ingress.Name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This labels
variable doesn't seem to be used for anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used when we create the check on checklyhq.com, all labels are added as tags, it helps identifying in the UI where the checks are coming from and in this case, you could see from the tags that it was generated from a specific ingess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mean that the variable is literally unused. The label map is created, and then a value is set, but then the labels
variable is not used for anything, so there is no way for it to end up in the UI. Frankly I'm a little surprised that Go doesn't complain about the variable being unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in the latest commit.
for _, rulePath := range rule.HTTP.Paths { | ||
if ingress.Annotations[annotationPath] == "" { | ||
if rulePath.Path == "" { | ||
paths = append(paths, "/") | ||
} else { | ||
paths = append(paths, rulePath.Path) | ||
} | ||
} else { | ||
paths = append(paths, ingress.Annotations[annotationPath]) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the logic here is correct. It might be, but it seems a little fishy. If there are multiple rule.HTTP.Paths
, then you'll iterate over them, but then if the annotationPath
exists, you add the same value (annotationPath
's value) to paths
in every iteration? Would that not basically just create the exact same check multiple times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct. I guess the right thing to do is to completely remove the path annotation. Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the path annotation used to be required, removing it would be a breaking change. I would rather not do that unless there is clear, worthy benefit, and I don't think there is (to be clear the feature in the PR is great, just saying I don't think removing the annotation is warranted). Would the following make sense instead:
If ingress.Annotations[annotationPath]
exists, then we simply append its value to paths
without looping through rule.HTTP.Paths
at all. And if it is not set, then we do loop through rule.HTTP.Paths
like now, except with all the annotation checking removed.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the logic you mentioned, if I understand correctly, please check.
} else { | ||
logger.Info( | ||
"ApiCheck data is not identical, update needed", "ApiCheck Name", existingApiCheck.Name, "old spec", existingApiCheck.Spec, "new spec", newApiCheck.Spec) | ||
updateApiChecks = append(updateApiChecks, newApiChecksMap[existingApiCheck.Name]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateApiChecks = append(updateApiChecks, newApiChecksMap[existingApiCheck.Name]) | |
updateApiChecks = append(updateApiChecks, newApiCheck) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in latest commit.
Vulnerabilities reported will be fixed once #63 is merged. |
* Add ability to create API Checks based on ingress resource host and path entries * Determine how internal resources are linked together * Add logic to add / update / delete internal ApiCheck resources * Small code improvements
* Updated based on the new code updates
* ghcr.io is getting constant rate limits
* if the resource annotation is no longer present * if the enabled annotation is changed to false
* Add tests to check if the annotation takes presedence over the ingress resource `Path` options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, with the usual disclaimer.
Thanks for the added tests too. They're going to make future approvals easier.
🎉 This PR is included in version 1.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
path entries