Enable passing config into controller #264

zmingxi · 2023-06-01T20:07:40Z

What type of PR is this?
bug

Which issue does this PR fix:
#261

What does this PR do / Why do we need it:
This PR enables controller to take input from user

If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:

Testing done on this change:
make e2etest
unit test
helm install --set awsRegion=us-west-2 --set awsAccountId=123456789 --set clusterVpcId=vpc-12345678 --dry-run --debug helm --generate-name
conformance test

Automation added to e2e:
N/A

Will this PR introduce any new dependencies?:
No

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No
Does this PR introduce any user-facing change?:
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

coveralls · 2023-06-01T20:11:37Z

Pull Request Test Coverage Report for Build 5148739696

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 31.963%

Totals
Change from base Build 5147188293:	0.0%
Covered Lines:	3195
Relevant Lines:	9996

💛 - Coveralls

liwenwu-amazon · 2023-06-01T20:12:37Z

pkg/config/controller_config.go


-func ConfigInit() {
-// discover VPC using environment first
-VpcID = os.Getenv("CLUSTER_VPC_ID")


Is there any reason to change this? Today we documented how to use these environments variables and they are used for developers as well (e..g make presubmit)

Document says: When running AWS Gateway API Controller outside the Kubernetes Cluster, this specify the VPC of the cluster.

This code changes is to take the input while running AWS Gateway API Controller inside the Kubernetes Cluster.

solmonk · 2023-06-01T23:45:46Z

pkg/config/controller_config.go

+} else {
+VpcID = os.Getenv("CLUSTER_VPC_ID")
+glog.V(2).Infoln("CLUSTER_VPC_ID from local dev environment: ", VpcID)
+}


From what I have seen, the most common practice on config is to have priority order flag > env > auto-resolve. I think env variable should be a way of configuring things regardless of dev environment. This way we can simply set env var as a default value of a flag, and run discovery if the current value is empty.

Actually... we have a lot of configuration variables (longname mode for example) and I think they all should be configurable by both flag and env. It makes things bigger so we can leave it to other task though.

Since we have two ways of configuring the same value I think the naming should be consistent

e.g. flag aws-vpc-id -> env AWS_VPC_ID

Yes, I discussed with Liwen, we plan to go with env, no flag for now. I am going to change this using env var.

solmonk · 2023-06-01T23:57:32Z

helm/templates/deployment.yaml

 {{ if .Values.deployment.priorityClassName -}}
 priorityClassName: {{ .Values.deployment.priorityClassName }}
 {{ end -}}
+ {{- if .aws.region }}


Should these be somewhere inside like args block?

coveralls · 2023-06-02T00:17:08Z

Pull Request Test Coverage Report for Build 5150455119

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 31.963%

Totals
Change from base Build 5147188293:	0.0%
Covered Lines:	3195
Relevant Lines:	9996

💛 - Coveralls

liwenwu-amazon · 2023-06-02T00:19:39Z

docs/developer.md

 kubectl apply -f config/crds/bases/k8s-gateway-v0.6.1.yaml
 kubectl apply -f config/crds/bases/multicluster.x-k8s.io_serviceexports.yaml
 kubectl apply -f config/crds/bases/multicluster.x-k8s.io_serviceimports.yaml
-kubectl apply -f examples/gatewayclass.yaml


why do u delete this?

liwenwu-amazon · 2023-06-02T00:20:22Z

docs/developer.md

-You should set up the correct `REGION` env variable and create `non-default`
-namespace if it doesn't exist.
-
-NOTE: You'll need to allow in-bound traffics from lattice prefix list in the security
-groups of your cluster.
-
-```bash
-# create non-default namespace if it hasn't existed yet
-kubectl create namespace non-default
-
+You should set up the correct `REGION` env variable


why do u delete all these?

I didnt intent to delete this, i need to fetch new code change.

liwenwu-amazon · 2023-06-02T00:20:51Z

docs/developer.md

 make build-deploy
 ```
