crossplane-contrib / provider-upjet-aws

AWS Provider for Crossplane.
https://marketplace.upbound.io/providers/upbound/provider-family-aws/
Apache License 2.0
146 stars 123 forks source link

apigateway parentIdSelector by matchLabels isn't working #453

Open vispav opened 1 year ago

vispav commented 1 year ago

What happened?

The API GW resource is not being created using parentIdSelector matchLabels. Creating resource under '/api/v1' getting an error.

Error: cannot resolve references: mg.Spec.ForProvider.ParentID: cannot get referenced resource: RestAPI.apigateway.aws.upbound.io "test0" not found

How can we reproduce it?

apiVersion: apigateway.aws.upbound.io/v1beta1
kind: RestAPI
metadata:
  labels:
    testing.upbound.io/name: test
  name: test
spec:
  forProvider:
    name: test
    region: eu-west-1
---
apiVersion: apigateway.aws.upbound.io/v1beta1
kind: Resource
metadata:
  labels:
    testing.upbound.io/resource: test0
  name: test0
spec:
  forProvider:
    parentIdSelector:
      matchLabels:
        testing.upbound.io/name: test
    pathPart: api
    region: eu-west-1
    restApiIdSelector:
      matchLabels:
        testing.upbound.io/name: test
---
apiVersion: apigateway.aws.upbound.io/v1beta1
kind: Resource
metadata:
  labels:
    testing.upbound.io/resource: test1  
  name: test1
spec:
  forProvider:
    parentIdSelector:
      matchLabels:
        testing.upbound.io/resource: test0
    pathPart: v1
    region: eu-west-1
    restApiIdSelector:
      matchLabels:
        testing.upbound.io/name: test

What environment did it happen in?

Crossplane version: crossplane/crossplane:v1.10.1 AWS provider version: xpkg.upbound.io/upbound/provider-aws:v0.22.0

ytsarev commented 1 year ago

Hi @vispav , I am not fully sure it is the reason but could you try removing parenthesis in the test resources? Looks like a potential mismatch

 name: "test0"
vispav commented 1 year ago

Hi @vispav , I am not fully sure it is the reason but could you try removing parenthesis in the test resources? Looks like a potential mismatch

 name: "test0"

Hi @ytsarev , thanks for the suggestion. I tried removing quotes, it is still same error.

ytsarev commented 1 year ago

@vispav thanks for checking. Could you please double-check if the RestAPI is in the Ready state? Does it have the correct label?

vispav commented 1 year ago

@ytsarev yes, RestAPI is in Ready state and every resource are labeled exactly same as described in the yaml files. But the problem is with parentIdSelector, it doesn't work when trying to create resource with child path, i.e. under api/v1

NAME   READY   SYNCED   EXTERNAL-NAME   AGE   LABELS
test   True    True     0qwlbv0t71      46m   testing.upbound.io/name=test
test0   True    True     qhkt8z          3m8s   testing.upbound.io/resource=test0
test1           False                    3m14s   testing.upbound.io/resource=test1

Warning CannotResolveResourceReferences 25s (x10 over 4m26s) managed/apigateway.aws.upbound.io/v1beta1, kind=resource cannot resolve references: mg.Spec.ForProvider.ParentID: no resources matched selector

In terraform such example would be:

resource "aws_api_gateway_resource" "api" {
  rest_api_id = var.rest_api_id
  parent_id   = var.parent_resource_id
  path_part   = "api"
}
resource "aws_api_gateway_resource" "v1" {
  rest_api_id = aws_api_gateway_resource.api.rest_api_id
  parent_id   = aws_api_gateway_resource.api.id
  path_part   = "v1"
}
turkenf commented 1 year ago

Hi @vispav, As I understand from the terraform documentation, you need to select the rest_api_id and parent_id parameters from a RestAPI resource.

rest_api_id: ID of the associated REST API
parent_id: ID of the parent API resource

You are getting error here because you are trying to select parent_id from kind: Resource resource. I think it will work if the example is like below:

apiVersion: apigateway.aws.upbound.io/v1beta1
kind: RestAPI
metadata:
  labels:
    testing.upbound.io/name: test
  name: test
spec:
  forProvider:
    name: test
    region: eu-west-1
---
apiVersion: apigateway.aws.upbound.io/v1beta1
kind: Resource
metadata:
  labels:
    testing.upbound.io/resource: test0
  name: test0
spec:
  forProvider:
    parentIdSelector:
      matchLabels:
        testing.upbound.io/name: test
    pathPart: api
    region: eu-west-1
    restApiIdSelector:
      matchLabels:
        testing.upbound.io/name: test
---
apiVersion: apigateway.aws.upbound.io/v1beta1
kind: Resource
metadata:
  labels:
    testing.upbound.io/resource: test1  
  name: test1
spec:
  forProvider:
    parentIdSelector:
      matchLabels:
        testing.upbound.io/resource: test
    pathPart: v1
    region: eu-west-1
    restApiIdSelector:
      matchLabels:
        testing.upbound.io/name: test
