Skip to content
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

migrate fleet integration_policy to framework, fix secret churn #797

Merged
merged 6 commits into from
Oct 1, 2024

Conversation

daemitus
Copy link
Contributor

@daemitus daemitus commented Sep 26, 2024

Dependent on #785 (will fix this up after merge to clean up the diff)
Solves #689

Actually testing the secrets required implementing an actual fleet server. So theres a bunch of changes in the makefile and github actions setting that up.

  • Change from previously: for integration policies, sortInputs would leave new in the original order if old was not present, e.g. during import. Since only the TCP integration was ever tested, the go maps are unordered/randomized issue never cropped up. Now new is sorted by inputID if old is not present. If both are present, existing functionality is preserved.

@daemitus daemitus marked this pull request as draft September 26, 2024 20:52
@daemitus daemitus force-pushed the newfeat branch 2 times, most recently from 26d5955 to 9310f58 Compare September 26, 2024 22:35
@breml
Copy link

breml commented Sep 27, 2024

@daemitus Thanks for the work you have put into this.

if I understand correctly, this PR is based on #785. In order to make the diff in this PR only containing the changes necessary to fix the issue with the secret ref (#689), I suggest to use daemitus:feat as merge target instead of main, at least for the time until daemitus:feat is merged. What do you think?

@daemitus
Copy link
Contributor Author

daemitus commented Sep 27, 2024 via email

@@ -63,7 +63,6 @@ jobs:
xpack.security.enabled: true
xpack.security.authc.api_key.enabled: true
xpack.security.authc.token.enabled: true
xpack.security.http.ssl.enabled: false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaults to false, not included in the makefile

KIBANA_FLEET_PASSWORD: ${{ env.ELASTIC_PASSWORD }}
ports:
- 8220:8220
options: --restart="unless-stopped"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test for secret storage, an actual fleet server is necessary. It'll fail until the kibana password is set, so keep restarting it.

@@ -123,8 +140,6 @@ jobs:
- name: Setup Kibana user
run: make set-kibana-password
env:
ELASTICSEARCH_ENDPOINTS: "http://localhost:9200"
ELASTICSEARCH_USERNAME: "elastic"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused arguments in this makefile directive

@@ -134,10 +149,17 @@ jobs:
run: |-
echo "apikey=$(make create-es-api-key | jq -r .encoded)" >> "$GITHUB_OUTPUT"
env:
ELASTICSEARCH_ENDPOINTS: "http://localhost:9200"
ELASTICSEARCH_USERNAME: "elastic"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused arguments in this makefile directive

SOURCE_LOCATION ?= $(shell pwd)
, := ,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

escaping JSON commas in the setup fleet payloads. This makes me sad, but not much of a better way to do it when wrapping in the retry func

}
}

handleVars(utils.Deref(resp.Vars))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, without using the x-go-remove-unnecessary-pointers flag, you have to sprinkle these deref methods everywhere. Being able to range a nil map is nice.

Key string
Path path.Path
Diags diag.Diagnostics
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea with using these, is to make the iter functions in each of these helpers a little less wordy. If you don't inline the function, you'll still have access to the parent metadata.

// 'existing' will be placed at the end of the list. Inputs are identified by
// their ID ('input_id'). The 'incoming' slice will be sorted in-place.
func sortInputs(incoming []integrationPolicyInputModel, existing []integrationPolicyInputModel) {
if len(existing) == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change from previous that I think addresses a bug that was not noticed before. When importing, existing will be empty/null. And if you have more than 1 import, the order will get randomized (thanks go-maps), so the import test will randomly fail.

@daemitus daemitus marked this pull request as ready for review September 29, 2024 10:53
Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we split this PR into one for the integration_policy resource and one for the output resource please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use a newer version of the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of the fleet changes in getschema (including secret_references) are still missing on the main branch. i was playing around with it yesterday, seeing what would need doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can look into bumping the spec once this is done. I wanted to incorporate some of the transforms upstream anyways. Based on what I've seen, its likely going to require some extra work on getschema anyways.

internal/fleet/integration_policy/models.go Outdated Show resolved Hide resolved
internal/fleet/integration_policy/secrets.go Outdated Show resolved Hide resolved
internal/fleet/integration_policy/secrets.go Outdated Show resolved Hide resolved
internal/fleet/output/models.go Outdated Show resolved Hide resolved
@daemitus daemitus changed the title migrate fleet integration_policy and output to framework, fix secret churn migrate fleet integration_policy to framework, fix secret churn Sep 30, 2024
Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks for making this change!

@tobio tobio merged commit ba9abc9 into elastic:main Oct 1, 2024
20 checks passed
@daemitus daemitus deleted the newfeat branch October 1, 2024 13:19
tobio added a commit that referenced this pull request Oct 3, 2024
* origin/main:
  fix package policy secrets (#821)
  chore(deps): update codecov/codecov-action digest to b9fd7d1 (#815)
  Bump release memory
  Switch to golang 1.23.2 in release
  Bump release memory
  Prepare release v0.11.8 (#810)
  Add key_id as an explicit attribute (#789)
  standalone-output resource (#811)
  Add URL support to data_view field_formats (#812)
  migrate fleet integration_policy to framework, fix secret churn (#797)
  Allow mappings to be unknown to support mappings defined in index templates (#803)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] elasticstack_fleet_integration_policy render consistent diff for policy secret
3 participants