Create K8s deployment yaml to deploy ipfix-collector #159

heanlan · 2021-04-03T01:51:49Z

Add templates and configuration files to generate the collector manifest
Add scripts for preparing and uploading release assets
Add corresponding guidance in README to deploy collector

Fixes: #95

codecov · 2021-04-03T01:52:39Z

Codecov Report

Merging #159 (eb4e42b) into main (3b4806b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #159   +/-   ##
=======================================
  Coverage   78.88%   78.88%           
=======================================
  Files          16       16           
  Lines        2008     2008           
=======================================
  Hits         1584     1584           
  Misses        280      280           
  Partials      144      144

Flag	Coverage Δ
integration-tests	`62.78% <ø> (-0.35%)`	⬇️
unit-tests	`78.32% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

srikartati · 2021-04-05T18:08:02Z

README.md

@@ -6,6 +6,11 @@ go-ipfix is an IPFIX library that can be used to implement an IPFIX exporter, wh
 ## Try it out
 This IPFIX library can be used to build an exporter. Please check out the [exporter tests](https://github.com/vmware/go-ipfix/blob/main/pkg/exporter/process_test.go) to get an idea on how to build exporter on top of TCP and UDP transport protocols given a IPFIX collector.

+To deploy a standalone go-ipfix collector, which is used to decode and log the IPFIX records, you can use:


Better to have a sub-section like "Deploy Go-IPFIX Collector"

Will do. Thanks!

srikartati · 2021-04-05T18:32:03Z

build/yamls/ipfix-collector.yml

+        app: ipfix-collector
+    spec:
+      containers:
+      - image: projects.registry.vmware.com/antrea/ipfix-collector:v0.4.8


how will the versioning work? I think our intention should not be to make users change this file manually.
One option is to use kustomize. We use that in Antrea. Please see this file for reference: https://github.com/vmware-tanzu/antrea/blob/main/hack/generate-manifest.sh
See the mode option with dev or release as possible values.

Thanks. Just to clarify, dev mode uses the "latest" image tag, while release mode uses the release version as image tag. Is that what you expect?

Yes, dev mode for latest tag with ToT main branch and release mode will have a release version image in the manifest.

@srikartati How about using a script for replacing latest with version when uploading release assets and keep latest in dev code? It will have same effect as Antrea. And we can add guide for deploying ipfix collector like this. Our yaml file is quite simple for now, we can probably support kustomization later.

Sure a script to replace yaml is ok. I think we may need a few more options in the script like collector address, port, protocol, etc. However, I do not think it will blow up (at least for now) where we require kustomize tool .

It is better to add the protocol editing option along with the image tag option in the script.

srikartati · 2021-04-05T18:53:03Z

build/yamls/ipfix-collector.yml

+  selector:
+    app: ipfix-collector
+  ports:
+    - protocol: TCP


We support UDP too.
https://github.com/vmware/go-ipfix/blob/main/cmd/collector/collector.go#L60

Do we have to add a configmap to support UDP?

Command argument --ipfix.transport="udp" in the k8s manifest should work.

It looks like in order to make it work, I have to modify the Dockerfile.build.collector.
Line19 is changed from:
ENTRYPOINT "collector"
to:
ENTRYPOINT ["collector"]
And now collector.go can effectively take arguments.

Thanks for trying it out and figuring out the fix.

dreamtalen · 2021-04-15T19:02:49Z

When using ipfix-collector on a cluster with ARM and Windows Nodes, Pods should be scheduled on Linux AMD64
Nodes. Could you please add the nodeSelector in the yaml file like this PR in antrea? antrea-io/antrea#2087

heanlan · 2021-04-15T19:13:23Z

When using ipfix-collector on a cluster with ARM and Windows Nodes, Pods should be scheduled on Linux AMD64
Nodes. Could you please add the nodeSelector in the yaml file like this PR in antrea? vmware-tanzu/antrea#2087

Thanks for the reminder! Added.

heanlan · 2021-04-15T19:16:05Z

Accidentally closed the PR, reopen now...

srikartati · 2021-04-26T19:40:56Z

Please rebase the commit with updated main branch.

zyiou · 2021-04-26T21:08:41Z

README.md

+To deploy a standalone go-ipfix collector, which is used to decode and log the IPFIX records, you can use:
+```
+cd <directory containing this README file>/build/yamls
+kubectl apply -f ./ipfix-collector.yaml