-Then follow [Deploying the AWS Gateway API Controller](https://github.com/aws/aws-application-networking-k8s/blob/main/docs/deploy.md) to configure and deploy the docker image
+Then follow [Deploying the AWS Gateway API Controller](https://github.com/aws/aws-application-networking-k8s/blob/main/docs/deploy.md) to configure and deploy the docker image 


Please remove all these white-space diff

liwenwu-amazon · 2023-06-02T00:27:38Z

Please also add or modify unit test for the change

coveralls · 2023-06-02T17:37:14Z

Pull Request Test Coverage Report for Build 5159899025

36 of 39 (92.31%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.5%) to 32.439%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/config/controller_config.go	36	39	92.31%

Totals
Change from base Build 5147188293:	0.5%
Covered Lines:	3275
Relevant Lines:	10096

💛 - Coveralls

liwenwu-amazon · 2023-06-02T17:37:26Z

docs/deploy.md

 --version=v0.0.12 \
- --set=aws.region=$AWS_REGION --set=serviceAccount.create=false --namespace aws-application-networking-system
+ --set=serviceAccount.create=false --namespace aws-application-networking-system \
+ # Region, clusterVpcId, awsAccountId are required for fargate use case


can you replace "fargate use case" to something " for the case where IMDS are not available for discovering Region, clusterVpcId, awsAccountId, for example fargate usecase"

liwenwu-amazon · 2023-06-02T17:39:38Z

pkg/config/controller_config.go

-var VpcID = "vpc-xxxx"
-var AccountID = "yyyyyy"
-var Region = "us-west-2"
+var VpcID = ""


NIT,can use use a constant for ""? e.g. UNKNOWN_VPCID=""

liwenwu-amazon · 2023-06-02T17:40:10Z

pkg/config/controller_config.go

+
+// VpcId
+VpcID = os.Getenv("CLUSTER_VPC_ID")
+if VpcID != "" {


NIT, use constant instead of ""

liwenwu-amazon · 2023-06-02T17:40:49Z

pkg/config/controller_config.go

 )

 // TODO endpoint, region
-var VpcID = "vpc-xxxx"


can u also remove //TODO comment? thanks

zmingxi and others added 3 commits June 1, 2023 00:09

Enable passing input into controller

a4610f1

Enable passing input into controller

80c9ac2

Merge branch 'aws:main' into main

a703b09

zmingxi requested a review from liwenwu-amazon June 1, 2023 20:07

liwenwu-amazon reviewed Jun 1, 2023

View reviewed changes

zmingxi requested a review from solmonk June 1, 2023 22:36

solmonk reviewed Jun 1, 2023

View reviewed changes

Improve input pass to controller

2702d21

liwenwu-amazon reviewed Jun 2, 2023

View reviewed changes

Change flag to env var for input

952991f

liwenwu-amazon reviewed Jun 2, 2023

View reviewed changes

Remove checking if running inside cluster code

cf9ea34

liwenwu-amazon reviewed Jun 2, 2023

View reviewed changes

zmingxi added 5 commits June 2, 2023 17:41

Update deploy docs

bdda6e6

Update default setting

f81282b

Update default setting

d443ae0

Update default setting

f37bc66

Add unit test for env var init

bab00cb

liwenwu-amazon approved these changes Jun 2, 2023

View reviewed changes

zmingxi merged commit 3a3de7b into aws:main Jun 2, 2023

zijun726911 mentioned this pull request Jun 9, 2023

Init Config for test framework #266

Merged

zijun726911 mentioned this pull request Oct 13, 2023

Add ability to pass CLUSTER_NAME env variable in helm chart #436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enable passing config into controller #264

Enable passing config into controller #264

Uh oh!

zmingxi commented Jun 1, 2023 •

edited

Loading

coveralls commented Jun 1, 2023 •

edited

Loading

liwenwu-amazon Jun 1, 2023 •

edited

Loading

zmingxi Jun 1, 2023 •

edited

Loading

solmonk Jun 1, 2023

solmonk Jun 1, 2023

zmingxi Jun 2, 2023 •

edited

Loading

solmonk Jun 1, 2023

coveralls commented Jun 2, 2023

liwenwu-amazon Jun 2, 2023

liwenwu-amazon Jun 2, 2023

zmingxi Jun 2, 2023 •

edited

Loading

liwenwu-amazon Jun 2, 2023

liwenwu-amazon commented Jun 2, 2023

coveralls commented Jun 2, 2023 •

edited

Loading

liwenwu-amazon Jun 2, 2023

liwenwu-amazon Jun 2, 2023

liwenwu-amazon Jun 2, 2023

liwenwu-amazon Jun 2, 2023

Labels

4 participants

Enable passing config into controller #264

Enable passing config into controller #264

Uh oh!

Conversation

zmingxi commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

coveralls commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 5148739696

💛 - Coveralls

liwenwu-amazon Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

zmingxi Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmingxi Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 2, 2023

Pull Request Test Coverage Report for Build 5150455119

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmingxi Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liwenwu-amazon commented Jun 2, 2023

coveralls commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 5159899025

💛 - Coveralls

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

Labels

4 participants

zmingxi commented Jun 1, 2023 •

edited

Loading

coveralls commented Jun 1, 2023 •

edited

Loading

liwenwu-amazon Jun 1, 2023 •

edited

Loading

zmingxi Jun 1, 2023 •

edited

Loading

zmingxi Jun 2, 2023 •

edited

Loading

zmingxi Jun 2, 2023 •

edited

Loading

coveralls commented Jun 2, 2023 •

edited

Loading