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

[NET-9376] Fix issue with multiple APIGW #4000

Merged
merged 5 commits into from
May 17, 2024

Conversation

jm96441n
Copy link
Member

@jm96441n jm96441n commented May 16, 2024

Changes proposed in this PR

  • Update role with correct policy when creating a new gateway
  • this works on upgrade process

How I've tested this PR

  • have kubectl on your system
  1. clone the following gist: https://gist.github.com/jm96441n/c0645d345c4a92f8b5ea6af272a19d63
  2. set the imageK8s field in the consul_values.yaml file to be hashicorp/consul-k8s-control-plane:1.4
  3. run the start.sh script
  4. run the following in your terminal
export CONSUL_HTTP_ADDR="127.0.0.1:8501"
export CONSUL_HTTP_SSL=true
export CONSUL_HTTP_SSL_VERIFY=false
export CONSUL_HTTP_TOKEN=$(kubectl get --namespace consul secrets/consul-consul-bootstrap-acl-token --template={{.data.token}} | base64 -d)
  1. in a tool like k9s see that api-gateway-2 is in an error state
  2. open consul at https://localhost:8501 (you'll need to allow the insecure traffic to bypass the warning)
  3. copy the value of CONSUL_HTTP_TOKEN to your clipboard and use that to login to the UI
  4. go to the roles tab and view the role for the api-gateway-2 and see the policy it is using is the api-gateway-token-policy which grants write access to the first gateway created
  5. on this branch run make docker-dev
  6. update the imageK8s field to be consul-k8s-control-plane:local
  7. in your terminal run: helm upgrade --install consul hashicorp/consul -f ./consul/consul_values.yaml -n consul --create-namespace --wait
  8. once that finishes in a tool like k9s you should see all the api-gateways spin up correctly
  9. run kubectl port-forward service/consul-consul-ui 8501:443 -n consul & in your terminal to re-establish the port-forward to the consul ui
  10. repeat steps 2/3/4 to open the consul ui and view the role for api-gateway-2 then you should see it is using the specific policy for this gateway that grants it write access to itself

How I expect reviewers to test this PR

read the code
run the above steps

Checklist

@jm96441n jm96441n added the pr/no-backport signals that a PR will not contain a backport label label May 16, 2024
@@ -0,0 +1,3 @@
```release-note:bug
api-gateway: fix bug where multiple logical APIGateways would share the same ACL policy.
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need 2 changelogs?

Copy link
Member Author

Choose a reason for hiding this comment

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

because I committed this one on the wrong branch 😂

Copy link
Contributor

@NiniOak NiniOak left a comment

Choose a reason for hiding this comment

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

LGTM.
Test cases:

  • No Upgrade: imageK8S: consul-k8s-control-plane:local.
    RESULT: two API Gateways successfully deployed
  • Direct version upgrade: imageK8S: hashicorp/consul-k8s-control-plane:1.4 > imageK8S: consul-k8s-control-plane:local
    RESULT: two API Gateways successfully deployed
  • SKIP version upgrade: imageK8S: hashicorp/consul-k8s-control-plane:1.3 > imageK8S: consul-k8s-control-plane:local
    RESULT: two API Gateways successfully deployed

@jm96441n jm96441n merged commit 2ba59b2 into release/1.4.x May 17, 2024
33 of 50 checks passed
@jm96441n jm96441n deleted the NET-9376-fix-multiple-apigw-role-policy branch May 17, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-backport signals that a PR will not contain a backport label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants