- 
                Notifications
    You must be signed in to change notification settings 
- Fork 121
Migrate Kibana connectors to use the bundled openapi generated client #1260
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
Conversation
| 
 I'm happy to split this out into a prefactor PR if that makes reviewing this easier. There's not a huge amount of code churn related to this change which is why it's included, but it definitely should be it's own PR. | 
| Computed: true, | ||
| Optional: true, | ||
| ForceNew: true, | ||
| ValidateFunc: validation.IsUUID, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure IsUUID is the right validation here? This seems to have a very particular view of what counts as a UUID.
func ParseUUID(uuid string) ([]byte, error) {
	if len(uuid) != 2 * uuidLen + 4 {
		return nil, fmt.Errorf("uuid string is wrong length")
	}
	if uuid[8] != '-' ||
		uuid[13] != '-' ||
		uuid[18] != '-' ||
		uuid[23] != '-' {
		return nil, fmt.Errorf("uuid is improperly formatted")
	}
	hexStr := uuid[0:8] + uuid[9:13] + uuid[14:18] + uuid[19:23] + uuid[24:36]
	ret, err := hex.DecodeString(hexStr)
	if err != nil {
		return nil, err
	}
	if len(ret) != uuidLen {
		return nil, fmt.Errorf("decoded hex is the wrong length")
	}
	return ret, nil
}
Notably it doesn't seem to allow the example given in the issue that inspired this change "lugoz-safes-rusin-bubov-fytex-cydeb".
I can't seem to find any details in the connector api docs about requirements (but maybe i'm just not looking in the right place).
This open api doc I found in the Kibana repo seems to have a less opinionated view though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the description of this field it mentions that the connector_id must be either UUIDv1 or UUIDv4, both of which are expected to match this validation. Manually testing with Kibana seems to back that assertion up, but I can't find anything in the Kibana docs to link to here.
Notably it doesn't seem to allow the example given in the issue that inspired this change "lugoz-safes-rusin-bubov-fytex-cydeb".
FWIW this value is rejected by Kibana (at least v9), I expect that ID has been made up for the issue rather than something that's expected to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the description of this field it mentions that the connector_id must be either UUIDv1 or UUIDv4
Yeah this is a good point. Presumably this comes from the Kibana open api spec somewhere?
This sounds good 👍 I just wanted to make sure we weren't adding a more strict validation than we intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would also be totally fine to just rely on the validation of the API. So it would construct a request with a wrong UUID, and then Kibana would respond with a validation error that we can present to the user.
(I thas the advantage that we avoid the risk of having a more strict validation than the underlying API and preventing something that should actually work)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
| @tobio Looks like you fixed #1135 "accidentaly" with this PR. #1135 started to really annoy me, and after I got the reply from support, that #1135 will most likely only get fixed by November, I decided to have a look at the code myself, only to find, that the current does not seem to have the issue anymore. This is when I relized this commit in the very recent commit history. Now, the only thing, that is missing to make me happy is a release (tag), such that the provider becomes available through the terraform registry. Do you see any chance for a release in the next days? | 
* origin/main: Migrate Elasticsearch enrich policy resource and data source to Terraform Plugin Framework (#1220) Add support for solution field in elasticsearch_kibana_space resource and data source (#1210) Don't force replacement when changing integration versions (#1255) chore(deps): update golang:1.25.0 docker digest to 5502b0e (#1267) chore(deps): update docker.elastic.co/kibana/kibana docker tag to v9.1.3 (#1263) Bump github.com/ulikunitz/xz from 0.5.12 to 0.5.14 (#1264) Migrate Kibana connectors to use the bundled openapi generated client (#1260) Extend CONTRIBUTING.md (#1262) fix: add `namespace` attribute to `elasticstack_kibana_synthetics_monitor` resource (#1247) Bump github.com/stretchr/testify from 1.10.0 to 1.11.0 (#1261) chore(deps): update codecov/codecov-action digest to fdcc847 (#1258) fix(deps): update module go.uber.org/mock to v0.6.0 (#1259)
Fixes #1088
This PR:
x-omitemptyinto the spec for any nullable parameter. We've been explicitly listing anything that 'breaks' up until now, which isn't really scalable or automatable. There's no real case where we don't want this applied, so lets just add it everywhere.connector_idduring creation. I'm not a huge fan of having to define a request editor to mutate the request path if there's no ID specified, but the Kibana spec doesn't include an operation for creating a connector without an ID instead it marks theidparameter as optional which is entirely valid. Sadly, requests with a trailing slash aren't properly handled by Kibana 8.7 and lower and so we need to trim that for those client version.