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

Add Api Gateway RestAPI and Resource resources #1230

Merged
merged 6 commits into from
Apr 15, 2022
Merged

Add Api Gateway RestAPI and Resource resources #1230

merged 6 commits into from
Apr 15, 2022

Conversation

tiagoposse
Copy link
Contributor

Description of your changes

Generate resources for the apigateway service and add restapi and resource as usable.
First part of #1185

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Tests and example yamls included.

Copy link
Contributor

@stevendborrelli stevendborrelli left a comment

Choose a reason for hiding this comment

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

This looks like a nice PR!

When running the examples, I get the following issue parsing the policy:

.648561326001369e+09   DEBUG   provider-aws    Cannot observe external resource        {"controller": "managed/restapi.apigateway.aws.crossplane.io", "request": "/test", "uid": "e3724d47-b3c1-42ad-ba4c-b41c74cd46ff", "version": "237985", "external-name": "onnex4cv1b", "error": "late-init failed: policies couldnt be compared: cant parse policies: cant unmarshal policy: invalid character '\\\\' looking for beginning of object key string", "errorVerbose": "policies couldnt be compared: cant parse policies: cant unmarshal policy: invalid character '\\\\' looking for beginning of object key string\
 {"object": {"kind":"RestAPI","name":"test","uid":"e3724d47-b3c1-42ad-ba4c-b41c74cd46ff","apiVersion":"apigateway.aws.crossplane.io/v1alpha1","resourceVersion":"237985"}, "reason": "CannotObserveExternalResource", "message": "late-init failed: policies couldnt be compared: cant parse policies: cant unmarshal policy: invalid character '\\\\' looking for beginning of object key string"}

apis/apigateway/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
Copy link
Contributor

@stevendborrelli stevendborrelli left a comment

Choose a reason for hiding this comment

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

I need to dive deeper into the policy parsing, as I am getting errors in testing and there is some odd behavior about quoting.

pkg/controller/apigateway/restapi/setup.go Outdated Show resolved Hide resolved
pkg/controller/apigateway/restapi/setup.go Outdated Show resolved Hide resolved
@tiagoposse
Copy link
Contributor Author

I managed to fix the policy parsing. For an explanation on what's happening:
A user will insert a policy like

{
	"Version": "2012-10-17",
	"Statement": [
		{
			"Effect": "Allow",
			"Principal": "*",
			"Action": "execute-api:Invoke",
			"Resource": "execute-api:/*/*/*"
		}
	]
}

but post-creation, aws will return

{\"Version\":\"2012-10-17\",\"Statement\":[{\"Action\":\"execute-api:Invoke\",\"Effect\":\"Allow\",\"Principal\":\"*\",\"Resource\":\"arn:aws:execute-api:eu-central-1:123456789012:abcdef1234/*/*/*\"}]}

This wasn't playing nice with json marshal/unmarshal. I found a solution that works here: https://stackoverflow.com/a/50151862. This substitutes the string replace approach.

@stevendborrelli
Copy link
Contributor

@tiagoposse Thanks! The new fixes look like they resolve the parsing issue.

NAME                                        READY   SYNCED   EXTERNAL-NAME
restapi.apigateway.aws.crossplane.io/test   True    True     onnex4cv1b

NAME                                         READY   SYNCED   EXTERNAL-NAME
resource.apigateway.aws.crossplane.io/test   True    True     f2dndj

Copy link
Contributor

@stevendborrelli stevendborrelli left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

I have a small PR to your branch to fix a few errors. https://github.com/tiagoposse/provider-aws/pull/1

pkg/controller/apigateway/restapi/setup.go Outdated Show resolved Hide resolved
@tiagoposse
Copy link
Contributor Author

My pleasure, looking forward to open PRs for rest of the resources once this is merged :)
Your fix branch is now merged

@stevendborrelli
Copy link
Contributor

@tiagoposse do you think you could rebase this PR on master and push your branch? There was a linter fix in #1250 that we think is affecting the checks on this pR.

tiagoposse and others added 4 commits April 13, 2022 09:00
Signed-off-by: Tiago Posse <tiagoposse@gmail.com>
Signed-off-by: Tiago Posse <tiagoposse@gmail.com>
Signed-off-by: Tiago Posse <tiagoposse@gmail.com>
Signed-off-by: Steven Borrelli <steve@borrelli.org>
@tiagoposse
Copy link
Contributor Author

done!

@tiagoposse
Copy link
Contributor Author

Doesn't seem to have fixed it, anything else I can do here ?

@stevendborrelli
Copy link
Contributor

I will pull your branch and see if there are any build issues in my environment. It's possible we still have issues in the GitHub action.

Signed-off-by: Steven Borrelli <steve@borrelli.org>
@stevendborrelli
Copy link
Contributor

@tiagoposse I think the PR got caught up in some changes in the upstream API. I've opened a PR against your repo that hopefully fixes the issues: https://github.com/tiagoposse/provider-aws/pull/2

@stevendborrelli
Copy link
Contributor

stevendborrelli commented Apr 13, 2022

Looks like all the checks have passed. We'll need one of the maintainers to approve and merge.

Copy link
Member

@haarchri haarchri left a comment

Choose a reason for hiding this comment

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

@stevendborrelli thanks for review testing and help with PRs ;) tested the resource also with success LGTM

@haarchri haarchri merged commit 219e5de into crossplane-contrib:master Apr 15, 2022
febarbosa182 pushed a commit to febarbosa182/provider-aws that referenced this pull request May 23, 2022
* generate apigateway service resources
* add resource and restapi apigateway resources

Signed-off-by: Tiago Posse <tiagoposse@gmail.com>

Co-authored-by: Steven Borrelli <steve@borrelli.org>
Signed-off-by: Felipe Barbosa <lybrbarbosa@gmail.com>
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.

None yet

3 participants