vispav commented 1 year ago

hi @turkenf , Thanks for suggestion, but it is not working. It is also not working if using parentid instead of matchLabels. From what I tested, same works with unofficial provider of aws.

turkenf commented 1 year ago

Hi @vispav, Thanks for trying, it looks like the label of the parentIdSelector in the test1 resource is wrong. If you use testing.upbound.io/name:test instead of testing.upbound.io/resource:test the issue will be solved. Can you try the example below again?

apiVersion: apigateway.aws.upbound.io/v1beta1
kind: RestAPI
metadata:
  labels:
    testing.upbound.io/name: test
  name: test
spec:
  forProvider:
    name: test
    region: eu-west-1
---
apiVersion: apigateway.aws.upbound.io/v1beta1
kind: Resource
metadata:
  labels:
    testing.upbound.io/resource: test0
  name: test0
spec:
  forProvider:
    parentIdSelector:
      matchLabels:
        testing.upbound.io/name: test
    pathPart: api
    region: eu-west-1
    restApiIdSelector:
      matchLabels:
        testing.upbound.io/name: test
---
apiVersion: apigateway.aws.upbound.io/v1beta1
kind: Resource
metadata:
  labels:
    testing.upbound.io/resource: test1  
  name: test1
spec:
  forProvider:
    parentIdSelector:
      matchLabels:
        testing.upbound.io/name: test
    pathPart: v1
    region: eu-west-1
    restApiIdSelector:
      matchLabels:
        testing.upbound.io/name: test
vispav commented 1 year ago

hi @turkenf , Have you tested it by yourself? :) This is the result of your example:

image

The desired result is to have "v1" under "api": api/v1

turkenf commented 1 year ago

Hi @vispav, thank you for your try I got it now, sorry. We currently cannot support multiple reference types: https://github.com/upbound/upjet/issues/95 But as a workaround, you can use the id of test0.Resource for parentId in test1.Resource

apiVersion: apigateway.aws.upbound.io/v1beta1
kind: RestAPI
metadata:
  labels:
    testing.upbound.io/name: test
  name: test
spec:
  forProvider:
    name: test
    region: eu-west-1
---
apiVersion: apigateway.aws.upbound.io/v1beta1
kind: Resource
metadata:
  labels:
    testing.upbound.io/resource: test0
  name: test0
spec:
  forProvider:
    parentIdSelector:
      matchLabels:
        testing.upbound.io/name: test
    pathPart: api
    region: eu-west-1
    restApiIdSelector:
      matchLabels:
        testing.upbound.io/name: test
---
apiVersion: apigateway.aws.upbound.io/v1beta1
kind: Resource
metadata:
  labels:
    testing.upbound.io/resource: test1  
  name: test1
spec:
  forProvider:
    parentId: <ID of the above resource(you can use EXTERNAL-NAME)>
    pathPart: v1
    region: eu-west-1
    restApiIdSelector:
      matchLabels:
        testing.upbound.io/name: test

After creating the example above, I think we reach the desired result.

NAME                                       READY   SYNCED   EXTERNAL-NAME   AGE
resource.apigateway.aws.upbound.io/test0   True    True     4f77d1          13m
resource.apigateway.aws.upbound.io/test1   True    True     yhsl74          13m

NAME                                     READY   SYNCED   EXTERNAL-NAME   AGE
restapi.apigateway.aws.upbound.io/test   True    True     fb0e9e2hpk      13m
status:
  atProvider:
    id: yhsl74
    path: /api/v1
nassercarlos19 commented 7 months ago

This is a bug; that field should reference the Resources kind, not the RestApi kind. I am having the same issue. Do you know if there is a solution to this soon? :D

Thanks

marianobilli commented 4 months ago

Same issue here, further more if instead of using parentIdSelector, you use parentID and you put the ID of the /api resource it works.

It seems it is a bug of the parentIdSelector logic, it should search for both RestAPI and Resource resources.

I could not find the source code for this, is it not open source?

turkenf commented 4 months ago

Hello folks, as I described above comment, we currently cannot support multiple reference types, and there is an upstream issue: https://github.com/upbound/upjet/issues/95. You can share your experiences and requests there, thank you 🙏

turkenf commented 4 months ago

Blocked by: https://github.com/crossplane/upjet/issues/95

marmol-dev commented 3 months ago

Having the same issue here.

Defining different properties to select and RestAPI or Resource would be a valid solution?

For example: parentIdSelector -> selects a RestAPI type (to keep compatibility) parentResourceIdSelector -> selects a Resource type.

Thanks.

O5ten commented 1 month ago

Having the exact same issue with this and i've been wrestling with getting it working for quite some time. I can't seem to specify subresources because parentIdRef or parentIdSelector always seems to go looking for the RestApi kind. :(

maor-paz-hs commented 3 weeks ago

having the same issue here as well. Putting the parentId of the resource will not be automated well, Please help us