gridscale / terraform-provider-gridscale

Terraform gridscale provider
https://registry.terraform.io/providers/gridscale/gridscale/latest/docs
Mozilla Public License 2.0
12 stars 11 forks source link

feature/support-multi-node-pools #394

Open alidaw opened 1 month ago

alidaw commented 1 month ago

The implementations out for review are used to add multi node pool support at the k8s resource.

nvthongswansea commented 1 month ago

@alidaw could you update the docs in path website/docs/r/k8s.html.md please?

alidaw commented 1 month ago

Rebased to be ahead of master branch!

alidaw commented 1 month ago

@alidaw could you update the docs in path website/docs/r/k8s.html.md please?

Done. Please have a look.

nvthongswansea commented 3 weeks ago

Could you also add some missing parameters in this PR as well @alidaw . The parameters are k8s_hubble (which is requested by #396 ), and log-related parameters.

nvthongswansea commented 3 weeks ago

@alidaw regarding the last commit of the k8s_hubble and and log-related parameters, I think all of them should be optional and computed, you don't need to set the default (since the default is set automatically by the API server when they are not set in the request payload). The reason why we should set them computed is because in the past there was a case where the default was changed in parameter schema (in paas service template) but it was not in terraform provider. Hence conflicts. Using computed will solve this issue since the param will be set automatically to the value returned by API server when the user do not set it in the tf file. Here is the log-related parameters I have just added in v1, you could take a look.

alidaw commented 3 weeks ago

@alidaw regarding the last commit of the k8s_hubble and and log-related parameters, I think all of them should be optional and computed, you don't need to set the default (since the default is set automatically by the API server when they are not set in the request payload). The reason why we should set them computed is because in the past there was a case where the default was changed in parameter schema (in paas service template) but it was not in terraform provider. Hence conflicts. Using computed will solve this issue since the param will be set automatically to the value returned by API server when the user do not set it in the tf file. Here is the log-related parameters I have just added in v1, you could take a look.

Ok. I see the issue. I've adjusted the schema definition of respective new parameters as you suggested via interactive rebase. I was just wondering why at commit you are applying validation on the new parameter called audit_log_level via hard coded values supposed to be allowed instead fetching what is allowed via respecive parameter's template definition.

You define the allowed values hard coded as global via

var k8sAuditLogLevels = []string{"Metadata", "RequestALLResponseCRUD", "RequestALLResponseALL"}

to then apply validation via validation.StringInSlice(k8sAuditLogLevels, false) as shown below

            "audit_log_level": {
                ...
                ValidateFunc: validation.StringInSlice(k8sAuditLogLevels, false),
            },

But I suggest to fetch what is allowed via respective parameter's template definition as I did as shown below:

func validateK8sParameters(d *schema.ResourceDiff, template gsclient.PaaSTemplate) error {
        ...
    templateParameterAuditLogLevel, templateParameterFound := template.Properties.ParametersSchema["k8s_audit_log_level"]
    if auditLogLevel, ok := d.GetOk("audit_log_level"); ok && templateParameterFound {
        var isValid bool
        for _, allowedValue := range templateParameterAuditLogLevel.Allowed {
            if auditLogLevel.(string) == allowedValue {
                isValid = true
            }
        }
        if !isValid {
            errorMessages = append(errorMessages,
                fmt.Sprintf("Invalid 'audit_log_level' value. Value must be one of these:\n\t%s",
                    strings.Join(templateParameterAuditLogLevel.Allowed, "\n\t"),
                ),
            )
        }
    }
        ...
}

So we do better in referring to validation known by template instead working with hard coded values.

alidaw commented 3 weeks ago

Could you also add some missing parameters in this PR as well @alidaw . The parameters are k8s_hubble (which is requested by #396 ), and log-related parameters.

Done.