We can follow the way antrea does here: specify the release manifest here and if users want to customize the protocol etc., they can download the folder from main branch.

Absolutely. The only concern I had before is that users will not be able to find any manifests under "releases assets" since we do not have the manifest for previous release. But I agree we can add the guide at this time for future need. Thanks!

zyiou · 2021-04-26T21:09:51Z

README.md

+
+```
+cd <directory containing this README file>/hack
+./generate-manifest-collector.sh --mode dev --addr "port:proto" > ../build/yamls/ipfix-collector.yaml


I think using separate params here would be better: --port 4739 --protocol tcp

Yes. I was doing this because originally we have three arguments addr:port:proto. I feel like four separate arguments (including --mode) might be too long. Then we remove the addr arg. Now I'm waiting for Srikar's opinion on whether we should keep the port argument or not.

zyiou · 2021-04-26T21:10:15Z

build/yamls/collector/Chart.yaml

@@ -0,0 +1,3 @@
+apiVersion: v2
+name: ipfix-collector
+version: 0.5.0


Fixed. Thanks.

zyiou · 2021-04-26T21:12:33Z

hack/.bin/darwin-amd64/LICENSE

@@ -0,0 +1,202 @@
+


Out of curiosity, are files in hack/.bin necessary for us to deploy ipfix-collector?

You're right. I shouldn't commit those to the repo. The verify-helm.sh will install Helm locally.

srikartati

I think we need a github job to make sure that the manifest is up-to-date if there is any change to the template. Look for "make manifest" and related github job in Antrea.

srikartati · 2021-04-26T23:57:12Z

.github/workflows/upload_release_assets.yml

+          TAG: ${{ github.ref }}
+        run: |
+          mkdir assets
+          VERSION="${TAG:10}" ./hack/release/prepare-assets.sh ./assets
      - name: Checkout
        uses: actions/checkout@v2


I do not think we need collector binary anymore as we have the Kubernetes manifest to deploy the go-ipfix collector. We can remove these. @zyiou do you agree?

yes, we don't need collector binary.

srikartati · 2021-04-26T23:59:14Z

build/yamls/collector/Chart.yaml

@@ -0,0 +1,3 @@
+apiVersion: v2
+name: ipfix-collector
+version: 0.5.0


shouldn't this version be generic one?

I have added something to update the version number when we have a new release. It will be updated to the most updated release tag. Is that what you are suggesting?

My understanding is that this is a template and we can use a generic value like <x.x.x>.

Btw, why does the name has to start with capital--"Chart.yaml"?
In addition, I see that Chart.yaml is not at all used when the mode is dev during the time of generation. Is this expected?

Based on my understanding of the documentation https://helm.sh/docs/topics/charts/, under the "chart and versioning" subtitle, the overall structured file collection (Chart.yaml values.yaml templates folder etc) is called a chart, and the version implies the version of the chart. It is used when we apply command like helm package, which package a chart directory into a chart archive. So the value is decided by the contents of the current project. There are words like "Helm v2 and later uses version numbers as release markers. Packages in repositories are identified by name plus version.". I'm not so sure on how should we assign the value of version, but I feel like the value is supposed to be meaningful.

The naming and existence of Chart.yaml is also part of the Helm chart structure in order to make it configurable. https://helm.sh/docs/chart_template_guide/getting_started/
And it is case sensitive https://helm.sh/docs/chart_best_practices/conventions/#usage-of-the-words-helm-and-chart

Thanks for the explanation. Can the version be taken from here? https://github.com/vmware/go-ipfix/blob/main/VERSION
This can be edited too when generating manifest.

Good to know that Chart has to case sensitive.

Sure. Two questions:

I'm assuming you are saying we will edit the file VERSION and Chart.yaml in the generate-manifest script. From where shall we get the version value(0.6.0) to put in VERSION? Just do release version +1? Like 0.5.0 -> 0.6.0?

When shall we update the version value? Every time we have a new release we also upgrade the dev version by 0.1.0?

No, I meant you should edit the Chart.yaml with the version from https://github.com/vmware/go-ipfix/blob/main/VERSION when you are generating the collector manifest using helm tool. Isn't that possible?

That's definitely possible. I'll do that. I thought you want the VERSION file also to be edited in the generate-manifest script.

srikartati · 2021-04-27T00:02:18Z

build/yamls/collector/templates/ipfix-collector.yaml

@@ -0,0 +1,57 @@
+apiVersion: v1


Lets use "template" or "base" instead of "templates". There is only one template.

I also feel uncomfortable about that. But unfortunately the folder has to be named as "templates" to make it configurable. It has to follow the Helm chart structure as mentioned in the similar issue: helm/helm#4152
I have tried to rename it and the configuration doesn't work as a result. I'm thinking maybe we can still switch back to kustomize in the future if there's a lot of inconvenience.

Sounds good. I was not aware that it is the requirement of the helm. We can keep as is.

srikartati · 2021-04-27T00:21:18Z

hack/generate-manifest-collector.sh

+
+This tool uses helm (https://github.com/helm/helm) to generate templated manifests for
+running ipfix collector. You can set the HELM environment variable to the path of the
+helm binary you want us to use. Otherwise we will look for kustomize in your PATH and your


Fixed. Thanks.

srikartati · 2021-04-27T01:41:04Z

hack/generate-manifest-collector.sh

+    sed -i.bak -e "s/^image_tag.*/image_tag: $IMG_TAG/g" values.yaml
+
+    $HELM template "." -f "./templates/ipfix-collector.yaml"
+    cp -f "values.yaml.bak" "values.yaml"


do you require this step? Why do we have to keep the values.yml updated?

These lines will keep values.yaml from being updated. It creates a backup of the original values.yaml, makes changes to the newly-generated values.yaml, generates templated ipfix-collector.yaml using the newly-generated values.yaml, and restore the backup one.

Sounds good.

srikartati · 2021-04-27T01:46:30Z

build/yamls/collector/Chart.yaml

@@ -0,0 +1,3 @@
+apiVersion: v2
+name: ipfix-collector
+version: 0.5.0


My understanding is that this is a template and we can use a generic value like <x.x.x>.

Btw, why does the name has to start with capital--"Chart.yaml"?
In addition, I see that Chart.yaml is not at all used when the mode is dev during the time of generation. Is this expected?

heanlan · 2021-04-27T17:30:08Z

I think we need a github job to make sure that the manifest is up-to-date if there is any change to the template. Look for "make manifest" and related github job in Antrea.

Added.

heanlan · 2021-04-27T20:39:57Z

hack/generate-manifest-collector.sh

+# to support sed to run both on OSX, Linux and windows
+if [[ "$OSTYPE" == "darwin"* ]]; then
+    if [ "$MODE" == "dev" ]; then
+        sed -i '' -e "s/^image_tag.*/image_tag: latest/g" values.yaml
+        if [[ $PORT != "" ]]; then
+            sed -i '' -e "s/^port.*/port: $PORT/g" values.yaml
+        fi
+        if [[ $PROTO != "" ]]; then
+            sed -i '' -e "s/^protocol.*/protocol: $PROTO/g" values.yaml
+        fi
+        $HELM template "." -f "./templates/ipfix-collector.yaml"
+    fi
+
+    if [ "$MODE" == "release" ]; then
+        # update the Chart version by the newest release version
+        sed -i '' -e "s/^version.*/version: ${IMG_TAG:1}/g" Chart.yaml
+        # replace the line starting with "image_tag", and create a backup
+        sed -i.bak -e "s/^image_tag.*/image_tag: $IMG_TAG/g" values.yaml
+
+        $HELM template "." -f "./templates/ipfix-collector.yaml"
+        cp -f "values.yaml.bak" "values.yaml"
+        rm "values.yaml.bak"
+    fi
+else
+    if [ "$MODE" == "dev" ]; then
+        sed -i -e "s/^image_tag.*/image_tag: latest/g" values.yaml
+        if [[ $PORT != "" ]]; then
+            sed -i -e "s/^port.*/port: $PORT/g" values.yaml
+        fi
+        if [[ $PROTO != "" ]]; then
+            sed -i -e "s/^protocol.*/protocol: $PROTO/g" values.yaml
+        fi
+        $HELM template "." -f "./templates/ipfix-collector.yaml"
+    fi
+
+    if [ "$MODE" == "release" ]; then
+        sed -i -e "s/^version.*/version: ${IMG_TAG:1}/g" Chart.yaml
+        sed -i.bak -e "s/^image_tag.*/image_tag: $IMG_TAG/g" values.yaml
+
+        $HELM template "." -f "./templates/ipfix-collector.yaml"
+        cp -f "values.yaml.bak" "values.yaml"
+        rm "values.yaml.bak"
+    fi
+fi


I can't figure out a good way to avoid the code repetition here. I tried a couple of ways but they didn't work.
For example introducing a variable SEDOPTION, and this would create an extra weird file named values.yaml''

if [[ "$OSTYPE" == "darwin"* ]]; then SEDOPTION="''" fi if [ "$MODE" == "dev" ]; then sed -i ${SEDOPTION} -e "s/^image_tag.*/image_tag: latest/g" values.yaml```

I am not a sed expert.
How about setting SEDOPTION="-i" or "-i ''" depending on OSTYPE?

Thanks Shih-Hao for the suggestion! I tried it but sadly the results is the same:( still bringing the extra file

Just curious, is extra file caused by "sed -i.bak" on line 114, 134 or elsewhere?

I'm testing it with command /generate-manifest-collector.sh --mode dev > build/yamls/ipfix-collector.yaml.
So I think it goes to line 100, which is the code below with the variable SEDOPTION="-i ''":
sed ${SEDOPTION} -e "s/^image_tag.*/image_tag: latest/g" values.yaml

I think I can understand the logic that sed will create a file ending with '', just like what I have in line 116, where sed will create a backup file values.yaml.bak. But the strange thing is, without the SEDOPTION, the original code on line 100 works well on mac and will not create the extra file, but with SEDOPTION, it creates the file.

Thanks for the info.
One dumb question. So we need to pass "" after -i on windows machine?

No, for MacOS we do sed -i '' -e, for Linux and Windows we do sed -i -e.

Got it now. Thanks.
Maybe we can always use a dummy backup file for all the sed commands (e.g. "sed -i.dummy ..."), except "sed -i.bak ...".
Then in the end, remove the dummy file.

Good idea! Thanks a lot. It works now.

srikartati · 2021-04-28T05:38:47Z

build/yamls/collector/templates/ipfix-collector.yaml

@@ -0,0 +1,57 @@
+apiVersion: v1


Sounds good. I was not aware that it is the requirement of the helm. We can keep as is.

srikartati · 2021-04-28T05:48:57Z

build/yamls/collector/Chart.yaml

@@ -0,0 +1,3 @@
+apiVersion: v2
+name: ipfix-collector
+version: 0.5.0


Thanks for the explanation. Can the version be taken from here? https://github.com/vmware/go-ipfix/blob/main/VERSION
This can be edited too when generating manifest.

Good to know that Chart has to case sensitive.

srikartati · 2021-04-28T06:02:45Z

hack/generate-manifest-collector.sh

+    sed -i.bak -e "s/^image_tag.*/image_tag: $IMG_TAG/g" values.yaml
+
+    $HELM template "." -f "./templates/ipfix-collector.yaml"
+    cp -f "values.yaml.bak" "values.yaml"


Sounds good.

srikartati · 2021-04-28T06:05:22Z

README.md

@@ -6,6 +6,35 @@ go-ipfix is an IPFIX library that can be used to implement an IPFIX exporter, wh
 ## Try it out
 This IPFIX library can be used to build an exporter. Please check out the [exporter tests](https://github.com/vmware/go-ipfix/blob/main/pkg/exporter/process_test.go) to get an idea on how to build exporter on top of TCP and UDP transport protocols given a IPFIX collector.

+### Deploy stand alone IPFIX collector
+To deploy a released version of go-ipfix collector, which is used to decode and log the IPFIX records, please choose one deployment manifest from the list of releases. For any given release <TAG> (e.g. v0.1.0), you can deploy the collector as follows:


"...the go-ipfix collector, which is used to collect, decode and log the IPFIX records.."

srikartati · 2021-04-28T06:06:00Z

README.md

+```
+kubectl apply -f https://github.com/vmware/go-ipfix/releases/download/<TAG>/ipfix-collector.yaml
+```
+To deploy the latest version of collector (built from the main branch), use the command below: 


..of the go-ipfix collector..

srikartati · 2021-04-28T06:07:10Z

README.md

+kubectl apply -f ./ipfix-collector.yaml
+```
+
+While deploying the latest version of collector, the port and protocol can be configured by using the command:


the go-ipfix collector, port and protocol...

srikartati

Thanks for adding this. LGTM.

srikartati · 2021-04-29T17:23:48Z

I see all comments are addressed. Merging now.

* add collector manifests templates * add github job to check manifest is up-to-date Co-authored-by: Anlan He <[email protected]>

vmwclabot added the cla-not-required label Apr 3, 2021

srikartati reviewed Apr 5, 2021

View reviewed changes

heanlan closed this Apr 15, 2021

heanlan reopened this Apr 15, 2021

heanlan force-pushed the ipfix-collector-yaml branch from ca06808 to 49d551e Compare April 25, 2021 22:21

heanlan force-pushed the ipfix-collector-yaml branch from 49d551e to ddfb300 Compare April 26, 2021 20:12

zyiou reviewed Apr 26, 2021

View reviewed changes

heanlan force-pushed the ipfix-collector-yaml branch from ddfb300 to 36160f4 Compare April 26, 2021 22:46

srikartati reviewed Apr 27, 2021

View reviewed changes

heanlan marked this pull request as draft April 27, 2021 00:51

add collector manifests templates

8a0ceb4

heanlan force-pushed the ipfix-collector-yaml branch from 36160f4 to 8a0ceb4 Compare April 27, 2021 01:27

heanlan marked this pull request as ready for review April 27, 2021 01:39

srikartati reviewed Apr 27, 2021

View reviewed changes

heanlan force-pushed the ipfix-collector-yaml branch 2 times, most recently from 67ed13e to a3beead Compare April 27, 2021 19:44

heanlan commented Apr 27, 2021

View reviewed changes

add github job to check manifest is up-to-date

5bb450d

heanlan force-pushed the ipfix-collector-yaml branch from a3beead to 5bb450d Compare April 28, 2021 01:05

srikartati reviewed Apr 28, 2021

View reviewed changes

resolve comments

eb4e42b

srikartati approved these changes Apr 29, 2021

View reviewed changes

srikartati merged commit 4364413 into vmware:main Apr 29, 2021

heanlan deleted the ipfix-collector-yaml branch April 30, 2021 16:25

heanlan mentioned this pull request May 2, 2021

Bug in update release assets workflow #177

Closed

zyiou pushed a commit to zyiou/go-ipfix that referenced this pull request May 13, 2021

Create K8s deployment yaml to deploy ipfix-collector (vmware#159)

f857d4a

* add collector manifests templates * add github job to check manifest is up-to-date Co-authored-by: Anlan He <[email protected]>

zyiou pushed a commit that referenced this pull request May 13, 2021

Create K8s deployment yaml to deploy ipfix-collector (#159)

c2ad878

* add collector manifests templates * add github job to check manifest is up-to-date Co-authored-by: Anlan He <[email protected]>

Create K8s deployment yaml to deploy ipfix-collector #159

Create K8s deployment yaml to deploy ipfix-collector #159

Conversation

heanlan commented Apr 3, 2021 • edited Loading

codecov bot commented Apr 3, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamtalen commented Apr 15, 2021

heanlan commented Apr 15, 2021 • edited Loading

heanlan commented Apr 15, 2021 • edited Loading

srikartati commented Apr 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heanlan Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heanlan commented Apr 27, 2021

heanlan Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heanlan Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati left a comment

Choose a reason for hiding this comment

srikartati commented Apr 29, 2021

heanlan commented Apr 3, 2021 •

edited

Loading

codecov bot commented Apr 3, 2021 •

edited

Loading

heanlan commented Apr 15, 2021 •

edited

Loading

heanlan commented Apr 15, 2021 •

edited

Loading

heanlan Apr 27, 2021 •

edited

Loading

heanlan Apr 27, 2021 •

edited

Loading

heanlan Apr 27, 2021 •

edited

Loading