-
Notifications
You must be signed in to change notification settings - Fork 0
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
Dev/rsearch #1
base: master
Are you sure you want to change the base?
Dev/rsearch #1
Changes from 14 commits
827d305
e14b248
5c70454
c67a610
0fd256a
ef6b920
d8a6dd9
f6df624
874e9ee
e165063
370dea5
7a4d9c9
35a61a8
f4f5939
4499626
44019a4
0edcdcd
f7f3702
4d62a2e
b87a4cc
773d8d6
6e60337
00be796
dc11e27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
vendor/ | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
1. When starting server read through namespace list and fire up ns.Producer per namespace. | ||
Read throug resource list per namespace and send them to Processor | ||
2. Import Kubernetes and use Objects from there |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
bin |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
package main | ||
|
||
import ( | ||
"flag" | ||
"fmt" | ||
search "github.com/romana/contrib/rsearch" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar for this, should potentially be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also why is the package being renamed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Historical reasons. Also i don't see any harm in it - does anyone feel like this need to be reverted back to package name ? Also, does anyone have ideas for better package name ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO it's surprising to be aliasing your own package, since you have the option of naming it differently. No better suggestions, just pick one and use it consistently. |
||
"log" | ||
// "net/http" | ||
"encoding/json" | ||
|
||
// "io" | ||
) | ||
|
||
func main() { | ||
var cfgFile = flag.String("c", "", "Kubernetes reverse search config file") | ||
var server = flag.Bool("s", false, "Start a server") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why isn't this just a server, and a separate tool for a client? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra package to maintain. Extra package to push. |
||
var host = flag.String("h", "", "Protocol://host for client to connect to") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A partial URL is neither a host or a URL. |
||
var searchTag = flag.String("r", "", "Search resources by tag") | ||
flag.Parse() | ||
|
||
done := make(chan search.Done) | ||
|
||
config, err := search.NewConfig(*cfgFile) | ||
if *host != "" { | ||
config.Server.Host = *host | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not an error check, though there should be one. |
||
} | ||
|
||
if err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could go directly below the point where |
||
fmt.Printf("Can not read config file %s, %s\n", *cfgFile, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some things use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only But maybe i was a bit less careful with main.go since it's just using the library. |
||
return | ||
} | ||
|
||
if *server { | ||
fmt.Println("Starting server") | ||
nsUrl := fmt.Sprintf("%s/%s", config.Api.Url, config.Api.NamespaceUrl) | ||
nsEvents, err := search.NsWatch(done, nsUrl, config) | ||
if err != nil { | ||
log.Fatal("Namespace watcher failed to start", err) | ||
} | ||
|
||
events := search.Conductor(nsEvents, done, config) | ||
req := search.Process(events, done, config) | ||
log.Println("All routines started") | ||
search.Serve(config, req) | ||
} else if len(*searchTag) > 0 { | ||
if config.Server.Debug { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And a server. |
||
fmt.Println("Making request t the server") | ||
} | ||
r := search.SearchResource(config, search.SearchRequest{Tag: *searchTag}) | ||
response, _ := json.Marshal(r) | ||
if err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||
panic(err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be log.Fatal instead. |
||
} | ||
fmt.Println(string(response)) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And if it were neither? |
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
#!/bin/bash | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing else has a build.sh, so this is a bit odd. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohh that's just a helper for myself - shouldn't be in the repo. |
||
find . -type d | while read line; do | ||
( echo Building in the $line ....; cd $line && go build .) | ||
done |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
// Copyright (c) 2016 Pani Networks | ||
// All Rights Reserved. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); you may | ||
// not use this file except in compliance with the License. You may obtain | ||
// a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
// License for the specific language governing permissions and limitations | ||
// under the License. | ||
|
||
package rsearch | ||
|
||
import ( | ||
"bytes" | ||
"encoding/json" | ||
"fmt" | ||
"log" | ||
"net/http" | ||
) | ||
|
||
// SearchResource connects to instance of a server | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instance of what server? |
||
// and resolves SearchRequest | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does "resolves SearchRequest" mean? And when I ask something like this, I mean: Please explain in the docstring. |
||
func SearchResource(config Config, req SearchRequest) SearchResponse { | ||
// TODO need to make url configurable | ||
url := config.Server.Host + ":" + config.Server.Port | ||
data := []byte(`{ "tag" : "` + req.Tag + `"}`) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something's happening with data, but I have no idea what's going on. Some comments would be really useful. |
||
if config.Server.Debug { | ||
log.Println("Making request with", string(data)) | ||
} | ||
|
||
// Make request | ||
request, err := http.NewRequest("POST", url, bytes.NewBuffer(data)) | ||
request.Header.Set("Content-Type", "application/json") | ||
client := &http.Client{} | ||
response, err := client.Do(request) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might as well use |
||
if err != nil { | ||
log.Println("HTTP request failed", url, err) | ||
panic(err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not log.Fatal? This is dumping the error twice and a stack trace. |
||
} | ||
|
||
defer response.Body.Close() | ||
decoder := json.NewDecoder(response.Body) | ||
sr := SearchResponse{} | ||
if config.Server.Debug { | ||
log.Println("Trying to decode", response.Body) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
err = decoder.Decode(&sr) | ||
if err != nil { | ||
log.Println("Failed to decode", response.Body) | ||
panic(err) | ||
} | ||
|
||
if config.Server.Debug { | ||
fmt.Println("Decoded response form a server", sr) | ||
} | ||
return sr | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
// Copyright (c) 2016 Pani Networks | ||
// All Rights Reserved. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); you may | ||
// not use this file except in compliance with the License. You may obtain | ||
// a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
// License for the specific language governing permissions and limitations | ||
// under the License. | ||
|
||
package rsearch | ||
|
||
func manageResources(ns Event, terminators map[string]chan Done, config Config, out chan Event) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docstring |
||
uid := ns.Object.Metadata.Uid | ||
if ns.Type == "ADDED" { | ||
done := make(chan Done) | ||
terminators[uid] = done | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if it already existed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't really run into such case but if we are we just update termination channel for such goroutine and let it's previous termination channel to be garbage collected. |
||
ns.Object.Produce(out, terminators[uid], config) | ||
} else if ns.Type == "DELETED" { | ||
close(terminators[uid]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if it doesn't exist? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't really happen but we will blow up in this case. However, spitting out readable message could be helpful. |
||
delete(terminators, uid) | ||
} else if ns.Type == "_CRASH" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be documented a bit better. It's not really a crash, but an internal management event when the connection went bad, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now it represents HTTP GET timeout which for us is a controlled disaster. But maybe i could turn it into a
what do you think ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need a new type for that. Normally, closing the channel would be a good way of communicating that. But the current implementation keeps it open even when the connection is re-established. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defining constants, rather than using string literals, is definitely an improvement. |
||
for uid, c := range terminators { | ||
close(c) | ||
delete(terminators, uid) | ||
} | ||
} | ||
} | ||
|
||
// Conductor manages a set of goroutines one per namespace. | ||
func Conductor(in <-chan Event, done <-chan Done, config Config) <-chan Event { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lots of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know, i don't see a better way of doing it since each goroutine must receive some There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For starters, you might add a comment that explains what's happening. Then this could also help to clear up 'done' ambiguities. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
// Idea of this map is to keep termination channels organized | ||
// so when DELETED event occurs on a namespace it would be possible | ||
// to terminater related goroutine | ||
var terminators map[string]chan Done | ||
terminators = make(map[string]chan Done) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's wrong with make ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't about avoiding make, but avoiding 'var' and repetition of the type. |
||
|
||
ns := Event{} | ||
out := make(chan Event) | ||
|
||
go func() { | ||
for { | ||
select { | ||
case ns = <-in: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is deliberate to show what kind of things coming out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't the declaration of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically yes. I just like it when i read it. I understand it's unnecessary, do you think it's also wrong thing to do ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not wrong, it was just surprising. It's a subjective thing. |
||
manageResources(ns, terminators, config, out) | ||
case <-done: | ||
return | ||
} | ||
} | ||
}() | ||
|
||
return out | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
// Copyright (c) 2016 Pani Networks | ||
// All Rights Reserved. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); you may | ||
// not use this file except in compliance with the License. You may obtain | ||
// a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
// License for the specific language governing permissions and limitations | ||
// under the License. | ||
|
||
package rsearch | ||
|
||
import ( | ||
"gopkg.in/gcfg.v1" | ||
) | ||
|
||
// Done is an alias for empty struct, used for termination channels | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a good habit to alias the types if you're not adding methods. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For that particular use case it's documented practice. https://blog.golang.org/pipelines
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is a "termination channel"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a pattern of sending broadcast message by means of closing a shared channel. There are no process management and only 2 ways of communicating. Shared state variables or channels. Shared state variables are bad so channels if obvious way. The problem with channels is that you can't send message to all gorotines listening on a channel. Termination channel or "done" channel is a pattern of creating a channel of nil types and have arbitrary number of goroutines sitting blocked on this channel forever. When you want to send message you close the channel and all goroutines receive broadcast message that channel was closed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Can you explain that in some comments, please? |
||
type Done struct{} | ||
|
||
// Config is a top level struct describing expected structure of config file. | ||
type Config struct { | ||
Resource Resource | ||
Server Server | ||
Api Api | ||
} | ||
|
||
// Server is a config section describing server instance of this package. | ||
type Server struct { | ||
Port string | ||
Host string | ||
Debug bool | ||
} | ||
|
||
// API is a config section describing kubernetes API parameters. | ||
type Api struct { | ||
Url string | ||
NamespaceUrl string | ||
} | ||
|
||
// Resource is a config section describing kubernetes resource to be cached and searched for. | ||
type Resource struct { | ||
Name string | ||
Type string | ||
Selector string | ||
Namespaced string | ||
UrlPrefix string | ||
UrlPostfix string | ||
} | ||
|
||
// NewConfig parsing config file and returning initialized instance of Config structure | ||
func NewConfig(configFile string) (Config, error) { | ||
cfg := Config{} | ||
if err := gcfg.ReadFileInto(&cfg, configFile); err != nil { | ||
return cfg, err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to the other comment. What state is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had an issue where it wouldn't accept There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, you'd have to return a zero-ed value of Config, ie: |
||
} | ||
|
||
return cfg, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
; comment | ||
[api] | ||
url=http://192.168.0.10:8080 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't this just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kube api url does change, namespace endpoint might not change but we want to be able to define api url separately as it's going to be used in few places. |
||
namespaceUrl=api/v1/namespaces/?watch=true | ||
|
||
[resource] | ||
;type=builtin # TBD | ||
type=3rdParty | ||
;namespaced=false # TBD | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add some comments after or before each config item to explain what the config is for? For example, it's not clear what "namespaced" means. |
||
namespaced=true | ||
urlPrefix=apis/romana.io/demo/v1/namespaces | ||
urlPostfix=networkpolicys/?watch=true | ||
name=NetworkPolicy | ||
selector=podSelector | ||
|
||
[server] | ||
port=9700 | ||
debug=false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Copyright (c) 2016 Pani Networks | ||
// All Rights Reserved. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); you may | ||
// not use this file except in compliance with the License. You may obtain | ||
// a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
// License for the specific language governing permissions and limitations | ||
// under the License. | ||
|
||
// This package prvides means of searching kubernets objects by their selectors | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spelling, and this becomes a single sentence in documentation, ie: it'll appear as "...selectors the goal is..." |
||
// the goal is to give the answer to a question - which resources are selecting pods with given label | ||
package rsearch |
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.
They seem to use godeps instead of vendoring, so this probably isn't needed.
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 maintain godep directory, but with VENDOREXPERIMENT
vendor/
folder gets created automatically. This line is to prevent accidental commits including this folder.