test: Set azure cloud config for e2e testing

[helayoty]

• Add support for different cloud environments in Azure configuration
• Implement E2E testing TLS configuration for Azure clients
• Refactore e2e client setup code to improve readability and maintainability and remove the unused code.

[kaito-pr-agent]

Title

(Describe updated until commit de2555c)

Enhance Azure Configuration and E2E Testing Setup

---

Description

• Added support for different cloud environments in Azure configuration

• Implemented E2E testing TLS configuration for Azure clients

• Refactored E2E client setup code for improved readability and maintainability

• Updated dependencies and module versions

---

Changes walkthrough 📝

	Relevant files
Enhancement	5 files autorest_auth.go Updated NewAuthorizer function signature and improved comments +6/-5      config.go Added E2E configuration fields and updated GetAzureClientConfig +35/-10  cred.go Refactored E2E certificate handling and added GetE2ETLSConfig +127/-75 operator.go Updated CreateAzClient call to include context                      +1/-1      azure_client.go Enhanced cloud configuration handling and E2E mode support +74/-36
Tests	1 files config_test.go Updated tests to include E2E environment variables and assertions +66/-83
Refactoring	2 files util.go Moved PEM block splitting function to util.go                        +18/-0    azure_client.go Minor formatting and import order adjustments                        +5/-5
Configuration changes	1 files values.yaml Added CLOUD_ENVIRONMENT default value                                        +2/-0
Dependencies	2 files go.mod Updated dependency versions and added new dependencies      +9/-9      go.sum Updated checksums for dependency updates                                  +26/-16

---

Need help?

Type /help how to ... in the comments thread for any questions about PR-Agent usage.

Check out the documentation for more information.

[kaito-pr-agent]

PR Reviewer Guide 🔍

(Review updated until commit de2555c)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 Security concerns Sensitive information exposure: The getE2ETestingCert function fetches a secret from Azure Key Vault. Ensure that the credentials used to access the Key Vault are properly secured and that the secret is not exposed in logs or other outputs.

⚡ Recommended focus areas for review Possible Issue The getE2ETestingCert function does not handle the case where result.Value is nil. This could lead to a panic when trying to dereference result.Value. if result.Value == nil { return nil, fmt.Errorf("secret %q not found", clientCertSecretName) Security Concern The InsecureSkipVerify option is set to true in the tlsConfig. This could expose the application to man-in-the-middle attacks. InsecureSkipVerify: true, Possible Issue The BaseVars method does not set the ResourceManagerEndpoint field in the Config struct. This could lead to issues if the ResourceManagerEndpoint is expected to be set.

[kaito-pr-agent]

PR Code Suggestions ✨

Latest suggestions up to de2555c

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Remove insecure skip verify Remove InsecureSkipVerify: true to avoid security risks. pkg/auth/cred.go [120-123] tlsConfig := &tls.Config{ Certificates: []tls.Certificate{cert}, RootCAs: caCertPool, - InsecureSkipVerify: true, } Suggestion importance[1-10]: 9 __ Why: Removing InsecureSkipVerify: true enhances security by ensuring that the TLS certificate is verified against the CA certificates.	High
Possible issue	Improve error handling Handle the error returned by configureHTTP2Transport. pkg/auth/cred.go [147-149] -if err = configureHTTP2Transport(transport); err != nil { //+gocover:ignore:block - can't make this fail. - return nil, err +if err = configureHTTP2Transport(transport); err != nil { + return nil, fmt.Errorf("failed to configure HTTP2 transport: %w", err) } Suggestion importance[1-10]: 7 __ Why: Improving error handling by providing a more descriptive error message helps in debugging and maintaining the code.	Medium
General	Use templating for environment variable Verify that CLOUD_ENVIRONMENT is correctly set for all environments where this configuration is used. charts/gpu-provisioner/values.yaml [121-123] - name: ARM_RESOURCE_GROUP value: - - name: CLOUD_ENVIRONMENT - value: "AzurePublic" +- name: CLOUD_ENVIRONMENT + value: "{{ .Values.cloudEnvironment | default \"AzurePublic\" }}" Suggestion importance[1-10]: 5 __ Why: The suggestion introduces templating for the CLOUD_ENVIRONMENT variable, which can make the configuration more flexible and easier to manage across different environments. However, it is not a critical change and primarily improves maintainability.	Low
	Use assert for validation Use assert.NoError for better readability. pkg/auth/config_test.go [58-59] -if err := cfg.validate(); err != nil { - t.Errorf("unexpected error: %v", err) -} +assert.NoError(t, cfg.validate()) Suggestion importance[1-10]: 2 __ Why: While using assert.NoError improves readability, it does not address any functional issues and is more about style preference.	Low

---

Previous suggestions

Suggestions up to commit 0a93d46

Category	Suggestion	Impact
Security	Remove insecure TLS skip verify Remove InsecureSkipVerify: true to avoid skipping TLS certificate verification, which can expose the application to man-in-the-middle attacks. pkg/auth/cred.go [96-153] +func GetE2ETLSConfig(ctx context.Context, cred azcore.TokenCredential, cfg *Config) (*http.Client, error) { + armClientCert, err := getE2ETestingCert(ctx, cred, cfg) + if err != nil { + return nil, err + } + certPEM, keyPEM := splitPEMBlock([]byte(to.String(armClientCert))) + if len(certPEM) == 0 { + return nil, errors.New("malformed cert pem format") + } + + // Load client cert + cert, err := tls.X509KeyPair(certPEM, keyPEM) + if err != nil { + return nil, err + } + + caCertPool := x509.NewCertPool() + ok := caCertPool.AppendCertsFromPEM([]byte(certPEM)) + if !ok { + return nil, errors.New("cannot append certs from pem") + } + + tlsConfig := &tls.Config{ + Certificates: []tls.Certificate{cert}, + RootCAs: caCertPool, -+ InsecureSkipVerify: true, ++ // InsecureSkipVerify: true, // Removed for security reasons + } + + dialer := &net.Dialer{ + Timeout: 30 * time.Second, + KeepAlive: 30 * time.Second, + } + rpIngressAddress := normalizeHostPort(constructE2ERPIngressEndpointAddress(ctx, cfg)) + transport := &http.Transport{ + TLSClientConfig: tlsConfig, + DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { + if addr == E2E_RP_INGRESS_ENDPOINT_ADDRESS { + addr = rpIngressAddress + } + return dialer.DialContext(ctx, network, addr) + }, + // Configuring DialContext disables HTTP/2 by default. + // our use of DialContext does not impair the use of http/2 so we can force it. + ForceAttemptHTTP2: true, + MaxIdleConns: 100, + MaxIdleConnsPerHost: 100, + IdleConnTimeout: 90 * time.Second, + TLSHandshakeTimeout: 10 * time.Second, + ExpectContinueTimeout: 1 * time.Second, + } + + if err = configureHTTP2Transport(transport); err != nil { //+gocover:ignore:block - can't make this fail. + return nil, err + } + client := &http.Client{ + Transport: transport, + } + return client, nil +} Suggestion importance[1-10]: 9 __ Why: Removing InsecureSkipVerify: true enhances the security of the application by ensuring TLS certificate verification is performed, preventing man-in-the-middle attacks.	High
General	Use templating for environment variable Verify that CLOUD_ENVIRONMENT is correctly set for all environments where this configuration is used. charts/gpu-provisioner/values.yaml [121-123] - name: ARM_RESOURCE_GROUP value: - - name: CLOUD_ENVIRONMENT - value: "AzurePublic" +- name: CLOUD_ENVIRONMENT + value: "{{ .Values.cloudEnvironment | default \"AzurePublic\" }}" Suggestion importance[1-10]: 5 __ Why: The suggestion introduces templating for the CLOUD_ENVIRONMENT variable, which can make the configuration more flexible and easier to manage across different environments. However, it is not a critical change and offers a moderate improvement.	Low

Suggestions up to commit 065f2da

Category	Suggestion	Impact
Security	Remove insecure skip verify Remove InsecureSkipVerify: true to avoid security risks. pkg/auth/cred.go [122-125] tlsConfig := &tls.Config{ Certificates: []tls.Certificate{cert}, RootCAs: caCertPool, - InsecureSkipVerify: true, } Suggestion importance[1-10]: 9 __ Why: Removing InsecureSkipVerify: true significantly improves security by preventing man-in-the-middle attacks.	High
Possible issue	Improve error handling Handle the error returned by configureHTTP2Transport. pkg/auth/cred.go [147-149] -if err = configureHTTP2Transport(transport); err != nil { //+gocover:ignore:block - can't make this fail. - return nil, err +if err = configureHTTP2Transport(transport); err != nil { + return nil, fmt.Errorf("failed to configure HTTP2 transport: %w", err) } Suggestion importance[1-10]: 7 __ Why: Enhancing error messages helps in debugging and understanding the root cause of failures.	Medium
General	Add failure test case Add a test case to verify the behavior when configureHTTP2Transport fails. pkg/auth/config_test.go [164-170] func TestConfigureHTTP2Transport(t *testing.T) { transport := &http.Transport{ ForceAttemptHTTP2: true, } err := configureHTTP2Transport(transport) assert.NoError(t, err) + + // Test failure scenario + transport = &http.Transport{} + err = configureHTTP2Transport(transport) + assert.Error(t, err) } Suggestion importance[1-10]: 6 __ Why: Adding a failure test case ensures that the function behaves correctly under error conditions.	Low
	Validate deploymentMode Ensure that {{ .Values.deploymentMode }} is properly validated and documented to prevent runtime errors. charts/gpu-provisioner/templates/deployment.yaml [84-85] +- name: DEPLOYMENT_MODE + value: {{ .Values.deploymentMode }} - Suggestion importance[1-10]: 2 __ Why: The suggestion is valid but only asks for validation without providing specific changes, reducing the score.	Low
	Validate cloudEnvironment Ensure that {{ .Values.cloudEnvironment }} is properly validated and documented to prevent runtime errors. charts/gpu-provisioner/templates/deployment.yaml [86-87] +- name: CLOUD_ENVIRONMENT + value: {{ .Values.cloudEnvironment }} - Suggestion importance[1-10]: 2 __ Why: The suggestion is valid but only asks for validation without providing specific changes, reducing the score.	Low

Suggestions up to commit 2e0162a

Category	Suggestion	Impact
Security	Enforce TLS verification Remove InsecureSkipVerify to enforce TLS certificate verification. pkg/auth/cred.go [125] -InsecureSkipVerify: true, +InsecureSkipVerify: false, Suggestion importance[1-10]: 8 __ Why: Removing InsecureSkipVerify is crucial for enforcing TLS certificate verification, which helps prevent man-in-the-middle attacks and ensures secure communication.	Medium
	Sanitize environment variable Ensure that cloudEnvironment is properly validated and sanitized before being used in the deployment to prevent injection attacks. charts/gpu-provisioner/templates/deployment.yaml [86-87] + - name: CLOUD_ENVIRONMENT -+ value: {{ .Values.cloudEnvironment }} ++ value: "{{ .Values.cloudEnvironment | quote }}" Suggestion importance[1-10]: 7 __ Why: Quoting the cloudEnvironment value helps prevent injection attacks, improving security.	Medium
General	Use configurable endpoint Ensure that addr is dynamically set based on the context or configuration, rather than being hardcoded. pkg/auth/cred.go [135] -addr = E2E_RP_INGRESS_ENDPOINT_ADDRESS +addr = cfg.E2EIngressEndpoint Suggestion importance[1-10]: 6 __ Why: Using a configurable endpoint enhances flexibility and security by allowing the endpoint to be set dynamically based on the environment or configuration, rather than being hardcoded.	Low
	Handle HTTP2 config error Handle the error appropriately or remove the comment indicating coverage ignore. pkg/auth/cred.go [149-150] -if err = configureHTTP2Transport(transport); err != nil { //+gocover:ignore:block - can't make this fail. +if err = configureHTTP2Transport(transport); err != nil { return nil, err } Suggestion importance[1-10]: 5 __ Why: Handling the error appropriately is important for robust error management. Removing the coverage ignore comment is also necessary to ensure proper testing and error handling practices.	Low
	Make configurable Consider making cloudEnvironment configurable via an environment variable or a command-line argument for flexibility. charts/gpu-provisioner/values.yaml [165] -+cloudEnvironment: "AzurePublic" ++cloudEnvironment: {{ env "CLOUD_ENVIRONMENT" | default "AzurePublic" }} Suggestion importance[1-10]: 5 __ Why: Making cloudEnvironment configurable via an environment variable enhances flexibility, but this suggestion is less critical than the security enhancement.	Low

Suggestions up to commit fb45733

Category	Suggestion	Impact
Security	Secure TLS connections Remove insecure skip verify for production environments. pkg/auth/cred.go [125] -InsecureSkipVerify: true, +InsecureSkipVerify: false, Suggestion importance[1-10]: 9 __ Why: Removing InsecureSkipVerify improves the security of TLS connections by verifying the server's certificate chain, which is crucial for production environments.	High
General	Handle secret retrieval errors Ensure proper handling of secret retrieval errors. pkg/auth/cred.go [199] +return nil, fmt.Errorf("secret %q not found", clientCertSecretName) - Suggestion importance[1-10]: 2 __ Why: The existing code already properly handles the secret retrieval error by returning a formatted error message. No change is needed.	Low

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 68 lines in your changes missing coverage. Please review.

Project coverage is 64.78%. Comparing base (407dc0e) to head (7ca2546).

Files with missing lines	Patch %	Lines
pkg/providers/instance/azure_client.go	0.00%	68 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #240      +/-   ##
==========================================
- Coverage   69.24%   64.78%   -4.46%     
==========================================
  Files           4        4              
  Lines         465      497      +32     
==========================================
  Hits          322      322              
- Misses        123      155      +32     
  Partials       20       20              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.