Skip to content
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

Support public/private API endpoints settings from file #1693

Merged

Conversation

ota42y
Copy link
Contributor

@ota42y ota42y commented Dec 23, 2019

Description

update-cluster-endpoints command changes API endpoint settings (#1149)

But this command doesn't read config file.
So when we use -f option, eksctl doesn't change settings.

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, and examples directory)
  • Manually tested
  • Added labels for change area (e.g. area/nodegroup) and target version (e.g. version/0.12.0)
  • Added note in docs/release_notes/draft.md (or relevant release note)

@ota42y ota42y force-pushed the feature/endpoints_load_from_file_test branch 2 times, most recently from bdca31d to 81634ac Compare December 23, 2019 09:09
@ota42y ota42y marked this pull request as ready for review December 23, 2019 09:20
@ota42y
Copy link
Contributor Author

ota42y commented Dec 23, 2019

@weaveworks/eksctl
I add changes and manually tested.
Do I need to add more tests and where to add tests ?

I got error in circleci but I think this isn't from my changes 🤔
In my PC, all test passed.

mkdir -p vendor/github.com/aws/
ln -sfn "/go/pkg/mod/github.com/weaveworks/[email protected]" vendor/github.com/aws/aws-sdk-go
time env GOBIN=/go/bin go generate ./pkg/eks/mocks
Unable to find CloudFormationAPI in any go files under this path
pkg/eks/mocks/mocks.go:16: running "/go/bin/mockery": exit status 1
Command exited with non-zero status 1
real	0m 0.19s
user	0m 0.23s
sys	0m 0.09s
make[1]: *** [Makefile:180: pkg/eks/mocks/SSMAPI.go] Error 1
make[1]: Leaving directory '/src'
make: *** [Makefile:85: test] Error 2

Copy link
Contributor

@martina-if martina-if left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ota42y ! Looks good. I added a minor comment. Can you also rebase the branch?

@martina-if martina-if self-assigned this Jan 9, 2020
@ota42y ota42y force-pushed the feature/endpoints_load_from_file_test branch from d76cb6c to d77e90e Compare January 10, 2020 00:34
@ota42y
Copy link
Contributor Author

ota42y commented Jan 10, 2020

I finished rebase!

@martina-if
Copy link
Contributor

I ran some manual tests and it seems to be working well:

---
apiVersion: eksctl.io/v1alpha5
kind: ClusterConfig

metadata:
  name: martina-test-priv-ep
  region: eu-north-1

nodeGroups:
  - name: ng-1
    instanceType: m5.large
    desiredCapacity: 1

vpc:
  clusterEndpoints:
    publicAccess: true
    privateAccess: true
./eksctl utils update-cluster-endpoints  -f examples/01-simple-cluster.yaml 
[ℹ]  using region eu-north-1
[ℹ]  current Kubernetes API endpoint access: privateAccess=false, publicAccess=true
[ℹ]  (plan) would update Kubernetes API endpoint access for cluster "martina-test-priv-ep" in "eu-north-1" to: privateAccess=true, publicAccess=true
[!]  no changes were applied, run again with '--approve' to apply the changes
 ./eksctl utils update-cluster-endpoints  -f examples/01-simple-cluster.yaml  --approve
[ℹ]  using region eu-north-1
[ℹ]  current Kubernetes API endpoint access: privateAccess=false, publicAccess=true
[ℹ]  will update Kubernetes API endpoint access for cluster "martina-test-priv-ep" in "eu-north-1" to: privateAccess=true, publicAccess=true
[✔]  the Kubernetes API endpoint access for cluster "martina-test-priv-ep" in "eu-north-1" has been updated to: privateAccess=true, publicAccess=true
$ ./eksctl  utils update-cluster-endpoints --name=martina-test-priv-ep --private-access=false --public-access=true
Flag --name has been deprecated, use --cluster
[ℹ]  using region eu-north-1
[ℹ]  current Kubernetes API endpoint access: privateAccess=true, publicAccess=true
[ℹ]  (plan) would update Kubernetes API endpoint access for cluster "martina-test-priv-ep" in "eu-north-1" to: privateAccess=false, publicAccess=true
[!]  no changes were applied, run again with '--approve' to apply the changes
./eksctl  utils update-cluster-endpoints --name=martina-test-priv-ep --private-access=false --public-access=true --approve
Flag --name has been deprecated, use --cluster
[ℹ]  using region eu-north-1
[ℹ]  current Kubernetes API endpoint access: privateAccess=true, publicAccess=true
[ℹ]  will update Kubernetes API endpoint access for cluster "martina-test-priv-ep" in "eu-north-1" to: privateAccess=false, publicAccess=true
[✔]  the Kubernetes API endpoint access for cluster "martina-test-priv-ep" in "eu-north-1" has been updated to: privateAccess=false, publicAccess=true
$ ./eksctl  utils update-cluster-endpoints --name=martina-test-priv-ep --private-access=false 
Flag --name has been deprecated, use --cluster
[ℹ]  using region eu-north-1
[ℹ]  current Kubernetes API endpoint access: privateAccess=false, publicAccess=true
[✔]  Kubernetes API endpoint access for cluster "martina-test-priv-ep" in "eu-north-1" is already up to date

@ota42y Sorry for the late reply. I think we can get this merged for version 0.14 :) can you please move the message from the draft file to 0.14.0-draft.md instead? Then if you rebase this will be ready to merge!

ota42y and others added 4 commits January 21, 2020 20:05
`update-cluster-endpoints` command changes API endpoint settings (eksctl-io#1149)

But this command doesn't read config file.
So when we use `-f` option, `eksctl` doesn't change settings.
@ota42y ota42y force-pushed the feature/endpoints_load_from_file_test branch from 592d2de to a396076 Compare January 21, 2020 11:07
@ota42y
Copy link
Contributor Author

ota42y commented Jan 21, 2020

rebased and moved message!

@martina-if martina-if merged commit bd775c3 into eksctl-io:master Jan 21, 2020
@martina-if
Copy link
Contributor

Thank you @ota42y !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants