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 multiple maestro instances. #28

Merged
merged 2 commits into from
Jan 2, 2024

Conversation

morvencao
Copy link
Contributor

@morvencao morvencao commented Dec 22, 2023

This pull request aims to enable the concurrent run of multiple Maestro instances.
All instances will utilize the same source ID while having distinct client IDs.

Otherwise you will see this in meastro logs:

E1222 02:53:55.962288       1 baseclient.go:79] the cloudevents client is disconnected, EOF
E1222 02:53:55.962419       1 baseclient.go:156] failed to receive cloudevents, error while opening the inbound connection: write tcp 10.128.1.49:36072->172.30.94.214:1883: use of closed network connection

@morvencao
Copy link
Contributor Author

Appended test results:

$ make test && make test-integration
CGO_ENABLED=1 GOEXPERIMENT=boringcrypto go install -ldflags="" ./cmd/maestro
OCM_ENV=testing gotestsum --format short-verbose -- -p 1 -v  \
	./pkg/... \
	./cmd/...
EMPTY pkg/api
EMPTY pkg/api/openapi
EMPTY pkg/api/presenters
EMPTY pkg/auth
EMPTY pkg/client/cloudevents
EMPTY pkg/client/ocm
PASS pkg/config.TestConfigReadStringFile (0.00s)
PASS pkg/config.TestConfigReadIntFile (0.00s)
PASS pkg/config.TestConfigReadBoolFile (0.00s)
PASS pkg/config.TestConfigReadQuotedFile (0.00s)
PASS pkg/config (cached)
PASS pkg/controllers.TestControllerFramework (0.00s)
PASS pkg/controllers (cached)
EMPTY pkg/dao
EMPTY pkg/dao/mocks
EMPTY pkg/db
EMPTY pkg/db/db_context
EMPTY pkg/db/db_session
EMPTY pkg/db/migrations
EMPTY pkg/db/mocks
EMPTY pkg/db/transaction
PASS pkg/errors.TestErrorFormatting (0.00s)
PASS pkg/errors.TestErrorFind (0.00s)
PASS pkg/errors (cached)
EMPTY pkg/handlers (cached)
EMPTY pkg/logger
PASS pkg/services.TestSQLTranslation (0.02s)
PASS pkg/services.TestResourceFindByConsumerID (0.00s)
PASS pkg/services
EMPTY pkg/util
EMPTY cmd/maestro
EMPTY cmd/maestro/agent
PASS cmd/maestro/environments.TestLoadServices (0.38s)
PASS cmd/maestro/environments
EMPTY cmd/maestro/migrate
EMPTY cmd/maestro/servecmd
EMPTY cmd/maestro/server
EMPTY cmd/maestro/server/logging

DONE 10 tests in 5.669s
CGO_ENABLED=1 GOEXPERIMENT=boringcrypto go install -ldflags="" ./cmd/maestro
OCM_ENV=testing gotestsum --format short-verbose -- -p 1 -ldflags -s -v -timeout 1h  \
		./test/integration
I1222 02:29:04.287716  218340 framework.go:76] Initializing testing environment
I1222 02:29:04.492247  218340 framework.go:161] Using Mock OCM Authz Client
I1222 02:29:04.493536  218340 framework.go:199] Disabling Sentry error reporting
{"level":"info","ts":1703212144.4938266,"logger":"fallback","caller":"v2@v2.0.0-20231030012137-0836a524e995/protocol.go:124","msg":"subscribing to topics: map[sources/clusters/+/specresync:{1 0 false false} sources/maestro-integration/clusters/+/status:{1 0 false false}]"}
I1222 02:29:04.499490  218340 api_server.go:145] Serving without TLS at localhost:8000
PASS test/integration.TestConsumerGet (0.32s)
PASS test/integration.TestConsumerPost (0.18s)
PASS test/integration.TestConsumerPaging (0.23s)
PASS test/integration.TestControllerRacing (1.73s)
PASS test/integration.TestResourceGet (0.32s)
PASS test/integration.TestResourcePost (2.23s)
PASS test/integration.TestResourcePatch (0.21s)
PASS test/integration.TestResourcePaging (0.25s)
PASS test/integration.TestResourceListSearch (0.36s)
PASS test/integration.TestUpdateResourceWithRacingRequests (0.36s)
I1222 02:29:10.694362  218340 api_server.go:151] Web server terminated
PASS test/integration (cached)

