Ability to apply licence by mhmxs · Pull Request #63 · storageos/portal-manager

mhmxs · 2022-04-13T06:56:06Z

This change gives the ability to portal manager to apply for licences. It uses device status to send initial status messages about device config acceptance and current licence.

szank · 2022-05-25T12:06:09Z

endpoints/portal.go

+	var pubPEM []byte
+
+	resp, code, err := utils.GetHttpContent(e.url+"/setup/signature-key?clusterId="+e.clusterID, headers)
+	if err != nil && code != http.StatusNotFound {


Can there be any other error code?
Now if err == nil and code == 404 then we are on the happy path? Is that OK also?

The endpoint does not necessarily exist, so we have to set pubPEM only if it exists. On the other hand err should be nil only if the status is 2XX, so I think the code is correct.

szank · 2022-05-25T12:16:10Z

endpoints/storageos.go

+	// TODO would be nice to properly stop refresh.
+	go func() {
+		for {
+			if err := e.client.Refresh(context.Background(), time.Minute); err != nil {


What do you think if you

Make the first refresh in the e.client.Refresh function do a refresh immediately when called and return an appropriate error, then start the loop running after every minute

Drop the go and for loop here, and check for the initial refresh error? That would allow you to just return an error from UpdateLicence immediately when the first refresh happens?

Not sure if this has impact on other places, but IMVHO it's a good pattern, if a bit annyoing to code in the e.client.Refresh implementation.

At this point we have an active connection because the connection has been established to fetch cluster id, so here what we want is to refresh the token until program ends in every minute.

Right. But inside the Refresh method you have already an infinite loop refreshing the token. so I assumed you've added an infinite loop here because that inner infinite loop might return an error and you want to retry?

Yes. Our future plan is to watch storageos API credentials secret, and if changes restart the pod. So here until credentials are the same we would like to push refresh until the end.

szank · 2022-05-25T12:19:58Z

main.go

+		}
+	}()
+
+	// Periodically send licence.


Canceling context on sigterm would do the trick, and it's not that much to code. Assuming that we terminate on exit only? But in that case who cares? And this is just a comment, I have no problem to leave it as is if we would only terminate this goroutine on exit.

we would only terminate this goroutine on exit and this project is still in a rapidly changing phase, so there are more important things to do.

szank · 2022-05-25T12:20:43Z

main.go

+			currentLicence, err := stosEndpoint.GetLicence()
+			if err != nil {
+				setupLogger.Error(err, "unable to fetch licence")
+				return


Suggested change

return

continue

?? At least that how it looks to me on a first glance

szank · 2022-05-25T12:30:05Z

pkg/action/action.go

+}
+
+// Do selects the right action and executes it.
+func (as *ActionService) Do(rawAction []byte, updateState func(interface{}) error) {


That's an interesting way of doing things :)

szank · 2022-05-25T12:38:16Z

pkg/publisher/proto/Makefile

 $(PROTOC_GEN_GO):
 	mkdir -p $(dir $(GOPATH)/src/$(PROTOBUF_PKG))
-	test -d $(GOPATH)/src/$(PROTOBUF_PKG)/.git || git clone https://$(PROTOBUF_PKG) $(GOPATH)/src/$(PROTOBUF_PKG)
+	test -d $(GOPATH)/src/$(PROTOBUF_PKG)/.git && (cd $(GOPATH)/src/$(PROTOBUF_PKG); git fetch -a && git reset --hard $(PROTOBUF_VERSION)) || git clone --branch $(PROTOBUF_VERSION) https://$(PROTOBUF_PKG) $(GOPATH)/src/$(PROTOBUF_PKG)


Using gopath in 2022 is risky. It works but there are better ways. If you have github.com/golang/protobuf/protoc-gen-go in your go mod (i.e using the tools package officially sanctioned hack) there's a way to grab the path to the source code of the appropriate version using go list --something-using-obscure-template-arg.

I'd add it to the list of the tech debt items if it's not there yet, unless you want to sit down and address it now (I can help ?)

Could you please add it 🙏

Ugh, In another PR, sorry! I need to understand first what is really happening here, which will take some time. I've seen a pattern that looks very similar before, but if I were to fix it on my own I'd spend some time trying to understand it.

Now, I am happy to do it, but I need more spare time than I have right now. So, another PR it is.

szank · 2022-05-25T12:40:16Z

pkg/publisher/publisher.go

+func (p *Publisher) PublishState(ctx context.Context, state interface{}) error {
+	for _, sink := range p.sinks {
+		if err := sink.PublishState(ctx, state); err != nil {
+			return errors.Wrap(err, "unable to publish state to sink")


Does it matter if the remaining sinks won't get the memo because one is not available? Maybe you could not return early here? Just a speculation. Please ignore if it's irrelevant.

Good question. I don't really understand why we need multiple message brokers here. So I followed the original designers' same kind of function above. If you ask me, I suggest removing multiple brokers complexity at all, but it isn't the scope of this change.

szank · 2022-05-26T13:51:15Z

endpoints/portal.go

+		Registry:     configResp.Registry,
+		Region:       configResp.Region,
+		ServerURL:    configResp.MttqURL,
+		SignatureKey: string(pubPEM),


Ugh, I am not sure about storing raw bytes in a string variable now that I think about it. I dooooont think it will cause bugs, but it looks strange. Not a blocker I think.

At some point, we have to convert it to string. So I did the conversion as early as possible (once). But lets talk about it.

mhmxs added 11 commits April 13, 2022 08:48

Ability to apply licence

764119a

Send error status on failed licence update

94e6c54

Send proper status response after action execution

0178abd

Verify data integrity of action

7cabfd9

ezaz

2d021e7

First working prototype

ee6277d

Fix action config ID parsing

9aaa36a

Fix MQTT and StorageOS connections

9e530cf

Fix error handling of storageos client refresh

3b73ddd

Fix IOT core logging name

32d1fc3

Make scripts in hack executable

a005858

mhmxs marked this pull request as ready for review May 25, 2022 09:40

mhmxs requested review from Mojachieee, Ricardo-Osorio and nolancon May 25, 2022 09:40

Retry action, Mutex for IOT subscribe, logging polish

9b3156c

szank reviewed May 25, 2022

View reviewed changes

Fix licence report error handling

29242ea

szank approved these changes May 25, 2022

View reviewed changes

mhmxs added 2 commits May 26, 2022 11:54

Fix message broker connection leak

9663577

Final polish

0fc1273

mhmxs requested a review from szank May 26, 2022 13:10

Fix docker build

f803bc6

szank reviewed May 26, 2022

View reviewed changes

szank approved these changes May 26, 2022

View reviewed changes

mhmxs merged commit 825d7de into main May 26, 2022

mhmxs deleted the apply-licence branch May 26, 2022 14:20

Conversation

mhmxs commented Apr 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants