Skip to content

Integrations: web API and tctl#24145

Merged
marcoandredinis merged 9 commits intomasterfrom
marco/integration_resource_clients
Apr 12, 2023
Merged

Integrations: web API and tctl#24145
marcoandredinis merged 9 commits intomasterfrom
marco/integration_resource_clients

Conversation

@marcoandredinis
Copy link
Copy Markdown
Contributor

@marcoandredinis marcoandredinis commented Apr 5, 2023

This PR adds end user interface to manage integrations:

tctl

$ tctl get integrations --config teleport.yaml --format text
Name        Type     Spec                                           
----------- -------- ---------------------------------------------- 
myawsint    aws-oidc RoleARN=arn:aws:iam::123456789012:role/DevTeam 
mynewawsint aws-oidc RoleARN=arn:aws:iam::123456789012:role/OpsTea

HTTP API

$ curl 'https://127.0.0.1.nip.io:3080/v1/webapi/sites/lenix/integrations'

{
  "items": [
    {
      "name": "myawsint",
      "subKind": "aws-oidc",
      "awsOIDC": {
        "roleARN": "arn:aws:iam::123456789012:role/DevTeam"
      }
    },
    {
      "name": "mynewawsint",
      "subKind": "aws-oidc",
      "awsOIDC": {
        "roleARN": "arn:aws:iam::123456789012:role/OpsTeam"
      }
    }
  ],
  "nextKey": ""
}

part of RFD #22556

@github-advanced-security
Copy link
Copy Markdown

You have successfully added a new Trivy configuration .github/workflows/trivy.yaml:trivy. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_client_server branch from c93c71c to eea92ca Compare April 5, 2023 18:36
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_clients branch from 11192ba to 4181c73 Compare April 5, 2023 18:38
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_client_server branch from eea92ca to 2e9c8d0 Compare April 6, 2023 15:35
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_clients branch from 4181c73 to 0c5ad39 Compare April 6, 2023 15:36
Comment thread lib/web/ui/integration.go Outdated
Comment thread lib/web/ui/integration.go Outdated
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_client_server branch from 2e9c8d0 to 2436872 Compare April 6, 2023 19:04
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_clients branch from 0c5ad39 to 095b5f2 Compare April 6, 2023 19:04
@marcoandredinis
Copy link
Copy Markdown
Contributor Author

marcoandredinis commented Apr 6, 2023