DONE 10 tests in 0.759s

@morvencao
Copy link
Contributor Author

/assign @clyang82 @skeeey

command:
- /usr/local/bin/maestro
- server
- --client-id=$(POD_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use a rand id here? we may need to consider how to use this client id do the authZ and authn with AWS IoT, right?

Copy link
Contributor Author

@morvencao morvencao Dec 22, 2023

Choose a reason for hiding this comment

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

Is it necessary to differentiate between different Maestro instances or pods connecting to AWS IoT? I checked the AWS IoT policy intended for Maestro, and it appears as follows:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Action": "iot:Connect",
      "Resource": "arn:aws:iot:<region>:<aws-accountID>:consumers/maestro-pub-consumer",
"arn:aws:iot:<region>:<aws-accountID>:consumers/maestro-sub-consumer"
    },
   {
      "Effect": "Allow",
      "Action": "iot:Publish",
      "Resource":
"arn:aws:iot:<region>:<aws-accountID>:topic/sources/maestro/consumers/*/spec",
"arn:aws:iot:<region>:<aws-accountID>:topic/sources/maestro/consumers/statusresync"
    },
    {
      "Effect": "Allow",
      "Action": "iot:Subscribe",
      "Resource":
"arn:aws:iot:<region>:<aws-accountID>:topic/sources/maestro/consumers/+/status",
"arn:aws:iot:<region>:<aws-accountID>:topic/sources/consumers/+/specresync"
    },
}

Copy link
Contributor

Choose a reason for hiding this comment

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

In iot:Connect part, I remembered consumers/maestro-sub-consume is the mqtt client id, I think you may have a test for this, we may need a wildcard (consumers/maestro-*-consumer) for source client in the policy?

Copy link
Contributor Author

@morvencao morvencao Dec 22, 2023

Choose a reason for hiding this comment

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

it works. I tried to add a policy: (the last resource item is added, other are from aws sample)

    {
      "Effect": "Allow",
      "Action": "iot:Connect",
      "Resource": [
        "arn:aws:iot:us-east-1:xxxxx:client/sdk-java",
        "arn:aws:iot:us-east-1:xxxxx:client/basicPubSub",
        "arn:aws:iot:us-east-1:xxxxx:client/sdk-nodejs-*",
        "arn:aws:iot:us-east-1:xxxxx:client/maestro-*-consumer"
      ]
    }

and send a message with the following comand:

$ mosquitto_pub -h xxxxxx-ats.iot.us-east-1.amazonaws.com --key lcao-thing-1.private.key --cert lcao-thing-1.cert.pem --cafile root-CA.crt -i maestro-test-consumer --topic sdk/test/js -m "test pub from maestro client."

Then I can see the message from thing page of AWS console.

Copy link
Contributor

@skeeey skeeey Dec 22, 2023

Choose a reason for hiding this comment

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

so we may set the client id to --client-id=maestro-$(POD_NAME) ? then we can have a policy

"Resource": [
        "arn:aws:iot:us-east-1:xxxxx:client/maestro-*"
      ]

Signed-off-by: morvencao <lcao@redhat.com>
Signed-off-by: morvencao <lcao@redhat.com>
@morvencao morvencao force-pushed the br_run_multiple_instances branch from fd6d5cb to 53787bb Compare January 2, 2024 02:42
@morvencao
Copy link
Contributor Author

rebased the code

@morvencao
Copy link
Contributor Author

/assign @clyang82

Copy link
Contributor

@clyang82 clyang82 left a comment

Choose a reason for hiding this comment

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

LGTM

@clyang82 clyang82 merged commit c6aaf30 into openshift-online:main Jan 2, 2024
1 check was pending
@morvencao morvencao deleted the br_run_multiple_instances branch January 2, 2024 08:15
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.

3 participants