Skip to content

Commit

Permalink
Fix webhook bugs found during functional testing
Browse files Browse the repository at this point in the history 
1: Started logging the return value of the HTTPS server
2: Changed default bind-address value from "0.0.0.0" to empty string
3: Changed to json.Decoder and started hard-enforcing total adherence to DanmNet schema (no unknown fields are allowed)
4: Added possibility to patch NetworkType parameter (setting to default "ipvlan")
5: To be able to handle modify operations, we only change Alloc if it was previously empty
6: Patch needs to be added when orig and new value are NOT equal :)
7: Value of patch needs to be force-enclosed with quotes, otherwise JSON marshalling fails
8: Other returns reason, and descriptive error logs to some previously not tracked execution paths
9: Alloc start and end were not properly set into the changedNetwork due to copy issues
10: Options related patching operations were changed to "replace", and use upper-case spelling to satisfy json-patch code
11: Added a new validation against manually setting Alloc
12: Dictionary type fields like Allocation_Pool needs to be replaced unblock if any of its fields changed
13: Webhook was coring because we tried to create allocation array even for empty CIDRs
14: Start being smaller than end check was not working due to substracting two uint32s from each other apparently does not work in Golang?
  • Loading branch information
@Levovar
Levovar committed May 17, 2019
1 parent a9e5b8a commit b334ce9
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 44 deletions.
8 changes: 5 additions & 3 deletions cmd/webhook/webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import (
func main() {
cert := flag.String("tls-cert-bundle", "", "File containing the x509 Certificate for HTTPS. (CA cert, if any, concatenated after server cert).")
key := flag.String("tls-private-key-file", "", "File containing the x509 private key matching --tls-cert-bundle.")
port := flag.Int("bind-port", 443, "The port on which to serve. Default is 443.")
address := flag.String("bind-address", "0.0.0.0", "The IP address on which to listen. Default is all interfaces.")
port := flag.Int("bind-port", 8443, "The port on which to serve. Default is 8443.")
address := flag.String("bind-address", "", "The IP address on which to listen. Default is all interfaces.")
flag.Parse()
if cert == nil || key == nil {
log.Println("ERROR: Configuring TLS is mandatory, --tls-cert-bundle and --tls-private-key-file cannot be empty!")
Expand All @@ -32,5 +32,7 @@ func main() {
ReadTimeout: 5 * time.Second,
WriteTimeout: 5 * time.Second,
}
server.ListenAndServeTLS("", "")
log.Println("INFO:DANM webhook is about to start listening on " + *address + ":" + strconv.Itoa(*port))
err = server.ListenAndServeTLS("", "")
log.Fatal(err)
}
77 changes: 50 additions & 27 deletions pkg/netadmit/netadmit.go
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package netadmit

import (
"bytes"
"errors"
"fmt"
"log"
"net"
"reflect"
"encoding/json"
"io/ioutil"
"net/http"
Expand All @@ -19,9 +22,9 @@ import (

var (
NetworkPatchPaths = map[string]string {
"Alloc": "/spec/options/alloc",
"Start": "/spec/options/allocation_pool/start",
"End": "/spec/options/allocation_pool/end",
"NetworkType": "/spec/NetworkType",
"Alloc": "/spec/Options/alloc",
"Pool": "/spec/Options/allocation_pool",
}
)

Expand All @@ -32,32 +35,39 @@ type Patch struct {
}

func ValidateNetwork(responseWriter http.ResponseWriter, request *http.Request) {
log.Println("INFO: got a request")
admissionReview, err := DecodeAdmissionReview(request)
if err != nil {
SendErroneousAdmissionResponse(responseWriter, admissionReview.Request.UID, err)
return
}
log.Println("INFO: after decode")
manifest, err := getNetworkManifest(admissionReview.Request.Object.Raw)
if err != nil {
SendErroneousAdmissionResponse(responseWriter, admissionReview.Request.UID, err)
return
}
log.Println("INFO: after get manifest")
origManifest := *manifest
isManifestValid, err := validateNetworkByType(manifest)
isManifestValid, err := validateNetworkByType(manifest, request.Method)
if !isManifestValid {
SendErroneousAdmissionResponse(responseWriter, admissionReview.Request.UID, err)
return
}
log.Println("INFO: after validate")
err = mutateManifest(manifest)
if err != nil {
SendErroneousAdmissionResponse(responseWriter, admissionReview.Request.UID, err)
return
}
log.Println("INFO: after mutate")
responseAdmissionReview := v1beta1.AdmissionReview {
Response: CreateReviewResponseFromPatches(createPatchListFromChanges(origManifest,manifest)),
}
responseAdmissionReview.Response.UID = admissionReview.Request.UID
fmt.Printf("This is the response we gonna send: %+v\n", responseAdmissionReview)
SendAdmissionResponse(responseWriter, responseAdmissionReview)
log.Println("INFO: we have sent a successful answer!")
}

func DecodeAdmissionReview(httpRequest *http.Request) (*v1beta1.AdmissionReview,error) {
Expand All @@ -77,6 +87,7 @@ func DecodeAdmissionReview(httpRequest *http.Request) (*v1beta1.AdmissionReview,
}

func SendErroneousAdmissionResponse(responseWriter http.ResponseWriter, uid types.UID, err error) {
log.Println("ERROR: Admitting resource failed with error:" + err.Error())
failedResponse := &v1beta1.AdmissionResponse {
Result: &metav1.Status{
Message: err.Error(),
Expand Down Expand Up @@ -104,15 +115,21 @@ func SendAdmissionResponse(responseWriter http.ResponseWriter, reviewResponse v1

func getNetworkManifest(objectToReview []byte) (*danmtypes.DanmNet,error) {
networkManifest := danmtypes.DanmNet{}
err := json.Unmarshal(objectToReview, &networkManifest)
return &networkManifest, err
decoder := json.NewDecoder(bytes.NewReader(objectToReview))
//We are using Decoder interface, because it can notify us if any unknown fields were put into the object
decoder.DisallowUnknownFields()
err := decoder.Decode(&networkManifest)
if err != nil {
return nil, errors.New("ERROR: unknown fields are not allowed:" + err.Error())
}
return &networkManifest, nil
}

func validateNetworkByType(manifest *danmtypes.DanmNet) (bool,error) {
func validateNetworkByType(manifest *danmtypes.DanmNet, httpMethod string) (bool,error) {
for _, validatorMapping := range danmValidationConfig.ValidatorMappings {
if validatorMapping.ApiType == manifest.TypeMeta.Kind {
for _, validator := range validatorMapping.Validators {
err := validator(manifest)
err := validator(manifest,httpMethod)
if err != nil {
return false, err
}
Expand All @@ -123,25 +140,26 @@ func validateNetworkByType(manifest *danmtypes.DanmNet) (bool,error) {
}

func mutateManifest(dnet *danmtypes.DanmNet) error {
allocationArray, err := CreateAllocationArray(dnet)
if err != nil {
return err
}
dnet.Spec.Options.Alloc = allocationArray.Encode()
if dnet.Spec.NetworkType == "" {
dnet.Spec.NetworkType = "ipvlan"
}
return nil
var err error
//L3, freshly added network
if dnet.Spec.Options.Cidr != "" && dnet.Spec.Options.Alloc == "" {
err = CreateAllocationArray(dnet)
}
return err
}

func CreateAllocationArray(dnet *danmtypes.DanmNet) (*bitarray.BitArray,error) {
func CreateAllocationArray(dnet *danmtypes.DanmNet) error {
_,ipnet,_ := net.ParseCIDR(dnet.Spec.Options.Cidr)
bitArray, err := bitarray.CreateBitArrayFromIpnet(ipnet)
if err != nil {
return nil, err
return err
}
reserveGatewayIps(dnet.Spec.Options.Routes, bitArray, ipnet)
return bitArray, nil
dnet.Spec.Options.Alloc = bitArray.Encode()
return nil
}

func reserveGatewayIps(routes map[string]string, bitArray *bitarray.BitArray, ipnet *net.IPNet) {
Expand All @@ -159,6 +177,7 @@ func CreateReviewResponseFromPatches(patchList []Patch) *v1beta1.AdmissionRespon
patches, err = json.Marshal(patchList)
if err != nil {
reviewResponse.Allowed = false
reviewResponse.Result = &metav1.Status{ Message: "List of patches could not be encoded, because:" + err.Error(),}
return &reviewResponse
}
}
Expand All @@ -172,24 +191,28 @@ func CreateReviewResponseFromPatches(patchList []Patch) *v1beta1.AdmissionRespon

func createPatchListFromChanges(origNetwork danmtypes.DanmNet, changedNetwork *danmtypes.DanmNet) []Patch {
patchList := make([]Patch, 0)
if origNetwork.Spec.Options.Alloc == changedNetwork.Spec.Options.Alloc {
if origNetwork.Spec.Options.Alloc != changedNetwork.Spec.Options.Alloc {
//TODO: Use some reflecting here to determine name of the struct field
patchList = append(patchList, CreatePatchFromChange(NetworkPatchPaths, "Alloc", changedNetwork.Spec.Options.Alloc))
patchList = append(patchList, CreateGenericPatchFromChange(NetworkPatchPaths, "Alloc",
json.RawMessage(`"` + changedNetwork.Spec.Options.Alloc + `"`)))
}
if origNetwork.Spec.Options.Pool.Start == changedNetwork.Spec.Options.Pool.Start {
patchList = append(patchList, CreatePatchFromChange(NetworkPatchPaths, "Start", changedNetwork.Spec.Options.Pool.Start))
if origNetwork.Spec.NetworkType != changedNetwork.Spec.NetworkType {
patchList = append(patchList, CreateGenericPatchFromChange(NetworkPatchPaths, "NetworkType",
json.RawMessage(`"` + changedNetwork.Spec.NetworkType + `"`)))
}
if origNetwork.Spec.Options.Pool.End == changedNetwork.Spec.Options.Pool.End {
patchList = append(patchList, CreatePatchFromChange(NetworkPatchPaths, "End", changedNetwork.Spec.Options.Pool.End))
if !reflect.DeepEqual(origNetwork.Spec.Options.Pool, changedNetwork.Spec.Options.Pool) {
patchList = append(patchList, CreateGenericPatchFromChange(NetworkPatchPaths, "Pool",
json.RawMessage(`{"Start":"` + changedNetwork.Spec.Options.Pool.Start + `","End":"` + changedNetwork.Spec.Options.Pool.End + `"}`)))
}
return patchList
}

func CreatePatchFromChange(attributePaths map[string]string, attribute, value string ) Patch {
func CreateGenericPatchFromChange(attributePaths map[string]string, attribute string, value []byte ) Patch {
patch := Patch {
Op: "add",
Op: "replace",
Path: attributePaths[attribute],
Value: json.RawMessage(value),
Value: value,
}
fmt.Printf("This is a patch we want to send: %+v\n", patch)
return patch
}
}
39 changes: 25 additions & 14 deletions pkg/netadmit/validators.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@ package netadmit

import (
"errors"
"log"
"net"
"strconv"
"strings"
"encoding/binary"
"net/http"
danmtypes "github.com/nokia/danm/crd/apis/danm/v1"
"github.com/nokia/danm/pkg/ipam"
)

type Validator func(netInfo *danmtypes.DanmNet) error
type Validator func(netInfo *danmtypes.DanmNet, httpMethod string) error

type ValidatorConfig struct {
ValidatorMappings []ValidatorMapping
Expand All @@ -33,11 +37,11 @@ var (
}
)

func validateIpv4Fields(dnet *danmtypes.DanmNet) error {
func validateIpv4Fields(dnet *danmtypes.DanmNet, httpMethod string) error {
return validateIpFields(dnet.Spec.Options.Cidr, dnet.Spec.Options.Routes)
}

func validateIpv6Fields(dnet *danmtypes.DanmNet) error {
func validateIpv6Fields(dnet *danmtypes.DanmNet, httpMethod string) error {
return validateIpFields(dnet.Spec.Options.Net6, dnet.Spec.Options.Routes6)
}

Expand All @@ -60,12 +64,18 @@ func validateIpFields(cidr string, routes map[string]string) error {
return nil
}

func validateAllocationPool(dnet *danmtypes.DanmNet) error {
func validateAllocationPool(dnet *danmtypes.DanmNet, httpMethod string) error {
log.Println("HTTP method was:" + httpMethod)
log.Println("HTTP method constant is:" + http.MethodPost)
log.Println("Alloc was:" + dnet.Spec.Options.Alloc)
log.Println("Strings compare res:" + strconv.Itoa(strings.Compare(httpMethod,http.MethodPost)))
//For some reason "==" didn't think the strings were equal
if (strings.Compare(httpMethod,http.MethodPost) == 0) && dnet.Spec.Options.Alloc != "" {
errors.New("Alloc field shall not be manually defined!")
}
cidr := dnet.Spec.Options.Cidr
apStart := dnet.Spec.Options.Pool.Start
apEnd := dnet.Spec.Options.Pool.End
if cidr == "" {
if apStart != "" || apEnd != "" {
if dnet.Spec.Options.Pool.Start != "" || dnet.Spec.Options.Pool.End != "" {
return errors.New("Allocation pool cannot be defined without CIDR!")
}
return nil
Expand All @@ -74,17 +84,18 @@ func validateAllocationPool(dnet *danmtypes.DanmNet) error {
if err != nil {
return errors.New("Invalid CIDR parameter: " + cidr)
}
if apStart == "" {
if dnet.Spec.Options.Pool.Start == "" {
dnet.Spec.Options.Pool.Start = (ipam.Int2ip(ipam.Ip2int(ipnet.IP) + 1)).String()
}
if apEnd == "" {
if dnet.Spec.Options.Pool.End == "" {
dnet.Spec.Options.Pool.End = (ipam.Int2ip(ipam.Ip2int(GetBroadcastAddress(ipnet)) - 1)).String()
}
if !ipnet.Contains(net.ParseIP(apStart)) || !ipnet.Contains(net.ParseIP(apEnd)) {
if !ipnet.Contains(net.ParseIP(dnet.Spec.Options.Pool.Start)) || !ipnet.Contains(net.ParseIP(dnet.Spec.Options.Pool.End)) {
return errors.New("Allocation pool is outside of defined CIDR")
}
if ipam.Ip2int(net.ParseIP(apEnd)) - ipam.Ip2int(net.ParseIP(apStart)) <= 0 {
return errors.New("Allocation pool start:" + apStart + " is bigger than end:" + apEnd)
log.Println("End IP:" + strconv.FormatUint(uint64(ipam.Ip2int(net.ParseIP(dnet.Spec.Options.Pool.End))),10) + " Start IP:" + strconv.FormatUint(uint64(ipam.Ip2int(net.ParseIP(dnet.Spec.Options.Pool.Start))),10))
if ipam.Ip2int(net.ParseIP(dnet.Spec.Options.Pool.End)) <= ipam.Ip2int(net.ParseIP(dnet.Spec.Options.Pool.Start)) {
return errors.New("Allocation pool start:" + dnet.Spec.Options.Pool.Start + " is bigger than or equal to allocation pool end:" + dnet.Spec.Options.Pool.End)
}
return nil
}
Expand All @@ -96,7 +107,7 @@ func GetBroadcastAddress(subnet *net.IPNet) (net.IP) {
return ip
}

func validateVids(dnet *danmtypes.DanmNet) error {
func validateVids(dnet *danmtypes.DanmNet, httpMethod string) error {
isVlanDefined := (dnet.Spec.Options.Vlan!=0)
isVxlanDefined := (dnet.Spec.Options.Vxlan!=0)
if isVlanDefined && isVxlanDefined {
Expand All @@ -105,7 +116,7 @@ func validateVids(dnet *danmtypes.DanmNet) error {
return nil
}

func validateNetworkId(dnet *danmtypes.DanmNet) error {
func validateNetworkId(dnet *danmtypes.DanmNet, httpMethod string) error {
if dnet.Spec.NetworkID == "" {
return errors.New("Spec.NetworkID mandatory parameter is missing!")
}
Expand Down

0 comments on commit b334ce9

Please sign in to comment.