Connection to EventDataPlatform #1212

jasiu001 · 2020-04-22T06:20:46Z

Description

Changes proposed in this pull request:

Add EventDataPlatform client
Implement EDP registration step
Implement EDP deregistration step

ksputo · 2020-04-23T15:04:49Z

chart/compass/charts/kyma-environment-broker/values.yaml

+  namespace: "kyma-dev"
+  environment: "dev"
+  required: false
+  disabled: false


I wonder if we should have this set to true by default, because we use overrides with other necessary values anyway

ksputo · 2020-04-23T15:22:00Z

components/kyma-environment-broker/cmd/broker/main.go

@@ -197,6 +202,11 @@ func main() {
 			step:     provisioning.NewProvideLmsTenantStep(lmsTenantManager, db.Operations()),
 			disabled: cfg.LMS.Disabled,
 		},
+		{
+			weight:   1,
+			step:     provisioning.NewEDPRegistration(db.Operations(), edpClient, cfg.EDP),


I'd suggest to rename it to be consistent with convention

Suggested change

step: provisioning.NewEDPRegistration(db.Operations(), edpClient, cfg.EDP),

step: provisioning.NewEDPRegistrationStep(db.Operations(), edpClient, cfg.EDP),

ksputo · 2020-04-23T15:22:29Z

components/kyma-environment-broker/cmd/broker/main.go

 	}{
 		{
 			weight: 1,
 			step:   deprovisioning.NewAvsEvaluationsRemovalStep(avsDel, db.Operations(), externalEvalAssistant, internalEvalAssistant),
 		},
+		{
+			weight:   1,
+			step:     deprovisioning.NewEDPDeregistration(edpClient, cfg.EDP),


same as above:

Suggested change

step: deprovisioning.NewEDPDeregistration(edpClient, cfg.EDP),

step: deprovisioning.NewEDPDeregistrationStep(edpClient, cfg.EDP),

mszostok · 2020-04-23T19:37:37Z

components/kyma-environment-broker/internal/edp/client.go

+	if err != nil {
+		return []MetadataItem{}, errors.Wrap(err, "while requesting about dataTenant metadata")
+	}
+	defer c.closeResponseBody(response)


please use the same pattern as used in this PR: #1190

same suggestion for other occurrences

mszostok · 2020-04-23T19:49:04Z

components/kyma-environment-broker/internal/edp/client.go

+		return kebError.NewTemporaryError("EDP server returns failed status %s", responseLog(response))
+	}
+
+	c.log.Errorf("EDP server notsupported response %s: %s", responseLog(response), body)


Suggested change

c.log.Errorf("EDP server notsupported response %s: %s", responseLog(response), body)

c.log.Errorf("EDP server not supported response %s: %s", responseLog(response), body)

mszostok · 2020-04-23T19:51:36Z

components/kyma-environment-broker/internal/edp/client_builder.go

+	Disabled    bool
+}
+
+func CreateEDPAdminClient(config Config, log logrus.FieldLogger) *Client {


cannot we make it simpler and just do that in NewClient method? because for me it's not a builder but normal constructor 🤔

I tried to avoid creating oAuth client inside EDP client constructor, thanks DI I can simply in test inject client from httptest.NewServer, I couldn't find oAuth client test. If you see any simple solution about how to create oAuth inside EDP client and easily handle it in the test not implement all oAuth paths then I am open to suggestions.

let's discuss that f2f. I know some trick for such tests :)

mszostok · 2020-04-23T20:06:29Z

components/kyma-environment-broker/internal/edp/client_builder.go

+
+func CreateEDPAdminClient(config Config, log logrus.FieldLogger) *Client {
+	data := url.Values{}
+	data.Add("grant_type", "client_credentials")


not needed - it is already set by clientcredentials client

mszostok · 2020-04-23T20:06:48Z

components/kyma-environment-broker/internal/edp/client_builder.go

+func CreateEDPAdminClient(config Config, log logrus.FieldLogger) *Client {
+	data := url.Values{}
+	data.Add("grant_type", "client_credentials")
+	data.Add("scope", "edp-namespace.read edp-namespace.update")


just use Scope field on clientcredentials.Config

mszostok · 2020-04-23T20:31:50Z

components/kyma-environment-broker/internal/edp/client_test.go

+	client := NewClient(config, testServer.Client(), logrus.New())
+
+	// when
+	err := client.CreateDataTenant(DataTenantPayload{})


changing impl to

func (c *Client) CreateDataTenant(data DataTenantPayload) error { return nil }

and test still green :/

same problems with other tests

fixed TestClient_CreateDataTenant and TestClient_DeleteDataTenant

mszostok · 2020-04-23T20:34:19Z

components/kyma-environment-broker/internal/edp/client_test.go

+		AdminURL:  testServer.URL,
+		Namespace: testNamespace,
+	}
+	client := NewClient(config, testServer.Client(), logrus.New())


Please use logger.NewLogDummy(). Because right now it produces logs which only mess the test output. If you want to assert that given msg was logged then use logger.NewLogSpy(). The same suggestion for all others occurrences

mszostok · 2020-04-23T20:41:39Z

components/kyma-environment-broker/internal/edp/client.go

+	var metadata []MetadataItem
+	err = json.NewDecoder(response.Body).Decode(&metadata)
+	if err != nil {
+		return metadata, errors.Wrap(err, "while decoding dataTenant metadata response")


nit: the best practice is to return zero-value in case of error

mszostok · 2020-04-23T20:43:13Z

components/kyma-environment-broker/internal/edp/client.go

+func (c *Client) processResponse(response *http.Response) error {
+	byteBody, err := ioutil.ReadAll(response.Body)
+	if err != nil {
+		return errors.Wrapf(err, "while reading response body")


please add also info about status code as this can be helpful

mszostok · 2020-04-23T20:55:19Z

chart/compass/charts/kyma-environment-broker/templates/secrets.yaml

+  labels:
+{{ include "kyma-env-broker.labels" . | indent 4 }}


Suggested change

labels:

{{ include "kyma-env-broker.labels" . | indent 4 }}

labels: {{ include "kyma-env-broker.labels" . | nindent 4 }}

IMO it's better because thanks to the nindent we do not need to take care of the proper indent before the {{

and we had already a few problems with wrong indent in past.

jasiu001 added kind/feature Categorizes issue or PR as related to a new feature. area/management-plane Related to all activities around Management Plane labels Apr 22, 2020

jasiu001 added this to the Sprint_Gopher_41 milestone Apr 22, 2020

jasiu001 requested review from ksputo, mszostok, piotrmiskiewicz, PK85 and polskikiel as code owners April 22, 2020 06:20

ksputo self-assigned this Apr 23, 2020

jasiu001 force-pushed the edp branch from 4eda224 to 10528f9 Compare April 23, 2020 12:57

ksputo reviewed Apr 23, 2020

View reviewed changes

mszostok reviewed Apr 23, 2020

View reviewed changes

mszostok self-assigned this Apr 23, 2020

jasiu001 force-pushed the edp branch from 10528f9 to ce246be Compare April 24, 2020 10:41

ksputo approved these changes Apr 24, 2020

View reviewed changes

jasiu001 force-pushed the edp branch from ce246be to 08fafc6 Compare April 24, 2020 12:50

jasiu001 requested review from alexandra-simeonova, bszwarc, kazydek, klaudiagrz, majakurcius, mmitoraj and tomekpapiernik as code owners April 24, 2020 12:50

klaudiagrz approved these changes Apr 24, 2020

View reviewed changes

Jasiak added 4 commits April 24, 2020 15:40

Add EventDataPlatform client

afd52a6

Implement EDP registration step

da96957

Implement EDP deregistration step

01e0f8e

add region from parameters

521ed9c

Jasiak added 3 commits April 24, 2020 15:43

fix after CR#1

4ba1cdc

fix after CR#2

7319552

rebase#1

f6f2838

jasiu001 force-pushed the edp branch from 08fafc6 to f6f2838 Compare April 24, 2020 13:47

jasiu001 merged commit 9ebddb7 into kyma-incubator:master Apr 27, 2020

jasiu001 mentioned this pull request Apr 27, 2020

Connection compass KEB to EventDataPlatform kyma-project/kyma#8245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection to EventDataPlatform #1212

Connection to EventDataPlatform #1212

jasiu001 commented Apr 22, 2020

ksputo Apr 23, 2020

ksputo Apr 23, 2020

ksputo Apr 23, 2020

mszostok Apr 23, 2020

mszostok Apr 23, 2020

mszostok Apr 23, 2020

jasiu001 Apr 24, 2020

mszostok Apr 24, 2020

mszostok Apr 23, 2020

mszostok Apr 23, 2020

mszostok Apr 23, 2020

jasiu001 Apr 24, 2020

mszostok Apr 23, 2020

mszostok Apr 23, 2020

mszostok Apr 23, 2020

mszostok Apr 23, 2020

	step: provisioning.NewEDPRegistration(db.Operations(), edpClient, cfg.EDP),
	step: provisioning.NewEDPRegistrationStep(db.Operations(), edpClient, cfg.EDP),

	step: deprovisioning.NewEDPDeregistration(edpClient, cfg.EDP),
	step: deprovisioning.NewEDPDeregistrationStep(edpClient, cfg.EDP),

	c.log.Errorf("EDP server notsupported response %s: %s", responseLog(response), body)
	c.log.Errorf("EDP server not supported response %s: %s", responseLog(response), body)

	labels:
	{{ include "kyma-env-broker.labels" . \| indent 4 }}
	labels: {{ include "kyma-env-broker.labels" . \| nindent 4 }}

Connection to EventDataPlatform #1212

Connection to EventDataPlatform #1212

Conversation

jasiu001 commented Apr 22, 2020

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

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

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

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