Adds the configmap for naming the security group #111

Nithish555 · 2023-01-26T11:07:44Z

The configmap conatins the name prefix for the security group

Signed-off-by: nithishs nithishs@vmware.com

codecov · 2023-01-26T11:16:09Z

Codecov Report

Merging #111 (22b1c12) into main (7f2e609) will decrease coverage by 0.10%.
The diff coverage is 53.63%.

❗ Current head 22b1c12 differs from pull request most recent head abd6568. Consider uploading reports for the commit abd6568 to get more accurate results

@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
- Coverage   58.20%   58.11%   -0.10%     
==========================================
  Files          50       52       +2     
  Lines        8248     8361     +113     
==========================================
+ Hits         4801     4859      +58     
- Misses       2959     3003      +44     
- Partials      488      499      +11

Flag	Coverage Δ
unit-tests	`58.11% <53.63%> (-0.10%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/controllers/cloud/networkpolicy_controller.go	`51.86% <33.33%> (-0.19%)`	⬇️
pkg/apiserver/webhook/configmap_webhook.go	`50.72% <50.72%> (ø)`
pkg/controllers/cloud/configmap_controller.go	`60.52% <60.52%> (ø)`
pkg/cloud-provider/cloudapi/aws/aws_ec2.go	`63.42% <0.00%> (-0.84%)`	⬇️
pkg/controllers/cloud/networkpolicy.go	`69.40% <0.00%> (-0.27%)`	⬇️
pkg/converter/source/virtualmachine_converter.go	`85.38% <0.00%> (-0.23%)`	⬇️
pkg/controllers/cloud/virtualmachine_controller.go	`50.87% <0.00%> (ø)`
pkg/cloud-provider/cloudapi/aws/aws_security.go	`50.50% <0.00%> (+0.71%)`	⬆️

pkg/cloud-provider/securitygroup/securitygroup.go

config/nephe.yml

Anandkumar26 · 2023-01-30T07:13:37Z

config/nephe.yml

+  namespace: nephe-system
+data:
+  nephe-controller.conf: |
+    ResourcePrefix: anp-


By default it should be commented out and default value should be nephe

Suggested change

ResourcePrefix: anp-

#ResourcePrefix: nephe

I see that the value is not commented out.

Because the nephe is the default value.

imo we should add the - separator in the AddResourcePrefix func or in options.go setting options, since I don't think we should let user customize separators, or it might cause errors if it's some cloud not allowed character. We can also have a separate const defaultResourceSeparator?
@reachjainrahul @Anandkumar26 Should we have some checks on the prefix validity in the webhook, like no cloud not allowed character, no special character etc.?

cmd/nephe-controller/main.go

config/nephe.yml

shenmo3

Minor nits, up to you.

shenmo3 · 2023-01-31T09:03:18Z

cmd/nephe-controller/options.go

+)
+
+const (
+	namePrefix = "nephe-"


nit: defaultResourcePrefix?

Try to be consistent on this option var name across all files.

shenmo3 · 2023-01-31T09:05:05Z

cmd/nephe-controller/options.go

+
+func newOptions() *Options {
+	return &Options{
+		config: &ControllerConfig{},


Maybe we can set the default values here then return. This saves checks for setting default later in complete() and imo its more readable.

reachjainrahul · 2023-01-31T10:41:34Z

cmd/nephe-controller/config.go

@@ -14,6 +14,10 @@

 package main

+type ControllerConfig struct {
+	ResourcePrefix string `yaml:"ResourcePrefix,omitempty"`


CloudResourcePrefix

reachjainrahul · 2023-01-31T10:41:56Z

cmd/nephe-controller/main.go


+	flag.StringVar(&opts.configFile, "config", opts.configFile, "The path to the configuration file")


reachjainrahul · 2023-01-31T10:43:31Z

cmd/nephe-controller/main.go

@@ -68,6 +71,8 @@ func main() {
 	logging.SetDebugLog(enableDebugLog)
 	ctrl.SetLogger(logging.GetLogger("setup"))

+	opts.complete()


if you fail to parse config, you should consider failing pod status to notReady

reachjainrahul · 2023-01-31T10:44:35Z

cmd/nephe-controller/main.go

@@ -86,6 +91,9 @@ func main() {
 	// Initialize Account poller map.
 	poller := controllers.InitPollers()

+	// Add the name prefix
+	securitygroup.AddResourcePrefix(opts.config.ResourcePrefix)


This can be set after you parse config.. Dont delay

reachjainrahul · 2023-01-31T10:45:54Z

config/manager/nephe-controller.yaml

+  namespace: system
+data:
+  nephe-controller.conf: |
+    ResourcePrefix: nephe-


This should be in under '#' and add a comment description indicating what this field does

reachjainrahul · 2023-01-31T10:46:27Z

pkg/cloud-provider/securitygroup/securitygroup.go

-	NepheControllerAddressGroupPrefix = NepheControllerPrefix + "ag-"
-	NepheControllerAppliedToPrefix    = NepheControllerPrefix + "at-"
+var (
+	NepheControllerPrefix             string


nit: Drop Nephe.. Just use ControllerPrefix

reachjainrahul · 2023-01-31T10:46:58Z

pkg/cloud-provider/securitygroup/securitygroup.go

@@ -141,6 +141,12 @@ type CloudResourceID struct {
 	Vpc  string
 }

+func AddResourcePrefix(name string) {


nit: SetCloudResourcePrefixes?

Anandkumar26 · 2023-02-06T06:07:17Z

cmd/nephe-controller/main.go

+	flag.StringVar(&opts.configFile, "config", opts.configFile, "The path to the configuration file.")
+	err := opts.complete()
+	if err != nil {
+		setupLog.Error(err, "Invalid controller config map data")


All error message should start with smallcase.

Suggested change

setupLog.Error(err, "Invalid controller config map data")

setupLog.Error(err, "invalid config map data")

reachjainrahul · 2023-02-06T10:37:53Z

cmd/nephe-controller/main.go

@@ -86,6 +96,9 @@ func main() {
 	// Initialize Account poller map.
 	poller := controllers.InitPollers()

+	// Add the CloudResourcePrefix


reachjainrahul · 2023-02-06T10:38:14Z

cmd/nephe-controller/options.go

+package main
+
+import (
+	"antrea.io/nephe/pkg/controllers/config"


Sort imports

reachjainrahul · 2023-02-06T10:38:51Z

cmd/nephe-controller/options.go

+			return err
+		}
+	}
+	o.Log.V(1).Info("CloudResourcePrefix")


reachjainrahul · 2023-02-06T10:39:37Z

config/manager/nephe-controller.yaml

+  namespace: system
+data:
+  nephe-controller.conf: |
+    CloudResourcePrefix: anp


This should be commented and a description on variable is required

reachjainrahul · 2023-02-06T10:39:59Z

pkg/apiserver/webhook/configmap_webhook.go

+package webhook
+
+import (
+	controllers "antrea.io/nephe/pkg/controllers/cloud"


Sort imports

reachjainrahul · 2023-02-06T10:41:43Z

pkg/controllers/config/labels.go

+	DefaultCloudResourcePrefix = "nephe"
+)
+
+type ControllerConfig struct {


why is the struct here..? Its mostly for const

Because controller config might contains more properties. This is how they followed in antrea controller.

I think we can have config.go and move ControllerConfig struct and ConfigMapName/Namespace constants there.

reachjainrahul · 2023-02-06T10:44:05Z

pkg/apiserver/webhook/configmap_webhook.go

+	v.Log.V(1).Info("new", "CloudResourcePrefix", newConfigMap.Data["CloudResourcePrefix"])
+	if req.OldObject.Raw != nil {
+		if err := json.Unmarshal(req.OldObject.Raw, &oldConfigMap); err != nil {
+			v.Log.Error(err, "Failed to decode old configMap", "ConfigMapValidator", req.Name)


all error message should begin with lower case

pkg/apiserver/webhook/configmap_webhook.go

reachjainrahul · 2023-02-15T04:04:28Z

pkg/apiserver/webhook/configmap_webhook.go

+	if len(newCloudResourcePrefix) > 0 {
+		if !config.ValidateName(newCloudResourcePrefix) {
+			return admission.Denied(fmt.Sprintf("invalid CloudResourcePrefix %s only "+
+				"alphanumeric and '-' characters are allowed.", newCloudResourcePrefix))


invalid CloudResourcePrefix xyz, only.. Basically , is required

This is suggested by anand. @Anandkumar26 Please comment about this.

Just add a comma after invalid CloudResourcePrefix %s,

Suggested change

"alphanumeric and '-' characters are allowed.", newCloudResourcePrefix))

"invalid CloudResourcePrefix %s only, "+

"alphanumeric and '-' characters are allowed."))

reachjainrahul · 2023-02-15T04:11:33Z

pkg/controllers/config/config.go

+	CloudResourcePrefix string `yaml:"CloudResourcePrefix,omitempty"`
+}
+
+func SetConfigFile(name string) {


why no move ConfigFile inside ControllerConfig struct?? avoid using global variables..

Because controller config is specifically for nephe-controller.conf. The config file contains more than one controller conf that's why we used this approach.

reachjainrahul · 2023-02-21T04:10:05Z

cmd/nephe-controller/main.go

@@ -15,6 +15,8 @@
 package main

 import (
+	"antrea.io/nephe/pkg/cloud-provider/securitygroup"


Sort imports

reachjainrahul · 2023-02-21T04:12:39Z

cmd/nephe-controller/main.go

+		os.Exit(1)
+	}
+
+	securitygroup.SetCloudResourcePrefix(&configMapController.ControllerConfig.CloudResourcePrefix)


Do we really need to set Prefix here?? Consider avoiding it

reachjainrahul · 2023-02-21T04:14:47Z

pkg/apiserver/webhook/configmap_webhook.go

+	}
+
+	if newCloudResourcePrefix == oldCloudResourcePrefix {
+		v.Log.V(1).Info("CloudResourcePrefix are same", "newCloudResourcePrefix",


this log is not required

reachjainrahul · 2023-02-21T04:15:26Z

pkg/apiserver/webhook/configmap_webhook_test.go

+	Context("Running Webhook tests for ConfigMap", func() {
+		var (
+			testNamespacedName1 = types.NamespacedName{Namespace: "nephe-system", Name: "nephe-config"}
+			// testNamespacedName2 = types.NamespacedName{Namespace: "nephe-system", Name: "config"}


remove comment

reachjainrahul · 2023-02-21T04:40:06Z

pkg/apiserver/webhook/configmap_webhook_test.go

+			decoder, err = admission.NewDecoder(newScheme)
+			Expect(err).Should(BeNil())
+			fakeClient = fake.NewClientBuilder().WithScheme(newScheme).Build()
+			npController = &controllers.NetworkPolicyReconciler{


is this needed?

reachjainrahul · 2023-02-21T04:40:35Z

pkg/apiserver/webhook/configmap_webhook_test.go

+			err = ConfigMapValidatorTest.InjectDecoder(decoder)
+			Expect(err).Should(BeNil())
+		})
+		AfterEach(func() {


check if you even need to declare it

reachjainrahul · 2023-02-21T04:55:04Z

pkg/controllers/cloud/configmap_controller_test.go

+					controllerConfig: conf,
+				},
+			}
+			if !errors.IsNotFound(retError) {


re-org the code

cmd/nephe-controller/main.go

The configmap conatins the name prefix for the security group Signed-off-by: nithishs <nithishs@vmware.com>

reachjainrahul

/LGTM

reachjainrahul · 2023-02-21T11:25:59Z

/nephe-test-e2e-kind

krishnamiriyala

Can nephe-controller.conf move from /tmp to /etc/nephe?

Anandkumar26 reviewed Jan 30, 2023

View reviewed changes

Nithish555 force-pushed the anp branch from 1af2af0 to ef69dd8 Compare January 30, 2023 07:06

Anandkumar26 reviewed Jan 30, 2023

View reviewed changes

config/nephe.yml Outdated Show resolved Hide resolved

Anandkumar26 reviewed Jan 30, 2023

View reviewed changes

cmd/nephe-controller/main.go Outdated Show resolved Hide resolved

Anandkumar26 reviewed Jan 30, 2023

View reviewed changes

config/nephe.yml Outdated Show resolved Hide resolved

Nithish555 force-pushed the anp branch 2 times, most recently from 84954e9 to 49f8164 Compare January 30, 2023 13:16

shenmo3 reviewed Jan 31, 2023

View reviewed changes

reachjainrahul reviewed Jan 31, 2023

View reviewed changes

Nithish555 force-pushed the anp branch 3 times, most recently from 3d31d32 to b4c2f55 Compare February 6, 2023 05:44

Anandkumar26 reviewed Feb 6, 2023

View reviewed changes

Nithish555 force-pushed the anp branch from b4c2f55 to 57d5929 Compare February 6, 2023 07:32

reachjainrahul reviewed Feb 6, 2023

View reviewed changes

pkg/apiserver/webhook/configmap_webhook.go Show resolved Hide resolved

reachjainrahul reviewed Feb 15, 2023

View reviewed changes

Nithish555 force-pushed the anp branch 11 times, most recently from fa73161 to 8d91e8c Compare February 20, 2023 12:24

reachjainrahul reviewed Feb 21, 2023

View reviewed changes

cmd/nephe-controller/main.go Outdated Show resolved Hide resolved

Nithish555 force-pushed the anp branch 3 times, most recently from ef065af to 18fcd4f Compare February 21, 2023 09:07

Adds the configmap for naming the security group

abd6568

The configmap conatins the name prefix for the security group Signed-off-by: nithishs <nithishs@vmware.com>

Nithish555 force-pushed the anp branch from 18fcd4f to abd6568 Compare February 21, 2023 09:13

reachjainrahul approved these changes Feb 21, 2023

View reviewed changes

reachjainrahul merged commit f507f02 into antrea-io:main Feb 21, 2023

krishnamiriyala reviewed May 17, 2023

View reviewed changes


		flag.StringVar(&opts.configFile, "config", opts.configFile, "The path to the configuration file")

	setupLog.Error(err, "Invalid controller config map data")
	setupLog.Error(err, "invalid config map data")

	"alphanumeric and '-' characters are allowed.", newCloudResourcePrefix))
	"invalid CloudResourcePrefix %s only, "+
	"alphanumeric and '-' characters are allowed."))

Adds the configmap for naming the security group #111

Adds the configmap for naming the security group #111

Conversation

Nithish555 commented Jan 26, 2023

codecov bot commented Jan 26, 2023 • 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

shenmo3 Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

shenmo3 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

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

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

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

reachjainrahul left a comment

Choose a reason for hiding this comment

reachjainrahul commented Feb 21, 2023

krishnamiriyala left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 26, 2023 •

edited

Loading

shenmo3 Jan 31, 2023 •

edited

Loading