-
Notifications
You must be signed in to change notification settings - Fork 85
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
Implement compute manager resource #920
Conversation
@ksamoray, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
340d107
to
f1dc682
Compare
08cb840
to
41c9bd5
Compare
/test-all |
41c9bd5
to
a998e85
Compare
/test-all |
a998e85
to
3a3beda
Compare
/test-all |
}, | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"thumbprint": { |
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.
Lets add Sensitive: true
to all secrets
}, | ||
}, | ||
}, | ||
"origin_type": { |
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.
is this an enumeration?
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.
No - and indeed it's quite awkward
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 wonder what else can it be. Perhaps we should default to vcenter
as UI does
Description: "Compute manager type like vCenter", | ||
Required: true, | ||
}, | ||
"reverse_proxy_https_port": { |
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.
should we add ValidateFunc: validateSinglePort()
for this port?
Description: "IP address or hostname of compute manager", | ||
Required: true, | ||
}, | ||
"set_as_oidc_provider": { |
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.
Default value here?
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've added defaults to all the attributes which have default values in NSX. But I'm not sure I understand this: if TF assigns a default value, wouldn't it clutter the state file with stuff that wasn't assigned by the user?
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.
If NSX assigns default, it will make its way to the state anyway.
tag = "tag1" | ||
} | ||
|
||
server = "%s" |
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.
lets format this
3a3beda
to
486c0c2
Compare
/test-all |
MaxItems: 1, | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"saml_login_credential": { |
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 would suggest to remove credential
suffix here since its already present in parent name
Description: "A login credential specifying saml token", | ||
Optional: true, | ||
MaxItems: 1, | ||
ExactlyOneOf: []string{ |
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.
shouldn't this be defined under credential
rather than under saml_login_credential
?
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've looked at examples such as this, it seemed correct as it is. It behaves as expected.
Schema: map[string]*schema.Schema{ | ||
"password": { | ||
Type: schema.TypeString, | ||
Description: "The authentication password for login", |
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 username
and password
are required
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.
For some reason they aren't listed as required.
Yet the UI forces to specify these.
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.
Can we check this against NSX to see if empty creds are rejected? Same regarding other types of creds.
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 that it's a waste of time - there's no way anyone could login without creds. I made these required already.
Type: schema.TypeString, | ||
Description: "Thumbprint of the server", | ||
Optional: true, | ||
}, |
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.
feels like this should be required
348341d
to
1870258
Compare
server = "192.168.244.144" | ||
|
||
credential { | ||
username_password_login_credential { |
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.
credential
no longer needed here
* `multi_nsx` - (Optional) Specifies whether multi nsx feature is enabled for compute manager. | ||
* `origin_type` - Compute manager type like vCenter. | ||
* `reverse_proxy_https_port` - (Optional) Proxy https port of compute manager. | ||
* `server` - IP address or hostname of compute manager. |
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.
Should be marked as (Required)
* `tag` - (Optional) A list of scope + tag pairs to associate with this resource. | ||
* `access_level_for_oidc` - (Optional) Specifies access level to NSX from the compute manager. | ||
* `create_service_account` - (Optional) Specifies whether service account is created or not on compute manager. | ||
* `credential` - Login credentials for the compute manager. Should contain exactly one credential enlisted below: |
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.
This is (Required) I think?
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.
General question of convention: (Optional) is optional, and (Required) is required, what about those who do not have any specification of neither?
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.
It think all attributes are one of Required, Optional or RO (Computed only), in which case its moves to Attributes
section
* `pem_encoded` - PEM encoded certificate data. | ||
* `private_key` - Private key of certificate. | ||
* `multi_nsx` - (Optional) Specifies whether multi nsx feature is enabled for compute manager. | ||
* `origin_type` - Compute manager type like vCenter. |
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.
This is Optional now, lets specify default as well.
* `display_name` - (Required) Display name of the resource. | ||
* `description` - (Optional) Description of the resource. | ||
* `tag` - (Optional) A list of scope + tag pairs to associate with this resource. | ||
* `access_level_for_oidc` - (Optional) Specifies access level to NSX from the compute manager. |
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.
Lets add the enum values and default here
1870258
to
67c9f66
Compare
"thumbprint": { | ||
Type: schema.TypeString, | ||
Description: "Thumbprint of the login server", | ||
Required: true, |
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.
thumbprint
is not required in UI
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.
Indeed - instead the UI fetches the thumbprint
and asks for verification.
with curl
the transaction fails when the thumbprint
is missing with the following error:
"Compute manager server x.x.x.x thumbprint input empty is not valid. Current retrieved compute manager server thumbprint is xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx . If correct, please re-submit with this thumbprint"
3c7085e
to
7b5f44c
Compare
Signed-off-by: Kobi Samoray <[email protected]>
Signed-off-by: Kobi Samoray <[email protected]>
7b5f44c
to
dd53510
Compare
/test-all |
|
||
credential { | ||
username_password_login { | ||
username = "%s" |
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.
would the API fail if credentials are incorrect? If no, I suggest to use dummy credentials here and avoid basing this test on testAccTestVCCredentials
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.
NSX validates the credentials and tests the connection. And it should be a vCenter host which isn't already connected to NSX (maybe this NSX instance?) so IDK how we run this in CI. Unless we keep an extra vCenter node for that purpose.
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.
Just one comment about test precheck that can be addressed later
No description provided.