Opening this PR for review because the base branch is approved
#24133
(I'm just fighting with some flaky tests to get it merged)

@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_clients branch from bf8103c to 07e8a94 Compare April 6, 2023 19:16
@marcoandredinis marcoandredinis marked this pull request as ready for review April 6, 2023 19:16
@github-actions github-actions Bot added size/lg tctl tctl - Teleport admin tool labels Apr 6, 2023
@marcoandredinis marcoandredinis requested a review from r0mant April 6, 2023 19:17
Comment thread lib/web/integrations.go Outdated
Comment on lines 105 to 108
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can this logic be encapsulated into a CanUpdate() method on the integration resource? So it can be used by other clients as well, e.g. tctl will also need it, right? Check out a similar method on the trusted cluster resource.

Copy link
Copy Markdown
Contributor Author

@marcoandredinis marcoandredinis Apr 11, 2023

Choose a reason for hiding this comment

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

I was able to apply that pattern to tctl but not here

We are using a type specific to Web.
To re-use the CanChangeStateTo here, we would need to re-create the integration to then pass it to the existing integration's CanChangeStateTo.

I think we should keep this if here.

Let me know what you think 🙏

Comment thread lib/web/integrations.go Outdated
Comment thread lib/web/ui/integration.go Outdated
Comment thread lib/web/ui/integration.go Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm do we really have "subkind_spec" field in the resource or is it just some test artifact? Why can't we just do

spec:
  aws_oidc:
    role_arn: ...
  somethin_else:
    ...

None of other resources had subkind_spec e.g. check out ProvisionToken, it also has a bunch of fields for different integrations (Github, AWS, etc.) and they're not nested under anything.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Integration resource uses the OneOf definition (it was suggested here #23553 (comment) )
So, when we MarshalJSON it comes with the OneOf type as a key:

{
    "kind": "integration",
    "sub_kind": "aws-oidc",
    "version": "v1",
    "metadata": {
        "name": "some-integration"
    },
    "spec": {
        "subkind_spec": {
            "aws_oidc": {
                "role_arn": "arn:aws:iam::123456789012:role/DevTeams"
            }
        }
    }
}

I can customize the Un/MarshalJSON to "hide" the subkind_spec, but I would rather not add more custom code

Let me know if you still think we should remove that extra field and I will remove it 👍

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If this was a non-user-facing resource, it wouldn't have mattered much but because users will have to create it the extra subkind_spec field just adds extra noise IMO.

With this structure we'd still have spec.subkind_spec.aws_oidc, spec.subkind_spec.gcp_xxx and so on, I don't really see how it's better than just spec.aws_oidc, spec.gcp_xxx, etc.

I agree we shouldn't just stuff all fields for all integrations in the integration's spec intermixed (that was main @rosstimothy's concern as I understood), but having separate sub-fields for different integrations like we have for provision tokens for example would make it easy to see which parameters are for which integration without extra noise.

Or I think you can keep using oneof but apply it to the whole spec? Something like this should work:

message IntegrationV1 {
  ResourceHeader Header = 1 [...];
  oneof Spec {
    AWSOIDCSpecV1 AWSOIDC = 2 [...];
    // Other integrations will go here
  }
}

Have you tried this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or I think you can keep using oneof but apply it to the whole spec?

I was trying to leave room for common properties
Things like status.

I'll change the serializers, that should be enough to get a better operator experience 👍

Comment thread integration/integrations/integration_test.go Outdated
Comment thread integration/integrations/integration_test.go Outdated
Comment thread integration/integrations/integration_test.go Outdated
Comment thread tool/tctl/common/collection.go Outdated
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_client_server branch 4 times, most recently from f8b8cba to b00d938 Compare April 11, 2023 09:34
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_clients branch 3 times, most recently from 919f885 to eb1e832 Compare April 11, 2023 11:31
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Apr 11, 2023

Please fix the tclt typo in the commit message before merging.

@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_client_server branch from b00d938 to cbfe0fc Compare April 11, 2023 15:47
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_clients branch from 96b25af to 1158f3e Compare April 11, 2023 15:50
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_client_server branch from cbfe0fc to 6f80969 Compare April 11, 2023 16:12
@marcoandredinis marcoandredinis changed the base branch from marco/integration_awsoidc to master April 12, 2023 13:56
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_clients branch 2 times, most recently from cabc24a to 46422b6 Compare April 12, 2023 13:59
Comment thread integration/integrations/integration_test.go Outdated
Comment thread integration/integrations/integration_test.go Outdated
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_clients branch 3 times, most recently from c5c2e23 to 05f1d7a Compare April 12, 2023 14:53
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Looks better as long as we update the godoc to match implementation

Comment thread integration/helpers/web.go Outdated
This PR adds end user interface to manage integrations:

`tctl`
```
$ tctl get integrations --config teleport.yaml --format text
Name        Type     Spec
----------- -------- ----------------------------------------------
myawsint    aws-oidc RoleARN=arn:aws:iam::123456789012:role/DevTeam
mynewawsint aws-oidc RoleARN=arn:aws:iam::123456789012:role/OpsTeam
```

HTTP API
```
$ curl 'https://127.0.0.1.nip.io:3080/v1/webapi/sites/lenix/integrations'

{
  "items": [
    {
      "name": "myawsint",
      "subKind": "aws-oidc",
      "awsOIDC": {
        "roleARN": "arn:aws:iam::123456789012:role/DevTeam"
      }
    },
    {
      "name": "mynewawsint",
      "subKind": "aws-oidc",
      "awsOIDC": {
        "roleARN": "arn:aws:iam::123456789012:role/OpsTeam"
      }
    }
  ],
  "nextKey": ""
}
```
@marcoandredinis marcoandredinis force-pushed the marco/integration_resource_clients branch from 05f1d7a to cba07b5 Compare April 12, 2023 15:28
@marcoandredinis marcoandredinis added discover Issues related to Teleport Discover backport/branch/v12 labels Apr 12, 2023
@marcoandredinis marcoandredinis added this pull request to the merge queue Apr 12, 2023
Merged via the queue into master with commit e0d6c1d Apr 12, 2023
@marcoandredinis marcoandredinis deleted the marco/integration_resource_clients branch April 12, 2023 16:23
@public-teleport-github-review-bot
Copy link
Copy Markdown

@marcoandredinis See the table below for backport results.

Branch Result
branch/v12 Failed

marcoandredinis added a commit that referenced this pull request Apr 12, 2023
* Integrations: web api and tclt

This PR adds end user interface to manage integrations:

`tctl`
```
$ tctl get integrations --config teleport.yaml --format text
Name        Type     Spec
----------- -------- ----------------------------------------------
myawsint    aws-oidc RoleARN=arn:aws:iam::123456789012:role/DevTeam
mynewawsint aws-oidc RoleARN=arn:aws:iam::123456789012:role/OpsTeam
```

HTTP API
```
$ curl 'https://127.0.0.1.nip.io:3080/v1/webapi/sites/lenix/integrations'

{
  "items": [
    {
      "name": "myawsint",
      "subKind": "aws-oidc",
      "awsOIDC": {
        "roleARN": "arn:aws:iam::123456789012:role/DevTeam"
      }
    },
    {
      "name": "mynewawsint",
      "subKind": "aws-oidc",
      "awsOIDC": {
        "roleARN": "arn:aws:iam::123456789012:role/OpsTeam"
      }
    }
  ],
  "nextKey": ""
}
```

* Add explicit type

* add awsoidc role arn setter

* change serializer

* ignore bodyclose linter false positive

* check for error before reading

* simplify webPack.DoRequest call

* fix godoc of WebClientPack.DoRequest

* return body and status code only
marcoandredinis added a commit that referenced this pull request Apr 13, 2023
* Integrations: web api and tclt

This PR adds end user interface to manage integrations:

`tctl`
```
$ tctl get integrations --config teleport.yaml --format text
Name        Type     Spec
----------- -------- ----------------------------------------------
myawsint    aws-oidc RoleARN=arn:aws:iam::123456789012:role/DevTeam
mynewawsint aws-oidc RoleARN=arn:aws:iam::123456789012:role/OpsTeam
```

HTTP API
```
$ curl 'https://127.0.0.1.nip.io:3080/v1/webapi/sites/lenix/integrations'

{
  "items": [
    {
      "name": "myawsint",
      "subKind": "aws-oidc",
      "awsOIDC": {
        "roleARN": "arn:aws:iam::123456789012:role/DevTeam"
      }
    },
    {
      "name": "mynewawsint",
      "subKind": "aws-oidc",
      "awsOIDC": {
        "roleARN": "arn:aws:iam::123456789012:role/OpsTeam"
      }
    }
  ],
  "nextKey": ""
}
```

* Add explicit type

* add awsoidc role arn setter

* change serializer

* ignore bodyclose linter false positive

* check for error before reading

* simplify webPack.DoRequest call

* fix godoc of WebClientPack.DoRequest

* return body and status code only
marcoandredinis added a commit that referenced this pull request Apr 13, 2023
* Integrations: web api and tclt

This PR adds end user interface to manage integrations:

`tctl`
```
$ tctl get integrations --config teleport.yaml --format text
Name        Type     Spec
----------- -------- ----------------------------------------------
myawsint    aws-oidc RoleARN=arn:aws:iam::123456789012:role/DevTeam
mynewawsint aws-oidc RoleARN=arn:aws:iam::123456789012:role/OpsTeam
```

HTTP API
```
$ curl 'https://127.0.0.1.nip.io:3080/v1/webapi/sites/lenix/integrations'

{
  "items": [
    {
      "name": "myawsint",
      "subKind": "aws-oidc",
      "awsOIDC": {
        "roleARN": "arn:aws:iam::123456789012:role/DevTeam"
      }
    },
    {
      "name": "mynewawsint",
      "subKind": "aws-oidc",
      "awsOIDC": {
        "roleARN": "arn:aws:iam::123456789012:role/OpsTeam"
      }
    }
  ],
  "nextKey": ""
}
```

* Add explicit type

* add awsoidc role arn setter

* change serializer

* ignore bodyclose linter false positive

* check for error before reading

* simplify webPack.DoRequest call

* fix godoc of WebClientPack.DoRequest

* return body and status code only
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discover Issues related to Teleport Discover size/lg tctl tctl - Teleport admin tool

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants